| Message ID | YtlrnhHyrHsSky9m@rowland.harvard.edu (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2191c00855b03aa59c20e698be713d952d51fc18 |
| Headers | show |
| Series | USB: gadget: Fix use-after-free Read in usb_udc_uevent() | expand |
On Thu, Jul 21, 2022 at 11:07:10AM -0400, Alan Stern wrote: > The syzbot fuzzer found a race between uevent callbacks and gadget > driver unregistration that can cause a use-after-free bug: > > --------------------------------------------------------------- > BUG: KASAN: use-after-free in usb_udc_uevent+0x11f/0x130 > drivers/usb/gadget/udc/core.c:1732 > Read of size 8 at addr ffff888078ce2050 by task udevd/2968 > > CPU: 1 PID: 2968 Comm: udevd Not tainted 5.19.0-rc4-next-20220628-syzkaller #0 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google > 06/29/2022 > Call Trace: > <TASK> > __dump_stack lib/dump_stack.c:88 [inline] > dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106 > print_address_description mm/kasan/report.c:317 [inline] > print_report.cold+0x2ba/0x719 mm/kasan/report.c:433 > kasan_report+0xbe/0x1f0 mm/kasan/report.c:495 > usb_udc_uevent+0x11f/0x130 drivers/usb/gadget/udc/core.c:1732 > dev_uevent+0x290/0x770 drivers/base/core.c:2424 > --------------------------------------------------------------- > > The bug occurs because usb_udc_uevent() dereferences udc->driver but > does so without acquiring the udc_lock mutex, which protects this > field. If the gadget driver is unbound from the udc concurrently with > uevent processing, the driver structure may be accessed after it has > been deallocated. > > To prevent the race, we make sure that the routine holds the mutex > around the racing accesses. > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu> > Reported-and-tested-by: syzbot+b0de012ceb1e2a97891b@syzkaller.appspotmail.com > CC: stable@vger.kernel.org > Link: <https://lore.kernel.org/all/0000000000004de90405a719c951@google.com> > > --- > > As far as I can tell, this bug has always been present. However, the > udc_lock mutex used by the patch was added in commit fc274c1e9973 > ("USB: gadget: Add a new bus for gadgets"), so this patch won't apply > to trees which don't include that commit or a backport of it. I don't > know what tag, if any, can express this requirement. As per the stable_kernel_rules document, you can say: cc: stable@vger.kernel.org # fc274c1e9973 and I should hopefully figure it out :) I'll add that here and see how well it works... thanks, greg k-h
Hi Alan, On 21.07.2022 17:07, Alan Stern wrote: > The syzbot fuzzer found a race between uevent callbacks and gadget > driver unregistration that can cause a use-after-free bug: > > --------------------------------------------------------------- > BUG: KASAN: use-after-free in usb_udc_uevent+0x11f/0x130 > drivers/usb/gadget/udc/core.c:1732 > Read of size 8 at addr ffff888078ce2050 by task udevd/2968 > > CPU: 1 PID: 2968 Comm: udevd Not tainted 5.19.0-rc4-next-20220628-syzkaller #0 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google > 06/29/2022 > Call Trace: > <TASK> > __dump_stack lib/dump_stack.c:88 [inline] > dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106 > print_address_description mm/kasan/report.c:317 [inline] > print_report.cold+0x2ba/0x719 mm/kasan/report.c:433 > kasan_report+0xbe/0x1f0 mm/kasan/report.c:495 > usb_udc_uevent+0x11f/0x130 drivers/usb/gadget/udc/core.c:1732 > dev_uevent+0x290/0x770 drivers/base/core.c:2424 > --------------------------------------------------------------- > > The bug occurs because usb_udc_uevent() dereferences udc->driver but > does so without acquiring the udc_lock mutex, which protects this > field. If the gadget driver is unbound from the udc concurrently with > uevent processing, the driver structure may be accessed after it has > been deallocated. > > To prevent the race, we make sure that the routine holds the mutex > around the racing accesses. > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu> > Reported-and-tested-by: syzbot+b0de012ceb1e2a97891b@syzkaller.appspotmail.com > CC: stable@vger.kernel.org > Link: <https://lore.kernel.org/all/0000000000004de90405a719c951@google.com> This patch landed recently in linux-next as commit 2191c00855b0 ("USB: gadget: Fix use-after-free Read in usb_udc_uevent()"). Unfortunately it fixes the issue by introducing another one. It doesn't look very probable, but it would be nice to fix it to make the lock dependency checker happy. ====================================================== WARNING: possible circular locking dependency detected 5.19.0-rc7+ #12510 Not tainted ------------------------------------------------------ udevadm/312 is trying to acquire lock: ffff80000aae1058 (udc_lock){+.+.}-{3:3}, at: usb_udc_uevent+0x54/0xe0 but task is already holding lock: ffff000002277548 (kn->active#4){++++}-{0:0}, at: kernfs_seq_start+0x34/0xe0 which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #3 (kn->active#4){++++}-{0:0}: lock_acquire+0x68/0x84 __kernfs_remove+0x268/0x380 kernfs_remove_by_name_ns+0x58/0xac sysfs_remove_file_ns+0x18/0x24 device_del+0x15c/0x440 device_link_drop_managed+0xa8/0xe0 device_links_driver_bound+0x1b8/0x230 driver_bound+0x68/0xc0 really_probe.part.0+0x1f8/0x2ac __driver_probe_device+0x98/0x144 driver_probe_device+0xac/0x14c __driver_attach+0x104/0x1b0 bus_for_each_dev+0x70/0xd0 driver_attach+0x24/0x30 bus_add_driver+0x154/0x204 driver_register+0x78/0x130 __platform_driver_register+0x28/0x34 simple_pm_bus_driver_init+0x1c/0x28 do_one_initcall+0x74/0x400 kernel_init_freeable+0x2f4/0x37c kernel_init+0x28/0x130 ret_from_fork+0x10/0x20 -> #2 (device_links_lock){+.+.}-{3:3}: lock_acquire+0x68/0x84 __mutex_lock+0x9c/0x430 mutex_lock_nested+0x38/0x64 device_link_remove+0x3c/0xa0 _regulator_put.part.0+0x168/0x190 regulator_put+0x3c/0x54 devm_regulator_release+0x14/0x20 release_nodes+0x5c/0x90 devres_release_all+0x8c/0xe0 device_unbind_cleanup+0x18/0x70 really_probe.part.0+0x174/0x2ac __driver_probe_device+0x98/0x144 driver_probe_device+0xac/0x14c __device_attach_driver+0xb8/0x120 bus_for_each_drv+0x78/0xd0 __device_attach_async_helper+0xb0/0xd4 async_run_entry_fn+0x34/0xd0 process_one_work+0x288/0x6bc worker_thread+0x74/0x450 kthread+0x118/0x11c ret_from_fork+0x10/0x20 -> #1 (regulator_list_mutex){+.+.}-{3:3}: lock_acquire+0x68/0x84 __mutex_lock+0x9c/0x430 mutex_lock_nested+0x38/0x64 regulator_lock_dependent+0x54/0x284 regulator_enable+0x34/0x80 phy_power_on+0x24/0x130 __dwc2_lowlevel_hw_enable+0x100/0x130 dwc2_lowlevel_hw_enable+0x18/0x40 dwc2_hsotg_udc_start+0x6c/0x2f0 gadget_bind_driver+0x124/0x1f4 really_probe.part.0+0x9c/0x2ac __driver_probe_device+0x98/0x144 driver_probe_device+0xac/0x14c __device_attach_driver+0xb8/0x120 bus_for_each_drv+0x78/0xd0 __device_attach+0xa8/0x1c0 device_initial_probe+0x14/0x20 bus_probe_device+0x9c/0xa4 device_add+0x3a0/0x890 usb_add_gadget+0x170/0x200 usb_add_gadget_udc+0x94/0xd4 dwc2_driver_probe+0x580/0x78c platform_probe+0x68/0xe0 really_probe.part.0+0x9c/0x2ac __driver_probe_device+0x98/0x144 driver_probe_device+0xac/0x14c __device_attach_driver+0xb8/0x120 bus_for_each_drv+0x78/0xd0 __device_attach+0xa8/0x1c0 device_initial_probe+0x14/0x20 bus_probe_device+0x9c/0xa4 device_add+0x3a0/0x890 of_device_add+0x48/0x6c of_platform_device_create_pdata+0x98/0x100 of_platform_bus_create+0x17c/0x37c of_platform_populate+0x58/0xec dwc3_meson_g12a_probe+0x314/0x5d0 platform_probe+0x68/0xe0 really_probe.part.0+0x9c/0x2ac __driver_probe_device+0x98/0x144 driver_probe_device+0xac/0x14c __device_attach_driver+0xb8/0x120 bus_for_each_drv+0x78/0xd0 __device_attach+0xa8/0x1c0 device_initial_probe+0x14/0x20 bus_probe_device+0x9c/0xa4 deferred_probe_work_func+0x88/0xc4 process_one_work+0x288/0x6bc worker_thread+0x74/0x450 kthread+0x118/0x11c ret_from_fork+0x10/0x20 -> #0 (udc_lock){+.+.}-{3:3}: __lock_acquire+0x1298/0x20cc lock_acquire.part.0+0xe0/0x230 lock_acquire+0x68/0x84 __mutex_lock+0x9c/0x430 mutex_lock_nested+0x38/0x64 usb_udc_uevent+0x54/0xe0 dev_uevent+0xb8/0x1ec uevent_show+0x8c/0x114 dev_attr_show+0x20/0x60 sysfs_kf_seq_show+0xa8/0x120 kernfs_seq_show+0x2c/0x40 seq_read_iter+0x1bc/0x4b0 kernfs_fop_read_iter+0x140/0x1d0 new_sync_read+0xd4/0x150 vfs_read+0x190/0x1dc ksys_read+0x68/0xfc __arm64_sys_read+0x20/0x30 invoke_syscall+0x48/0x114 el0_svc_common.constprop.0+0x60/0x11c do_el0_svc_compat+0x1c/0x50 el0_svc_compat+0x58/0x100 el0t_32_sync_handler+0x90/0x140 el0t_32_sync+0x190/0x194 other info that might help us debug this: Chain exists of: udc_lock --> device_links_lock --> kn->active#4 Possible unsafe locking scenario: CPU0 CPU1 ---- ---- lock(kn->active#4); lock(device_links_lock); lock(kn->active#4); lock(udc_lock); *** DEADLOCK *** 3 locks held by udevadm/312: #0: ffff000001ab80a0 (&p->lock){+.+.}-{3:3}, at: seq_read_iter+0x60/0x4b0 #1: ffff00003cdf7e88 (&of->mutex){+.+.}-{3:3}, at: kernfs_seq_start+0x2c/0xe0 #2: ffff000002277548 (kn->active#4){++++}-{0:0}, at: kernfs_seq_start+0x34/0xe0 stack backtrace: CPU: 2 PID: 312 Comm: udevadm Not tainted 5.19.0-rc7+ #12510 Hardware name: Khadas VIM3 (DT) Call trace: dump_backtrace.part.0+0xd0/0xe0 show_stack+0x18/0x6c dump_stack_lvl+0x8c/0xb8 dump_stack+0x18/0x34 print_circular_bug+0x1f8/0x200 check_noncircular+0x130/0x144 __lock_acquire+0x1298/0x20cc lock_acquire.part.0+0xe0/0x230 lock_acquire+0x68/0x84 __mutex_lock+0x9c/0x430 mutex_lock_nested+0x38/0x64 usb_udc_uevent+0x54/0xe0 dev_uevent+0xb8/0x1ec uevent_show+0x8c/0x114 dev_attr_show+0x20/0x60 sysfs_kf_seq_show+0xa8/0x120 kernfs_seq_show+0x2c/0x40 seq_read_iter+0x1bc/0x4b0 kernfs_fop_read_iter+0x140/0x1d0 new_sync_read+0xd4/0x150 vfs_read+0x190/0x1dc ksys_read+0x68/0xfc __arm64_sys_read+0x20/0x30 invoke_syscall+0x48/0x114 el0_svc_common.constprop.0+0x60/0x11c do_el0_svc_compat+0x1c/0x50 el0_svc_compat+0x58/0x100 el0t_32_sync_handler+0x90/0x140 el0t_32_sync+0x190/0x194 > --- > > As far as I can tell, this bug has always been present. However, the > udc_lock mutex used by the patch was added in commit fc274c1e9973 > ("USB: gadget: Add a new bus for gadgets"), so this patch won't apply > to trees which don't include that commit or a backport of it. I don't > know what tag, if any, can express this requirement. > > > [as1983] > > > drivers/usb/gadget/udc/core.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > Index: usb-devel/drivers/usb/gadget/udc/core.c > =================================================================== > --- usb-devel.orig/drivers/usb/gadget/udc/core.c > +++ usb-devel/drivers/usb/gadget/udc/core.c > @@ -1728,13 +1728,14 @@ static int usb_udc_uevent(struct device > return ret; > } > > - if (udc->driver) { > + mutex_lock(&udc_lock); > + if (udc->driver) > ret = add_uevent_var(env, "USB_UDC_DRIVER=%s", > udc->driver->function); > - if (ret) { > - dev_err(dev, "failed to add uevent USB_UDC_DRIVER\n"); > - return ret; > - } > + mutex_unlock(&udc_lock); > + if (ret) { > + dev_err(dev, "failed to add uevent USB_UDC_DRIVER\n"); > + return ret; > } > > return 0; > Best regards
On Mon, Aug 08, 2022 at 04:57:35PM +0200, Marek Szyprowski wrote: > Hi Alan, Hi. > This patch landed recently in linux-next as commit 2191c00855b0 ("USB: > gadget: Fix use-after-free Read in usb_udc_uevent()"). Unfortunately it > fixes the issue by introducing another one. It doesn't look very > probable, but it would be nice to fix it to make the lock dependency > checker happy. Indeed. > ====================================================== > WARNING: possible circular locking dependency detected > 5.19.0-rc7+ #12510 Not tainted > ------------------------------------------------------ > udevadm/312 is trying to acquire lock: > ffff80000aae1058 (udc_lock){+.+.}-{3:3}, at: usb_udc_uevent+0x54/0xe0 > > but task is already holding lock: > ffff000002277548 (kn->active#4){++++}-{0:0}, at: kernfs_seq_start+0x34/0xe0 > > which lock already depends on the new lock. > > > the existing dependency chain (in reverse order) is: > > -> #3 (kn->active#4){++++}-{0:0}: > lock_acquire+0x68/0x84 > __kernfs_remove+0x268/0x380 > kernfs_remove_by_name_ns+0x58/0xac > sysfs_remove_file_ns+0x18/0x24 > device_del+0x15c/0x440 > device_link_drop_managed+0xa8/0xe0 > device_links_driver_bound+0x1b8/0x230 > driver_bound+0x68/0xc0 > really_probe.part.0+0x1f8/0x2ac > __driver_probe_device+0x98/0x144 > driver_probe_device+0xac/0x14c > __driver_attach+0x104/0x1b0 > bus_for_each_dev+0x70/0xd0 > driver_attach+0x24/0x30 > bus_add_driver+0x154/0x204 > driver_register+0x78/0x130 > __platform_driver_register+0x28/0x34 > simple_pm_bus_driver_init+0x1c/0x28 > do_one_initcall+0x74/0x400 > kernel_init_freeable+0x2f4/0x37c > kernel_init+0x28/0x130 > ret_from_fork+0x10/0x20 > > -> #2 (device_links_lock){+.+.}-{3:3}: > lock_acquire+0x68/0x84 > __mutex_lock+0x9c/0x430 > mutex_lock_nested+0x38/0x64 > device_link_remove+0x3c/0xa0 > _regulator_put.part.0+0x168/0x190 > regulator_put+0x3c/0x54 > devm_regulator_release+0x14/0x20 > release_nodes+0x5c/0x90 > devres_release_all+0x8c/0xe0 > device_unbind_cleanup+0x18/0x70 > really_probe.part.0+0x174/0x2ac > __driver_probe_device+0x98/0x144 > driver_probe_device+0xac/0x14c > __device_attach_driver+0xb8/0x120 > bus_for_each_drv+0x78/0xd0 > __device_attach_async_helper+0xb0/0xd4 > async_run_entry_fn+0x34/0xd0 > process_one_work+0x288/0x6bc > worker_thread+0x74/0x450 > kthread+0x118/0x11c > ret_from_fork+0x10/0x20 > > -> #1 (regulator_list_mutex){+.+.}-{3:3}: > lock_acquire+0x68/0x84 > __mutex_lock+0x9c/0x430 > mutex_lock_nested+0x38/0x64 > regulator_lock_dependent+0x54/0x284 > regulator_enable+0x34/0x80 > phy_power_on+0x24/0x130 > __dwc2_lowlevel_hw_enable+0x100/0x130 > dwc2_lowlevel_hw_enable+0x18/0x40 > dwc2_hsotg_udc_start+0x6c/0x2f0 > gadget_bind_driver+0x124/0x1f4 > really_probe.part.0+0x9c/0x2ac > __driver_probe_device+0x98/0x144 > driver_probe_device+0xac/0x14c > __device_attach_driver+0xb8/0x120 > bus_for_each_drv+0x78/0xd0 > __device_attach+0xa8/0x1c0 > device_initial_probe+0x14/0x20 > bus_probe_device+0x9c/0xa4 > device_add+0x3a0/0x890 > usb_add_gadget+0x170/0x200 > usb_add_gadget_udc+0x94/0xd4 > dwc2_driver_probe+0x580/0x78c > platform_probe+0x68/0xe0 > really_probe.part.0+0x9c/0x2ac > __driver_probe_device+0x98/0x144 > driver_probe_device+0xac/0x14c > __device_attach_driver+0xb8/0x120 > bus_for_each_drv+0x78/0xd0 > __device_attach+0xa8/0x1c0 > device_initial_probe+0x14/0x20 > bus_probe_device+0x9c/0xa4 > device_add+0x3a0/0x890 > of_device_add+0x48/0x6c > of_platform_device_create_pdata+0x98/0x100 > of_platform_bus_create+0x17c/0x37c > of_platform_populate+0x58/0xec > dwc3_meson_g12a_probe+0x314/0x5d0 > platform_probe+0x68/0xe0 > really_probe.part.0+0x9c/0x2ac > __driver_probe_device+0x98/0x144 > driver_probe_device+0xac/0x14c > __device_attach_driver+0xb8/0x120 > bus_for_each_drv+0x78/0xd0 > __device_attach+0xa8/0x1c0 > device_initial_probe+0x14/0x20 > bus_probe_device+0x9c/0xa4 > deferred_probe_work_func+0x88/0xc4 > process_one_work+0x288/0x6bc > worker_thread+0x74/0x450 > kthread+0x118/0x11c > ret_from_fork+0x10/0x20 > > -> #0 (udc_lock){+.+.}-{3:3}: > __lock_acquire+0x1298/0x20cc > lock_acquire.part.0+0xe0/0x230 > lock_acquire+0x68/0x84 > __mutex_lock+0x9c/0x430 > mutex_lock_nested+0x38/0x64 > usb_udc_uevent+0x54/0xe0 > dev_uevent+0xb8/0x1ec > uevent_show+0x8c/0x114 > dev_attr_show+0x20/0x60 > sysfs_kf_seq_show+0xa8/0x120 > kernfs_seq_show+0x2c/0x40 > seq_read_iter+0x1bc/0x4b0 > kernfs_fop_read_iter+0x140/0x1d0 > new_sync_read+0xd4/0x150 > vfs_read+0x190/0x1dc > ksys_read+0x68/0xfc > __arm64_sys_read+0x20/0x30 > invoke_syscall+0x48/0x114 > el0_svc_common.constprop.0+0x60/0x11c > do_el0_svc_compat+0x1c/0x50 > el0_svc_compat+0x58/0x100 > el0t_32_sync_handler+0x90/0x140 > el0t_32_sync+0x190/0x194 > > other info that might help us debug this: > > Chain exists of: > udc_lock --> device_links_lock --> kn->active#4 Peter, shouldn't this say: udc_lock --> regulator_list_mutex --> device_links_lock --> kn->active#4 ? Is that a bug in the lockdep reporting? It looks like this is a bad interaction between the udc_lock and the sysfs/kernfs locking. If a lock such as udc_lock is held (or is part of a chain) while a sysfs file is removed, the file's ->show or ->store routine better not acquire that lock. I suspect the problem is that udc_lock is held for too long. Probably it should be released during the calls to udc->driver->bind and udc->driver->unbind. Getting this right will require some careful study. Marek, if I send you a patch later, will you be able to test it? Alan Stern
Hi Alan, On 08.08.2022 22:26, Alan Stern wrote: > On Mon, Aug 08, 2022 at 04:57:35PM +0200, Marek Szyprowski wrote: >> This patch landed recently in linux-next as commit 2191c00855b0 ("USB: >> gadget: Fix use-after-free Read in usb_udc_uevent()"). Unfortunately it >> fixes the issue by introducing another one. It doesn't look very >> probable, but it would be nice to fix it to make the lock dependency >> checker happy. > Indeed. > >> ====================================================== >> WARNING: possible circular locking dependency detected >> 5.19.0-rc7+ #12510 Not tainted >> ------------------------------------------------------ >> udevadm/312 is trying to acquire lock: >> ffff80000aae1058 (udc_lock){+.+.}-{3:3}, at: usb_udc_uevent+0x54/0xe0 >> >> but task is already holding lock: >> ffff000002277548 (kn->active#4){++++}-{0:0}, at: kernfs_seq_start+0x34/0xe0 >> >> which lock already depends on the new lock. >> >> >> the existing dependency chain (in reverse order) is: >> >> -> #3 (kn->active#4){++++}-{0:0}: >> lock_acquire+0x68/0x84 >> __kernfs_remove+0x268/0x380 >> kernfs_remove_by_name_ns+0x58/0xac >> sysfs_remove_file_ns+0x18/0x24 >> device_del+0x15c/0x440 >> device_link_drop_managed+0xa8/0xe0 >> device_links_driver_bound+0x1b8/0x230 >> driver_bound+0x68/0xc0 >> really_probe.part.0+0x1f8/0x2ac >> __driver_probe_device+0x98/0x144 >> driver_probe_device+0xac/0x14c >> __driver_attach+0x104/0x1b0 >> bus_for_each_dev+0x70/0xd0 >> driver_attach+0x24/0x30 >> bus_add_driver+0x154/0x204 >> driver_register+0x78/0x130 >> __platform_driver_register+0x28/0x34 >> simple_pm_bus_driver_init+0x1c/0x28 >> do_one_initcall+0x74/0x400 >> kernel_init_freeable+0x2f4/0x37c >> kernel_init+0x28/0x130 >> ret_from_fork+0x10/0x20 >> >> -> #2 (device_links_lock){+.+.}-{3:3}: >> lock_acquire+0x68/0x84 >> __mutex_lock+0x9c/0x430 >> mutex_lock_nested+0x38/0x64 >> device_link_remove+0x3c/0xa0 >> _regulator_put.part.0+0x168/0x190 >> regulator_put+0x3c/0x54 >> devm_regulator_release+0x14/0x20 >> release_nodes+0x5c/0x90 >> devres_release_all+0x8c/0xe0 >> device_unbind_cleanup+0x18/0x70 >> really_probe.part.0+0x174/0x2ac >> __driver_probe_device+0x98/0x144 >> driver_probe_device+0xac/0x14c >> __device_attach_driver+0xb8/0x120 >> bus_for_each_drv+0x78/0xd0 >> __device_attach_async_helper+0xb0/0xd4 >> async_run_entry_fn+0x34/0xd0 >> process_one_work+0x288/0x6bc >> worker_thread+0x74/0x450 >> kthread+0x118/0x11c >> ret_from_fork+0x10/0x20 >> >> -> #1 (regulator_list_mutex){+.+.}-{3:3}: >> lock_acquire+0x68/0x84 >> __mutex_lock+0x9c/0x430 >> mutex_lock_nested+0x38/0x64 >> regulator_lock_dependent+0x54/0x284 >> regulator_enable+0x34/0x80 >> phy_power_on+0x24/0x130 >> __dwc2_lowlevel_hw_enable+0x100/0x130 >> dwc2_lowlevel_hw_enable+0x18/0x40 >> dwc2_hsotg_udc_start+0x6c/0x2f0 >> gadget_bind_driver+0x124/0x1f4 >> really_probe.part.0+0x9c/0x2ac >> __driver_probe_device+0x98/0x144 >> driver_probe_device+0xac/0x14c >> __device_attach_driver+0xb8/0x120 >> bus_for_each_drv+0x78/0xd0 >> __device_attach+0xa8/0x1c0 >> device_initial_probe+0x14/0x20 >> bus_probe_device+0x9c/0xa4 >> device_add+0x3a0/0x890 >> usb_add_gadget+0x170/0x200 >> usb_add_gadget_udc+0x94/0xd4 >> dwc2_driver_probe+0x580/0x78c >> platform_probe+0x68/0xe0 >> really_probe.part.0+0x9c/0x2ac >> __driver_probe_device+0x98/0x144 >> driver_probe_device+0xac/0x14c >> __device_attach_driver+0xb8/0x120 >> bus_for_each_drv+0x78/0xd0 >> __device_attach+0xa8/0x1c0 >> device_initial_probe+0x14/0x20 >> bus_probe_device+0x9c/0xa4 >> device_add+0x3a0/0x890 >> of_device_add+0x48/0x6c >> of_platform_device_create_pdata+0x98/0x100 >> of_platform_bus_create+0x17c/0x37c >> of_platform_populate+0x58/0xec >> dwc3_meson_g12a_probe+0x314/0x5d0 >> platform_probe+0x68/0xe0 >> really_probe.part.0+0x9c/0x2ac >> __driver_probe_device+0x98/0x144 >> driver_probe_device+0xac/0x14c >> __device_attach_driver+0xb8/0x120 >> bus_for_each_drv+0x78/0xd0 >> __device_attach+0xa8/0x1c0 >> device_initial_probe+0x14/0x20 >> bus_probe_device+0x9c/0xa4 >> deferred_probe_work_func+0x88/0xc4 >> process_one_work+0x288/0x6bc >> worker_thread+0x74/0x450 >> kthread+0x118/0x11c >> ret_from_fork+0x10/0x20 >> >> -> #0 (udc_lock){+.+.}-{3:3}: >> __lock_acquire+0x1298/0x20cc >> lock_acquire.part.0+0xe0/0x230 >> lock_acquire+0x68/0x84 >> __mutex_lock+0x9c/0x430 >> mutex_lock_nested+0x38/0x64 >> usb_udc_uevent+0x54/0xe0 >> dev_uevent+0xb8/0x1ec >> uevent_show+0x8c/0x114 >> dev_attr_show+0x20/0x60 >> sysfs_kf_seq_show+0xa8/0x120 >> kernfs_seq_show+0x2c/0x40 >> seq_read_iter+0x1bc/0x4b0 >> kernfs_fop_read_iter+0x140/0x1d0 >> new_sync_read+0xd4/0x150 >> vfs_read+0x190/0x1dc >> ksys_read+0x68/0xfc >> __arm64_sys_read+0x20/0x30 >> invoke_syscall+0x48/0x114 >> el0_svc_common.constprop.0+0x60/0x11c >> do_el0_svc_compat+0x1c/0x50 >> el0_svc_compat+0x58/0x100 >> el0t_32_sync_handler+0x90/0x140 >> el0t_32_sync+0x190/0x194 >> >> other info that might help us debug this: >> >> Chain exists of: >> udc_lock --> device_links_lock --> kn->active#4 > Peter, shouldn't this say: > > udc_lock --> regulator_list_mutex --> device_links_lock --> kn->active#4 > > ? Is that a bug in the lockdep reporting? > > It looks like this is a bad interaction between the udc_lock and the > sysfs/kernfs locking. If a lock such as udc_lock is held (or is part of > a chain) while a sysfs file is removed, the file's ->show or ->store > routine better not acquire that lock. > > I suspect the problem is that udc_lock is held for too long. Probably it > should be released during the calls to udc->driver->bind and > udc->driver->unbind. > > Getting this right will require some careful study. Marek, if I send you > a patch later, will you be able to test it? Yes, sure. I'm available till 12th August, then I'm going for holidays. Best regards
On Mon, Aug 08, 2022 at 04:26:49PM -0400, Alan Stern wrote: > On Mon, Aug 08, 2022 at 04:57:35PM +0200, Marek Szyprowski wrote: > > Hi Alan, > > Hi. > > > This patch landed recently in linux-next as commit 2191c00855b0 ("USB: > > gadget: Fix use-after-free Read in usb_udc_uevent()"). Unfortunately it > > fixes the issue by introducing another one. It doesn't look very > > probable, but it would be nice to fix it to make the lock dependency > > checker happy. > > Indeed. > I suspect the problem is that udc_lock is held for too long. Probably it > should be released during the calls to udc->driver->bind and > udc->driver->unbind. > > Getting this right will require some careful study. Marek, if I send you > a patch later, will you be able to test it? Here's a patch for you to try, when you have the chance. It reduces the scope of udc_lock to cover only the fields it's supposed to protect and changes the locking in a few other places. There's still the possibility of a locking cycle, because udc_lock is held in the ->disconnect pathway. It's very hard to know whether that might cause any trouble; it depends on how the function drivers handle disconnections. Alan Stern Index: usb-devel/drivers/usb/gadget/udc/core.c =================================================================== --- usb-devel.orig/drivers/usb/gadget/udc/core.c +++ usb-devel/drivers/usb/gadget/udc/core.c @@ -736,7 +736,10 @@ int usb_gadget_disconnect(struct usb_gad ret = gadget->ops->pullup(gadget, 0); if (!ret) { gadget->connected = 0; - gadget->udc->driver->disconnect(gadget); + mutex_lock(&udc_lock); + if (gadget->udc->driver) + gadget->udc->driver->disconnect(gadget); + mutex_unlock(&udc_lock); } out: @@ -1489,7 +1492,6 @@ static int gadget_bind_driver(struct dev usb_gadget_udc_set_speed(udc, driver->max_speed); - mutex_lock(&udc_lock); ret = driver->bind(udc->gadget, driver); if (ret) goto err_bind; @@ -1499,7 +1501,6 @@ static int gadget_bind_driver(struct dev goto err_start; usb_gadget_enable_async_callbacks(udc); usb_udc_connect_control(udc); - mutex_unlock(&udc_lock); kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); return 0; @@ -1512,6 +1513,7 @@ static int gadget_bind_driver(struct dev dev_err(&udc->dev, "failed to start %s: %d\n", driver->function, ret); + mutex_lock(&udc_lock); udc->driver = NULL; driver->is_bound = false; mutex_unlock(&udc_lock); @@ -1529,7 +1531,6 @@ static void gadget_unbind_driver(struct kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); - mutex_lock(&udc_lock); usb_gadget_disconnect(gadget); usb_gadget_disable_async_callbacks(udc); if (gadget->irq) @@ -1537,6 +1538,7 @@ static void gadget_unbind_driver(struct udc->driver->unbind(gadget); usb_gadget_udc_stop(udc); + mutex_lock(&udc_lock); driver->is_bound = false; udc->driver = NULL; mutex_unlock(&udc_lock); @@ -1612,7 +1614,7 @@ static ssize_t soft_connect_store(struct struct usb_udc *udc = container_of(dev, struct usb_udc, dev); ssize_t ret; - mutex_lock(&udc_lock); + device_lock(&udc->gadget->dev); if (!udc->driver) { dev_err(dev, "soft-connect without a gadget driver\n"); ret = -EOPNOTSUPP; @@ -1633,7 +1635,7 @@ static ssize_t soft_connect_store(struct ret = n; out: - mutex_unlock(&udc_lock); + device_unlock(&udc->gadget->dev); return ret; } static DEVICE_ATTR_WO(soft_connect); @@ -1652,11 +1654,15 @@ static ssize_t function_show(struct devi char *buf) { struct usb_udc *udc = container_of(dev, struct usb_udc, dev); - struct usb_gadget_driver *drv = udc->driver; + struct usb_gadget_driver *drv; + int rc = 0; - if (!drv || !drv->function) - return 0; - return scnprintf(buf, PAGE_SIZE, "%s\n", drv->function); + mutex_lock(&udc_lock); + drv = udc->driver; + if (drv && drv->function) + rc = scnprintf(buf, PAGE_SIZE, "%s\n", drv->function); + mutex_unlock(&udc_lock); + return rc; } static DEVICE_ATTR_RO(function);
Hi Alan, On 10.08.2022 21:33, Alan Stern wrote: > On Mon, Aug 08, 2022 at 04:26:49PM -0400, Alan Stern wrote: >> On Mon, Aug 08, 2022 at 04:57:35PM +0200, Marek Szyprowski wrote: >>> This patch landed recently in linux-next as commit 2191c00855b0 ("USB: >>> gadget: Fix use-after-free Read in usb_udc_uevent()"). Unfortunately it >>> fixes the issue by introducing another one. It doesn't look very >>> probable, but it would be nice to fix it to make the lock dependency >>> checker happy. >> Indeed. >> I suspect the problem is that udc_lock is held for too long. Probably it >> should be released during the calls to udc->driver->bind and >> udc->driver->unbind. >> >> Getting this right will require some careful study. Marek, if I send you >> a patch later, will you be able to test it? > Here's a patch for you to try, when you have the chance. It reduces the > scope of udc_lock to cover only the fields it's supposed to protect and > changes the locking in a few other places. > > There's still the possibility of a locking cycle, because udc_lock is > held in the ->disconnect pathway. It's very hard to know whether that > might cause any trouble; it depends on how the function drivers handle > disconnections. It looks this fixed the issue I've reported. I've checked it on all my test systems and none reported any issue related to the udc. Feel free to add: Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > Alan Stern > > > > Index: usb-devel/drivers/usb/gadget/udc/core.c > =================================================================== > --- usb-devel.orig/drivers/usb/gadget/udc/core.c > +++ usb-devel/drivers/usb/gadget/udc/core.c > @@ -736,7 +736,10 @@ int usb_gadget_disconnect(struct usb_gad > ret = gadget->ops->pullup(gadget, 0); > if (!ret) { > gadget->connected = 0; > - gadget->udc->driver->disconnect(gadget); > + mutex_lock(&udc_lock); > + if (gadget->udc->driver) > + gadget->udc->driver->disconnect(gadget); > + mutex_unlock(&udc_lock); > } > > out: > @@ -1489,7 +1492,6 @@ static int gadget_bind_driver(struct dev > > usb_gadget_udc_set_speed(udc, driver->max_speed); > > - mutex_lock(&udc_lock); > ret = driver->bind(udc->gadget, driver); > if (ret) > goto err_bind; > @@ -1499,7 +1501,6 @@ static int gadget_bind_driver(struct dev > goto err_start; > usb_gadget_enable_async_callbacks(udc); > usb_udc_connect_control(udc); > - mutex_unlock(&udc_lock); > > kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); > return 0; > @@ -1512,6 +1513,7 @@ static int gadget_bind_driver(struct dev > dev_err(&udc->dev, "failed to start %s: %d\n", > driver->function, ret); > > + mutex_lock(&udc_lock); > udc->driver = NULL; > driver->is_bound = false; > mutex_unlock(&udc_lock); > @@ -1529,7 +1531,6 @@ static void gadget_unbind_driver(struct > > kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); > > - mutex_lock(&udc_lock); > usb_gadget_disconnect(gadget); > usb_gadget_disable_async_callbacks(udc); > if (gadget->irq) > @@ -1537,6 +1538,7 @@ static void gadget_unbind_driver(struct > udc->driver->unbind(gadget); > usb_gadget_udc_stop(udc); > > + mutex_lock(&udc_lock); > driver->is_bound = false; > udc->driver = NULL; > mutex_unlock(&udc_lock); > @@ -1612,7 +1614,7 @@ static ssize_t soft_connect_store(struct > struct usb_udc *udc = container_of(dev, struct usb_udc, dev); > ssize_t ret; > > - mutex_lock(&udc_lock); > + device_lock(&udc->gadget->dev); > if (!udc->driver) { > dev_err(dev, "soft-connect without a gadget driver\n"); > ret = -EOPNOTSUPP; > @@ -1633,7 +1635,7 @@ static ssize_t soft_connect_store(struct > > ret = n; > out: > - mutex_unlock(&udc_lock); > + device_unlock(&udc->gadget->dev); > return ret; > } > static DEVICE_ATTR_WO(soft_connect); > @@ -1652,11 +1654,15 @@ static ssize_t function_show(struct devi > char *buf) > { > struct usb_udc *udc = container_of(dev, struct usb_udc, dev); > - struct usb_gadget_driver *drv = udc->driver; > + struct usb_gadget_driver *drv; > + int rc = 0; > > - if (!drv || !drv->function) > - return 0; > - return scnprintf(buf, PAGE_SIZE, "%s\n", drv->function); > + mutex_lock(&udc_lock); > + drv = udc->driver; > + if (drv && drv->function) > + rc = scnprintf(buf, PAGE_SIZE, "%s\n", drv->function); > + mutex_unlock(&udc_lock); > + return rc; > } > static DEVICE_ATTR_RO(function); > > Best regards
On Thu, Aug 11, 2022 at 09:31:34AM +0200, Marek Szyprowski wrote: > Hi Alan, > > On 10.08.2022 21:33, Alan Stern wrote: > > On Mon, Aug 08, 2022 at 04:26:49PM -0400, Alan Stern wrote: > >> On Mon, Aug 08, 2022 at 04:57:35PM +0200, Marek Szyprowski wrote: > >>> This patch landed recently in linux-next as commit 2191c00855b0 ("USB: > >>> gadget: Fix use-after-free Read in usb_udc_uevent()"). Unfortunately it > >>> fixes the issue by introducing another one. It doesn't look very > >>> probable, but it would be nice to fix it to make the lock dependency > >>> checker happy. > >> Indeed. > >> I suspect the problem is that udc_lock is held for too long. Probably it > >> should be released during the calls to udc->driver->bind and > >> udc->driver->unbind. > >> > >> Getting this right will require some careful study. Marek, if I send you > >> a patch later, will you be able to test it? > > Here's a patch for you to try, when you have the chance. It reduces the > > scope of udc_lock to cover only the fields it's supposed to protect and > > changes the locking in a few other places. > > > > There's still the possibility of a locking cycle, because udc_lock is > > held in the ->disconnect pathway. It's very hard to know whether that > > might cause any trouble; it depends on how the function drivers handle > > disconnections. > > It looks this fixed the issue I've reported. I've checked it on all my > test systems and none reported any issue related to the udc. > > Feel free to add: > > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> > > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> Thanks for the quick testing. I'll submit the patch when the current merge window ends. Alan Stern
On 11.08.2022 18:06, Alan Stern wrote: > On Thu, Aug 11, 2022 at 09:31:34AM +0200, Marek Szyprowski wrote: >> On 10.08.2022 21:33, Alan Stern wrote: >>> On Mon, Aug 08, 2022 at 04:26:49PM -0400, Alan Stern wrote: >>>> On Mon, Aug 08, 2022 at 04:57:35PM +0200, Marek Szyprowski wrote: >>>>> This patch landed recently in linux-next as commit 2191c00855b0 ("USB: >>>>> gadget: Fix use-after-free Read in usb_udc_uevent()"). Unfortunately it >>>>> fixes the issue by introducing another one. It doesn't look very >>>>> probable, but it would be nice to fix it to make the lock dependency >>>>> checker happy. >>>> Indeed. >>>> I suspect the problem is that udc_lock is held for too long. Probably it >>>> should be released during the calls to udc->driver->bind and >>>> udc->driver->unbind. >>>> >>>> Getting this right will require some careful study. Marek, if I send you >>>> a patch later, will you be able to test it? >>> Here's a patch for you to try, when you have the chance. It reduces the >>> scope of udc_lock to cover only the fields it's supposed to protect and >>> changes the locking in a few other places. >>> >>> There's still the possibility of a locking cycle, because udc_lock is >>> held in the ->disconnect pathway. It's very hard to know whether that >>> might cause any trouble; it depends on how the function drivers handle >>> disconnections. >> It looks this fixed the issue I've reported. I've checked it on all my >> test systems and none reported any issue related to the udc. >> >> Feel free to add: >> >> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> >> >> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > Thanks for the quick testing. I'll submit the patch when the current > merge window ends. Gentle ping for the final patch... Best regards
On Fri, Aug 26, 2022 at 08:30:50AM +0200, Marek Szyprowski wrote: > On 11.08.2022 18:06, Alan Stern wrote: > > On Thu, Aug 11, 2022 at 09:31:34AM +0200, Marek Szyprowski wrote: > >> On 10.08.2022 21:33, Alan Stern wrote: > >>> On Mon, Aug 08, 2022 at 04:26:49PM -0400, Alan Stern wrote: > >>>> On Mon, Aug 08, 2022 at 04:57:35PM +0200, Marek Szyprowski wrote: > >>>>> This patch landed recently in linux-next as commit 2191c00855b0 ("USB: > >>>>> gadget: Fix use-after-free Read in usb_udc_uevent()"). Unfortunately it > >>>>> fixes the issue by introducing another one. It doesn't look very > >>>>> probable, but it would be nice to fix it to make the lock dependency > >>>>> checker happy. > >>>> Indeed. > >>>> I suspect the problem is that udc_lock is held for too long. Probably it > >>>> should be released during the calls to udc->driver->bind and > >>>> udc->driver->unbind. > >>>> > >>>> Getting this right will require some careful study. Marek, if I send you > >>>> a patch later, will you be able to test it? > >>> Here's a patch for you to try, when you have the chance. It reduces the > >>> scope of udc_lock to cover only the fields it's supposed to protect and > >>> changes the locking in a few other places. > >>> > >>> There's still the possibility of a locking cycle, because udc_lock is > >>> held in the ->disconnect pathway. It's very hard to know whether that > >>> might cause any trouble; it depends on how the function drivers handle > >>> disconnections. > >> It looks this fixed the issue I've reported. I've checked it on all my > >> test systems and none reported any issue related to the udc. > >> > >> Feel free to add: > >> > >> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> > >> > >> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > > Thanks for the quick testing. I'll submit the patch when the current > > merge window ends. > > Gentle ping for the final patch... On its way. It turns out that the hardest part of this patch is writing up the descriptions and justifications in the Changelog. :-( (Part of the reason for the delay is that I usually wait until the -rc2 or -rc3 release before starting to work on a new kernel version...) I think in the end there is more work still to do. In particular, the gadget core doesn't seem to be careful about not connecting (i.e., not turning on the D+ pullup) when no driver is bound to the gadget. Those actions need to be synchronized somehow. Alan Stern
On Wed, Aug 10, 2022 at 03:33:00PM -0400, Alan Stern wrote: > On Mon, Aug 08, 2022 at 04:26:49PM -0400, Alan Stern wrote: > > On Mon, Aug 08, 2022 at 04:57:35PM +0200, Marek Szyprowski wrote: > > > Hi Alan, > > > > Hi. > > > > > This patch landed recently in linux-next as commit 2191c00855b0 ("USB: > > > gadget: Fix use-after-free Read in usb_udc_uevent()"). Unfortunately it > > > fixes the issue by introducing another one. It doesn't look very > > > probable, but it would be nice to fix it to make the lock dependency > > > checker happy. Hi just bisected a different warning [1], triggered by the same patch and I was about to send a revert while I noticed you already have a fix more or less ready, it would be great if you could send it. Please find my tested-by afterward. [1] https://lore.kernel.org/all/20220901122129.GA493609@francesco-nb.int.toradex.com/ [ 25.942837] ====================================================== [ 25.942845] WARNING: possible circular locking dependency detected [ 25.942854] 6.0.0-rc3-0.0.0-devel+git.b90cb1053190 #1 Not tainted [ 25.942866] ------------------------------------------------------ [ 25.942873] connmand/667 is trying to acquire lock: [ 25.942887] c2b03348 (kn->active#9){++++}-{0:0}, at: kernfs_remove_by_name_ns+0x50/0xa0 [ 25.942948] but task is already holding lock: [ 25.942956] c17af6a0 (rtnl_mutex){+.+.}-{3:3}, at: devinet_ioctl+0xb8/0x8e0 [ 25.942996] which lock already depends on the new lock. [ 25.943003] the existing dependency chain (in reverse order) is: [ 25.943010] -> #2 (rtnl_mutex){+.+.}-{3:3}: [ 25.943039] mutex_lock_killable_nested+0x1c/0x28 [ 25.943058] register_netdev+0xc/0x34 [ 25.943072] gether_register_netdev+0x38/0xb0 [ 25.943087] rndis_bind+0x230/0x3a0 [ 25.943102] usb_add_function+0x7c/0x1d4 [ 25.943119] configfs_composite_bind+0x1c0/0x360 [ 25.943133] gadget_bind_driver+0x9c/0x208 [ 25.943148] really_probe+0xd8/0x410 [ 25.943165] __driver_probe_device+0x94/0x204 [ 25.943180] driver_probe_device+0x2c/0xc4 [ 25.943195] __driver_attach+0xe0/0x1f0 [ 25.943209] bus_for_each_dev+0x70/0xb0 [ 25.943224] bus_add_driver+0x164/0x218 [ 25.943238] driver_register+0x88/0x11c [ 25.943253] usb_gadget_register_driver_owner+0x40/0xd4 [ 25.943267] gadget_dev_desc_UDC_store+0xd4/0x110 [ 25.943281] configfs_write_iter+0xac/0x118 [ 25.943295] vfs_write+0x2c4/0x46c [ 25.943312] ksys_write+0x5c/0xd4 [ 25.943326] ret_fast_syscall+0x0/0x1c [ 25.943340] 0xbe9deb88 [ 25.943352] -> #1 (udc_lock){+.+.}-{3:3}: [ 25.943381] mutex_lock_nested+0x1c/0x24 [ 25.943396] usb_udc_uevent+0x34/0xb0 [ 25.943409] dev_uevent+0xfc/0x2cc [ 25.943423] uevent_show+0x90/0x10c [ 25.943436] dev_attr_show+0x18/0x48 [ 25.943453] sysfs_kf_seq_show+0x88/0x118 [ 25.943465] seq_read_iter+0x1a4/0x4d4 [ 25.943479] vfs_read+0x1bc/0x288 [ 25.943493] ksys_read+0x5c/0xd4 [ 25.943508] ret_fast_syscall+0x0/0x1c [ 25.943520] 0xbeb5d840 [ 25.943530] -> #0 (kn->active#9){++++}-{0:0}: [ 25.943564] lock_acquire+0xf4/0x364 [ 25.943581] __kernfs_remove+0x330/0x410 [ 25.943595] kernfs_remove_by_name_ns+0x50/0xa0 [ 25.943611] device_del+0x158/0x3dc [ 25.943623] device_unregister+0x20/0x64 [ 25.943636] wakeup_source_unregister.part.0+0x20/0x54 [ 25.943655] device_set_wakeup_enable+0x54/0x64 [ 25.943668] fec_enet_open+0x2ec/0x394 [ 25.943684] __dev_open+0xe0/0x1a0 [ 25.943696] __dev_change_flags+0x180/0x214 [ 25.943708] dev_change_flags+0x14/0x44 [ 25.943720] devinet_ioctl+0x878/0x8e0 [ 25.943733] inet_ioctl+0x180/0x238 [ 25.943746] sock_ioctl+0x4a0/0x5b4 [ 25.943760] sys_ioctl+0x530/0xdbc [ 25.943775] ret_fast_syscall+0x0/0x1c [ 25.943787] 0xbe91d960 [ 25.943797] other info that might help us debug this: [ 25.943804] Chain exists of: kn->active#9 --> udc_lock --> rtnl_mutex [ 25.943844] Possible unsafe locking scenario: [ 25.943851] CPU0 CPU1 [ 25.943857] ---- ---- [ 25.943863] lock(rtnl_mutex); [ 25.943879] lock(udc_lock); [ 25.943894] lock(rtnl_mutex); [ 25.943910] lock(kn->active#9); [ 25.943930] *** DEADLOCK *** [ 25.943937] 1 lock held by connmand/667: [ 25.943947] #0: c17af6a0 (rtnl_mutex){+.+.}-{3:3}, at: devinet_ioctl+0xb8/0x8e0 [ 25.943989] stack backtrace: [ 25.943997] CPU: 1 PID: 667 Comm: connmand Not tainted 6.0.0-rc3-0.0.0-devel+git.b90cb1053190 #1 [ 25.944012] Hardware name: Freescale i.MX7 Dual (Device Tree) [ 25.944025] unwind_backtrace from show_stack+0x10/0x14 [ 25.944049] show_stack from dump_stack_lvl+0x58/0x70 [ 25.944071] dump_stack_lvl from check_noncircular+0xe8/0x158 [ 25.944094] check_noncircular from __lock_acquire+0x1510/0x288c [ 25.944115] __lock_acquire from lock_acquire+0xf4/0x364 [ 25.944135] lock_acquire from __kernfs_remove+0x330/0x410 [ 25.944157] __kernfs_remove from kernfs_remove_by_name_ns+0x50/0xa0 [ 25.944179] kernfs_remove_by_name_ns from device_del+0x158/0x3dc [ 25.944199] device_del from device_unregister+0x20/0x64 [ 25.944216] device_unregister from wakeup_source_unregister.part.0+0x20/0x54 [ 25.944238] wakeup_source_unregister.part.0 from device_set_wakeup_enable+0x54/0x64 [ 25.944259] device_set_wakeup_enable from fec_enet_open+0x2ec/0x394 [ 25.944279] fec_enet_open from __dev_open+0xe0/0x1a0 [ 25.944298] __dev_open from __dev_change_flags+0x180/0x214 [ 25.944314] __dev_change_flags from dev_change_flags+0x14/0x44 [ 25.944331] dev_change_flags from devinet_ioctl+0x878/0x8e0 [ 25.944348] devinet_ioctl from inet_ioctl+0x180/0x238 [ 25.944365] inet_ioctl from sock_ioctl+0x4a0/0x5b4 [ 25.944383] sock_ioctl from sys_ioctl+0x530/0xdbc [ 25.944401] sys_ioctl from ret_fast_syscall+0x0/0x1c [ 25.944419] Exception stack(0xe14e9fa8 to 0xe14e9ff0) [ 25.944435] 9fa0: 00000000 be91d984 00000010 00008914 be91d984 be91d978 [ 25.944450] 9fc0: 00000000 be91d984 00000010 00000036 00000003 00001002 00000b20 be91db3c [ 25.944462] 9fe0: 00000036 be91d960 b6ba8189 b6b21ae6 > > > I suspect the problem is that udc_lock is held for too long. Probably it > > should be released during the calls to udc->driver->bind and > > udc->driver->unbind. > > > > Getting this right will require some careful study. Marek, if I send you > > a patch later, will you be able to test it? > > Here's a patch for you to try, when you have the chance. It reduces the > scope of udc_lock to cover only the fields it's supposed to protect and > changes the locking in a few other places. > > Index: usb-devel/drivers/usb/gadget/udc/core.c > =================================================================== > --- usb-devel.orig/drivers/usb/gadget/udc/core.c > +++ usb-devel/drivers/usb/gadget/udc/core.c > @@ -736,7 +736,10 @@ int usb_gadget_disconnect(struct usb_gad > ret = gadget->ops->pullup(gadget, 0); > if (!ret) { > gadget->connected = 0; > - gadget->udc->driver->disconnect(gadget); > + mutex_lock(&udc_lock); > + if (gadget->udc->driver) > + gadget->udc->driver->disconnect(gadget); > + mutex_unlock(&udc_lock); > } > > out: > @@ -1489,7 +1492,6 @@ static int gadget_bind_driver(struct dev > > usb_gadget_udc_set_speed(udc, driver->max_speed); > > - mutex_lock(&udc_lock); > ret = driver->bind(udc->gadget, driver); > if (ret) > goto err_bind; > @@ -1499,7 +1501,6 @@ static int gadget_bind_driver(struct dev > goto err_start; > usb_gadget_enable_async_callbacks(udc); > usb_udc_connect_control(udc); > - mutex_unlock(&udc_lock); > > kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); > return 0; > @@ -1512,6 +1513,7 @@ static int gadget_bind_driver(struct dev > dev_err(&udc->dev, "failed to start %s: %d\n", > driver->function, ret); > > + mutex_lock(&udc_lock); > udc->driver = NULL; > driver->is_bound = false; > mutex_unlock(&udc_lock); > @@ -1529,7 +1531,6 @@ static void gadget_unbind_driver(struct > > kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); > > - mutex_lock(&udc_lock); > usb_gadget_disconnect(gadget); > usb_gadget_disable_async_callbacks(udc); > if (gadget->irq) > @@ -1537,6 +1538,7 @@ static void gadget_unbind_driver(struct > udc->driver->unbind(gadget); > usb_gadget_udc_stop(udc); > > + mutex_lock(&udc_lock); > driver->is_bound = false; > udc->driver = NULL; > mutex_unlock(&udc_lock); > @@ -1612,7 +1614,7 @@ static ssize_t soft_connect_store(struct > struct usb_udc *udc = container_of(dev, struct usb_udc, dev); > ssize_t ret; > > - mutex_lock(&udc_lock); > + device_lock(&udc->gadget->dev); > if (!udc->driver) { > dev_err(dev, "soft-connect without a gadget driver\n"); > ret = -EOPNOTSUPP; > @@ -1633,7 +1635,7 @@ static ssize_t soft_connect_store(struct > > ret = n; > out: > - mutex_unlock(&udc_lock); > + device_unlock(&udc->gadget->dev); > return ret; > } > static DEVICE_ATTR_WO(soft_connect); > @@ -1652,11 +1654,15 @@ static ssize_t function_show(struct devi > char *buf) > { > struct usb_udc *udc = container_of(dev, struct usb_udc, dev); > - struct usb_gadget_driver *drv = udc->driver; > + struct usb_gadget_driver *drv; > + int rc = 0; > > - if (!drv || !drv->function) > - return 0; > - return scnprintf(buf, PAGE_SIZE, "%s\n", drv->function); > + mutex_lock(&udc_lock); > + drv = udc->driver; > + if (drv && drv->function) > + rc = scnprintf(buf, PAGE_SIZE, "%s\n", drv->function); > + mutex_unlock(&udc_lock); > + return rc; > } > static DEVICE_ATTR_RO(function); > Tested-by: Francesco Dolcini <francesco.dolcini@toradex.com> Francesco
On Thu, Sep 01, 2022 at 09:22:04PM +0200, Francesco Dolcini wrote: > On Wed, Aug 10, 2022 at 03:33:00PM -0400, Alan Stern wrote: > > On Mon, Aug 08, 2022 at 04:26:49PM -0400, Alan Stern wrote: > > > On Mon, Aug 08, 2022 at 04:57:35PM +0200, Marek Szyprowski wrote: > > > > Hi Alan, > > > > > > Hi. > > > > > > > This patch landed recently in linux-next as commit 2191c00855b0 ("USB: > > > > gadget: Fix use-after-free Read in usb_udc_uevent()"). Unfortunately it > > > > fixes the issue by introducing another one. It doesn't look very > > > > probable, but it would be nice to fix it to make the lock dependency > > > > checker happy. > > Hi just bisected a different warning [1], triggered by the same patch and I > was about to send a revert while I noticed you already have a fix more > or less ready, it would be great if you could send it. Please find my > tested-by afterward. It has already been submitted: https://lore.kernel.org/linux-usb/YwkfhdxA%2FI2nOcK7@rowland.harvard.edu/ Greg hasn't merged it yet, though. Alan Stern
Index: usb-devel/drivers/usb/gadget/udc/core.c =================================================================== --- usb-devel.orig/drivers/usb/gadget/udc/core.c +++ usb-devel/drivers/usb/gadget/udc/core.c @@ -1728,13 +1728,14 @@ static int usb_udc_uevent(struct device return ret; } - if (udc->driver) { + mutex_lock(&udc_lock); + if (udc->driver) ret = add_uevent_var(env, "USB_UDC_DRIVER=%s", udc->driver->function); - if (ret) { - dev_err(dev, "failed to add uevent USB_UDC_DRIVER\n"); - return ret; - } + mutex_unlock(&udc_lock); + if (ret) { + dev_err(dev, "failed to add uevent USB_UDC_DRIVER\n"); + return ret; } return 0;
The syzbot fuzzer found a race between uevent callbacks and gadget driver unregistration that can cause a use-after-free bug: --------------------------------------------------------------- BUG: KASAN: use-after-free in usb_udc_uevent+0x11f/0x130 drivers/usb/gadget/udc/core.c:1732 Read of size 8 at addr ffff888078ce2050 by task udevd/2968 CPU: 1 PID: 2968 Comm: udevd Not tainted 5.19.0-rc4-next-20220628-syzkaller #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 06/29/2022 Call Trace: <TASK> __dump_stack lib/dump_stack.c:88 [inline] dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106 print_address_description mm/kasan/report.c:317 [inline] print_report.cold+0x2ba/0x719 mm/kasan/report.c:433 kasan_report+0xbe/0x1f0 mm/kasan/report.c:495 usb_udc_uevent+0x11f/0x130 drivers/usb/gadget/udc/core.c:1732 dev_uevent+0x290/0x770 drivers/base/core.c:2424 --------------------------------------------------------------- The bug occurs because usb_udc_uevent() dereferences udc->driver but does so without acquiring the udc_lock mutex, which protects this field. If the gadget driver is unbound from the udc concurrently with uevent processing, the driver structure may be accessed after it has been deallocated. To prevent the race, we make sure that the routine holds the mutex around the racing accesses. Signed-off-by: Alan Stern <stern@rowland.harvard.edu> Reported-and-tested-by: syzbot+b0de012ceb1e2a97891b@syzkaller.appspotmail.com CC: stable@vger.kernel.org Link: <https://lore.kernel.org/all/0000000000004de90405a719c951@google.com> --- As far as I can tell, this bug has always been present. However, the udc_lock mutex used by the patch was added in commit fc274c1e9973 ("USB: gadget: Add a new bus for gadgets"), so this patch won't apply to trees which don't include that commit or a backport of it. I don't know what tag, if any, can express this requirement. [as1983] drivers/usb/gadget/udc/core.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)