[PATCH v2] usb: dwc3: gadget: Prevent EP queuing while stopping transfers

[PATCH v2] usb: dwc3: gadget: Prevent EP queuing while stopping transfers

[Wesley Cheng]

In the situations where the DWC3 gadget stops active transfers, once
calling the dwc3_gadget_giveback(), there is a chance where a function
driver can queue a new USB request in between the time where the dwc3
lock has been released and re-aquired.  This occurs after we've already
issued an ENDXFER command.  When the stop active transfers continues
to remove USB requests from all dep lists, the newly added request will
also be removed, while controller still has an active TRB for it.
This can lead to the controller accessing an unmapped memory address.

Fix this by ensuring parameters to prevent EP queuing are set before
calling the stop active transfers API.

Fixes: ae7e86108b12 ("usb: dwc3: Stop active transfers before halting the controller")
Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
---
Changes since V1:
 - Added Fixes tag to point to the commit this is addressing

 drivers/usb/dwc3/gadget.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 4780983..4d98fbf 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -783,8 +783,6 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep)
 
 	trace_dwc3_gadget_ep_disable(dep);
 
-	dwc3_remove_requests(dwc, dep);
-
 	/* make sure HW endpoint isn't stalled */
 	if (dep->flags & DWC3_EP_STALL)
 		__dwc3_gadget_ep_set_halt(dep, 0, false);
@@ -803,6 +801,8 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep)
 		dep->endpoint.desc = NULL;
 	}
 
+	dwc3_remove_requests(dwc, dep);
+
 	return 0;
 }
 
@@ -1617,7 +1617,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
 {
 	struct dwc3		*dwc = dep->dwc;
 
-	if (!dep->endpoint.desc || !dwc->pullups_connected) {
+	if (!dep->endpoint.desc || !dwc->pullups_connected || !dwc->connected) {
 		dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n",
 				dep->name);
 		return -ESHUTDOWN;
@@ -2247,6 +2247,7 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
 	if (!is_on) {
 		u32 count;
 
+		dwc->connected = false;
 		/*
 		 * In the Synopsis DesignWare Cores USB3 Databook Rev. 3.30a
 		 * Section 4.1.8 Table 4-7, it states that for a device-initiated
@@ -3329,8 +3330,6 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
 {
 	u32			reg;
 
-	dwc->connected = true;
-
 	/*
 	 * WORKAROUND: DWC3 revisions <1.88a have an issue which
 	 * would cause a missing Disconnect Event if there's a
@@ -3370,6 +3369,7 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
 	 * transfers."
 	 */
 	dwc3_stop_active_transfers(dwc);
+	dwc->connected = true;
 
 	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
 	reg &= ~DWC3_DCTL_TSTCTRL_MASK;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2] usb: dwc3: gadget: Prevent EP queuing while stopping transfers

[Thinh Nguyen]

Wesley Cheng wrote:
> In the situations where the DWC3 gadget stops active transfers, once
> calling the dwc3_gadget_giveback(), there is a chance where a function
> driver can queue a new USB request in between the time where the dwc3
> lock has been released and re-aquired.  This occurs after we've already
> issued an ENDXFER command.  When the stop active transfers continues
> to remove USB requests from all dep lists, the newly added request will
> also be removed, while controller still has an active TRB for it.
> This can lead to the controller accessing an unmapped memory address.
> 
> Fix this by ensuring parameters to prevent EP queuing are set before
> calling the stop active transfers API.
> 
> Fixes: ae7e86108b12 ("usb: dwc3: Stop active transfers before halting the controller")
> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
> ---
> Changes since V1:
>  - Added Fixes tag to point to the commit this is addressing
> 
>  drivers/usb/dwc3/gadget.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 4780983..4d98fbf 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -783,8 +783,6 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep)
>  
>  	trace_dwc3_gadget_ep_disable(dep);
>  
> -	dwc3_remove_requests(dwc, dep);
> -
>  	/* make sure HW endpoint isn't stalled */
>  	if (dep->flags & DWC3_EP_STALL)
>  		__dwc3_gadget_ep_set_halt(dep, 0, false);
> @@ -803,6 +801,8 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep)
>  		dep->endpoint.desc = NULL;
>  	}
>  
> +	dwc3_remove_requests(dwc, dep);
> +
>  	return 0;
>  }
>  
> @@ -1617,7 +1617,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
>  {
>  	struct dwc3		*dwc = dep->dwc;
>  
> -	if (!dep->endpoint.desc || !dwc->pullups_connected) {
> +	if (!dep->endpoint.desc || !dwc->pullups_connected || !dwc->connected) {
>  		dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n",
>  				dep->name);
>  		return -ESHUTDOWN;
> @@ -2247,6 +2247,7 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>  	if (!is_on) {
>  		u32 count;
>  
> +		dwc->connected = false;


You want to set this before __dwc3_gadget_stop() right? Then you may
want to remove this same setting few lines below this. Otherwise, this
looks good.

Thanks,
Thinh


>  		/*
>  		 * In the Synopsis DesignWare Cores USB3 Databook Rev. 3.30a
>  		 * Section 4.1.8 Table 4-7, it states that for a device-initiated
> @@ -3329,8 +3330,6 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
>  {
>  	u32			reg;
>  
> -	dwc->connected = true;
> -
>  	/*
>  	 * WORKAROUND: DWC3 revisions <1.88a have an issue which
>  	 * would cause a missing Disconnect Event if there's a
> @@ -3370,6 +3369,7 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
>  	 * transfers."
>  	 */
>  	dwc3_stop_active_transfers(dwc);
> +	dwc->connected = true;
>  
>  	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>  	reg &= ~DWC3_DCTL_TSTCTRL_MASK;
> 

Re: [PATCH v2] usb: dwc3: gadget: Prevent EP queuing while stopping transfers

[Wesley Cheng]

On 3/11/2021 3:18 PM, Thinh Nguyen wrote:
> Wesley Cheng wrote:
>> In the situations where the DWC3 gadget stops active transfers, once
>> calling the dwc3_gadget_giveback(), there is a chance where a function
>> driver can queue a new USB request in between the time where the dwc3
>> lock has been released and re-aquired.  This occurs after we've already
>> issued an ENDXFER command.  When the stop active transfers continues
>> to remove USB requests from all dep lists, the newly added request will
>> also be removed, while controller still has an active TRB for it.
>> This can lead to the controller accessing an unmapped memory address.
>>
>> Fix this by ensuring parameters to prevent EP queuing are set before
>> calling the stop active transfers API.
>>
>> Fixes: ae7e86108b12 ("usb: dwc3: Stop active transfers before halting the controller")
>> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
>> ---
>> Changes since V1:
>>  - Added Fixes tag to point to the commit this is addressing
>>
>>  drivers/usb/dwc3/gadget.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 4780983..4d98fbf 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -783,8 +783,6 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep)
>>  
>>  	trace_dwc3_gadget_ep_disable(dep);
>>  
>> -	dwc3_remove_requests(dwc, dep);
>> -
>>  	/* make sure HW endpoint isn't stalled */
>>  	if (dep->flags & DWC3_EP_STALL)
>>  		__dwc3_gadget_ep_set_halt(dep, 0, false);
>> @@ -803,6 +801,8 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep)
>>  		dep->endpoint.desc = NULL;
>>  	}
>>  
>> +	dwc3_remove_requests(dwc, dep);
>> +
>>  	return 0;
>>  }
>>  
>> @@ -1617,7 +1617,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
>>  {
>>  	struct dwc3		*dwc = dep->dwc;
>>  
>> -	if (!dep->endpoint.desc || !dwc->pullups_connected) {
>> +	if (!dep->endpoint.desc || !dwc->pullups_connected || !dwc->connected) {
>>  		dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n",
>>  				dep->name);
>>  		return -ESHUTDOWN;
>> @@ -2247,6 +2247,7 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>>  	if (!is_on) {
>>  		u32 count;
>>  
>> +		dwc->connected = false;
> 
> 
> You want to set this before __dwc3_gadget_stop() right? Then you may
> want to remove this same setting few lines below this. Otherwise, this
> looks good.
> 
> Thanks,
> Thinh
> 
Hi Thinh,

Ah, good catch, must've missed that while porting this over from my
local branch.  Will remove that connected false setting a few lines down.

Thanks
Wesley Cheng

> 
>>  		/*
>>  		 * In the Synopsis DesignWare Cores USB3 Databook Rev. 3.30a
>>  		 * Section 4.1.8 Table 4-7, it states that for a device-initiated
>> @@ -3329,8 +3330,6 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
>>  {
>>  	u32			reg;
>>  
>> -	dwc->connected = true;
>> -
>>  	/*
>>  	 * WORKAROUND: DWC3 revisions <1.88a have an issue which
>>  	 * would cause a missing Disconnect Event if there's a
>> @@ -3370,6 +3369,7 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
>>  	 * transfers."
>>  	 */
>>  	dwc3_stop_active_transfers(dwc);
>> +	dwc->connected = true;
>>  
>>  	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>>  	reg &= ~DWC3_DCTL_TSTCTRL_MASK;
>>
> 

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project