Message ID | 20200111163709.4181-1-ap420073@gmail.com |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | netdevsim: fix a several bugs in netdevsim module | expand |
On Sat, 11 Jan 2020 16:37:09 +0000, Taehee Yoo wrote: > When netdevsim dev is being created, a debugfs directory is created. > The variable "dev_ddir_name" is 16bytes device name pointer and device > name is "netdevsim<dev id>". > The maximum dev id length is 10. > So, 16bytes for device name isn't enough. > > Test commands: > modprobe netdevsim > echo "1000000000 0" > /sys/bus/netdevsim/new_device > > Splat looks like: > [ 90.624922][ T1000] BUG: KASAN: stack-out-of-bounds in number+0x824/0x880 > [ 90.626999][ T1000] Write of size 1 at addr ffff8880b7f47988 by task bash/1000 > [ 90.627798][ T1000] > [ 90.628076][ T1000] CPU: 0 PID: 1000 Comm: bash Not tainted 5.5.0-rc5+ #270 > [ 90.628806][ T1000] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006 > [ 90.629752][ T1000] Call Trace: > [ 90.630080][ T1000] dump_stack+0x96/0xdb > [ 90.630512][ T1000] ? number+0x824/0x880 > [ 90.630939][ T1000] print_address_description.constprop.5+0x1be/0x360 > [ 90.631610][ T1000] ? number+0x824/0x880 > [ 90.632038][ T1000] ? number+0x824/0x880 > [ 90.632469][ T1000] __kasan_report+0x12a/0x16f > [ 90.632939][ T1000] ? number+0x824/0x880 > [ 90.633397][ T1000] kasan_report+0xe/0x20 > [ 90.633954][ T1000] number+0x824/0x880 > [ 90.634513][ T1000] ? put_dec+0xa0/0xa0 > [ 90.635047][ T1000] ? rcu_read_lock_sched_held+0x90/0xc0 > [ 90.636469][ T1000] vsnprintf+0x63c/0x10b0 > [ 90.637187][ T1000] ? pointer+0x5b0/0x5b0 > [ 90.637871][ T1000] ? mark_lock+0x11d/0xc40 > [ 90.638591][ T1000] sprintf+0x9b/0xd0 > [ 90.639164][ T1000] ? scnprintf+0xe0/0xe0 > [ 90.639802][ T1000] nsim_dev_probe+0x63c/0xbf0 [netdevsim] > [ ... ] > > Fixes: 83c9e13aa39a ("netdevsim: add software driver for testing offloads") The correct Fixes tag is: Fixes: ab1d0cc004d7 ("netdevsim: change debugfs tree topology") > Signed-off-by: Taehee Yoo <ap420073@gmail.com> > --- > drivers/net/netdevsim/dev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c > index 634eb5cdcbbe..a0c80a70bb23 100644 > --- a/drivers/net/netdevsim/dev.c > +++ b/drivers/net/netdevsim/dev.c > @@ -88,7 +88,7 @@ static const struct file_operations nsim_dev_take_snapshot_fops = { > > static int nsim_dev_debugfs_init(struct nsim_dev *nsim_dev) > { > - char dev_ddir_name[16]; > + char dev_ddir_name[32]; nit: it'd be tempting to size this to the correct value (20? or sizeof(DRV_NAME) + 10) rather than the arbitrary 32, perhaps? > sprintf(dev_ddir_name, DRV_NAME "%u", nsim_dev->nsim_bus_dev->dev.id); > nsim_dev->ddir = debugfs_create_dir(dev_ddir_name, nsim_dev_ddir);
On Sun, 12 Jan 2020 at 22:47, Jakub Kicinski <kubakici@wp.pl> wrote: > Hi Jakub, Thank you for your review! > On Sat, 11 Jan 2020 16:37:09 +0000, Taehee Yoo wrote: > > When netdevsim dev is being created, a debugfs directory is created. > > The variable "dev_ddir_name" is 16bytes device name pointer and device > > name is "netdevsim<dev id>". > > The maximum dev id length is 10. > > So, 16bytes for device name isn't enough. > > > > Test commands: > > modprobe netdevsim > > echo "1000000000 0" > /sys/bus/netdevsim/new_device > > > > Splat looks like: > > [ 90.624922][ T1000] BUG: KASAN: stack-out-of-bounds in number+0x824/0x880 > > [ 90.626999][ T1000] Write of size 1 at addr ffff8880b7f47988 by task bash/1000 > > [ 90.627798][ T1000] > > [ 90.628076][ T1000] CPU: 0 PID: 1000 Comm: bash Not tainted 5.5.0-rc5+ #270 > > [ 90.628806][ T1000] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006 > > [ 90.629752][ T1000] Call Trace: > > [ 90.630080][ T1000] dump_stack+0x96/0xdb > > [ 90.630512][ T1000] ? number+0x824/0x880 > > [ 90.630939][ T1000] print_address_description.constprop.5+0x1be/0x360 > > [ 90.631610][ T1000] ? number+0x824/0x880 > > [ 90.632038][ T1000] ? number+0x824/0x880 > > [ 90.632469][ T1000] __kasan_report+0x12a/0x16f > > [ 90.632939][ T1000] ? number+0x824/0x880 > > [ 90.633397][ T1000] kasan_report+0xe/0x20 > > [ 90.633954][ T1000] number+0x824/0x880 > > [ 90.634513][ T1000] ? put_dec+0xa0/0xa0 > > [ 90.635047][ T1000] ? rcu_read_lock_sched_held+0x90/0xc0 > > [ 90.636469][ T1000] vsnprintf+0x63c/0x10b0 > > [ 90.637187][ T1000] ? pointer+0x5b0/0x5b0 > > [ 90.637871][ T1000] ? mark_lock+0x11d/0xc40 > > [ 90.638591][ T1000] sprintf+0x9b/0xd0 > > [ 90.639164][ T1000] ? scnprintf+0xe0/0xe0 > > [ 90.639802][ T1000] nsim_dev_probe+0x63c/0xbf0 [netdevsim] > > [ ... ] > > > > Fixes: 83c9e13aa39a ("netdevsim: add software driver for testing offloads") > > The correct Fixes tag is: > > Fixes: ab1d0cc004d7 ("netdevsim: change debugfs tree topology") > Thank you! I will fix the Fixes tag. > > Signed-off-by: Taehee Yoo <ap420073@gmail.com> > > --- > > drivers/net/netdevsim/dev.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c > > index 634eb5cdcbbe..a0c80a70bb23 100644 > > --- a/drivers/net/netdevsim/dev.c > > +++ b/drivers/net/netdevsim/dev.c > > @@ -88,7 +88,7 @@ static const struct file_operations nsim_dev_take_snapshot_fops = { > > > > static int nsim_dev_debugfs_init(struct nsim_dev *nsim_dev) > > { > > - char dev_ddir_name[16]; > > + char dev_ddir_name[32]; > > nit: it'd be tempting to size this to the correct value > (20? or sizeof(DRV_NAME) + 10) rather than the arbitrary 32, > perhaps? > I agree with this. This is more correct. I will fix this too. Thank you Taehee Yoo.
diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c index 634eb5cdcbbe..a0c80a70bb23 100644 --- a/drivers/net/netdevsim/dev.c +++ b/drivers/net/netdevsim/dev.c @@ -88,7 +88,7 @@ static const struct file_operations nsim_dev_take_snapshot_fops = { static int nsim_dev_debugfs_init(struct nsim_dev *nsim_dev) { - char dev_ddir_name[16]; + char dev_ddir_name[32]; sprintf(dev_ddir_name, DRV_NAME "%u", nsim_dev->nsim_bus_dev->dev.id); nsim_dev->ddir = debugfs_create_dir(dev_ddir_name, nsim_dev_ddir);
When netdevsim dev is being created, a debugfs directory is created. The variable "dev_ddir_name" is 16bytes device name pointer and device name is "netdevsim<dev id>". The maximum dev id length is 10. So, 16bytes for device name isn't enough. Test commands: modprobe netdevsim echo "1000000000 0" > /sys/bus/netdevsim/new_device Splat looks like: [ 90.624922][ T1000] BUG: KASAN: stack-out-of-bounds in number+0x824/0x880 [ 90.626999][ T1000] Write of size 1 at addr ffff8880b7f47988 by task bash/1000 [ 90.627798][ T1000] [ 90.628076][ T1000] CPU: 0 PID: 1000 Comm: bash Not tainted 5.5.0-rc5+ #270 [ 90.628806][ T1000] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006 [ 90.629752][ T1000] Call Trace: [ 90.630080][ T1000] dump_stack+0x96/0xdb [ 90.630512][ T1000] ? number+0x824/0x880 [ 90.630939][ T1000] print_address_description.constprop.5+0x1be/0x360 [ 90.631610][ T1000] ? number+0x824/0x880 [ 90.632038][ T1000] ? number+0x824/0x880 [ 90.632469][ T1000] __kasan_report+0x12a/0x16f [ 90.632939][ T1000] ? number+0x824/0x880 [ 90.633397][ T1000] kasan_report+0xe/0x20 [ 90.633954][ T1000] number+0x824/0x880 [ 90.634513][ T1000] ? put_dec+0xa0/0xa0 [ 90.635047][ T1000] ? rcu_read_lock_sched_held+0x90/0xc0 [ 90.636469][ T1000] vsnprintf+0x63c/0x10b0 [ 90.637187][ T1000] ? pointer+0x5b0/0x5b0 [ 90.637871][ T1000] ? mark_lock+0x11d/0xc40 [ 90.638591][ T1000] sprintf+0x9b/0xd0 [ 90.639164][ T1000] ? scnprintf+0xe0/0xe0 [ 90.639802][ T1000] nsim_dev_probe+0x63c/0xbf0 [netdevsim] [ ... ] Fixes: 83c9e13aa39a ("netdevsim: add software driver for testing offloads") Signed-off-by: Taehee Yoo <ap420073@gmail.com> --- drivers/net/netdevsim/dev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)