linux-kernel - Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for handling delay STATUS phase

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]

Date:   Mon, 16 Jan 2017 12:56:21 +0200
From:   Felipe Balbi <balbi@...nel.org>
To:     Baolin Wang <baolin.wang@...aro.org>, gregkh@...uxfoundation.org
Cc:     linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        linaro-kernel@...ts.linaro.org, broonie@...nel.org,
        baolin.wang@...aro.org
Subject: Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for handling delay STATUS phase

Hi,

Baolin Wang <baolin.wang@...aro.org> writes:
> When handing the SETUP packet by composite_setup(), we will release the
> dwc->lock. If we get the 'USB_GADGET_DELAYED_STATUS' result from setup
> function, which means we need to delay handling the STATUS phase.

this sentence needs a little work. Seems like it's missing some
information.

anyway, I get that we release the lock but...

> But during the lock release period, maybe the request for handling delay

execution of ->setup() itself should be locked. I can see that it's only
locked for set_config() which is rather peculiar.

What exact request you had when you triggered this? (Hint: dwc3
tracepoints print out ctrl request bytes). IIRC, only set_config() or
f->set_alt() can actually return USB_GADGET_DELAYED_STATUS.

Which gadget driver were you using when you triggered this?

Another point here is that the really robust way of fixing this is to
get rid of USB_GADGET_DELAYED_STATUS altogether and just make sure
gadget drivers know how to queue requests for all three phases of a
Control Transfer.

A lot of code will be removed from all gadget drivers and UDC drivers
while combining all of it in a single place in composite.c.

The reason I'm saying this is that other UDC drivers might have similar
races already but they just haven't triggered.

> STATUS phase has been queued into list before we set 'dwc->delayed_status'
> flag or entering 'EP0_STATUS_PHASE' phase, then we will miss the chance
> to handle the STATUS phase. Thus we should check if the request for delay
> STATUS phase has been enqueued when entering 'EP0_STATUS_PHASE' phase in
> dwc3_ep0_xfernotready(), if so, we should handle it.
>
> Signed-off-by: Baolin Wang <baolin.wang@...aro.org>
> ---
>  drivers/usb/dwc3/ep0.c |   14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 9bb1f85..e689ced 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -1123,7 +1123,21 @@ static void dwc3_ep0_xfernotready(struct dwc3 *dwc,
>  		dwc->ep0state = EP0_STATUS_PHASE;
>  
>  		if (dwc->delayed_status) {
> +			struct dwc3_ep *dep = dwc->eps[0];
> +
>  			WARN_ON_ONCE(event->endpoint_number != 1);
> +			/*
> +			 * We should handle the delay STATUS phase here if the
> +			 * request for handling delay STATUS has been queued
> +			 * into the list.
> +			 */
> +			if (!list_empty(&dep->pending_list)) {
> +				dwc->delayed_status = false;
> +				usb_gadget_set_state(&dwc->gadget,
> +						     USB_STATE_CONFIGURED);

Isn't this patch also changing the normal case when usb_ep_queue() comes
later? I guess list_empty() protects against that...

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (833 bytes)