[PATCH] usb: musb: host: clear stale TT buffers after aborted transfers

[PATCH] usb: musb: host: clear stale TT buffers after aborted transfers

[Jhon Wilker Rodrigues Aquino]

From: Jhon Wilker Rodrigues Aquino <jhon.aquino@autotrac.com.br>

On AM335x with MUSB and a Microchip USB2514 hub, disconnecting a
full-speed CDC ACM device during bulk traffic can leave its TT unable
to enumerate full-speed devices. High-speed devices still work, and
Reset_TT restores operation.

Queue Clear-TT-Buffer for failed or unlinked control and bulk URBs
behind an external TT. Park the affected endpoint queue until the
asynchronous clear completes, so new URBs cannot use a buffer being
cleared. Keep the parked queue alive until the callback finishes,
including during endpoint disable.

Signed-off-by: Jhon Wilker Rodrigues Aquino <jhonwilker0112@gmail.com>
---
 drivers/usb/musb/musb_host.c | 166 +++++++++++++++++++++++++++++++----
 drivers/usb/musb/musb_host.h |   2 +
 2 files changed, 152 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 31b0612489911..6e8cf462676ec 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -22,6 +22,11 @@
 #include "musb_host.h"
 #include "musb_trace.h"
 
+static bool tt_clear = true;
+module_param(tt_clear, bool, 0644);
+MODULE_PARM_DESC(tt_clear,
+		 "Clear hub TT buffers after failed or unlinked control/bulk transfers (default Y; set N only for A/B comparison or if a hub mishandles CLEAR_TT_BUFFER; affects new requests only)");
+
 /* MUSB HOST status 22-mar-2006
  *
  * - There's still lots of partial code duplication for fault paths, so
@@ -287,6 +292,33 @@ __acquires(musb->lock)
 	spin_lock(&musb->lock);
 }
 
+/*
+ * A failed or unlinked control/bulk split can leave the hub's transaction
+ * translator buffer busy; the TT then NAKs later splits using that buffer
+ * (USB 2.0 11.17.5).  A STALL completes the split, so it needs no clear.
+ *
+ * Context: caller owns controller lock and an URB which was on hardware
+ */
+static void musb_clear_tt_buffer(struct musb *musb, struct musb_qh *qh,
+				 struct urb *urb, int status)
+{
+	struct usb_tt		*tt = urb->dev->tt;
+
+	if (!READ_ONCE(tt_clear) || qh->clearing_tt)
+		return;
+	if (!tt || !tt->hub || tt->hub == musb->hcd->self.root_hub)
+		return;
+	if (!usb_pipecontrol(urb->pipe) && !usb_pipebulk(urb->pipe))
+		return;
+	if (!urb->unlinked && (!status || status == -EPIPE))
+		return;
+
+	if (usb_hub_clear_tt_buffer(urb))
+		return;
+
+	qh->clearing_tt = true;
+}
+
 /*
  * Advance this hardware endpoint's queue, completing the specified URB and
  * advancing to either the next URB queued to that qh, or else invalidating
@@ -329,11 +361,18 @@ static void musb_advance_schedule(struct musb *musb, struct urb *urb,
 	qh = musb_ep_get_qh(hw_ep, is_in);
 
 	/* reclaim resources (and bandwidth) ASAP; deschedule it, and
-	 * invalidate qh as soon as list_empty(&hep->urb_list)
+	 * invalidate qh as soon as list_empty(&hep->urb_list).
+	 *
+	 * While Clear-TT-Buffer is pending the endpoint must not use the TT,
+	 * so a qh with clearing_tt also releases hw_ep, letting other qhs
+	 * (including the hub's own control requests) run.  It stays in
+	 * hep->hcpriv, so new URBs queue on it, until
+	 * musb_clear_tt_buffer_complete() resumes or releases it.
 	 */
-	if (qh && list_empty(&qh->hep->urb_list)) {
+	if (qh && (qh->clearing_tt || list_empty(&qh->hep->urb_list))) {
 		struct list_head	*head;
 		struct dma_controller	*dma = musb->dma_controller;
+		bool			park = qh->clearing_tt;
 
 		if (is_in) {
 			ep->rx_reinit = 1;
@@ -351,7 +390,10 @@ static void musb_advance_schedule(struct musb *musb, struct urb *urb,
 
 		/* Clobber old pointers to this qh */
 		musb_ep_set_qh(ep, is_in, NULL);
-		qh->hep->hcpriv = NULL;
+		if (park)
+			qh->tt_parked = true;
+		else
+			qh->hep->hcpriv = NULL;
 
 		switch (qh->type) {
 
@@ -362,8 +404,9 @@ static void musb_advance_schedule(struct musb *musb, struct urb *urb,
 			 */
 			if (qh->mux == 1) {
 				head = qh->ring.prev;
-				list_del(&qh->ring);
-				kfree(qh);
+				list_del_init(&qh->ring);
+				if (!park)
+					kfree(qh);
 				qh = first_qh(head);
 				break;
 			}
@@ -375,7 +418,8 @@ static void musb_advance_schedule(struct musb *musb, struct urb *urb,
 			 * de-allocated if it's tracked and allocated;
 			 * and where we'd update the schedule tree...
 			 */
-			kfree(qh);
+			if (!park)
+				kfree(qh);
 			qh = NULL;
 			break;
 		}
@@ -1168,8 +1212,10 @@ irqreturn_t musb_h_ep0_irq(struct musb *musb)
 		musb->ep0_stage = MUSB_EP0_IDLE;
 
 	/* call completion handler if done */
-	if (complete)
+	if (complete) {
+		musb_clear_tt_buffer(musb, qh, urb, status);
 		musb_advance_schedule(musb, urb, hw_ep, 1);
+	}
 done:
 	return retval;
 }
@@ -1406,6 +1452,7 @@ void musb_host_tx(struct musb *musb, u8 epnum)
 		/* set status */
 		urb->status = status;
 		urb->actual_length = qh->offset;
+		musb_clear_tt_buffer(musb, qh, urb, status);
 		musb_advance_schedule(musb, urb, hw_ep, USB_DIR_OUT);
 		return;
 	} else if ((usb_pipeisoc(pipe) || transfer_pending) && dma) {
@@ -1993,6 +2040,7 @@ void musb_host_rx(struct musb *musb, u8 epnum)
 
 		if (urb->status == -EINPROGRESS)
 			urb->status = status;
+		musb_clear_tt_buffer(musb, qh, urb, urb->status);
 		musb_advance_schedule(musb, urb, hw_ep, USB_DIR_IN);
 	}
 }
@@ -2122,6 +2170,56 @@ static int musb_schedule(
 	return 0;
 }
 
+/*
+ * Continue a qh parked by musb_advance_schedule() once its Clear-TT-Buffer
+ * finished: release it when no URB is queued, else schedule it again.  While
+ * the endpoint is being disabled (!is_ready) or the host is inactive it stays
+ * parked; the dequeue and endpoint_disable paths finish it.
+ *
+ * Context: caller owns controller lock, irqs blocked
+ */
+static void musb_tt_resume(struct musb *musb, struct musb_qh *qh)
+{
+	struct usb_host_endpoint	*hep = qh->hep;
+	int				is_in = usb_endpoint_dir_in(&hep->desc);
+
+	if (!qh->tt_parked || qh->clearing_tt || !qh->is_ready)
+		return;
+
+	if (list_empty(&hep->urb_list)) {
+		hep->hcpriv = NULL;
+		kfree(qh);
+		return;
+	}
+
+	if (!is_host_active(musb) || !musb->is_active)
+		return;
+
+	qh->tt_parked = false;
+	/* as for a new control/bulk qh; the shared bulk_ep sets a NAK limit */
+	qh->intv_reg = 0;
+	if (musb_schedule(musb, qh, is_in))
+		qh->tt_parked = true;
+}
+
+/* Called by hub_tt_work() after the hub processed (or failed) the request */
+static void musb_clear_tt_buffer_complete(struct usb_hcd *hcd,
+					  struct usb_host_endpoint *hep)
+{
+	struct musb		*musb = hcd_to_musb(hcd);
+	struct musb_qh		*qh;
+	unsigned long		flags;
+
+	spin_lock_irqsave(&musb->lock, flags);
+	/* musb_h_disable() keeps hep->hcpriv valid while clearing_tt is set */
+	qh = hep->hcpriv;
+	if (qh && qh->clearing_tt) {
+		qh->clearing_tt = false;
+		musb_tt_resume(musb, qh);
+	}
+	spin_unlock_irqrestore(&musb->lock, flags);
+}
+
 static int musb_urb_enqueue(
 	struct usb_hcd			*hcd,
 	struct urb			*urb,
@@ -2145,13 +2243,17 @@ static int musb_urb_enqueue(
 	spin_lock_irqsave(&musb->lock, flags);
 	ret = usb_hcd_link_urb_to_ep(hcd, urb);
 	qh = ret ? NULL : hep->hcpriv;
-	if (qh)
+	if (qh) {
 		urb->hcpriv = qh;
+		/* restart a parked qh left idle while the host was inactive */
+		musb_tt_resume(musb, qh);
+	}
 	spin_unlock_irqrestore(&musb->lock, flags);
 
 	/* DMA mapping was already done, if needed, and this urb is on
 	 * hep->urb_list now ... so we're done, unless hep wasn't yet
-	 * scheduled onto a live qh.
+	 * scheduled onto a live qh.  A qh parked for Clear-TT-Buffer
+	 * also holds new URBs until musb_tt_resume() schedules it.
 	 *
 	 * REVISIT best to keep hep->hcpriv valid until the endpoint gets
 	 * disabled, testing for empty qh->ring and avoiding qh setup costs
@@ -2367,8 +2469,11 @@ static int musb_cleanup_urb(struct urb *urb, struct musb_qh *qh)
 	} else  {
 		musb_h_ep0_flush_fifo(ep);
 	}
-	if (status == 0)
+	if (status == 0) {
+		/* hardware stopped; the TT may still hold this transaction */
+		musb_clear_tt_buffer(musb, qh, urb, urb->status);
 		musb_advance_schedule(ep->musb, urb, ep, is_in);
+	}
 	return status;
 }
 
@@ -2412,10 +2517,13 @@ static int musb_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 		musb_giveback(musb, urb, 0);
 		qh->is_ready = ready;
 
-		/* If nothing else (usually musb_giveback) is using it
-		 * and its URB list has emptied, recycle this qh.
-		 */
-		if (ready && list_empty(&qh->hep->urb_list)) {
+		/* A parked qh is not on hw_ep; never clobber hw_ep for it */
+		if (qh->tt_parked) {
+			musb_tt_resume(musb, qh);
+		} else if (ready && list_empty(&qh->hep->urb_list)) {
+			/* If nothing else (usually musb_giveback) is using it
+			 * and its URB list has emptied, recycle this qh.
+			 */
 			musb_ep_set_qh(qh->hw_ep, is_in, NULL);
 			qh->hep->hcpriv = NULL;
 			list_del(&qh->ring);
@@ -2460,13 +2568,18 @@ musb_h_disable(struct usb_hcd *hcd, struct usb_host_endpoint *hep)
 
 		/* Then nuke all the others ... and advance the
 		 * queue on hw_ep (e.g. bulk ring) when we're done.
+		 * A qh parked by the cleanup no longer owns hw_ep.
 		 */
 		while (!list_empty(&hep->urb_list)) {
 			urb = next_urb(qh);
 			urb->status = -ESHUTDOWN;
-			musb_advance_schedule(musb, urb, qh->hw_ep, is_in);
+			if (qh->tt_parked)
+				musb_giveback(musb, urb, -ESHUTDOWN);
+			else
+				musb_advance_schedule(musb, urb, qh->hw_ep,
+						      is_in);
 		}
-	} else {
+	} else if (!qh->tt_parked) {
 		/* Just empty the queue; the hardware is busy with
 		 * other transfers, and since !qh->is_ready nothing
 		 * will activate any of these as it advances.
@@ -2478,6 +2591,26 @@ musb_h_disable(struct usb_hcd *hcd, struct usb_host_endpoint *hep)
 		list_del(&qh->ring);
 		kfree(qh);
 	}
+
+	/*
+	 * hub_tt_work() passes hep to musb_clear_tt_buffer_complete(), which
+	 * dereferences hep->hcpriv: free a parked qh only after its clear
+	 * finished (bounded by the hub control request timeout).
+	 */
+	while ((qh = hep->hcpriv) && qh->tt_parked) {
+		while (!list_empty(&hep->urb_list))
+			musb_giveback(musb, next_urb(qh), -ESHUTDOWN);
+
+		if (!qh->clearing_tt) {
+			hep->hcpriv = NULL;
+			kfree(qh);
+			break;
+		}
+
+		spin_unlock_irqrestore(&musb->lock, flags);
+		schedule_timeout_uninterruptible(1);
+		spin_lock_irqsave(&musb->lock, flags);
+	}
 exit:
 	spin_unlock_irqrestore(&musb->lock, flags);
 }
@@ -2689,6 +2822,7 @@ static const struct hc_driver musb_hc_driver = {
 	.urb_enqueue		= musb_urb_enqueue,
 	.urb_dequeue		= musb_urb_dequeue,
 	.endpoint_disable	= musb_h_disable,
+	.clear_tt_buffer_complete = musb_clear_tt_buffer_complete,
 
 #ifndef CONFIG_MUSB_PIO_ONLY
 	.map_urb_for_dma	= musb_map_urb_for_dma,
diff --git a/drivers/usb/musb/musb_host.h b/drivers/usb/musb/musb_host.h
index 63298bd233e02..8241fe2aaaa4c 100644
--- a/drivers/usb/musb/musb_host.h
+++ b/drivers/usb/musb/musb_host.h
@@ -40,6 +40,8 @@ struct musb_qh {
 	unsigned		iso_idx;	/* in urb->iso_frame_desc[] */
 	struct sg_mapping_iter sg_miter;	/* for highmem in PIO mode */
 	bool			use_sg;		/* to track urb using sglist */
+	bool			clearing_tt;	/* Clear-TT-Buffer in progress */
+	bool			tt_parked;	/* off hw_ep until TT is cleared */
 };
 
 /* map from control or bulk queue head to the first qh on that ring */
-- 
2.43.0

Re: [PATCH] usb: musb: host: clear stale TT buffers after aborted transfers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] usb: musb: qh UAF and is_ready corruption in musb_urb_dequeue()
- [High] usb: musb: unparked qh race across musb_giveback() in musb_advance_schedule()
- [High] usb: musb: stranded URBs on parked qh after inactive host in musb_tt_resume()
- [Medium] usb: musb: spurious TT clear on -EREMOTEIO in musb_clear_tt_buffer()
- [Medium] usb: musb: unreset qh->hb_mult before rescheduling in musb_tt_resume()
- [Low] usb: musb: undocumented tt_clear module parameter in musb_host.c
- [Low] usb: musb: open-coded spinlock polling loop in musb_h_disable()
--

commit 64201419e6f04cb16c53e5b8f07d1c18f4fb355b
Author: Jhon Wilker Rodrigues Aquino <jhon.aquino@autotrac.com.br>

usb: musb: host: clear stale TT buffers after aborted transfers

This commit queues Clear-TT-Buffer requests for failed or unlinked control
and bulk URBs behind an external hub transaction translator, and parks the
affected endpoint queue until the asynchronous clear finishes.

> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
> index 31b0612489911..6e8cf462676ec 100644
> --- a/drivers/usb/musb/musb_host.c
> +++ b/drivers/usb/musb/musb_host.c
> @@ -22,6 +22,11 @@
>  #include "musb_host.h"
>  #include "musb_trace.h"
>  
> +static bool tt_clear = true;
> +module_param(tt_clear, bool, 0644);
> +MODULE_PARM_DESC(tt_clear,
> +		 "Clear hub TT buffers after failed or unlinked control/bulk transfers (default Y; set N only for A/B comparison or if a hub mishandles CLEAR_TT_BUFFER; affects new requests only)");

[Severity: Low]
Should the new tt_clear module parameter (settable at boot as
musb_hdrc.tt_clear) also be documented in
Documentation/admin-guide/kernel-parameters.txt and mentioned in the commit
message?

> @@ -287,6 +292,33 @@ __acquires(musb->lock)
>  	spin_lock(&musb->lock);
>  }
>  
> +/*
> + * A failed or unlinked control/bulk split can leave the hub's transaction
> + * translator buffer busy; the TT then NAKs later splits using that buffer
> + * (USB 2.0 11.17.5).  A STALL completes the split, so it needs no clear.
> + *
> + * Context: caller owns controller lock and an URB which was on hardware
> + */
> +static void musb_clear_tt_buffer(struct musb *musb, struct musb_qh *qh,
> +				 struct urb *urb, int status)
> +{
> +	struct usb_tt		*tt = urb->dev->tt;
> +
> +	if (!READ_ONCE(tt_clear) || qh->clearing_tt)
> +		return;
> +	if (!tt || !tt->hub || tt->hub == musb->hcd->self.root_hub)
> +		return;
> +	if (!usb_pipecontrol(urb->pipe) && !usb_pipebulk(urb->pipe))
> +		return;
> +	if (!urb->unlinked && (!status || status == -EPIPE))
> +		return;
> +
> +	if (usb_hub_clear_tt_buffer(urb))
> +		return;
> +
> +	qh->clearing_tt = true;
> +}

[Severity: Medium]
Can this trigger a spurious Clear-TT-Buffer when a bulk or control IN URB
with URB_SHORT_NOT_OK completes normally with a short packet?

In musb_host_packet_rx(), receiving a short packet when URB_SHORT_NOT_OK is
set marks the URB as done with -EREMOTEIO without requesting another IN
token:

drivers/usb/musb/musb_host.c:musb_host_packet_rx() {
	...
		if (done
				&& (urb->status == -EINPROGRESS)
				&& (urb->transfer_flags & URB_SHORT_NOT_OK)
				&& (urb->actual_length
					< urb->transfer_buffer_length))
			urb->status = -EREMOTEIO;
	...
}

Then musb_host_rx() passes urb->status (-EREMOTEIO) into
musb_clear_tt_buffer(). Since a short packet completes the split transaction
on the bus and leaves the TT buffer free, should musb_clear_tt_buffer() also
exclude status == -EREMOTEIO (or should musb_host_rx() pass the hardware
status)?

> @@ -329,11 +361,18 @@ static void musb_advance_schedule(struct musb *musb, struct urb *urb,
>  	qh = musb_ep_get_qh(hw_ep, is_in);
>  
>  	/* reclaim resources (and bandwidth) ASAP; deschedule it, and
> -	 * invalidate qh as soon as list_empty(&hep->urb_list)
> +	 * invalidate qh as soon as list_empty(&hep->urb_list).
> +	 *
> +	 * While Clear-TT-Buffer is pending the endpoint must not use the TT,
> +	 * so a qh with clearing_tt also releases hw_ep, letting other qhs
> +	 * (including the hub's own control requests) run.  It stays in
> +	 * hep->hcpriv, so new URBs queue on it, until
> +	 * musb_clear_tt_buffer_complete() resumes or releases it.
>  	 */
> -	if (qh && list_empty(&qh->hep->urb_list)) {
> +	if (qh && (qh->clearing_tt || list_empty(&qh->hep->urb_list))) {
>  		struct list_head	*head;
>  		struct dma_controller	*dma = musb->dma_controller;
> +		bool			park = qh->clearing_tt;
>  
>  		if (is_in) {
>  			ep->rx_reinit = 1;
> @@ -351,7 +390,10 @@ static void musb_advance_schedule(struct musb *musb, struct urb *urb,
>  
>  		/* Clobber old pointers to this qh */
>  		musb_ep_set_qh(ep, is_in, NULL);
> -		qh->hep->hcpriv = NULL;
> +		if (park)
> +			qh->tt_parked = true;
> +		else
> +			qh->hep->hcpriv = NULL;

[Severity: High]
Because musb_clear_tt_buffer() sets qh->clearing_tt = true and schedules
tt->clear_work before musb_advance_schedule() calls musb_giveback(),
musb->lock is dropped inside musb_giveback() while qh is still attached to
hw_ep and qh->tt_parked is still false:

drivers/usb/musb/musb_host.c:musb_advance_schedule() {
	...
	qh->is_ready = 0;
	musb_giveback(musb, urb, status);
	qh->is_ready = ready;

	qh = musb_ep_get_qh(hw_ep, is_in);

	if (qh && (qh->clearing_tt || list_empty(&qh->hep->urb_list))) {
	...
}

What happens if another CPU runs during that window while musb->lock is
dropped in musb_giveback()?

1. After usb_hcd_unlink_urb_from_ep() empties hep->urb_list (or after
__usb_hcd_giveback_urb() wakes usb_kill_urb()), a concurrent
musb_h_disable() can acquire musb->lock before musb_giveback() re-acquires
it. In musb_h_disable(), musb_ep_get_qh(qh->hw_ep, is_in) == qh is still
true, so it calls urb = next_urb(qh) on the empty list and dereferences a
NULL urb at if (!urb->unlinked).

2. If a concurrent musb_urb_dequeue() for another URB on the same qh started
before musb_advance_schedule() and finishes its own musb_giveback() while
hep->urb_list is now empty, it sees !qh->tt_parked && ready &&
list_empty(&qh->hep->urb_list) and frees qh while qh->clearing_tt is active.

3. If hub_tt_work() runs musb_clear_tt_buffer_complete() while musb->lock is
dropped in musb_giveback(), it clears qh->clearing_tt = false while
qh->tt_parked is still false. When musb_advance_schedule() resumes with
another URB queued on hep->urb_list, it sees !qh->clearing_tt and starts the
next URB without setting ep->rx_reinit or ep->tx_reinit.

[ ... ]

> @@ -2122,6 +2170,56 @@ static int musb_schedule(
>  	return 0;
>  }
>  
> +/*
> + * Continue a qh parked by musb_advance_schedule() once its Clear-TT-Buffer
> + * finished: release it when no URB is queued, else schedule it again.  While
> + * the endpoint is being disabled (!is_ready) or the host is inactive it stays
> + * parked; the dequeue and endpoint_disable paths finish it.
> + *
> + * Context: caller owns controller lock, irqs blocked
> + */
> +static void musb_tt_resume(struct musb *musb, struct musb_qh *qh)
> +{
> +	struct usb_host_endpoint	*hep = qh->hep;
> +	int				is_in = usb_endpoint_dir_in(&hep->desc);
> +
> +	if (!qh->tt_parked || qh->clearing_tt || !qh->is_ready)
> +		return;
> +
> +	if (list_empty(&hep->urb_list)) {
> +		hep->hcpriv = NULL;
> +		kfree(qh);
> +		return;
> +	}
> +
> +	if (!is_host_active(musb) || !musb->is_active)
> +		return;

[Severity: High]
If musb_clear_tt_buffer_complete() runs while !is_host_active(musb) ||
!musb->is_active (for example, during root port suspend/resume before
musb_host_finish_resume() sets musb->is_active = 1) and hep->urb_list is
non-empty, musb_clear_tt_buffer_complete() clears qh->clearing_tt = false and
musb_tt_resume() returns early here with qh->tt_parked still true and qh
detached from the hardware rings.

When the host becomes active again, nothing calls musb_tt_resume() unless a
new URB is submitted via musb_urb_enqueue(). If the driver is waiting for
the URBs already queued in hep->urb_list to complete before submitting more,
won't those URBs remain stranded on the parked qh?

> +
> +	qh->tt_parked = false;
> +	/* as for a new control/bulk qh; the shared bulk_ep sets a NAK limit */
> +	qh->intv_reg = 0;
> +	if (musb_schedule(musb, qh, is_in))
> +		qh->tt_parked = true;
> +}

[Severity: Medium]
Does musb_tt_resume() also need to reset qh->hb_mult before calling
musb_schedule()?

When a bulk OUT transfer is programmed on a controller where
can_bulk_split(musb, qh->type) is true, musb_ep_program() overwrites
qh->hb_mult with hw_ep->max_packet_sz_tx / packet_sz:

drivers/usb/musb/musb_host.c:musb_ep_program() {
	...
			if (can_bulk_split(musb, qh->type)) {
				qh->hb_mult = hw_ep->max_packet_sz_tx
						/ packet_sz;
				musb_writew(epio, MUSB_TXMAXP, packet_sz
					| ((qh->hb_mult) - 1) << 11);
	...
}

When musb_tt_resume() later calls musb_schedule(), musb_schedule() checks
candidate endpoints against diff -= (qh->maxpacket * qh->hb_mult). If
qh->hb_mult still holds the multiplier from the previous endpoint's TX FIFO
size instead of 1, won't musb_schedule() reject smaller free hardware
endpoints and fall back to the shared musb->bulk_ep?

[ ... ]

> @@ -2412,10 +2517,13 @@ static int musb_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
>  		musb_giveback(musb, urb, 0);
>  		qh->is_ready = ready;
>  
> -		/* If nothing else (usually musb_giveback) is using it
> -		 * and its URB list has emptied, recycle this qh.
> -		 */
> -		if (ready && list_empty(&qh->hep->urb_list)) {
> +		/* A parked qh is not on hw_ep; never clobber hw_ep for it */
> +		if (qh->tt_parked) {
> +			musb_tt_resume(musb, qh);
> +		} else if (ready && list_empty(&qh->hep->urb_list)) {
> +			/* If nothing else (usually musb_giveback) is using it
> +			 * and its URB list has emptied, recycle this qh.
> +			 */
>  			musb_ep_set_qh(qh->hw_ep, is_in, NULL);
>  			qh->hep->hcpriv = NULL;
>  			list_del(&qh->ring);

[Severity: Critical]
Can this cause a use-after-free on qh or leave qh->is_ready stuck at 0 when
musb_urb_dequeue() drops musb->lock inside musb_giveback()?

In musb_urb_dequeue():

	if (!qh->is_ready
			|| urb->urb_list.prev != &qh->hep->urb_list
			|| musb_ep_get_qh(qh->hw_ep, is_in) != qh) {
		int	ready = qh->is_ready;

		qh->is_ready = 0;
		musb_giveback(musb, urb, 0);
		qh->is_ready = ready;

		/* A parked qh is not on hw_ep; never clobber hw_ep for it */
		if (qh->tt_parked) {
			musb_tt_resume(musb, qh);
		} else if (ready && list_empty(&qh->hep->urb_list)) {

There are two races across musb_giveback() here:

1. If urb1 is already inside musb_giveback() (either in
musb_advance_schedule() while parking qh, or in musb_urb_dequeue() on an
already-parked qh), it has saved ready = 1 and set qh->is_ready = 0. If a
second CPU calls musb_urb_dequeue() for urb2 on the same qh while musb->lock
is dropped, CPU 2 observes qh->is_ready == 0, saves ready = 0, unlinks urb2,
and drops musb->lock in musb_giveback().

If CPU 1 finishes musb_giveback() first, it restores qh->is_ready = 1, and
once qh->clearing_tt is false, musb_tt_resume() sees qh->is_ready == 1 and
list_empty(&hep->urb_list) and frees qh via kfree(qh). When CPU 2 returns
from musb_giveback(), it writes qh->is_ready = 0 and reads qh->tt_parked on
the freed qh.

Conversely, if qh->clearing_tt is still true when CPU 2 returns from
musb_giveback() after CPU 1 restored qh->is_ready = 1, CPU 2 overwrites
qh->is_ready = 0. When musb_clear_tt_buffer_complete() later calls
musb_tt_resume(), it bails out on !qh->is_ready, leaking qh if hep->urb_list
is empty or permanently wedging the endpoint queue if new URBs were queued.

2. Because musb_giveback() unlinks urb from hep->urb_list before dropping
musb->lock and __usb_hcd_giveback_urb() wakes usb_kill_urb() before
musb_giveback() re-acquires musb->lock, usb_disable_endpoint() ->
musb_h_disable() can run on another CPU while musb_urb_dequeue() is still in
musb_giveback(). In musb_h_disable(), if !qh->clearing_tt, it frees qh right
away; when musb_urb_dequeue() re-acquires musb->lock, it writes
qh->is_ready = 1 and calls musb_tt_resume() on the freed qh, leading to a
use-after-free and double-free.

> @@ -2460,13 +2568,18 @@ musb_h_disable(struct usb_hcd *hcd, struct usb_host_endpoint *hep)
>  
>  		/* Then nuke all the others ... and advance the
>  		 * queue on hw_ep (e.g. bulk ring) when we're done.
> +		 * A qh parked by the cleanup no longer owns hw_ep.
>  		 */
>  		while (!list_empty(&hep->urb_list)) {
>  			urb = next_urb(qh);
>  			urb->status = -ESHUTDOWN;
> -			musb_advance_schedule(musb, urb, qh->hw_ep, is_in);
> +			if (qh->tt_parked)
> +				musb_giveback(musb, urb, -ESHUTDOWN);
> +			else
> +				musb_advance_schedule(musb, urb, qh->hw_ep,
> +						      is_in);
>  		}
> -	} else {
> +	} else if (!qh->tt_parked) {
>  		/* Just empty the queue; the hardware is busy with
>  		 * other transfers, and since !qh->is_ready nothing
>  		 * will activate any of these as it advances.
> @@ -2478,6 +2591,26 @@ musb_h_disable(struct usb_hcd *hcd, struct usb_host_endpoint *hep)
>  		list_del(&qh->ring);
>  		kfree(qh);
>  	}
> +
> +	/*
> +	 * hub_tt_work() passes hep to musb_clear_tt_buffer_complete(), which
> +	 * dereferences hep->hcpriv: free a parked qh only after its clear
> +	 * finished (bounded by the hub control request timeout).
> +	 */
> +	while ((qh = hep->hcpriv) && qh->tt_parked) {
> +		while (!list_empty(&hep->urb_list))
> +			musb_giveback(musb, next_urb(qh), -ESHUTDOWN);
> +
> +		if (!qh->clearing_tt) {
> +			hep->hcpriv = NULL;
> +			kfree(qh);
> +			break;
> +		}
> +
> +		spin_unlock_irqrestore(&musb->lock, flags);
> +		schedule_timeout_uninterruptible(1);
> +		spin_lock_irqsave(&musb->lock, flags);
> +	}

[Severity: Low]
Instead of polling !qh->clearing_tt in a loop with
schedule_timeout_uninterruptible(1) under musb->lock in musb_h_disable(),
could this use a completion variable signaled by
musb_clear_tt_buffer_complete()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103190333.87207-1-jhonwilker0112@gmail.com?part=1