ACK/Cmnt: [SRU][OEM-5.17][PATCH 1/1] USB: core: Prevent nested device-reset calls

Thu Apr 6 10:00:18 UTC 2023

On 23/04/05 05:49PM, Magali Lemes wrote:
> From: Alan Stern <stern at rowland.harvard.edu>
> 
> Automatic kernel fuzzing revealed a recursive locking violation in
> usb-storage:
> 
> ============================================
> WARNING: possible recursive locking detected
> 5.18.0 #3 Not tainted
> --------------------------------------------
> kworker/1:3/1205 is trying to acquire lock:
> ffff888018638db8 (&us_interface_key[i]){+.+.}-{3:3}, at:
> usb_stor_pre_reset+0x35/0x40 drivers/usb/storage/usb.c:230
> 
> but task is already holding lock:
> ffff888018638db8 (&us_interface_key[i]){+.+.}-{3:3}, at:
> usb_stor_pre_reset+0x35/0x40 drivers/usb/storage/usb.c:230
> 
> ...
> 
> stack backtrace:
> CPU: 1 PID: 1205 Comm: kworker/1:3 Not tainted 5.18.0 #3
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> 1.13.0-1ubuntu1.1 04/01/2014
> Workqueue: usb_hub_wq hub_event
> Call Trace:
> <TASK>
> __dump_stack lib/dump_stack.c:88 [inline]
> dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
> print_deadlock_bug kernel/locking/lockdep.c:2988 [inline]
> check_deadlock kernel/locking/lockdep.c:3031 [inline]
> validate_chain kernel/locking/lockdep.c:3816 [inline]
> __lock_acquire.cold+0x152/0x3ca kernel/locking/lockdep.c:5053
> lock_acquire kernel/locking/lockdep.c:5665 [inline]
> lock_acquire+0x1ab/0x520 kernel/locking/lockdep.c:5630
> __mutex_lock_common kernel/locking/mutex.c:603 [inline]
> __mutex_lock+0x14f/0x1610 kernel/locking/mutex.c:747
> usb_stor_pre_reset+0x35/0x40 drivers/usb/storage/usb.c:230
> usb_reset_device+0x37d/0x9a0 drivers/usb/core/hub.c:6109
> r871xu_dev_remove+0x21a/0x270 drivers/staging/rtl8712/usb_intf.c:622
> usb_unbind_interface+0x1bd/0x890 drivers/usb/core/driver.c:458
> device_remove drivers/base/dd.c:545 [inline]
> device_remove+0x11f/0x170 drivers/base/dd.c:537
> __device_release_driver drivers/base/dd.c:1222 [inline]
> device_release_driver_internal+0x1a7/0x2f0 drivers/base/dd.c:1248
> usb_driver_release_interface+0x102/0x180 drivers/usb/core/driver.c:627
> usb_forced_unbind_intf+0x4d/0xa0 drivers/usb/core/driver.c:1118
> usb_reset_device+0x39b/0x9a0 drivers/usb/core/hub.c:6114
> 
> This turned out not to be an error in usb-storage but rather a nested
> device reset attempt.  That is, as the rtl8712 driver was being
> unbound from a composite device in preparation for an unrelated USB
> reset (that driver does not have pre_reset or post_reset callbacks),
> its ->remove routine called usb_reset_device() -- thus nesting one
> reset call within another.
> 
> Performing a reset as part of disconnect processing is a questionable
> practice at best.  However, the bug report points out that the USB
> core does not have any protection against nested resets.  Adding a
> reset_in_progress flag and testing it will prevent such errors in the
> future.
> 
> Link: https://lore.kernel.org/all/CAB7eexKUpvX-JNiLzhXBDWgfg2T9e9_0Tw4HQ6keN==voRbP0g@mail.gmail.com/
> Cc: stable at vger.kernel.org
> Reported-and-tested-by: Rondreis <linhaoguo86 at gmail.com>
> Signed-off-by: Alan Stern <stern at rowland.harvard.edu>
> Link: https://lore.kernel.org/r/YwkflDxvg0KWqyZK@rowland.harvard.edu
> Signed-off-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> 

This is a NIT (and can easily be addressed if required when the patch is
applied): there is a superfluous newline here.

> CVE-2022-4662
> (cherry picked from commit 9c6d778800b921bde3bff3cff5003d1650f942d1)
> Signed-off-by: Magali Lemes <magali.lemes at canonical.com>

Acked-by: Andrei Gherzan <andrei.gherzan at canonical.com>

> ---
>  drivers/usb/core/hub.c | 10 ++++++++++
>  include/linux/usb.h    |  2 ++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 4a585b38917f..2bb4f8150f18 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -6091,6 +6091,11 @@ static int usb_reset_and_verify_device(struct usb_device *udev)
>   * the reset is over (using their post_reset method).
>   *
>   * Return: The same as for usb_reset_and_verify_device().
> + * However, if a reset is already in progress (for instance, if a
> + * driver doesn't have pre_ or post_reset() callbacks, and while
> + * being unbound or re-bound during the ongoing reset its disconnect()
> + * or probe() routine tries to perform a second, nested reset), the
> + * routine returns -EINPROGRESS.
>   *
>   * Note:
>   * The caller must own the device lock.  For example, it's safe to use
> @@ -6124,6 +6129,10 @@ int usb_reset_device(struct usb_device *udev)
>  		return -EISDIR;
>  	}
>  
> +	if (udev->reset_in_progress)
> +		return -EINPROGRESS;
> +	udev->reset_in_progress = 1;
> +
>  	port_dev = hub->ports[udev->portnum - 1];
>  
>  	/*
> @@ -6188,6 +6197,7 @@ int usb_reset_device(struct usb_device *udev)
>  
>  	usb_autosuspend_device(udev);
>  	memalloc_noio_restore(noio_flag);
> +	udev->reset_in_progress = 0;
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(usb_reset_device);
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index 200b7b79acb5..7fdfe477442f 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -575,6 +575,7 @@ struct usb3_lpm_parameters {
>   * @devaddr: device address, XHCI: assigned by HW, others: same as devnum
>   * @can_submit: URBs may be submitted
>   * @persist_enabled:  USB_PERSIST enabled for this device
> + * @reset_in_progress: the device is being reset
>   * @have_langid: whether string_langid is valid
>   * @authorized: policy has said we can use it;
>   *	(user space) policy determines if we authorize this device to be
> @@ -661,6 +662,7 @@ struct usb_device {
>  
>  	unsigned can_submit:1;
>  	unsigned persist_enabled:1;
> +	unsigned reset_in_progress:1;
>  	unsigned have_langid:1;
>  	unsigned authorized:1;
>  	unsigned authenticated:1;
> -- 
> 2.34.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

-- 
Andrei Gherzan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20230406/4595dfb0/attachment-0001.sig>