linux-kernel - Re: dwc3: unusual handling of setup requests with wLength == 0

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: <08a3759d-4c6b-4034-8516-685e4d96a41e@rowland.harvard.edu>
Date:   Wed, 23 Aug 2023 15:19:47 -0400
From:   Alan Stern <stern@...land.harvard.edu>
To:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>
Cc:     Andrey Konovalov <andreyknvl@...il.com>,
        Felipe Balbi <balbi@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        USB list <linux-usb@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: dwc3: unusual handling of setup requests with wLength == 0

On Wed, Aug 23, 2023 at 05:59:07PM +0000, Thinh Nguyen wrote:
> On Wed, Aug 23, 2023, Alan Stern wrote:
> > STALL is not a valid status for usb_requests on the gadget side; it 
> > applies only on the host side (the host doesn't halt its endpoints).
> 
> The host can send a CLEAR_FEATURE(halt_ep). This will reset the data
> sequence of the endpoint. In xhci spec (section 4.6.8), it suggests to
> send this when the endpoint is reset. The endpoint is reset typically
> when there's a transaction error.

It's important to be careful about the distinction between an actual 
endpoint in the gadget and the logical representation of an endpoint 
inside a host controller.  The host cannot reset the first; it can only 
reset the second.

So yes, the usb_clear_halt() routine on the host does a 
CLEAR_FEATURE(HALT) control transfer and then calls 
usb_reset_endpoint(), which calls usb_hcd_reset_endpoint().

> The problem here is that typical protocol spec like MSC/UVC don't
> specify how to handle CLEAR_FEATURE(halt_ep).

MSC does specify this.  I don't know about UVC.

> For Windows MSC driver, when the host recovers from the transaction
> error, it sends CLEAR_FEATURE(halt_ep) and expects the transfer to be
> cancelled. To synchronize with the host, the gadget driver needs to
> cancel the request. Dwc3 needs to notify the gadget driver of this.

No, that's not what happens in the Mass Storage Class.

For the Bulk-Only Transport version of MSC, when a Windows or Linux host 
detects a transaction error, it performs a USB port reset.  This clears 
all the state on the gadget.  The gadget gets re-enumerated, and the 
host proceeds to re-issue the MSC command.  The gadget driver doesn't 
need any special notifications; outstanding requests get cancelled as a 
normal part of the reset handling.

(In fact, this is not what the BOT spec says to do.  It says that when 
the host detects a transaction error, it should a Bulk-Only Mass Storage 
Reset -- this is a special class-specific control transfer.  In 
response, the gadget driver is supposed to reset its internal state and 
cancel all of its outstanding requests.  Then the host issues 
CLEAR_FEATURE(HALT) to both the bulk-IN and bulk-OUT endpoints and 
proceeds to issue its next MSC command.  A lot of MSC devices don't 
handle this properly, probably because Windows didn't use this 
approach.)

In the UAS version of MSC, the endpoints never halt.  If there's a 
transaction error, the host simply re-issues the transaction.  If that 
fails too, error recovery is started by the SCSI layer; it involves a 
USB port reset.

But as you can see, in each case the UDC driver doesn't have to cancel 
anything in particular when it gets a Clear-Halt.

> For other class driver, it may expect the transfer to resume after data
> sequence reset.

Indeed.  In which case, the UDC driver shouldn't cancel anything.

> As a result, for an endpoint that's STALL (or not), and if the host
> sends CLEAR_FEATURE(halt_ep), the dwc3 returns the request with some
> status code and let the gadget driver handle it. If the gadget driver
> wants to cancel the transfer, it can drop the transfer. If the gadget
> driver wants to resume, it can requeue the same requests with the saved
> status to resume where it left off.

The UDC driver should not dequeue a request merely because the endpoint 
is halted.  The gadget driver can take care of everything necessary.  
After all, it knows when an endpoint gets halted, because the gadget 
driver is what calls usb_ep_set_halt() or usb_ep_set_wedge() in the 
first place.

As for handling CLEAR_FEATURE(HALT), all the UDC driver needs to do is 
clear the HALT feature for the endpoint.  (Although if the endpoint is 
wedged, the HALT feature should not be cleared.)  It doesn't need to 
cancel any outstanding requests or inform the gadget driver in any way.

(Again, this is something that a lot of USB devices don't handle 
properly.  They get very confused if the host sends a Clear-Halt 
transfer for an endpoint that isn't halted.)

> > Putting this together, I get the following status codes:
> > 
> > -ESHUTDOWN	Request aborted because ep was disabled
> > -EREMOTEIO	Request was for an aborted control transfer
> > -ECONNRESET	Request was cancelled by usb_ep_dequeue()
> > -EXDEV		Data dropped (isoc only)
> > -EOVERFLOW	The host sent more data than the request wanted
> > 		(will never happen if the request's length is a
> > 		nonzero multiple of the maxpacket size)
> > 
> > This applies only to the .status field of struct usb_request.  Calls to 
> > usb_ep_queue() may return different error codes.
> > 
> > How does that sound?
> > 
> 
> That looks great!

At some point I'll write a patch adding this to the documentation.

Alan Stern