On Fri, Aug 9, 2019 at 6:51 PM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
>
> Greg:
>
> See below...
>
> On Fri, 9 Aug 2019, syzbot wrote:
>
> > Hello,
> >
> > syzbot found the following crash on:
> >
> > HEAD commit: e96407b4 usb-fuzzer: main usb gadget fuzzer driver
> > git tree: https://github.com/google/kasan.git usb-fuzzer
> > console output: https://syzkaller.appspot.com/x/log.txt?x=15a16f26600000
> > kernel config: https://syzkaller.appspot.com/x/.config?x=cfa2c18fb6a8068e
> > dashboard link: https://syzkaller.appspot.com/bug?extid=30cf45ebfe0b0c4847a1
> > compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1416df26600000
> > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11ce511c600000
> >
> > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > Reported-by: syzbot+30cf45ebfe0b0c4847a1@xxxxxxxxxxxxxxxxxxxxxxxxx
> >
> > ==================================================================
> > BUG: KASAN: use-after-free in __mutex_lock_common
> > kernel/locking/mutex.c:912 [inline]
> > BUG: KASAN: use-after-free in __mutex_lock+0xf23/0x1360
> > kernel/locking/mutex.c:1077
> > Read of size 8 at addr ffff8881d21fc2d8 by task syz-executor834/1878
> >
> > CPU: 0 PID: 1878 Comm: syz-executor834 Not tainted 5.3.0-rc2+ #25
> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> > Google 01/01/2011
> > Call Trace:
> > __dump_stack lib/dump_stack.c:77 [inline]
> > dump_stack+0xca/0x13e lib/dump_stack.c:113
> > print_address_description+0x6a/0x32c mm/kasan/report.c:351
> > __kasan_report.cold+0x1a/0x33 mm/kasan/report.c:482
> > kasan_report+0xe/0x12 mm/kasan/common.c:612
> > __mutex_lock_common kernel/locking/mutex.c:912 [inline]
> > __mutex_lock+0xf23/0x1360 kernel/locking/mutex.c:1077
> > ld_usb_release+0xb1/0x400 drivers/usb/misc/ldusb.c:386
> > __fput+0x2d7/0x840 fs/file_table.c:280
> > task_work_run+0x13f/0x1c0 kernel/task_work.c:113
> > tracehook_notify_resume include/linux/tracehook.h:188 [inline]
> > exit_to_usermode_loop+0x1d2/0x200 arch/x86/entry/common.c:163
> > prepare_exit_to_usermode arch/x86/entry/common.c:194 [inline]
> > syscall_return_slowpath arch/x86/entry/common.c:274 [inline]
> > do_syscall_64+0x45f/0x580 arch/x86/entry/common.c:299
> > entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > RIP: 0033:0x406b31
> > Code: 75 14 b8 03 00 00 00 0f 05 48 3d 01 f0 ff ff 0f 83 04 19 00 00 c3 48
> > 83 ec 08 e8 6a fc ff ff 48 89 04 24 b8 03 00 00 00 0f 05 <48> 8b 3c 24 48
> > 89 c2 e8 b3 fc ff ff 48 89 d0 48 83 c4 08 48 3d 01
> > RSP: 002b:00007ffcf13bd080 EFLAGS: 00000293 ORIG_RAX: 0000000000000003
> > RAX: 0000000000000000 RBX: 0000000000000005 RCX: 0000000000406b31
> > RDX: fffffffffffffff7 RSI: 0000000000000080 RDI: 0000000000000004
> > RBP: 0000000000000159 R08: 0000000000000020 R09: 0000000000000020
> > R10: 00007ffcf13bd0b0 R11: 0000000000000293 R12: 000000000001d884
> > R13: 0000000000000004 R14: 00000000006e39ec R15: 0000000000000064
> >
> > Allocated by task 1775:
> > save_stack+0x1b/0x80 mm/kasan/common.c:69
> > set_track mm/kasan/common.c:77 [inline]
> > __kasan_kmalloc mm/kasan/common.c:487 [inline]
> > __kasan_kmalloc.constprop.0+0xbf/0xd0 mm/kasan/common.c:460
> > kmalloc include/linux/slab.h:552 [inline]
> > kzalloc include/linux/slab.h:748 [inline]
> > ld_usb_probe+0x6e/0xa65 drivers/usb/misc/ldusb.c:661
> > usb_probe_interface+0x305/0x7a0 drivers/usb/core/driver.c:361
> > really_probe+0x281/0x650 drivers/base/dd.c:548
> > driver_probe_device+0x101/0x1b0 drivers/base/dd.c:709
> > __device_attach_driver+0x1c2/0x220 drivers/base/dd.c:816
> > bus_for_each_drv+0x15c/0x1e0 drivers/base/bus.c:454
> > __device_attach+0x217/0x360 drivers/base/dd.c:882
> > bus_probe_device+0x1e4/0x290 drivers/base/bus.c:514
> > device_add+0xae6/0x16f0 drivers/base/core.c:2114
> > usb_set_configuration+0xdf6/0x1670 drivers/usb/core/message.c:2023
> > generic_probe+0x9d/0xd5 drivers/usb/core/generic.c:210
> > usb_probe_device+0x99/0x100 drivers/usb/core/driver.c:266
> > really_probe+0x281/0x650 drivers/base/dd.c:548
> > driver_probe_device+0x101/0x1b0 drivers/base/dd.c:709
> > __device_attach_driver+0x1c2/0x220 drivers/base/dd.c:816
> > bus_for_each_drv+0x15c/0x1e0 drivers/base/bus.c:454
> > __device_attach+0x217/0x360 drivers/base/dd.c:882
> > bus_probe_device+0x1e4/0x290 drivers/base/bus.c:514
> > device_add+0xae6/0x16f0 drivers/base/core.c:2114
> > usb_new_device.cold+0x6a4/0xe79 drivers/usb/core/hub.c:2536
> > hub_port_connect drivers/usb/core/hub.c:5098 [inline]
> > hub_port_connect_change drivers/usb/core/hub.c:5213 [inline]
> > port_event drivers/usb/core/hub.c:5359 [inline]
> > hub_event+0x1b5c/0x3640 drivers/usb/core/hub.c:5441
> > process_one_work+0x92b/0x1530 kernel/workqueue.c:2269
> > worker_thread+0x96/0xe20 kernel/workqueue.c:2415
> > kthread+0x318/0x420 kernel/kthread.c:255
> > ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
> >
> > Freed by task 1775:
> > save_stack+0x1b/0x80 mm/kasan/common.c:69
> > set_track mm/kasan/common.c:77 [inline]
> > __kasan_slab_free+0x130/0x180 mm/kasan/common.c:449
> > slab_free_hook mm/slub.c:1423 [inline]
> > slab_free_freelist_hook mm/slub.c:1470 [inline]
> > slab_free mm/slub.c:3012 [inline]
> > kfree+0xe4/0x2f0 mm/slub.c:3953
> > ld_usb_probe+0x728/0xa65 drivers/usb/misc/ldusb.c:744
> > usb_probe_interface+0x305/0x7a0 drivers/usb/core/driver.c:361
> > really_probe+0x281/0x650 drivers/base/dd.c:548
> > driver_probe_device+0x101/0x1b0 drivers/base/dd.c:709
> > __device_attach_driver+0x1c2/0x220 drivers/base/dd.c:816
> > bus_for_each_drv+0x15c/0x1e0 drivers/base/bus.c:454
> > __device_attach+0x217/0x360 drivers/base/dd.c:882
> > bus_probe_device+0x1e4/0x290 drivers/base/bus.c:514
> > device_add+0xae6/0x16f0 drivers/base/core.c:2114
> > usb_set_configuration+0xdf6/0x1670 drivers/usb/core/message.c:2023
> > generic_probe+0x9d/0xd5 drivers/usb/core/generic.c:210
> > usb_probe_device+0x99/0x100 drivers/usb/core/driver.c:266
> > really_probe+0x281/0x650 drivers/base/dd.c:548
> > driver_probe_device+0x101/0x1b0 drivers/base/dd.c:709
> > __device_attach_driver+0x1c2/0x220 drivers/base/dd.c:816
> > bus_for_each_drv+0x15c/0x1e0 drivers/base/bus.c:454
> > __device_attach+0x217/0x360 drivers/base/dd.c:882
> > bus_probe_device+0x1e4/0x290 drivers/base/bus.c:514
> > device_add+0xae6/0x16f0 drivers/base/core.c:2114
> > usb_new_device.cold+0x6a4/0xe79 drivers/usb/core/hub.c:2536
> > hub_port_connect drivers/usb/core/hub.c:5098 [inline]
> > hub_port_connect_change drivers/usb/core/hub.c:5213 [inline]
> > port_event drivers/usb/core/hub.c:5359 [inline]
> > hub_event+0x1b5c/0x3640 drivers/usb/core/hub.c:5441
> > process_one_work+0x92b/0x1530 kernel/workqueue.c:2269
> > worker_thread+0x96/0xe20 kernel/workqueue.c:2415
> > kthread+0x318/0x420 kernel/kthread.c:255
> > ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
> >
> > The buggy address belongs to the object at ffff8881d21fc280
> > which belongs to the cache kmalloc-512 of size 512
> > The buggy address is located 88 bytes inside of
> > 512-byte region [ffff8881d21fc280, ffff8881d21fc480)
> > The buggy address belongs to the page:
> > page:ffffea0007487f00 refcount:1 mapcount:0 mapping:ffff8881da002500
> > index:0x0 compound_mapcount: 0
> > flags: 0x200000000010200(slab|head)
> > raw: 0200000000010200 ffffea000739fc80 0000000900000009 ffff8881da002500
> > raw: 0000000000000000 00000000000c000c 00000001ffffffff 0000000000000000
> > page dumped because: kasan: bad access detected
> >
> > Memory state around the buggy address:
> > ffff8881d21fc180: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> > ffff8881d21fc200: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > > ffff8881d21fc280: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> > ^
> > ffff8881d21fc300: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> > ffff8881d21fc380: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> > ==================================================================
>
> I have tracked this bug down. The root causes lie in
> usb_register_dev() and usb_deregister_dev().
>
> The first problem is that usb_deregister_dev() sets
> usb_minors[intf->minor] to NULL before calling device_destroy() on the
> class device. This leaves a window during which another thread can
> allocate the same minor number but will encounter a duplicate name
> error when it tries to register its own class device.
>
> This error shows up in the kernel log from the syzbot test (not shown
> in the bug report above, though):
>
> [ 120.583776][ T1775] sysfs: cannot create duplicate filename '/class/usbmisc/ldusb0'
>
> This can be fixed easily enough by reordering the statements in
> usb_deregister_dev().
>
> The second problem is in usb_register_dev(). When that routine runs,
> it first allocates a minor number, then drops minor_rwsem, and then
> creates the class device. If the device creation fails, the minor
> number is deallocated and the whole routine returns an error. But
> during the time while minor_rwsem was dropped, there is a window in
> which the minor number is allocated and so another thread could
> successfully open the device file!
>
> These two scenarios are exactly what happened during the syzbot test.
> Minor number 0 was deallocated and then allocated in another thread.
> The second allocation failed because the old class device was still in
> existence. As a result of this failure, ldusb's private data structure
> was released. Nevertheless, a third thread managed to open the device
> file during the brief time that minor number 0 was re-allocated. When
> that thread closed the file, it tried to access the private data
> structure that had already been released.
>
> I believe the patch below will fix both problems. But you should take
> a look at it first to see if it seems right; syzbot can't really test
> the patch because it involves two separate races both coming out wrong!
This report has a reproducer, so we can try it:
#syz test: https://github.com/google/kasan.git e96407b4
>
> Alan Stern
>
>
> drivers/usb/core/file.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> Index: usb-devel/drivers/usb/core/file.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/core/file.c
> +++ usb-devel/drivers/usb/core/file.c
> @@ -193,9 +193,10 @@ int usb_register_dev(struct usb_interfac
> intf->minor = minor;
> break;
> }
> - up_write(&minor_rwsem);
> - if (intf->minor < 0)
> + if (intf->minor < 0) {
> + up_write(&minor_rwsem);
> return -EXFULL;
> + }
>
> /* create a usb class device for this usb interface */
> snprintf(name, sizeof(name), class_driver->name, minor - minor_base);
> @@ -203,12 +204,11 @@ int usb_register_dev(struct usb_interfac
> MKDEV(USB_MAJOR, minor), class_driver,
> "%s", kbasename(name));
> if (IS_ERR(intf->usb_dev)) {
> - down_write(&minor_rwsem);
> usb_minors[minor] = NULL;
> intf->minor = -1;
> - up_write(&minor_rwsem);
> retval = PTR_ERR(intf->usb_dev);
> }
> + up_write(&minor_rwsem);
> return retval;
> }
> EXPORT_SYMBOL_GPL(usb_register_dev);
> @@ -234,12 +234,12 @@ void usb_deregister_dev(struct usb_inter
> return;
>
> dev_dbg(&intf->dev, "removing %d minor\n", intf->minor);
> + device_destroy(usb_class->class, MKDEV(USB_MAJOR, intf->minor));
>
> down_write(&minor_rwsem);
> usb_minors[intf->minor] = NULL;
> up_write(&minor_rwsem);
>
> - device_destroy(usb_class->class, MKDEV(USB_MAJOR, intf->minor));
> intf->usb_dev = NULL;
> intf->minor = -1;
> destroy_usb_class();
>
Index: usb-devel/drivers/usb/core/file.c
===================================================================
--- usb-devel.orig/drivers/usb/core/file.c
+++ usb-devel/drivers/usb/core/file.c
@@ -193,9 +193,10 @@ int usb_register_dev(struct usb_interfac
intf->minor = minor;
break;
}
- up_write(&minor_rwsem);
- if (intf->minor < 0)
+ if (intf->minor < 0) {
+ up_write(&minor_rwsem);
return -EXFULL;
+ }
/* create a usb class device for this usb interface */
snprintf(name, sizeof(name), class_driver->name, minor - minor_base);
@@ -203,12 +204,11 @@ int usb_register_dev(struct usb_interfac
MKDEV(USB_MAJOR, minor), class_driver,
"%s", kbasename(name));
if (IS_ERR(intf->usb_dev)) {
- down_write(&minor_rwsem);
usb_minors[minor] = NULL;
intf->minor = -1;
- up_write(&minor_rwsem);
retval = PTR_ERR(intf->usb_dev);
}
+ up_write(&minor_rwsem);
return retval;
}
EXPORT_SYMBOL_GPL(usb_register_dev);
@@ -234,12 +234,12 @@ void usb_deregister_dev(struct usb_inter
return;
dev_dbg(&intf->dev, "removing %d minor\n", intf->minor);
+ device_destroy(usb_class->class, MKDEV(USB_MAJOR, intf->minor));
down_write(&minor_rwsem);
usb_minors[intf->minor] = NULL;
up_write(&minor_rwsem);
- device_destroy(usb_class->class, MKDEV(USB_MAJOR, intf->minor));
intf->usb_dev = NULL;
intf->minor = -1;
destroy_usb_class();
