[PATCH] usb: storage: don't hold dev_mutex across the max_lun control transfer

[PATCH] usb: storage: don't hold dev_mutex across the max_lun control transfer

[Shuai Liu]

usb_stor_scan_dwork() acquires us->dev_mutex and then calls
usb_stor_Bulk_max_lun(), which issues a synchronous GET_MAX_LUN control
transfer (usb_stor_control_msg -> usb_control_msg, 10s timeout).  The
reset and suspend callbacks (usb_stor_pre_reset / usb_stor_suspend) also
acquire us->dev_mutex, but they run while the USB core already holds the
device lock (udev->dev.mutex) during usb_reset_device() or disconnect.
This creates an AB-BA deadlock:

  scan work           reset/disconnect
  ---------           ----------------
  mutex_lock(dev_mutex)
                      device_lock(udev)      [USB core]
  usb_control_msg() -- blocks waiting for
                        the device lock
                      mutex_lock(dev_mutex)  [pre_reset]
                        -- blocks forever

syzbot reports this as a task hung in usb_stor_scan_dwork(), with
cancel_delayed_work_sync() in quiesce_and_remove_host() unable to make
progress.

Fix it by making usb_stor_Bulk_max_lun() self-contained: use a private
on-stack buffer and the usbcore usb_control_msg() helper instead of the
shared per-device resources (us->iobuf, us->cr, us->current_urb) that
also belong to the control thread and the reset path.  With no shared
state touched, the transfer no longer needs us->dev_mutex.

In usb_stor_scan_dwork() call usb_stor_Bulk_max_lun() outside the lock
and only take us->dev_mutex briefly to publish us->max_lun (and the
SCSI host max_lun).  us->recv_ctrl_pipe and us->ifnum are set during
probe and never change, so they are safe to read lockless.

Reported-by: syzbot+30552b4cbe99d6d91306@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=30552b4cbe99d6d91306
Signed-off-by: Shuai Liu <847807867@qq.com>
---
 drivers/usb/storage/transport.c | 27 +++++++++++++++++----------
 drivers/usb/storage/usb.c       | 20 ++++++++++++++++----
 2 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c
index 8de3015bce05..961595a6faca 100644
--- a/drivers/usb/storage/transport.c
+++ b/drivers/usb/storage/transport.c
@@ -1075,27 +1075,34 @@ EXPORT_SYMBOL_GPL(usb_stor_CB_transport);
 /* Determine what the maximum LUN supported is */
 int usb_stor_Bulk_max_lun(struct us_data *us)
 {
+	u8 lun;
 	int result;
 
-	/* issue the command */
-	us->iobuf[0] = 0;
-	result = usb_stor_control_msg(us, us->recv_ctrl_pipe,
-				 US_BULK_GET_MAX_LUN, 
-				 USB_DIR_IN | USB_TYPE_CLASS | 
+	/*
+	 * Issue the GET_MAX_LUN request using a private buffer and the
+	 * usbcore control-message helper.  This avoids touching the shared
+	 * per-device resources (us->iobuf, us->cr, us->current_urb) that are
+	 * also used by the control thread and the reset path, so the caller
+	 * does not need to hold us->dev_mutex while this (possibly sleeping)
+	 * transfer runs.
+	 */
+	result = usb_control_msg(us->pusb_dev, us->recv_ctrl_pipe,
+				 US_BULK_GET_MAX_LUN,
+				 USB_DIR_IN | USB_TYPE_CLASS |
 				 USB_RECIP_INTERFACE,
-				 0, us->ifnum, us->iobuf, 1, 10*HZ);
+				 0, us->ifnum, &lun, 1, 10 * HZ);
 
 	usb_stor_dbg(us, "GetMaxLUN command result is %d, data is %d\n",
-		     result, us->iobuf[0]);
+		     result, lun);
 
 	/* If we have a successful request, return the result if valid. */
 	if (result > 0) {
-		if (us->iobuf[0] <= US_BULK_MAX_LUN_LIMIT) {
-			return us->iobuf[0];
+		if (lun <= US_BULK_MAX_LUN_LIMIT) {
+			return lun;
 		} else {
 			dev_info(&us->pusb_intf->dev,
 				 "Max LUN %d is not valid, using 0 instead",
-				 us->iobuf[0]);
+				 lun);
 		}
 	}
 
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index 823a4b2e9033..43d333f3ec4f 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -980,19 +980,31 @@ static void usb_stor_scan_dwork(struct work_struct *work)
 
 	dev_dbg(dev, "starting scan\n");
 
-	/* For bulk-only devices, determine the max LUN value */
+	/*
+	 * For bulk-only devices, determine the max LUN value.
+	 *
+	 * usb_stor_Bulk_max_lun() performs a synchronous control transfer and
+	 * must NOT run while holding us->dev_mutex: the reset and suspend
+	 * callbacks acquire us->dev_mutex (via usb_stor_pre_reset() /
+	 * usb_stor_suspend()) while the USB core holds the device lock, and a
+	 * concurrent scan would then deadlock against them.  The transfer now
+	 * uses only private resources, so take the lock only to publish the
+	 * result.
+	 */
 	if (us->protocol == USB_PR_BULK &&
 	    !(us->fflags & US_FL_SINGLE_LUN) &&
 	    !(us->fflags & US_FL_SCM_MULT_TARG)) {
+		int max_lun = usb_stor_Bulk_max_lun(us);
+
 		mutex_lock(&us->dev_mutex);
-		us->max_lun = usb_stor_Bulk_max_lun(us);
+		us->max_lun = max_lun;
 		/*
 		 * Allow proper scanning of devices that present more than 8 LUNs
 		 * While not affecting other devices that may need the previous
 		 * behavior
 		 */
-		if (us->max_lun >= 8)
-			us_to_host(us)->max_lun = us->max_lun+1;
+		if (max_lun >= 8)
+			us_to_host(us)->max_lun = max_lun + 1;
 		mutex_unlock(&us->dev_mutex);
 	}
 	scsi_scan_host(us_to_host(us));
-- 
2.53.0

Re: [PATCH] usb: storage: don't hold dev_mutex across the max_lun control transfer

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] usb: storage: DMA from stack buffer in usb_stor_Bulk_max_lun()
- [High] usb: storage: unlocked usb_stor_Bulk_max_lun() races with reset
  and I/O
- [Medium] usb: storage: uninitialized lun read in
  usb_stor_Bulk_max_lun()
- [Medium] usb: storage: wrong timeout unit in usb_stor_Bulk_max_lun()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] usb: storage: scan_dwork race with usb_stor_disconnect() causes
  hub stall [1]
--

commit 92856a6bb6e2428e581352525fb02cdea2d9e022
Author: Shuai Liu <847807867@qq.com>

usb: storage: don't hold dev_mutex across the max_lun control transfer

This commit changes usb_stor_Bulk_max_lun() to issue the GET_MAX_LUN
control transfer via usb_control_msg() using a stack variable and calls
it outside us->dev_mutex in usb_stor_scan_dwork().

> diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c
> index 8de3015bce050..961595a6facad 100644
> --- a/drivers/usb/storage/transport.c
> +++ b/drivers/usb/storage/transport.c
> @@ -1075,27 +1075,34 @@ EXPORT_SYMBOL_GPL(usb_stor_CB_transport);
>  /* Determine what the maximum LUN supported is */
>  int usb_stor_Bulk_max_lun(struct us_data *us)
>  {
> +	u8 lun;
>  	int result;
>  
> -	/* issue the command */
> -	us->iobuf[0] = 0;
> -	result = usb_stor_control_msg(us, us->recv_ctrl_pipe,
> -				 US_BULK_GET_MAX_LUN, 
> -				 USB_DIR_IN | USB_TYPE_CLASS | 
> +	/*
> +	 * Issue the GET_MAX_LUN request using a private buffer and the
> +	 * usbcore control-message helper.  This avoids touching the shared
> +	 * per-device resources (us->iobuf, us->cr, us->current_urb) that are
> +	 * also used by the control thread and the reset path, so the caller
> +	 * does not need to hold us->dev_mutex while this (possibly sleeping)
> +	 * transfer runs.
> +	 */
> +	result = usb_control_msg(us->pusb_dev, us->recv_ctrl_pipe,
> +				 US_BULK_GET_MAX_LUN,
> +				 USB_DIR_IN | USB_TYPE_CLASS |
>  				 USB_RECIP_INTERFACE,
> -				 0, us->ifnum, us->iobuf, 1, 10*HZ);
> +				 0, us->ifnum, &lun, 1, 10 * HZ);

[Severity: High]
Does passing &lun to usb_control_msg() perform DMA into the kernel stack?

Unlike usb_control_msg_recv(), usb_control_msg() uses the caller's data
pointer directly as urb->transfer_buffer without copying to a heap buffer.
On DMA-capable host controllers, usb_hcd_map_urb_for_dma() rejects
stack-allocated transfer buffers with a WARN_ONCE() splat and returns
-EAGAIN, which makes usb_stor_Bulk_max_lun() fall back to 0 and breaks
multi-LUN detection.

[Severity: Medium]
Does usb_control_msg() also expect its timeout argument in milliseconds
rather than jiffies?

usb_stor_control_msg() took jiffies, whereas usb_control_msg() passes
timeout to usb_start_wait_urb() -> msecs_to_jiffies().  Passing 10 * HZ
shortens the timeout from 10 seconds to 1 second on CONFIG_HZ=100 kernels
(or 2.5 seconds on CONFIG_HZ=250), risking premature -ETIMEDOUT failures
on slow devices.

>  
>  	usb_stor_dbg(us, "GetMaxLUN command result is %d, data is %d\n",
> -		     result, us->iobuf[0]);
> +		     result, lun);

[Severity: Medium]
Can usb_stor_dbg() read uninitialized stack memory from lun when
usb_control_msg() fails or transfers 0 bytes?

Previously us->iobuf[0] was zero-initialized before the transfer.  Here
lun is uninitialized, so when a single-LUN device stalls GET_MAX_LUN
(-EPIPE) or returns a zero-length packet, usb_stor_dbg() reads an
uninitialized value before the result > 0 check.

[ ... ]

> diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
> index 823a4b2e9033b..43d333f3ec4fe 100644
> --- a/drivers/usb/storage/usb.c
> +++ b/drivers/usb/storage/usb.c
> @@ -980,19 +980,31 @@ static void usb_stor_scan_dwork(struct work_struct *work)
>  
>  	dev_dbg(dev, "starting scan\n");
>  
> -	/* For bulk-only devices, determine the max LUN value */
> +	/*
> +	 * For bulk-only devices, determine the max LUN value.
> +	 *
> +	 * usb_stor_Bulk_max_lun() performs a synchronous control transfer and
> +	 * must NOT run while holding us->dev_mutex: the reset and suspend
> +	 * callbacks acquire us->dev_mutex (via usb_stor_pre_reset() /
> +	 * usb_stor_suspend()) while the USB core holds the device lock, and a
> +	 * concurrent scan would then deadlock against them.  The transfer now
> +	 * uses only private resources, so take the lock only to publish the
> +	 * result.
> +	 */
>  	if (us->protocol == USB_PR_BULK &&
>  	    !(us->fflags & US_FL_SINGLE_LUN) &&
>  	    !(us->fflags & US_FL_SCM_MULT_TARG)) {
> +		int max_lun = usb_stor_Bulk_max_lun(us);
> +
>  		mutex_lock(&us->dev_mutex);
> -		us->max_lun = usb_stor_Bulk_max_lun(us);
> +		us->max_lun = max_lun;

[Severity: High]
Does running usb_stor_Bulk_max_lun() outside us->dev_mutex allow a
GET_MAX_LUN control transfer to run concurrently with port resets, PM
suspend, or active SCSI commands?

usb_stor_pre_reset() and usb_stor_suspend() lock us->dev_mutex specifically
to ensure no URBs are in flight or submitted during a USB reset or suspend:

drivers/usb/storage/usb.c:usb_stor_pre_reset() {
	/* Make sure no command runs during the reset */
	mutex_lock(&us->dev_mutex);
	return 0;
}

Also, because scsi_add_host() has already been called in usb_stor_probe2(),
usb_stor_control_thread() can acquire us->dev_mutex while GET_MAX_LUN is
still in flight, check srb->device->lun > us->max_lun against the stale 0
value, and issue bulk transfers or resets concurrently with GET_MAX_LUN.
Note as well that neither usb_stor_control_msg() nor usb_control_msg()
takes udev->dev.mutex.

>  		/*
>  		 * Allow proper scanning of devices that present more than 8 LUNs
>  		 * While not affecting other devices that may need the previous
>  		 * behavior
>  		 */
> -		if (us->max_lun >= 8)
> -			us_to_host(us)->max_lun = us->max_lun+1;
> +		if (max_lun >= 8)
> +			us_to_host(us)->max_lun = max_lun + 1;
>  		mutex_unlock(&us->dev_mutex);
>  	}
>  	scsi_scan_host(us_to_host(us));

[Severity: High]
This problem wasn't introduced by this patch, but can usb_stor_scan_dwork()
proceed to scsi_scan_host() on a disconnecting or failed device and stall
the USB hub workqueue [1]?

In subdrivers such as ene_ub6250_probe(), usb_stor_probe2() schedules
scan_dwork before hardware initialization completes, and on failure the
probe path calls usb_stor_disconnect():

CPU0 (probe thread)               CPU1 (scan_dwork)
-------------------               -----------------
ene_ub6250_probe()
  usb_stor_probe2()
    schedule_delayed_work(&us->scan_dwork, ...)
                                  usb_stor_scan_dwork()
  mutex_lock(&us->dev_mutex)
                                    mutex_lock(&us->dev_mutex)
  ene_get_card_type() (fails)
  mutex_unlock(&us->dev_mutex)
                                    ...
                                    mutex_unlock(&us->dev_mutex)
  usb_stor_disconnect()             scsi_scan_host()  <-- stalls on dead dev
    quiesce_and_remove_host()
      cancel_delayed_work_sync()

Because quiesce_and_remove_host() waits in cancel_delayed_work_sync()
before setting US_FLIDX_DISCONNECTING, scsi_scan_host() issues SCSI
commands that time out against the unresponsive device, blocking the USB
hub thread for over 90 seconds.

[1] https://sashiko.dev/#/bug/linux-0529026a-086b-4e39-8ee0-4a1314c92146

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_7682CAFD52BA8845497EA2C9047754EC6605@qq.com?part=1