From: Dongliang Mu <[email protected]>
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: [email protected]
Signed-off-by: Dongliang Mu <[email protected]>
---
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
On Sat, Apr 09, 2022 at 08:09:00PM +0800, Dongliang Mu wrote:
> From: Dongliang Mu <[email protected]>
>
> 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: [email protected]
> Signed-off-by: Dongliang Mu <[email protected]>
> ---
> 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);
On Sun, Apr 10, 2022 at 5:14 AM Dongliang Mu <[email protected]> wrote:
>
> From: Dongliang Mu <[email protected]>
>
> 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
On Mon, Apr 11, 2022 at 9:33 PM Johan Hovold <[email protected]> wrote:
> On Sat, Apr 09, 2022 at 08:09:00PM +0800, Dongliang Mu wrote:
> > From: Dongliang Mu <[email protected]>
> >
> > 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
On Mon, Apr 11, 2022 at 10:55 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Sun, Apr 10, 2022 at 5:14 AM Dongliang Mu <[email protected]> wrote:
> >
> > From: Dongliang Mu <[email protected]>
> >
> > 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
On Mon, Apr 11, 2022 at 8:14 PM Johan Hovold <[email protected]> wrote:
>
> On Sat, Apr 09, 2022 at 08:09:00PM +0800, Dongliang Mu wrote:
> > From: Dongliang Mu <[email protected]>
> >
> > 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: [email protected]
> > Signed-off-by: Dongliang Mu <[email protected]>
> > ---
> > 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);
On Thu, Apr 14, 2022 at 9:58 PM Dongliang Mu <[email protected]> wrote:
>
> On Mon, Apr 11, 2022 at 8:14 PM Johan Hovold <[email protected]> wrote:
> >
> > On Sat, Apr 09, 2022 at 08:09:00PM +0800, Dongliang Mu wrote:
> > > From: Dongliang Mu <[email protected]>
> > >
> > > 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: [email protected]
> > > Signed-off-by: Dongliang Mu <[email protected]>
> > > ---
> > > 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);
On Thu, Apr 14, 2022 at 06:01:57PM +0300, Andy Shevchenko wrote:
> On Mon, Apr 11, 2022 at 9:33 PM Johan Hovold <[email protected]> wrote:
> > On Sat, Apr 09, 2022 at 08:09:00PM +0800, Dongliang Mu wrote:
> > > From: Dongliang Mu <[email protected]>
> > >
> > > 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/[email protected]/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 |
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
Oliver Neukum <[email protected]> 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
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
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
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 <[email protected]>
> 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 <[email protected]>
> ---
> 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
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