[PATCH] driver: usb: nullify dangling pointer in cdc_ncm_free

[PATCH] driver: usb: nullify dangling pointer in cdc_ncm_free

[Dongliang Mu]

From: Dongliang Mu <mudongliangabcd@gmail.com>

cdc_ncm_bind calls cdc_ncm_bind_common and sets dev->data[0]
with ctx. However, in the unbind function - cdc_ncm_unbind,
it calls cdc_ncm_free and frees ctx, leaving dev->data[0] as
a dangling pointer. The following ioctl operation will trigger
the UAF in the function cdc_ncm_set_dgram_size.

Fix this by setting dev->data[0] as zero.

==================================================================
BUG: KASAN: use-after-free in cdc_ncm_set_dgram_size+0xc91/0xde0
Read of size 8 at addr ffff8880755210b0 by task dhcpcd/3174

Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:88 [inline]
 dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
 print_address_description.constprop.0.cold+0xeb/0x495 mm/kasan/report.c:313
 print_report mm/kasan/report.c:429 [inline]
 kasan_report.cold+0xf4/0x1c6 mm/kasan/report.c:491
 cdc_ncm_set_dgram_size+0xc91/0xde0 drivers/net/usb/cdc_ncm.c:608
 cdc_ncm_change_mtu+0x10c/0x140 drivers/net/usb/cdc_ncm.c:798
 __dev_set_mtu net/core/dev.c:8519 [inline]
 dev_set_mtu_ext+0x352/0x5b0 net/core/dev.c:8572
 dev_set_mtu+0x8e/0x120 net/core/dev.c:8596
 dev_ifsioc+0xb87/0x1090 net/core/dev_ioctl.c:332
 dev_ioctl+0x1b9/0xe30 net/core/dev_ioctl.c:586
 sock_do_ioctl+0x15a/0x230 net/socket.c:1136
 sock_ioctl+0x2f1/0x640 net/socket.c:1239
 vfs_ioctl fs/ioctl.c:51 [inline]
 __do_sys_ioctl fs/ioctl.c:870 [inline]
 __se_sys_ioctl fs/ioctl.c:856 [inline]
 __x64_sys_ioctl+0x193/0x200 fs/ioctl.c:856
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x35/0x80 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x44/0xae
RIP: 0033:0x7f00859e70e7
RSP: 002b:00007ffedd503dd8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 00007f00858f96c8 RCX: 00007f00859e70e7
RDX: 00007ffedd513fc8 RSI: 0000000000008922 RDI: 0000000000000018
RBP: 00007ffedd524178 R08: 00007ffedd513f88 R09: 00007ffedd513f38
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
R13: 00007ffedd513fc8 R14: 0000000000000028 R15: 0000000000008922
 </TASK>

Allocated by task 26:
 kasan_save_stack+0x1e/0x40 mm/kasan/common.c:38
 kasan_set_track mm/kasan/common.c:45 [inline]
 set_alloc_info mm/kasan/common.c:436 [inline]
 ____kasan_kmalloc mm/kasan/common.c:515 [inline]
 ____kasan_kmalloc mm/kasan/common.c:474 [inline]
 __kasan_kmalloc+0xa9/0xd0 mm/kasan/common.c:524
 kmalloc include/linux/slab.h:581 [inline]
 kzalloc include/linux/slab.h:714 [inline]
 cdc_ncm_bind_common+0xb8/0x2df0 drivers/net/usb/cdc_ncm.c:826
 cdc_ncm_bind+0x7c/0x1c0 drivers/net/usb/cdc_ncm.c:1069
 usbnet_probe+0xaf8/0x2580 drivers/net/usb/usbnet.c:1747
 usb_probe_interface+0x315/0x7f0 drivers/usb/core/driver.c:396
 call_driver_probe drivers/base/dd.c:541 [inline]
 really_probe+0x23e/0xb20 drivers/base/dd.c:620
 __driver_probe_device+0x338/0x4d0 drivers/base/dd.c:751
 driver_probe_device+0x4c/0x1a0 drivers/base/dd.c:781
 __device_attach_driver+0x20b/0x2f0 drivers/base/dd.c:898
 bus_for_each_drv+0x15f/0x1e0 drivers/base/bus.c:427
 __device_attach+0x228/0x4a0 drivers/base/dd.c:969
 bus_probe_device+0x1e4/0x290 drivers/base/bus.c:487
 device_add+0xb83/0x1e20 drivers/base/core.c:3405
 usb_set_configuration+0x101e/0x1900 drivers/usb/core/message.c:2170
 usb_generic_driver_probe+0xba/0x100 drivers/usb/core/generic.c:238
 usb_probe_device+0xd9/0x2c0 drivers/usb/core/driver.c:293
 call_driver_probe drivers/base/dd.c:541 [inline]
 really_probe+0x23e/0xb20 drivers/base/dd.c:620
 __driver_probe_device+0x338/0x4d0 drivers/base/dd.c:751
 driver_probe_device+0x4c/0x1a0 drivers/base/dd.c:781
 __device_attach_driver+0x20b/0x2f0 drivers/base/dd.c:898
 bus_for_each_drv+0x15f/0x1e0 drivers/base/bus.c:427
 __device_attach+0x228/0x4a0 drivers/base/dd.c:969
 bus_probe_device+0x1e4/0x290 drivers/base/bus.c:487
 device_add+0xb83/0x1e20 drivers/base/core.c:3405
 usb_new_device.cold+0x641/0x1091 drivers/usb/core/hub.c:2566
 hub_port_connect drivers/usb/core/hub.c:5363 [inline]
 hub_port_connect_change drivers/usb/core/hub.c:5507 [inline]
 port_event drivers/usb/core/hub.c:5665 [inline]
 hub_event+0x25c6/0x4680 drivers/usb/core/hub.c:5747
 process_one_work+0x996/0x1610 kernel/workqueue.c:2289
 worker_thread+0x665/0x1080 kernel/workqueue.c:2436
 kthread+0x2e9/0x3a0 kernel/kthread.c:376
 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:298

Freed by task 3742:
 kasan_save_stack+0x1e/0x40 mm/kasan/common.c:38
 kasan_set_track+0x21/0x30 mm/kasan/common.c:45
 kasan_set_free_info+0x20/0x30 mm/kasan/generic.c:370
 ____kasan_slab_free mm/kasan/common.c:366 [inline]
 ____kasan_slab_free+0x166/0x1a0 mm/kasan/common.c:328
 kasan_slab_free include/linux/kasan.h:200 [inline]
 slab_free_hook mm/slub.c:1728 [inline]
 slab_free_freelist_hook+0x8b/0x1c0 mm/slub.c:1754
 slab_free mm/slub.c:3510 [inline]
 kfree+0xd6/0x4d0 mm/slub.c:4552
 cdc_ncm_free+0x145/0x1a0 drivers/net/usb/cdc_ncm.c:786
 cdc_ncm_unbind+0x1a7/0x340 drivers/net/usb/cdc_ncm.c:1021
 usbnet_disconnect+0x103/0x270 drivers/net/usb/usbnet.c:1620
 usb_unbind_interface+0x1d8/0x8e0 drivers/usb/core/driver.c:458
 device_remove drivers/base/dd.c:531 [inline]
 device_remove+0x11f/0x170 drivers/base/dd.c:523
 __device_release_driver drivers/base/dd.c:1199 [inline]
 device_release_driver_internal+0x4a3/0x680 drivers/base/dd.c:1222
 bus_remove_device+0x2eb/0x5a0 drivers/base/bus.c:529
 device_del+0x4f3/0xc80 drivers/base/core.c:3592
 usb_disable_device+0x35b/0x7b0 drivers/usb/core/message.c:1419
 usb_disconnect.cold+0x278/0x6ec drivers/usb/core/hub.c:2228
 hub_port_connect drivers/usb/core/hub.c:5207 [inline]
 hub_port_connect_change drivers/usb/core/hub.c:5507 [inline]
 port_event drivers/usb/core/hub.c:5665 [inline]
 hub_event+0x1e74/0x4680 drivers/usb/core/hub.c:5747
 process_one_work+0x996/0x1610 kernel/workqueue.c:2289
 worker_thread+0x665/0x1080 kernel/workqueue.c:2436
 kthread+0x2e9/0x3a0 kernel/kthread.c:376
 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:298

Reported-by: syzbot+eabbf2aaa999cc507108@syzkaller.appspotmail.com
Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
---
 drivers/net/usb/cdc_ncm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 15f91d691bba..9fc2df9f0b63 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -1019,6 +1019,7 @@ void cdc_ncm_unbind(struct usbnet *dev, struct usb_interface *intf)
 
 	usb_set_intfdata(intf, NULL);
 	cdc_ncm_free(ctx);
+	dev->data[0] = 0;
 }
 EXPORT_SYMBOL_GPL(cdc_ncm_unbind);
 
-- 
2.25.1

Re: [PATCH] driver: usb: nullify dangling pointer in cdc_ncm_free

[Johan Hovold]

On Sat, Apr 09, 2022 at 08:09:00PM +0800, Dongliang Mu wrote:
> From: Dongliang Mu <mudongliangabcd@gmail.com>
> 
> cdc_ncm_bind calls cdc_ncm_bind_common and sets dev->data[0]
> with ctx. However, in the unbind function - cdc_ncm_unbind,
> it calls cdc_ncm_free and frees ctx, leaving dev->data[0] as
> a dangling pointer. The following ioctl operation will trigger
> the UAF in the function cdc_ncm_set_dgram_size.
> 
> Fix this by setting dev->data[0] as zero.

This sounds like a poor band-aid. Please explain how this prevent the
ioctl() from racing with unbind(). 

Johan

> ==================================================================
> BUG: KASAN: use-after-free in cdc_ncm_set_dgram_size+0xc91/0xde0
> Read of size 8 at addr ffff8880755210b0 by task dhcpcd/3174
> 
> Call Trace:
>  <TASK>
>  __dump_stack lib/dump_stack.c:88 [inline]
>  dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
>  print_address_description.constprop.0.cold+0xeb/0x495 mm/kasan/report.c:313
>  print_report mm/kasan/report.c:429 [inline]
>  kasan_report.cold+0xf4/0x1c6 mm/kasan/report.c:491
>  cdc_ncm_set_dgram_size+0xc91/0xde0 drivers/net/usb/cdc_ncm.c:608
>  cdc_ncm_change_mtu+0x10c/0x140 drivers/net/usb/cdc_ncm.c:798
>  __dev_set_mtu net/core/dev.c:8519 [inline]
>  dev_set_mtu_ext+0x352/0x5b0 net/core/dev.c:8572
>  dev_set_mtu+0x8e/0x120 net/core/dev.c:8596
>  dev_ifsioc+0xb87/0x1090 net/core/dev_ioctl.c:332
>  dev_ioctl+0x1b9/0xe30 net/core/dev_ioctl.c:586
>  sock_do_ioctl+0x15a/0x230 net/socket.c:1136
>  sock_ioctl+0x2f1/0x640 net/socket.c:1239
>  vfs_ioctl fs/ioctl.c:51 [inline]
>  __do_sys_ioctl fs/ioctl.c:870 [inline]
>  __se_sys_ioctl fs/ioctl.c:856 [inline]
>  __x64_sys_ioctl+0x193/0x200 fs/ioctl.c:856
>  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>  do_syscall_64+0x35/0x80 arch/x86/entry/common.c:80
>  entry_SYSCALL_64_after_hwframe+0x44/0xae
> RIP: 0033:0x7f00859e70e7
> RSP: 002b:00007ffedd503dd8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> RAX: ffffffffffffffda RBX: 00007f00858f96c8 RCX: 00007f00859e70e7
> RDX: 00007ffedd513fc8 RSI: 0000000000008922 RDI: 0000000000000018
> RBP: 00007ffedd524178 R08: 00007ffedd513f88 R09: 00007ffedd513f38
> R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
> R13: 00007ffedd513fc8 R14: 0000000000000028 R15: 0000000000008922
>  </TASK>

> Reported-by: syzbot+eabbf2aaa999cc507108@syzkaller.appspotmail.com
> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> ---
>  drivers/net/usb/cdc_ncm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> index 15f91d691bba..9fc2df9f0b63 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -1019,6 +1019,7 @@ void cdc_ncm_unbind(struct usbnet *dev, struct usb_interface *intf)
>  
>  	usb_set_intfdata(intf, NULL);
>  	cdc_ncm_free(ctx);
> +	dev->data[0] = 0;
>  }
>  EXPORT_SYMBOL_GPL(cdc_ncm_unbind);

Re: [PATCH] driver: usb: nullify dangling pointer in cdc_ncm_free

[Andy Shevchenko]

On Sun, Apr 10, 2022 at 5:14 AM Dongliang Mu <dzm91@hust.edu.cn> wrote:
>
> From: Dongliang Mu <mudongliangabcd@gmail.com>
>
> cdc_ncm_bind calls cdc_ncm_bind_common and sets dev->data[0]
> with ctx. However, in the unbind function - cdc_ncm_unbind,
> it calls cdc_ncm_free and frees ctx, leaving dev->data[0] as
> a dangling pointer. The following ioctl operation will trigger
> the UAF in the function cdc_ncm_set_dgram_size.

First of all, please use the standard form of referring to the func()
as in this sentence.

> Fix this by setting dev->data[0] as zero.
>
> ==================================================================
> BUG: KASAN: use-after-free in cdc_ncm_set_dgram_size+0xc91/0xde0
> Read of size 8 at addr ffff8880755210b0 by task dhcpcd/3174
>

Please, avoid SO noisy commit messages. Find the core part of the
traceback(s) which should be rarely more than 5-10 lines.

...

The code seems fine.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] driver: usb: nullify dangling pointer in cdc_ncm_free

[Dongliang Mu]

On Mon, Apr 11, 2022 at 8:14 PM Johan Hovold <johan@kernel.org> wrote:
>
> On Sat, Apr 09, 2022 at 08:09:00PM +0800, Dongliang Mu wrote:
> > From: Dongliang Mu <mudongliangabcd@gmail.com>
> >
> > cdc_ncm_bind calls cdc_ncm_bind_common and sets dev->data[0]
> > with ctx. However, in the unbind function - cdc_ncm_unbind,
> > it calls cdc_ncm_free and frees ctx, leaving dev->data[0] as
> > a dangling pointer. The following ioctl operation will trigger
> > the UAF in the function cdc_ncm_set_dgram_size.
> >
> > Fix this by setting dev->data[0] as zero.
>
> This sounds like a poor band-aid. Please explain how this prevent the
> ioctl() from racing with unbind().

You mean the following thread interlaving?

ioctl                                unbind
                                cdc_ncm_free(ctx);
dev->data[0]
                                 dev->data[0] = 0;

It seems this will still trigger UAF. Maybe we need to add mutex to
prevent this. But I am not sure.

>
> Johan
>
> > ==================================================================
> > BUG: KASAN: use-after-free in cdc_ncm_set_dgram_size+0xc91/0xde0
> > Read of size 8 at addr ffff8880755210b0 by task dhcpcd/3174
> >
> > Call Trace:
> >  <TASK>
> >  __dump_stack lib/dump_stack.c:88 [inline]
> >  dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
> >  print_address_description.constprop.0.cold+0xeb/0x495 mm/kasan/report.c:313
> >  print_report mm/kasan/report.c:429 [inline]
> >  kasan_report.cold+0xf4/0x1c6 mm/kasan/report.c:491
> >  cdc_ncm_set_dgram_size+0xc91/0xde0 drivers/net/usb/cdc_ncm.c:608
> >  cdc_ncm_change_mtu+0x10c/0x140 drivers/net/usb/cdc_ncm.c:798
> >  __dev_set_mtu net/core/dev.c:8519 [inline]
> >  dev_set_mtu_ext+0x352/0x5b0 net/core/dev.c:8572
> >  dev_set_mtu+0x8e/0x120 net/core/dev.c:8596
> >  dev_ifsioc+0xb87/0x1090 net/core/dev_ioctl.c:332
> >  dev_ioctl+0x1b9/0xe30 net/core/dev_ioctl.c:586
> >  sock_do_ioctl+0x15a/0x230 net/socket.c:1136
> >  sock_ioctl+0x2f1/0x640 net/socket.c:1239
> >  vfs_ioctl fs/ioctl.c:51 [inline]
> >  __do_sys_ioctl fs/ioctl.c:870 [inline]
> >  __se_sys_ioctl fs/ioctl.c:856 [inline]
> >  __x64_sys_ioctl+0x193/0x200 fs/ioctl.c:856
> >  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> >  do_syscall_64+0x35/0x80 arch/x86/entry/common.c:80
> >  entry_SYSCALL_64_after_hwframe+0x44/0xae
> > RIP: 0033:0x7f00859e70e7
> > RSP: 002b:00007ffedd503dd8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> > RAX: ffffffffffffffda RBX: 00007f00858f96c8 RCX: 00007f00859e70e7
> > RDX: 00007ffedd513fc8 RSI: 0000000000008922 RDI: 0000000000000018
> > RBP: 00007ffedd524178 R08: 00007ffedd513f88 R09: 00007ffedd513f38
> > R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
> > R13: 00007ffedd513fc8 R14: 0000000000000028 R15: 0000000000008922
> >  </TASK>
>
> > Reported-by: syzbot+eabbf2aaa999cc507108@syzkaller.appspotmail.com
> > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > ---
> >  drivers/net/usb/cdc_ncm.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> > index 15f91d691bba..9fc2df9f0b63 100644
> > --- a/drivers/net/usb/cdc_ncm.c
> > +++ b/drivers/net/usb/cdc_ncm.c
> > @@ -1019,6 +1019,7 @@ void cdc_ncm_unbind(struct usbnet *dev, struct usb_interface *intf)
> >
> >       usb_set_intfdata(intf, NULL);
> >       cdc_ncm_free(ctx);
> > +     dev->data[0] = 0;
> >  }
> >  EXPORT_SYMBOL_GPL(cdc_ncm_unbind);

Re: [PATCH] driver: usb: nullify dangling pointer in cdc_ncm_free

[Dongliang Mu]

On Mon, Apr 11, 2022 at 10:55 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sun, Apr 10, 2022 at 5:14 AM Dongliang Mu <dzm91@hust.edu.cn> wrote:
> >
> > From: Dongliang Mu <mudongliangabcd@gmail.com>
> >
> > cdc_ncm_bind calls cdc_ncm_bind_common and sets dev->data[0]
> > with ctx. However, in the unbind function - cdc_ncm_unbind,
> > it calls cdc_ncm_free and frees ctx, leaving dev->data[0] as
> > a dangling pointer. The following ioctl operation will trigger
> > the UAF in the function cdc_ncm_set_dgram_size.
>
> First of all, please use the standard form of referring to the func()
> as in this sentence.

OK, no problem.

>
> > Fix this by setting dev->data[0] as zero.
> >
> > ==================================================================
> > BUG: KASAN: use-after-free in cdc_ncm_set_dgram_size+0xc91/0xde0
> > Read of size 8 at addr ffff8880755210b0 by task dhcpcd/3174
> >
>
> Please, avoid SO noisy commit messages. Find the core part of the
> traceback(s) which should be rarely more than 5-10 lines.

Sure. I will revise them in the v2 patch.

>
> ...
>
> The code seems fine.
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH] driver: usb: nullify dangling pointer in cdc_ncm_free

[Dongliang Mu]

On Thu, Apr 14, 2022 at 9:58 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
>
> On Mon, Apr 11, 2022 at 8:14 PM Johan Hovold <johan@kernel.org> wrote:
> >
> > On Sat, Apr 09, 2022 at 08:09:00PM +0800, Dongliang Mu wrote:
> > > From: Dongliang Mu <mudongliangabcd@gmail.com>
> > >
> > > cdc_ncm_bind calls cdc_ncm_bind_common and sets dev->data[0]
> > > with ctx. However, in the unbind function - cdc_ncm_unbind,
> > > it calls cdc_ncm_free and frees ctx, leaving dev->data[0] as
> > > a dangling pointer. The following ioctl operation will trigger
> > > the UAF in the function cdc_ncm_set_dgram_size.
> > >
> > > Fix this by setting dev->data[0] as zero.
> >
> > This sounds like a poor band-aid. Please explain how this prevent the
> > ioctl() from racing with unbind().
>
> You mean the following thread interlaving?
>
> ioctl                                unbind
>                                 cdc_ncm_free(ctx);
> dev->data[0]
>                                  dev->data[0] = 0;
>
> It seems this will still trigger UAF. Maybe we need to add mutex to
> prevent this. But I am not sure.

ioctl                                   unbind
                                  cdc_ncm_free(ctx);
                                  dev->data[0] = 0;
dev->data[0]

This will trigger a null pointer dereference if my patch is applied, right?

>
> >
> > Johan
> >
> > > ==================================================================
> > > BUG: KASAN: use-after-free in cdc_ncm_set_dgram_size+0xc91/0xde0
> > > Read of size 8 at addr ffff8880755210b0 by task dhcpcd/3174
> > >
> > > Call Trace:
> > >  <TASK>
> > >  __dump_stack lib/dump_stack.c:88 [inline]
> > >  dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
> > >  print_address_description.constprop.0.cold+0xeb/0x495 mm/kasan/report.c:313
> > >  print_report mm/kasan/report.c:429 [inline]
> > >  kasan_report.cold+0xf4/0x1c6 mm/kasan/report.c:491
> > >  cdc_ncm_set_dgram_size+0xc91/0xde0 drivers/net/usb/cdc_ncm.c:608
> > >  cdc_ncm_change_mtu+0x10c/0x140 drivers/net/usb/cdc_ncm.c:798
> > >  __dev_set_mtu net/core/dev.c:8519 [inline]
> > >  dev_set_mtu_ext+0x352/0x5b0 net/core/dev.c:8572
> > >  dev_set_mtu+0x8e/0x120 net/core/dev.c:8596
> > >  dev_ifsioc+0xb87/0x1090 net/core/dev_ioctl.c:332
> > >  dev_ioctl+0x1b9/0xe30 net/core/dev_ioctl.c:586
> > >  sock_do_ioctl+0x15a/0x230 net/socket.c:1136
> > >  sock_ioctl+0x2f1/0x640 net/socket.c:1239
> > >  vfs_ioctl fs/ioctl.c:51 [inline]
> > >  __do_sys_ioctl fs/ioctl.c:870 [inline]
> > >  __se_sys_ioctl fs/ioctl.c:856 [inline]
> > >  __x64_sys_ioctl+0x193/0x200 fs/ioctl.c:856
> > >  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> > >  do_syscall_64+0x35/0x80 arch/x86/entry/common.c:80
> > >  entry_SYSCALL_64_after_hwframe+0x44/0xae
> > > RIP: 0033:0x7f00859e70e7
> > > RSP: 002b:00007ffedd503dd8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> > > RAX: ffffffffffffffda RBX: 00007f00858f96c8 RCX: 00007f00859e70e7
> > > RDX: 00007ffedd513fc8 RSI: 0000000000008922 RDI: 0000000000000018
> > > RBP: 00007ffedd524178 R08: 00007ffedd513f88 R09: 00007ffedd513f38
> > > R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
> > > R13: 00007ffedd513fc8 R14: 0000000000000028 R15: 0000000000008922
> > >  </TASK>
> >
> > > Reported-by: syzbot+eabbf2aaa999cc507108@syzkaller.appspotmail.com
> > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > > ---
> > >  drivers/net/usb/cdc_ncm.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> > > index 15f91d691bba..9fc2df9f0b63 100644
> > > --- a/drivers/net/usb/cdc_ncm.c
> > > +++ b/drivers/net/usb/cdc_ncm.c
> > > @@ -1019,6 +1019,7 @@ void cdc_ncm_unbind(struct usbnet *dev, struct usb_interface *intf)
> > >
> > >       usb_set_intfdata(intf, NULL);
> > >       cdc_ncm_free(ctx);
> > > +     dev->data[0] = 0;
> > >  }
> > >  EXPORT_SYMBOL_GPL(cdc_ncm_unbind);

Re: [PATCH] driver: usb: nullify dangling pointer in cdc_ncm_free

[Andy Shevchenko]

On Mon, Apr 11, 2022 at 9:33 PM Johan Hovold <johan@kernel.org> wrote:
> On Sat, Apr 09, 2022 at 08:09:00PM +0800, Dongliang Mu wrote:
> > From: Dongliang Mu <mudongliangabcd@gmail.com>
> >
> > cdc_ncm_bind calls cdc_ncm_bind_common and sets dev->data[0]
> > with ctx. However, in the unbind function - cdc_ncm_unbind,
> > it calls cdc_ncm_free and frees ctx, leaving dev->data[0] as
> > a dangling pointer. The following ioctl operation will trigger
> > the UAF in the function cdc_ncm_set_dgram_size.
> >
> > Fix this by setting dev->data[0] as zero.
>
> This sounds like a poor band-aid. Please explain how this prevent the
> ioctl() from racing with unbind().

Good question. Isn't it the commit 2c9d6c2b871d ("usbnet: run unbind()
before unregister_netdev()") which changed the ordering of the
interface shutdown and basically makes this race happen? I don't see
how we can guarantee that IOCTL won't be called until we quiescence
the network device — my understanding that on device surprise removal
we have to first shutdown what it created and then unbind the device.
If I understand the original issue correctly then the problem is in
usbnet->unbind and it should actually be split to two hooks, otherwise
it seems every possible IOCTL callback must have some kind of
reference counting and keep an eye on the surprise removal.

Johan, can you correct me if my understanding is wrong?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] driver: usb: nullify dangling pointer in cdc_ncm_free

[Oleksij Rempel]

On Thu, Apr 14, 2022 at 06:01:57PM +0300, Andy Shevchenko wrote:
> On Mon, Apr 11, 2022 at 9:33 PM Johan Hovold <johan@kernel.org> wrote:
> > On Sat, Apr 09, 2022 at 08:09:00PM +0800, Dongliang Mu wrote:
> > > From: Dongliang Mu <mudongliangabcd@gmail.com>
> > >
> > > cdc_ncm_bind calls cdc_ncm_bind_common and sets dev->data[0]
> > > with ctx. However, in the unbind function - cdc_ncm_unbind,
> > > it calls cdc_ncm_free and frees ctx, leaving dev->data[0] as
> > > a dangling pointer. The following ioctl operation will trigger
> > > the UAF in the function cdc_ncm_set_dgram_size.
> > >
> > > Fix this by setting dev->data[0] as zero.
> >
> > This sounds like a poor band-aid. Please explain how this prevent the
> > ioctl() from racing with unbind().
> 
> Good question. Isn't it the commit 2c9d6c2b871d ("usbnet: run unbind()
> before unregister_netdev()") which changed the ordering of the
> interface shutdown and basically makes this race happen? I don't see
> how we can guarantee that IOCTL won't be called until we quiescence
> the network device — my understanding that on device surprise removal
> we have to first shutdown what it created and then unbind the device.
> If I understand the original issue correctly then the problem is in
> usbnet->unbind and it should actually be split to two hooks, otherwise
> it seems every possible IOCTL callback must have some kind of
> reference counting and keep an eye on the surprise removal.
> 
> Johan, can you correct me if my understanding is wrong?

Hi,

the possible fix for this issue is under discussion here:
https://lore.kernel.org/netdev/d13e3a34-7e85-92dd-d0c0-5efb3fb08182@suse.com/T/

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] driver: usb: nullify dangling pointer in cdc_ncm_free

[Oliver Neukum]

[-- Attachment #1: Type: text/plain, Size: 1570 bytes --]



On 14.04.22 17:01, Andy Shevchenko wrote:
>
> Good question. Isn't it the commit 2c9d6c2b871d ("usbnet: run unbind()
> before unregister_netdev()") which changed the ordering of the
> interface shutdown and basically makes this race happen? I don't see
> how we can guarantee that IOCTL won't be called until we quiescence
> the network device — my understanding that on device surprise removal
True. The best we could do is introduce a mutex for ioctl() and
disconnect(). That seems the least preferable solution to me.
> we have to first shutdown what it created and then unbind the device.
> If I understand the original issue correctly then the problem is in
> usbnet->unbind and it should actually be split to two hooks, otherwise
> it seems every possible IOCTL callback must have some kind of
> reference counting and keep an eye on the surprise removal.
>
> Johan, can you correct me if my understanding is wrong?
>
It seems to me that fundamentally the order of actions to handle
a hotunplug must mirror the order in a hotplug. We can add more hooks
if that turns out to be necessary for some drivers, but the basic
reverse mirrored order must be supported and I very much favor
restoring it as default.

So I am afraid I have to ask again, whether anybody sees a fundamental
issue with the attached patch, as opposed to it not being an elegant
solution?
It looks to me that we are in a fundamental disagreement on the correct
order in this question and there is no productive way forward other than
offering both ways.

    Regards
        Oliver

[-- Attachment #2: 0001-usbnet-split-unbind-callback.patch --]
[-- Type: text/x-patch, Size: 4357 bytes --]

From 2e07ccbd1769889963d129ec474909bdcaa4c64a Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@suse.com>
Date: Thu, 10 Mar 2022 13:18:38 +0100
Subject: [PATCH] usbnet: split unbind callback

Some devices need to be informed of a disconnect before
the generic layer is informed, others need their notification
later to avoid race conditions. Hence we provide two callbacks.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/net/usb/asix_devices.c | 8 ++++----
 drivers/net/usb/smsc95xx.c     | 4 ++--
 drivers/net/usb/usbnet.c       | 7 +++++--
 include/linux/usb/usbnet.h     | 3 +++
 4 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
index 6ea44e53713a..e6cfa9a39a87 100644
--- a/drivers/net/usb/asix_devices.c
+++ b/drivers/net/usb/asix_devices.c
@@ -808,7 +808,7 @@ static int ax88772_stop(struct usbnet *dev)
 	return 0;
 }
 
-static void ax88772_unbind(struct usbnet *dev, struct usb_interface *intf)
+static void ax88772_disable(struct usbnet *dev, struct usb_interface *intf)
 {
 	struct asix_common_private *priv = dev->driver_priv;
 
@@ -1214,7 +1214,7 @@ static const struct driver_info hawking_uf200_info = {
 static const struct driver_info ax88772_info = {
 	.description = "ASIX AX88772 USB 2.0 Ethernet",
 	.bind = ax88772_bind,
-	.unbind = ax88772_unbind,
+	.unbind = ax88772_disable,
 	.status = asix_status,
 	.reset = ax88772_reset,
 	.stop = ax88772_stop,
@@ -1226,7 +1226,7 @@ static const struct driver_info ax88772_info = {
 static const struct driver_info ax88772b_info = {
 	.description = "ASIX AX88772B USB 2.0 Ethernet",
 	.bind = ax88772_bind,
-	.unbind = ax88772_unbind,
+	.unbind = ax88772_disable,
 	.status = asix_status,
 	.reset = ax88772_reset,
 	.stop = ax88772_stop,
@@ -1262,7 +1262,7 @@ static const struct driver_info ax88178_info = {
 static const struct driver_info hg20f9_info = {
 	.description = "HG20F9 USB 2.0 Ethernet",
 	.bind = ax88772_bind,
-	.unbind = ax88772_unbind,
+	.unbind = ax88772_disable,
 	.status = asix_status,
 	.reset = ax88772_reset,
 	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_LINK_INTR |
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 5567220e9d16..62db57021f5f 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -1211,7 +1211,7 @@ static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf)
 	return ret;
 }
 
-static void smsc95xx_unbind(struct usbnet *dev, struct usb_interface *intf)
+static void smsc95xx_disable(struct usbnet *dev, struct usb_interface *intf)
 {
 	struct smsc95xx_priv *pdata = dev->driver_priv;
 
@@ -1985,7 +1985,7 @@ static int smsc95xx_manage_power(struct usbnet *dev, int on)
 static const struct driver_info smsc95xx_info = {
 	.description	= "smsc95xx USB 2.0 Ethernet",
 	.bind		= smsc95xx_bind,
-	.unbind		= smsc95xx_unbind,
+	.unbind		= smsc95xx_disable,
 	.link_reset	= smsc95xx_link_reset,
 	.reset		= smsc95xx_reset,
 	.check_connect	= smsc95xx_start_phy,
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index b1f93810a6f3..5249a7d7efa5 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1641,8 +1641,8 @@ void usbnet_disconnect (struct usb_interface *intf)
 		   xdev->bus->bus_name, xdev->devpath,
 		   dev->driver_info->description);
 
-	if (dev->driver_info->unbind)
-		dev->driver_info->unbind(dev, intf);
+	if (dev->driver_info->disable)
+		dev->driver_info->disable(dev, intf);
 
 	net = dev->net;
 	unregister_netdev (net);
@@ -1651,6 +1651,9 @@ void usbnet_disconnect (struct usb_interface *intf)
 
 	usb_scuttle_anchored_urbs(&dev->deferred);
 
+	if (dev->driver_info->unbind)
+		dev->driver_info->unbind (dev, intf);
+
 	usb_kill_urb(dev->interrupt);
 	usb_free_urb(dev->interrupt);
 	kfree(dev->padding_pkt);
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 8336e86ce606..4d2407f1ae93 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -129,6 +129,9 @@ struct driver_info {
 	/* cleanup device ... can sleep, but can't fail */
 	void	(*unbind)(struct usbnet *, struct usb_interface *);
 
+	/* disable device ... can sleep, but can't fail */
+	void	(*disable)(struct usbnet *, struct usb_interface *);
+
 	/* reset device ... can sleep */
 	int	(*reset)(struct usbnet *);
 
-- 
2.34.1

Re: [PATCH] driver: usb: nullify dangling pointer in cdc_ncm_free

[Bjørn Mork]

Oliver Neukum <oneukum@suse.com> writes:

> It seems to me that fundamentally the order of actions to handle
> a hotunplug must mirror the order in a hotplug. We can add more hooks
> if that turns out to be necessary for some drivers, but the basic
> reverse mirrored order must be supported and I very much favor
> restoring it as default.

FWIW, I agree 100% with this.  Please go ahead with the revert of commit
2c9d6c2b871d ("usbnet: run unbind() before unregister_netdev()").

AFAICS, your proposed new hook should solve the original problem just
fine.


Bjørn

Re: [PATCH] driver: usb: nullify dangling pointer in cdc_ncm_free

[Johan Hovold]

On Tue, Apr 19, 2022 at 01:47:40PM +0200, Oliver Neukum wrote:
> On 14.04.22 17:01, Andy Shevchenko wrote:
> >
> > Good question. Isn't it the commit 2c9d6c2b871d ("usbnet: run unbind()
> > before unregister_netdev()") which changed the ordering of the
> > interface shutdown and basically makes this race happen? I don't see
> > how we can guarantee that IOCTL won't be called until we quiescence
> > the network device — my understanding that on device surprise removal
> True. The best we could do is introduce a mutex for ioctl() and
> disconnect(). That seems the least preferable solution to me.
> > we have to first shutdown what it created and then unbind the device.

Yes, indeed, commit 2c9d6c2b871d ("usbnet: run unbind() before
unregister_netdev()") is fundamentally broken. You can't just start
freeing driver private data before deregistering the device.

> > If I understand the original issue correctly then the problem is in
> > usbnet->unbind and it should actually be split to two hooks, otherwise
> > it seems every possible IOCTL callback must have some kind of
> > reference counting and keep an eye on the surprise removal.
> >
> > Johan, can you correct me if my understanding is wrong?

That sounds correct. I only noticed that the proposed patch looked
insufficient at best and didn't really look into the backstory until
just now.

> It seems to me that fundamentally the order of actions to handle
> a hotunplug must mirror the order in a hotplug. We can add more hooks
> if that turns out to be necessary for some drivers, but the basic
> reverse mirrored order must be supported and I very much favor
> restoring it as default.

I agree, we need to strive to maintain symmetry. Anything else is likely
broken and at least makes things harder to reason about and maintenance
a pain.

> So I am afraid I have to ask again, whether anybody sees a fundamental
> issue with the attached patch, as opposed to it not being an elegant
> solution?
> It looks to me that we are in a fundamental disagreement on the correct
> order in this question and there is no productive way forward other than
> offering both ways.
> 
>     Regards
>         Oliver

> From 2e07ccbd1769889963d129ec474909bdcaa4c64a Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oneukum@suse.com>
> Date: Thu, 10 Mar 2022 13:18:38 +0100
> Subject: [PATCH] usbnet: split unbind callback
> 
> Some devices need to be informed of a disconnect before
> the generic layer is informed, others need their notification
> later to avoid race conditions. Hence we provide two callbacks.
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
>  drivers/net/usb/asix_devices.c | 8 ++++----
>  drivers/net/usb/smsc95xx.c     | 4 ++--
>  drivers/net/usb/usbnet.c       | 7 +++++--
>  include/linux/usb/usbnet.h     | 3 +++
>  4 files changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
> index 6ea44e53713a..e6cfa9a39a87 100644
> --- a/drivers/net/usb/asix_devices.c
> +++ b/drivers/net/usb/asix_devices.c
> @@ -808,7 +808,7 @@ static int ax88772_stop(struct usbnet *dev)
>  	return 0;
>  }
>  
> -static void ax88772_unbind(struct usbnet *dev, struct usb_interface *intf)
> +static void ax88772_disable(struct usbnet *dev, struct usb_interface *intf)
>  {
>  	struct asix_common_private *priv = dev->driver_priv;
>  
> @@ -1214,7 +1214,7 @@ static const struct driver_info hawking_uf200_info = {
>  static const struct driver_info ax88772_info = {
>  	.description = "ASIX AX88772 USB 2.0 Ethernet",
>  	.bind = ax88772_bind,
> -	.unbind = ax88772_unbind,
> +	.unbind = ax88772_disable,

These should all be

	.disable = ...

but you probably need to split the callback and keep unbind as well for
the actual clean up (freeing resources etc).

>  	.status = asix_status,
>  	.reset = ax88772_reset,
>  	.stop = ax88772_stop,
> @@ -1226,7 +1226,7 @@ static const struct driver_info ax88772_info = {
>  static const struct driver_info ax88772b_info = {
>  	.description = "ASIX AX88772B USB 2.0 Ethernet",
>  	.bind = ax88772_bind,
> -	.unbind = ax88772_unbind,
> +	.unbind = ax88772_disable,
>  	.status = asix_status,
>  	.reset = ax88772_reset,
>  	.stop = ax88772_stop,
> @@ -1262,7 +1262,7 @@ static const struct driver_info ax88178_info = {
>  static const struct driver_info hg20f9_info = {
>  	.description = "HG20F9 USB 2.0 Ethernet",
>  	.bind = ax88772_bind,
> -	.unbind = ax88772_unbind,
> +	.unbind = ax88772_disable,
>  	.status = asix_status,
>  	.reset = ax88772_reset,
>  	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_LINK_INTR |
> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
> index 5567220e9d16..62db57021f5f 100644
> --- a/drivers/net/usb/smsc95xx.c
> +++ b/drivers/net/usb/smsc95xx.c
> @@ -1211,7 +1211,7 @@ static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf)
>  	return ret;
>  }
>  
> -static void smsc95xx_unbind(struct usbnet *dev, struct usb_interface *intf)
> +static void smsc95xx_disable(struct usbnet *dev, struct usb_interface *intf)
>  {
>  	struct smsc95xx_priv *pdata = dev->driver_priv;
>  
> @@ -1985,7 +1985,7 @@ static int smsc95xx_manage_power(struct usbnet *dev, int on)
>  static const struct driver_info smsc95xx_info = {
>  	.description	= "smsc95xx USB 2.0 Ethernet",
>  	.bind		= smsc95xx_bind,
> -	.unbind		= smsc95xx_unbind,
> +	.unbind		= smsc95xx_disable,
>  	.link_reset	= smsc95xx_link_reset,
>  	.reset		= smsc95xx_reset,
>  	.check_connect	= smsc95xx_start_phy,
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index b1f93810a6f3..5249a7d7efa5 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -1641,8 +1641,8 @@ void usbnet_disconnect (struct usb_interface *intf)
>  		   xdev->bus->bus_name, xdev->devpath,
>  		   dev->driver_info->description);
>  
> -	if (dev->driver_info->unbind)
> -		dev->driver_info->unbind(dev, intf);
> +	if (dev->driver_info->disable)
> +		dev->driver_info->disable(dev, intf);
>  
>  	net = dev->net;
>  	unregister_netdev (net);
> @@ -1651,6 +1651,9 @@ void usbnet_disconnect (struct usb_interface *intf)
>  
>  	usb_scuttle_anchored_urbs(&dev->deferred);
>  
> +	if (dev->driver_info->unbind)
> +		dev->driver_info->unbind (dev, intf);
> +
>  	usb_kill_urb(dev->interrupt);

Don't you need to quiesce all I/O, including stopping the interrupt URB,
before unbind?

>  	usb_free_urb(dev->interrupt);
>  	kfree(dev->padding_pkt);
> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
> index 8336e86ce606..4d2407f1ae93 100644
> --- a/include/linux/usb/usbnet.h
> +++ b/include/linux/usb/usbnet.h
> @@ -129,6 +129,9 @@ struct driver_info {
>  	/* cleanup device ... can sleep, but can't fail */
>  	void	(*unbind)(struct usbnet *, struct usb_interface *);
>  
> +	/* disable device ... can sleep, but can't fail */
> +	void	(*disable)(struct usbnet *, struct usb_interface *);
> +
>  	/* reset device ... can sleep */
>  	int	(*reset)(struct usbnet *);

Johan

Re: [PATCH] driver: usb: nullify dangling pointer in cdc_ncm_free

[Oliver Neukum]

On 20.04.22 08:56, Johan Hovold wrote:
>
>>  
>> @@ -1214,7 +1214,7 @@ static const struct driver_info hawking_uf200_info = {
>>  static const struct driver_info ax88772_info = {
>>  	.description = "ASIX AX88772 USB 2.0 Ethernet",
>>  	.bind = ax88772_bind,
>> -	.unbind = ax88772_unbind,
>> +	.unbind = ax88772_disable,
> These should all be
>
> 	.disable = ...
Thanks, noted.
>
> but you probably need to split the callback and keep unbind as well for
> the actual clean up (freeing resources etc).
That is the driver the problematic commit was requested for.
I am looking into it.
>
>> -	if (dev->driver_info->unbind)
>> -		dev->driver_info->unbind(dev, intf);
>> +	if (dev->driver_info->disable)
>> +		dev->driver_info->disable(dev, intf);
>>  
>>  	net = dev->net;
>>  	unregister_netdev (net);
>> @@ -1651,6 +1651,9 @@ void usbnet_disconnect (struct usb_interface *intf)
>>  
>>  	usb_scuttle_anchored_urbs(&dev->deferred);
>>  
>> +	if (dev->driver_info->unbind)
>> +		dev->driver_info->unbind (dev, intf);
>> +
>>  	usb_kill_urb(dev->interrupt);
> Don't you need to quiesce all I/O, including stopping the interrupt URB,
> before unbind?
If I do that, how do I prevent people from relaunching the URB between
kill and unbind? Do I need to poison it?

    Regards
        Oliver

Re: [PATCH] driver: usb: nullify dangling pointer in cdc_ncm_free

[Johan Hovold]

On Wed, Apr 20, 2022 at 11:45:49AM +0200, Oliver Neukum wrote:

> >> -	if (dev->driver_info->unbind)
> >> -		dev->driver_info->unbind(dev, intf);
> >> +	if (dev->driver_info->disable)
> >> +		dev->driver_info->disable(dev, intf);
> >>  
> >>  	net = dev->net;
> >>  	unregister_netdev (net);
> >> @@ -1651,6 +1651,9 @@ void usbnet_disconnect (struct usb_interface *intf)
> >>  
> >>  	usb_scuttle_anchored_urbs(&dev->deferred);
> >>  
> >> +	if (dev->driver_info->unbind)
> >> +		dev->driver_info->unbind (dev, intf);
> >> +
> >>  	usb_kill_urb(dev->interrupt);

> > Don't you need to quiesce all I/O, including stopping the interrupt URB,
> > before unbind?

> If I do that, how do I prevent people from relaunching the URB between
> kill and unbind? Do I need to poison it?

You could, but it would seem you have bigger problems if something can
submit the URB after having deregistered the netdev.

Looks like the URB should already have been stopped by
usbnet_status_stop() so that the usb_kill_urb() above is (or should be)
a noop.

Johan

Re: [PATCH] driver: usb: nullify dangling pointer in cdc_ncm_free

[Oliver Neukum]

[-- Attachment #1: Type: text/plain, Size: 348 bytes --]


So I am afraid I have to ask again, whether anybody sees a fundamental
issue with the new version attached patch, as opposed to it not being an
elegant
solution?

I corrected the stuff Johan found and split the method in the asix driver.

I do not understand smsc95xx well, so I left it in the current state.


    Regards
        Oliver

[-- Attachment #2: 0001-usbnet-split-unbind-callback.patch --]
[-- Type: text/x-patch, Size: 4554 bytes --]

From 5953b3b12dd6cdd8d0bdb0119ee627d62219ab1e Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@suse.com>
Date: Thu, 10 Mar 2022 13:18:38 +0100
Subject: [PATCH] usbnet: split unbind callback

Some devices need to be informed of a disconnect before
the generic layer is informed, others need their notification
later to avoid race conditions. Hence we provide two callbacks.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/net/usb/asix_devices.c | 13 +++++++++++--
 drivers/net/usb/smsc95xx.c     |  4 ++--
 drivers/net/usb/usbnet.c       | 10 +++++++---
 include/linux/usb/usbnet.h     |  3 +++
 4 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
index 38e47a93fb83..cde4d234b975 100644
--- a/drivers/net/usb/asix_devices.c
+++ b/drivers/net/usb/asix_devices.c
@@ -804,12 +804,18 @@ static int ax88772_stop(struct usbnet *dev)
 	return 0;
 }
 
-static void ax88772_unbind(struct usbnet *dev, struct usb_interface *intf)
+static void ax88772_disable(struct usbnet *dev, struct usb_interface *intf)
 {
 	struct asix_common_private *priv = dev->driver_priv;
 
 	phy_disconnect(priv->phydev);
-	asix_rx_fixup_common_free(dev->driver_priv);
+}
+
+static void ax88772_unbind(struct usbnet *dev, struct usb_interface *intf)
+{
+	struct asix_common_private *priv = dev->driver_priv;
+
+	asix_rx_fixup_common_free(priv);
 }
 
 static void ax88178_unbind(struct usbnet *dev, struct usb_interface *intf)
@@ -1211,6 +1217,7 @@ static const struct driver_info ax88772_info = {
 	.description = "ASIX AX88772 USB 2.0 Ethernet",
 	.bind = ax88772_bind,
 	.unbind = ax88772_unbind,
+	.disable = ax88772_disable,
 	.status = asix_status,
 	.reset = ax88772_reset,
 	.stop = ax88772_stop,
@@ -1223,6 +1230,7 @@ static const struct driver_info ax88772b_info = {
 	.description = "ASIX AX88772B USB 2.0 Ethernet",
 	.bind = ax88772_bind,
 	.unbind = ax88772_unbind,
+	.disable = ax88772_disable,
 	.status = asix_status,
 	.reset = ax88772_reset,
 	.stop = ax88772_stop,
@@ -1259,6 +1267,7 @@ static const struct driver_info hg20f9_info = {
 	.description = "HG20F9 USB 2.0 Ethernet",
 	.bind = ax88772_bind,
 	.unbind = ax88772_unbind,
+	.disable = ax88772_disable,
 	.status = asix_status,
 	.reset = ax88772_reset,
 	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_LINK_INTR |
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 4ef61f6b85df..20ce88373809 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -1223,7 +1223,7 @@ static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf)
 	return ret;
 }
 
-static void smsc95xx_unbind(struct usbnet *dev, struct usb_interface *intf)
+static void smsc95xx_disable(struct usbnet *dev, struct usb_interface *intf)
 {
 	struct smsc95xx_priv *pdata = dev->driver_priv;
 
@@ -1997,7 +1997,7 @@ static int smsc95xx_manage_power(struct usbnet *dev, int on)
 static const struct driver_info smsc95xx_info = {
 	.description	= "smsc95xx USB 2.0 Ethernet",
 	.bind		= smsc95xx_bind,
-	.unbind		= smsc95xx_unbind,
+	.disable	= smsc95xx_disable,
 	.link_reset	= smsc95xx_link_reset,
 	.reset		= smsc95xx_reset,
 	.check_connect	= smsc95xx_start_phy,
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index b1f93810a6f3..5f3851e61573 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1641,17 +1641,21 @@ void usbnet_disconnect (struct usb_interface *intf)
 		   xdev->bus->bus_name, xdev->devpath,
 		   dev->driver_info->description);
 
-	if (dev->driver_info->unbind)
-		dev->driver_info->unbind(dev, intf);
+	if (dev->driver_info->disable)
+		dev->driver_info->disable(dev, intf);
 
 	net = dev->net;
 	unregister_netdev (net);
 
+	usb_kill_urb(dev->interrupt);
+
 	cancel_work_sync(&dev->kevent);
 
 	usb_scuttle_anchored_urbs(&dev->deferred);
 
-	usb_kill_urb(dev->interrupt);
+	if (dev->driver_info->unbind)
+		dev->driver_info->unbind (dev, intf);
+
 	usb_free_urb(dev->interrupt);
 	kfree(dev->padding_pkt);
 
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 1b4d72d5e891..dd4a1104e332 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -129,6 +129,9 @@ struct driver_info {
 	/* cleanup device ... can sleep, but can't fail */
 	void	(*unbind)(struct usbnet *, struct usb_interface *);
 
+	/* disable device ... can sleep, but can't fail */
+	void	(*disable)(struct usbnet *, struct usb_interface *);
+
 	/* reset device ... can sleep */
 	int	(*reset)(struct usbnet *);
 
-- 
2.34.1