[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