Message-ID: <CAAeHK+xZgjD+gP=pCkk0uKVkuPG+XZ26mgNQCGzw2ea5mqFTJg@mail.gmail.com>
Date:   Mon, 12 Aug 2019 14:07:56 +0200
From:   Andrey Konovalov <andreyknvl@...gle.com>
To:     Alan Stern <stern@...land.harvard.edu>
Cc:     Greg KH <greg@...ah.com>,
        syzbot <syzbot+30cf45ebfe0b0c4847a1@...kaller.appspotmail.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>, kirr@...edi.com,
        Kernel development list <linux-kernel@...r.kernel.org>,
        USB list <linux-usb@...r.kernel.org>,
        Guenter Roeck <linux@...ck-us.net>, lkundrak@...sk,
        logang@...tatee.com,
        syzkaller-bugs <syzkaller-bugs@...glegroups.com>
Subject: Re: KASAN: use-after-free Read in ld_usb_release

On Fri, Aug 9, 2019 at 6:51 PM Alan Stern <stern@...land.harvard.edu> 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@...kaller.appspotmail.com
> >
> > ==================================================================
> > 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!
>
> Alan Stern

Alan, could you submit this patch (if you haven't already)? Looks like
it fixes this bug (and might fix some others).

>
>
>  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();
>

Powered by blists - more mailing lists