diff mbox series

USB: gadget: Fix use-after-free Read in usb_udc_uevent()

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

Commit Message

Alan Stern July 21, 2022, 3:07 p.m. UTC
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(-)

Comments

Greg Kroah-Hartman July 27, 2022, 12:17 p.m. UTC | #1
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
Marek Szyprowski Aug. 8, 2022, 2:57 p.m. UTC | #2
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
Alan Stern Aug. 8, 2022, 8:26 p.m. UTC | #3
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
Marek Szyprowski Aug. 9, 2022, 6:29 a.m. UTC | #4
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
Alan Stern Aug. 10, 2022, 7:33 p.m. UTC | #5
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);
Marek Szyprowski Aug. 11, 2022, 7:31 a.m. UTC | #6
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
Alan Stern Aug. 11, 2022, 4:06 p.m. UTC | #7
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
Marek Szyprowski Aug. 26, 2022, 6:30 a.m. UTC | #8
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
Alan Stern Aug. 26, 2022, 2:50 p.m. UTC | #9
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
Francesco Dolcini Sept. 1, 2022, 7:22 p.m. UTC | #10
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
Alan Stern Sept. 1, 2022, 7:29 p.m. UTC | #11
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
diff mbox series

Patch

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;