| Message ID | 20220409120901.267526-1-dzm91@hust.edu.cn |
|---|---|
| State | New |
| Headers | show |
| Series | driver: usb: nullify dangling pointer in cdc_ncm_free | expand |
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);
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.
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);
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
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);
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?
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
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
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);