linux-kernel - Re: [PATCH] usb: dwc3: gadget: Do not clear ep delayed stop flag during ep disable

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] [day] [month] [year] [list]

Date:   Wed, 21 Sep 2022 02:18:02 +0000
From:   Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To:     Wesley Cheng <quic_wcheng@...cinc.com>
CC:     "balbi@...nel.org" <balbi@...nel.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "quic_jackp@...cinc.com" <quic_jackp@...cinc.com>
Subject: Re: [PATCH] usb: dwc3: gadget: Do not clear ep delayed stop flag
 during ep disable

On Mon, Sep 19, 2022, Wesley Cheng wrote:
> DWC3_EP_DELAYED_STOP is utilized to defer issuing the end transfer command
> until the subsequent SETUP stage, in order to avoid end transfer timeouts.
> During cable disconnect scenarios, __dwc3_gadget_ep_disable() is
> responsible for ensuring endpoints have no active transfers pending.  Since
> dwc3_remove_request() can now exit early if the EP delayed stop is set,
> avoid clearing all DEP flags, otherwise the transition back into the SETUP
> stage won't issue an endxfer command.
> 
> Fixes: 2b2da6574e77 ("usb: dwc3: Avoid unmapping USB requests if endxfer is not complete")
> Signed-off-by: Wesley Cheng <quic_wcheng@...cinc.com>
> ---
>  drivers/usb/dwc3/gadget.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index b75e1b8b3f05..3e2baf22824b 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1011,6 +1011,7 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep)
>  {
>  	struct dwc3		*dwc = dep->dwc;
>  	u32			reg;
> +	u32			mask;
>  
>  	trace_dwc3_gadget_ep_disable(dep);
>  
> @@ -1032,7 +1033,15 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep)
>  
>  	dep->stream_capable = false;
>  	dep->type = 0;
> -	dep->flags &= DWC3_EP_TXFIFO_RESIZED;
> +	mask = DWC3_EP_TXFIFO_RESIZED;
> +	/*
> +	 * 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;
>  
>  	return 0;
>  }

The conditions are starting to get complicated... It would be nice if we
can clear all the flags after the transfer had ended. If the gadget
driver misbehaves and decides to queue a new request after it had
disabled the endpoint but before the end transfer command completes,
then DWC3_EP_DELAY_START may get set. Then things can go wrong?

Regardless, I think this should be fine.

Reviewed-by: Thinh Nguyen <Thinh.Nguyen@...opsys.com>

Thanks,
Thinh