[PATCH 02/19] USB: core: signal the end of an sg request last

[Greg Kroah-Hartman]

From: Josef Bacik <josef@xxxxxxxxxxxxxx>

sg_complete() and usb_sg_cancel() call complete() with io->lock held and
unlock afterwards. usb_sg_wait() does not take the lock again after
wait_for_completion(), so it can return while the other CPU has not
unlocked yet:

  CPU0 (giveback)                     CPU1 (usb_sg_wait())
  spin_lock_irqsave(&io->lock)
  io->count--
  complete(&io->complete)
                                      wait_for_completion() returns
                                      sg_clean(io), return
                                      the caller reuses the request
  spin_unlock_irqrestore(&io->lock)

The request is the caller's. gud_usb_bulk(), sur40_process_video() and
usbtest have it on the stack, so the unlock writes to a frame which is
gone. usb-storage, rtsx_usb and vub300 call usb_sg_init() for the next
transfer, so the unlock hits a lock which was just initialised again.
The window is a few instructions with interrupts off, so this needs
something which stalls CPU0 there, like a preempted virtual CPU.

Note that the count reached zero, drop the lock and call complete() as
the last access.

This alone would open another window. After its submit loop
usb_sg_wait() takes the lock and calls complete() itself if it finds the
count at zero, also if it was the last URB which brought it there. It
could then return while sg_complete() is between its unlock and its
complete(). So far the lock kept the two apart. Let usb_sg_wait() only
complete the request if it has URBs to subtract which were not
submitted. Then the count reaches zero once, and only who brought it
there calls complete().

Fixes: 056ad39ee925 ("USB: core: Fix free-while-in-use bug in the USB S-Glibrary")
Cc: stable <stable@xxxxxxxxxx>
Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
Cc: Johan Hovold <johan@xxxxxxxxxx>
Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
---
 drivers/usb/core/message.c | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index a738d03c23ea..204fbfd8171b 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -465,6 +465,7 @@ static void sg_complete(struct urb *urb)
 	unsigned long flags;
 	struct usb_sg_request *io = urb->context;
 	int status = urb->status;
+	bool done;
 
 	spin_lock_irqsave(&io->lock, flags);
 
@@ -520,13 +521,16 @@ static void sg_complete(struct urb *urb)
 		spin_lock_irqsave(&io->lock, flags);
 	}
 
-	/* on the last completion, signal usb_sg_wait() */
 	io->bytes += urb->actual_length;
-	io->count--;
-	if (!io->count)
-		complete(&io->complete);
-
+	done = !--io->count;
 	spin_unlock_irqrestore(&io->lock, flags);
+
+	/*
+	 * On the last completion, signal usb_sg_wait().  Its caller may free
+	 * or reuse the request as soon as it returns, so this comes last.
+	 */
+	if (done)
+		complete(&io->complete);
 }
 
 
@@ -753,9 +757,16 @@ void usb_sg_wait(struct usb_sg_request *io)
 		if (retval && (io->status == 0 || io->status == -ECONNRESET))
 			io->status = retval;
 	}
-	io->count -= entries - i;
-	if (io->count == 0)
-		complete(&io->complete);
+	/*
+	 * Whoever brings the count to zero calls complete(), once, after he has
+	 * dropped the lock. If all URBs were submitted this is not us, even if
+	 * the count is zero by now.
+	 */
+	if (i < entries) {
+		io->count -= entries - i;
+		if (io->count == 0)
+			complete(&io->complete);
+	}
 	spin_unlock_irq(&io->lock);
 
 	/* OK, yes, this could be packaged as non-blocking.
@@ -780,6 +791,7 @@ void usb_sg_cancel(struct usb_sg_request *io)
 {
 	unsigned long flags;
 	int i, retval;
+	bool done;
 
 	spin_lock_irqsave(&io->lock, flags);
 	if (io->status || io->count == 0) {
@@ -804,10 +816,12 @@ void usb_sg_cancel(struct usb_sg_request *io)
 	}
 
 	spin_lock_irqsave(&io->lock, flags);
-	io->count--;
-	if (!io->count)
-		complete(&io->complete);
+	done = !--io->count;
 	spin_unlock_irqrestore(&io->lock, flags);
+
+	/* Don't touch the request after this, see sg_complete() */
+	if (done)
+		complete(&io->complete);
 }
 EXPORT_SYMBOL_GPL(usb_sg_cancel);
 

-- 
2.56.0