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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <35079ed1-d730-6dc7-9e4c-639b33115c79@synopsys.com>
Date:   Thu, 11 Mar 2021 23:18:20 +0000
From:   Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To:     Wesley Cheng <wcheng@...eaurora.org>,
        "balbi@...nel.org" <balbi@...nel.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>
Subject: Re: [PATCH v2] usb: dwc3: gadget: Prevent EP queuing while stopping
 transfers

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@...eaurora.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;
>