Message-ID: <20251029234724.pg2icfi3a63ojsxn@synopsys.com>
Date: Wed, 29 Oct 2025 23:47:33 +0000
From: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To: Manish Nagar <manish.nagar@....qualcomm.com>
CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] usb: dwc3: Fix race condition between concurrent
 dwc3_remove_requests() call paths

Hi,

On Tue, Oct 28, 2025, Manish Nagar wrote:
> This patch addresses a race condition caused by unsynchronized
> execution of multiple call paths invoking `dwc3_remove_requests()`,
> leading to premature freeing of USB requests and subsequent crashes.
> 
> Three distinct execution paths interact with `dwc3_remove_requests()`:
> Path 1:
> Triggered via `dwc3_gadget_reset_interrupt()` during USB reset
> handling. The call stack includes:
> - `dwc3_ep0_reset_state()`
> - `dwc3_ep0_stall_and_restart()`
> - `dwc3_ep0_out_start()`
> - `dwc3_remove_requests()`
> - `dwc3_gadget_del_and_unmap_request()`
> 
> Path 2:
> Also initiated from `dwc3_gadget_reset_interrupt()`, but through
> `dwc3_stop_active_transfers()`. The call stack includes:
> - `dwc3_stop_active_transfers()`
> - `dwc3_remove_requests()`
> - `dwc3_gadget_del_and_unmap_request()`
> 
> Path 3:
> Occurs independently during `adb root` execution, which triggers
> USB function unbind and bind operations. The sequence includes:
> - `gserial_disconnect()`
> - `usb_ep_disable()`
> - `dwc3_gadget_ep_disable()`
> - `dwc3_remove_requests()` with `-ESHUTDOWN` status
> 
> Path 3 operates asynchronously and lacks synchronization with Paths
> 1 and 2. When Path 3 completes, it disables endpoints and frees 'out'
> requests. If Paths 1 or 2 are still processing these requests,
> accessing freed memory leads to a crash due to use-after-free conditions.
> 
> To prevent this race condition, `usb_ep_disable()` should be made
> synchronous. Specifically:
> - Issue an `ENDXFER` command to stop the endpoint.
> - Ensure all pending USB requests are returned to the function driver
>   via `dwc3_gadget_giveback()` before freeing.
> 
> Since `gserial_disconnect` calls `usb_ep_disable()` first, modifying
> `ep_disable()` to invoke the `complete()` callback for gser USB
> requests ensures safe deallocation.

A gadget driver can deallocate the request on request completion other
than usb_ep_disable().

> 
> Additionally, the driver already includes the `dwc->ep0_in_setup`
> completion mechanism, which is triggered upon returning to the
> SETUP stage. This can be leveraged to coordinate and synchronize
> the cleanup process effectively.
> 
> Signed-off-by: Manish Nagar <manish.nagar@....qualcomm.com>
> ---
>  drivers/usb/dwc3/gadget.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 6f18b4840a25..93c20d5edea1 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1064,7 +1064,7 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep)
>  {
>  	struct dwc3		*dwc = dep->dwc;
>  	u32			reg;
> -	u32			mask;
> +	int			ret;
>  
>  	trace_dwc3_gadget_ep_disable(dep);
>  
> @@ -1077,18 +1077,23 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep)
>  	dwc3_writel(dwc->regs, DWC3_DALEPENA, reg);
>  
>  	dwc3_remove_requests(dwc, dep, -ESHUTDOWN);
> +	/*
> +	 * Stop the endpoint by issuing ENDXFER and synchronously complete
> +	 * all pending USB requests before returning from ep disable.
> +	 */
> +	if (dep->flags & DWC3_EP_DELAY_STOP) {

The DWC3_EP_DELAY_STOP flag is not meant for this. There are times when
we want to delay issuing End Transfer command, and the flag is meant for
that. You're selectively wait for End Transfer. So, this function is
sometime synchronous and sometime it's not.

> +		spin_unlock(&dwc->lock);
> +		reinit_completion(&dwc->ep0_in_setup);
> +		ret = wait_for_completion_timeout(&dwc->ep0_in_setup,
> +						  msecs_to_jiffies(DWC3_PULL_UP_TIMEOUT));

Don't use DWC3_PULL_UP_TIMEOUT, it's meant for PULL_UP timeout?

> +		spin_lock(&dwc->lock);
> +		if (ret == 0)
> +			dwc3_ep0_reset_state(dwc);
> +	}
>  
>  	dep->stream_capable = false;
>  	dep->type = 0;
> -	mask = DWC3_EP_TXFIFO_RESIZED | DWC3_EP_RESOURCE_ALLOCATED;
> -	/*
> -	 * dwc3_remove_requests() can exit early if DWC3 EP delayed stop is
> -	 * set.  Do not clear DEP flags, so that the end transfer command will
> -	 * be reattempted during the next SETUP stage.
> -	 */
> -	if (dep->flags & DWC3_EP_DELAY_STOP)
> -		mask |= (DWC3_EP_DELAY_STOP | DWC3_EP_TRANSFER_STARTED);
> -	dep->flags &= mask;
> +	dep->flags &= DWC3_EP_TXFIFO_RESIZED;

What happen to DWC3_EP_RESOURCE_ALLOCATED flag, why was it removed?

>  
>  	/* Clear out the ep descriptors for non-ep0 */
>  	if (dep->number > 1) {
> -- 
> 2.25.1
> 

What you're doing here only mitigates the issue.

Whenever we call dwc3_gadget_giveback(), there's a momentary release of
spinlock, and there will be a chance for a race. It can also come from
usb_ep_dequeue() and not just usb_ep_disable().

Perhaps just this change is sufficient to solve the race issue.
Hopefully we don't need reference count for dwc3_request.

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 1f67fb6aead5..a1d2ff9254b4 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -228,6 +228,13 @@ void dwc3_gadget_giveback(struct dwc3_ep *dep, struct dwc3_request *req,
 {
 	struct dwc3			*dwc = dep->dwc;
 
+	/*
+	 * The request might have been processed and completed while the
+	 * spinlock was released. Skip processing if already completed.
+	 */
+	if (req->status == DWC3_REQUEST_STATUS_COMPLETED)
+		return;
+
 	dwc3_gadget_del_and_unmap_request(dep, req, status);
 	req->status = DWC3_REQUEST_STATUS_COMPLETED;

BR,
Thinh

Powered by blists - more mailing lists