[PATCH] usb: dwc3: Fix race condition between concurrent dwc3_remove_requests() call paths

[PATCH] usb: dwc3: Fix race condition between concurrent dwc3_remove_requests() call paths

[Manish Nagar]

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.

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@oss.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) {
+		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));
+		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;
 
 	/* Clear out the ep descriptors for non-ep0 */
 	if (dep->number > 1) {
-- 
2.25.1

Re: [PATCH] usb: dwc3: Fix race condition between concurrent dwc3_remove_requests() call paths

[Greg Kroah-Hartman]

On Tue, Oct 28, 2025 at 01:35:53PM +0530, 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.
> 
> 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@oss.qualcomm.com>
> ---
>  drivers/usb/dwc3/gadget.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)

What commit id does this fix?

Re: [PATCH] usb: dwc3: Fix race condition between concurrent dwc3_remove_requests() call paths

[Thinh Nguyen]

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@oss.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

Re: [PATCH] usb: dwc3: Fix race condition between concurrent dwc3_remove_requests() call paths

[Thinh Nguyen]

On Thu, Nov 13, 2025, Manish Nagar wrote:
> Hi ,
> 
> Thanks for the suggestion ,
> 
> On above your suggestion, I added a check for  dep->number as 
> 
> if ((req->status == DWC3_REQUEST_STATUS_COMPLETED) && (dep->number > 1))
> return;
> 
> I included this condition to prevent enumeration failures. During the sequence
> dwc3_gadget_giveback → dwc3_ep0_interrupt → dwc3_thread_interrupt, the giveback
> for ep0 does not complete, so this check ensures proper handling.
> 
> Please share your feedback, and I will proceed with v2 accordingly.
> 

No, add checks for ep0 too.

Try this:

diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index b4229aa13f37..e0bad5708664 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -94,6 +94,7 @@ static int __dwc3_gadget_ep0_queue(struct dwc3_ep *dep,
 	req->request.actual	= 0;
 	req->request.status	= -EINPROGRESS;
 	req->epnum		= dep->number;
+	req->status		= DWC3_REQUEST_STATUS_QUEUED;
 
 	list_add_tail(&req->list, &dep->pending_list);
 
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

Re: [PATCH] usb: dwc3: Fix race condition between concurrent dwc3_remove_requests() call paths

[Manish Nagar]

Hi Greg,

I can’t identify a specific commit ID that I’m confident was the patch
introducing the bug.But I found the race condition and suggested this
patch to fix it.

Regards,

Manish


On 11/14/2025 5:59 AM, Thinh Nguyen wrote:
> On Thu, Nov 13, 2025, Manish Nagar wrote:
>> Hi ,
>>
>> Thanks for the suggestion ,
>>
>> On above your suggestion, I added a check for  dep->number as
>>
>> if ((req->status == DWC3_REQUEST_STATUS_COMPLETED) && (dep->number > 1))
>> return;
>>
>> I included this condition to prevent enumeration failures. During the sequence
>> dwc3_gadget_giveback → dwc3_ep0_interrupt → dwc3_thread_interrupt, the giveback
>> for ep0 does not complete, so this check ensures proper handling.
>>
>> Please share your feedback, and I will proceed with v2 accordingly.
>>
> No, add checks for ep0 too.
>
> Try this:
>
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index b4229aa13f37..e0bad5708664 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -94,6 +94,7 @@ static int __dwc3_gadget_ep0_queue(struct dwc3_ep *dep,
>       req->request.actual     = 0;
>       req->request.status     = -EINPROGRESS;
>       req->epnum              = dep->number;
> +     req->status             = DWC3_REQUEST_STATUS_QUEUED;
>
>       list_add_tail(&req->list, &dep->pending_list);
>
> 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