linux-kernel - Re: [PATCH] usb: dwc3: gadget: Stall and restart EP0 if host is unresponsive

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: <20230404011108.727htmnllj7ojwqm@synopsys.com>
Date:   Tue, 4 Apr 2023 01:11:13 +0000
From:   Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To:     Wesley Cheng <quic_wcheng@...cinc.com>
CC:     "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>,
        "quic_ugoswami@...cinc.com" <quic_ugoswami@...cinc.com>
Subject: Re: [PATCH] usb: dwc3: gadget: Stall and restart EP0 if host is
 unresponsive

On Fri, Mar 31, 2023, Wesley Cheng wrote:
> It was observed that there are hosts that may complete pending SETUP
> transactions before the stop active transfers and controller halt occurs,
> leading to lingering endxfer commands on DEPs on subsequent pullup/gadget
> start iterations.

Can you clarify this a bit further? Even though the controller is
halted, you still observed activity?

> 
> dwc3_gadget_ep_disable   name=ep8in flags=0x3009  direction=1
> dwc3_gadget_ep_disable   name=ep4in flags=1  direction=1
> dwc3_gadget_ep_disable   name=ep3out flags=1  direction=0
> usb_gadget_disconnect   deactivated=0  connected=0  ret=0
> 
> The sequence shows that the USB gadget disconnect (dwc3_gadget_pullup(0))
> routine completed successfully, allowing for the USB gadget to proceed with
> a USB gadget connect.  However, if this occurs the system runs into an
> issue where:
> 
> BUG: spinlock already unlocked on CPU
>  spin_bug+0x0
>  dwc3_remove_requests+0x278
>  dwc3_ep0_out_start+0xb0
>  __dwc3_gadget_start+0x25c
> 
> This is due to the pending endxfers, leading to gadget start (w/o lock
> held) to execute the remove requests, which will unlock the dwc3 spinlock
> as part of giveback.
> 
> To mitigate this, resolve the pending endxfers on the pullup disable path
> by:
>  1. Re-locating the SETUP phase check after stop active transfers, since
>  that is where the DWC3_EP_DELAY_STOP is potentially set.  This also allows
>  for handling of a host that may be unresponsive by using the completion
>  timeout to trigger the stall and restart for EP0.
> 
>  2. Do not call gadget stop until the poll for controller halt is
>  completed.  DEVTEN is cleared as part of gadget stop, so the intention to
>  allow ep0 events to continue while waiting for controller halt is not
>  happening.
> 
> Fixes: c96683798e27 ("usb: dwc3: ep0: Don't prepare beyond Setup stage")
> Signed-off-by: Wesley Cheng <quic_wcheng@...cinc.com>
> ---
>  drivers/usb/dwc3/gadget.c | 101 ++++++++++++++++++++++----------------
>  1 file changed, 58 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 3c63fa97a680..9715de8e99bc 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -139,6 +139,24 @@ int dwc3_gadget_set_link_state(struct dwc3 *dwc, enum dwc3_link_state state)
>  	return -ETIMEDOUT;
>  }
>  
> +static void dwc3_ep0_reset_state(struct dwc3 *dwc)
> +{
> +	unsigned int	dir;
> +
> +	if (dwc->ep0state != EP0_SETUP_PHASE) {
> +		dir = !!dwc->ep0_expect_in;
> +		if (dwc->ep0state == EP0_DATA_PHASE)
> +			dwc3_ep0_end_control_data(dwc, dwc->eps[dir]);
> +		else
> +			dwc3_ep0_end_control_data(dwc, dwc->eps[!dir]);
> +
> +		dwc->eps[0]->trb_enqueue = 0;
> +		dwc->eps[1]->trb_enqueue = 0;
> +
> +		dwc3_ep0_stall_and_restart(dwc);
> +	}
> +}
> +

Can we separate refactoring changes other functional changes? It's
difficult to review with too many things to keep track of.

Thanks,
Thinh