3.16.52-rc1 review patch. If anyone has any objections, please let me know.
------------------
From: Alan Stern stern@rowland.harvard.edu
commit f16443a034c7aa359ddf6f0f9bc40d01ca31faea upstream.
Using the syzkaller kernel fuzzer, Andrey Konovalov generated the following error in gadgetfs:
BUG: KASAN: use-after-free in __lock_acquire+0x3069/0x3690 kernel/locking/lockdep.c:3246 Read of size 8 at addr ffff88003a2bdaf8 by task kworker/3:1/903
CPU: 3 PID: 903 Comm: kworker/3:1 Not tainted 4.12.0-rc4+ #35 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 Workqueue: usb_hub_wq hub_event Call Trace: __dump_stack lib/dump_stack.c:16 [inline] dump_stack+0x292/0x395 lib/dump_stack.c:52 print_address_description+0x78/0x280 mm/kasan/report.c:252 kasan_report_error mm/kasan/report.c:351 [inline] kasan_report+0x230/0x340 mm/kasan/report.c:408 __asan_report_load8_noabort+0x19/0x20 mm/kasan/report.c:429 __lock_acquire+0x3069/0x3690 kernel/locking/lockdep.c:3246 lock_acquire+0x22d/0x560 kernel/locking/lockdep.c:3855 __raw_spin_lock include/linux/spinlock_api_smp.h:142 [inline] _raw_spin_lock+0x2f/0x40 kernel/locking/spinlock.c:151 spin_lock include/linux/spinlock.h:299 [inline] gadgetfs_suspend+0x89/0x130 drivers/usb/gadget/legacy/inode.c:1682 set_link_state+0x88e/0xae0 drivers/usb/gadget/udc/dummy_hcd.c:455 dummy_hub_control+0xd7e/0x1fb0 drivers/usb/gadget/udc/dummy_hcd.c:2074 rh_call_control drivers/usb/core/hcd.c:689 [inline] rh_urb_enqueue drivers/usb/core/hcd.c:846 [inline] usb_hcd_submit_urb+0x92f/0x20b0 drivers/usb/core/hcd.c:1650 usb_submit_urb+0x8b2/0x12c0 drivers/usb/core/urb.c:542 usb_start_wait_urb+0x148/0x5b0 drivers/usb/core/message.c:56 usb_internal_control_msg drivers/usb/core/message.c:100 [inline] usb_control_msg+0x341/0x4d0 drivers/usb/core/message.c:151 usb_clear_port_feature+0x74/0xa0 drivers/usb/core/hub.c:412 hub_port_disable+0x123/0x510 drivers/usb/core/hub.c:4177 hub_port_init+0x1ed/0x2940 drivers/usb/core/hub.c:4648 hub_port_connect drivers/usb/core/hub.c:4826 [inline] hub_port_connect_change drivers/usb/core/hub.c:4999 [inline] port_event drivers/usb/core/hub.c:5105 [inline] hub_event+0x1ae1/0x3d40 drivers/usb/core/hub.c:5185 process_one_work+0xc08/0x1bd0 kernel/workqueue.c:2097 process_scheduled_works kernel/workqueue.c:2157 [inline] worker_thread+0xb2b/0x1860 kernel/workqueue.c:2233 kthread+0x363/0x440 kernel/kthread.c:231 ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:424
Allocated by task 9958: save_stack_trace+0x1b/0x20 arch/x86/kernel/stacktrace.c:59 save_stack+0x43/0xd0 mm/kasan/kasan.c:513 set_track mm/kasan/kasan.c:525 [inline] kasan_kmalloc+0xad/0xe0 mm/kasan/kasan.c:617 kmem_cache_alloc_trace+0x87/0x280 mm/slub.c:2745 kmalloc include/linux/slab.h:492 [inline] kzalloc include/linux/slab.h:665 [inline] dev_new drivers/usb/gadget/legacy/inode.c:170 [inline] gadgetfs_fill_super+0x24f/0x540 drivers/usb/gadget/legacy/inode.c:1993 mount_single+0xf6/0x160 fs/super.c:1192 gadgetfs_mount+0x31/0x40 drivers/usb/gadget/legacy/inode.c:2019 mount_fs+0x9c/0x2d0 fs/super.c:1223 vfs_kern_mount.part.25+0xcb/0x490 fs/namespace.c:976 vfs_kern_mount fs/namespace.c:2509 [inline] do_new_mount fs/namespace.c:2512 [inline] do_mount+0x41b/0x2d90 fs/namespace.c:2834 SYSC_mount fs/namespace.c:3050 [inline] SyS_mount+0xb0/0x120 fs/namespace.c:3027 entry_SYSCALL_64_fastpath+0x1f/0xbe
Freed by task 9960: save_stack_trace+0x1b/0x20 arch/x86/kernel/stacktrace.c:59 save_stack+0x43/0xd0 mm/kasan/kasan.c:513 set_track mm/kasan/kasan.c:525 [inline] kasan_slab_free+0x72/0xc0 mm/kasan/kasan.c:590 slab_free_hook mm/slub.c:1357 [inline] slab_free_freelist_hook mm/slub.c:1379 [inline] slab_free mm/slub.c:2961 [inline] kfree+0xed/0x2b0 mm/slub.c:3882 put_dev+0x124/0x160 drivers/usb/gadget/legacy/inode.c:163 gadgetfs_kill_sb+0x33/0x60 drivers/usb/gadget/legacy/inode.c:2027 deactivate_locked_super+0x8d/0xd0 fs/super.c:309 deactivate_super+0x21e/0x310 fs/super.c:340 cleanup_mnt+0xb7/0x150 fs/namespace.c:1112 __cleanup_mnt+0x1b/0x20 fs/namespace.c:1119 task_work_run+0x1a0/0x280 kernel/task_work.c:116 exit_task_work include/linux/task_work.h:21 [inline] do_exit+0x18a8/0x2820 kernel/exit.c:878 do_group_exit+0x14e/0x420 kernel/exit.c:982 get_signal+0x784/0x1780 kernel/signal.c:2318 do_signal+0xd7/0x2130 arch/x86/kernel/signal.c:808 exit_to_usermode_loop+0x1ac/0x240 arch/x86/entry/common.c:157 prepare_exit_to_usermode arch/x86/entry/common.c:194 [inline] syscall_return_slowpath+0x3ba/0x410 arch/x86/entry/common.c:263 entry_SYSCALL_64_fastpath+0xbc/0xbe
The buggy address belongs to the object at ffff88003a2bdae0 which belongs to the cache kmalloc-1024 of size 1024 The buggy address is located 24 bytes inside of 1024-byte region [ffff88003a2bdae0, ffff88003a2bdee0) The buggy address belongs to the page: page:ffffea0000e8ae00 count:1 mapcount:0 mapping: (null) index:0x0 compound_mapcount: 0 flags: 0x100000000008100(slab|head) raw: 0100000000008100 0000000000000000 0000000000000000 0000000100170017 raw: ffffea0000ed3020 ffffea0000f5f820 ffff88003e80efc0 0000000000000000 page dumped because: kasan: bad access detected
Memory state around the buggy address: ffff88003a2bd980: fb fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc ffff88003a2bda00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
ffff88003a2bda80: fc fc fc fc fc fc fc fc fc fc fc fc fb fb fb fb
^ffff88003a2bdb00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb ffff88003a2bdb80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb ==================================================================
What this means is that the gadgetfs_suspend() routine was trying to access dev->lock after it had been deallocated. The root cause is a race in the dummy_hcd driver; the dummy_udc_stop() routine can race with the rest of the driver because it contains no locking. And even when proper locking is added, it can still race with the set_link_state() function because that function incorrectly drops the private spinlock before invoking any gadget driver callbacks.
The result of this race, as seen above, is that set_link_state() can invoke a callback in gadgetfs even after gadgetfs has been unbound from dummy_hcd's UDC and its private data structures have been deallocated.
include/linux/usb/gadget.h documents that the ->reset, ->disconnect, ->suspend, and ->resume callbacks may be invoked in interrupt context. In general this is necessary, to prevent races with gadget driver removal. This patch fixes dummy_hcd to retain the spinlock across these calls, and it adds a spinlock acquisition to dummy_udc_stop() to prevent the race.
The net2280 driver makes the same mistake of dropping the private spinlock for its ->disconnect and ->reset callback invocations. The patch fixes it too.
Lastly, since gadgetfs_suspend() may be invoked in interrupt context, it cannot assume that interrupts are enabled when it runs. It must use spin_lock_irqsave() instead of spin_lock_irq(). The patch fixes that bug as well.
Signed-off-by: Alan Stern stern@rowland.harvard.edu Reported-and-tested-by: Andrey Konovalov andreyknvl@google.com Acked-by: Felipe Balbi felipe.balbi@linux.intel.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org [bwh: Backported to 3.16: - Drop changes in net2280's handle_stat1_irqs() - Adjust filenames, context] Signed-off-by: Ben Hutchings ben@decadent.org.uk --- --- a/drivers/usb/gadget/inode.c +++ b/drivers/usb/gadget/inode.c @@ -1798,9 +1798,10 @@ static void gadgetfs_suspend (struct usb_gadget *gadget) { struct dev_data *dev = get_gadget_data (gadget); + unsigned long flags;
INFO (dev, "suspended from state %d\n", dev->state); - spin_lock (&dev->lock); + spin_lock_irqsave(&dev->lock, flags); switch (dev->state) { case STATE_DEV_SETUP: // VERY odd... host died?? case STATE_DEV_CONNECTED: @@ -1811,7 +1812,7 @@ gadgetfs_suspend (struct usb_gadget *gad default: break; } - spin_unlock (&dev->lock); + spin_unlock_irqrestore(&dev->lock, flags); }
static struct usb_gadget_driver gadgetfs_driver = { --- a/drivers/usb/gadget/dummy_hcd.c +++ b/drivers/usb/gadget/dummy_hcd.c @@ -379,20 +379,13 @@ static void set_link_state(struct dummy_ (dum_hcd->old_status & USB_PORT_STAT_RESET) == 0 && dum->driver) { stop_activity(dum); - spin_unlock(&dum->lock); dum->driver->disconnect(&dum->gadget); - spin_lock(&dum->lock); } } else if (dum_hcd->active != dum_hcd->old_active) { - if (dum_hcd->old_active && dum->driver->suspend) { - spin_unlock(&dum->lock); + if (dum_hcd->old_active && dum->driver->suspend) dum->driver->suspend(&dum->gadget); - spin_lock(&dum->lock); - } else if (!dum_hcd->old_active && dum->driver->resume) { - spin_unlock(&dum->lock); + else if (!dum_hcd->old_active && dum->driver->resume) dum->driver->resume(&dum->gadget); - spin_lock(&dum->lock); - } }
dum_hcd->old_status = dum_hcd->port_status; @@ -926,7 +919,9 @@ static int dummy_udc_stop(struct usb_gad dev_dbg(udc_dev(dum), "unregister gadget driver '%s'\n", driver->driver.name);
+ spin_lock_irq(&dum->lock); dum->driver = NULL; + spin_unlock_irq(&dum->lock);
return 0; } --- a/drivers/usb/gadget/net2280.c +++ b/drivers/usb/gadget/net2280.c @@ -1941,11 +1941,8 @@ stop_activity (struct net2280 *dev, stru nuke (&dev->ep [i]);
/* report disconnect; the driver is already quiesced */ - if (driver) { - spin_unlock(&dev->lock); + if (driver) driver->disconnect(&dev->gadget); - spin_lock(&dev->lock); - }
usb_reinit (dev); }