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: <e63ba783-f5a4-4442-8736-987a3b134e7f@rowland.harvard.edu>
Date:   Sun, 20 Aug 2023 10:20:38 -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 Sat, Aug 19, 2023 at 12:06:53AM +0000, Thinh Nguyen wrote:
> On Fri, Aug 18, 2023, Alan Stern wrote:
> > Actually I agree with you.  When a new SETUP packet arrives before the 
> > old control transfer has finished, the UDC driver should cancel all 
> > pending requests and then invoke the ->setup() callback.  (I don't think 
> > there is a standard error code for the cancelled requests; net2280 seems 
> > to use -EPROTO whereas dummy-hcd seems to use -EOVERFLOW.)
> 
> Those are very odd choice of error codes for cancelled request. Even
> though the gadget side doesn't have very defined error codes, I try to
> follow the equivalent doc from the host side
> (driver-api/usb/error-codes.rst), which is -ECONNRESET.
> 
> Whenever I see -EPROTO, I associate that to a specific host error:
> transaction error. For -EOVERFLOW, I think of babble errors.

Do you have a suggestion for an error code that all the UDCs should use 
in this situation?  -ECONNRESET is currently being used for requests 
that were cancelled by usb_ep_dequeue().  Would -EREMOTEIO be more 
suitable for requests attached to an aborted control transfer?


> > My impression from his initial email was that something different was 
> > happening.  It sounded like his ->setup() callback was invoked with 
> > wLength = 0, but before the Raw Gadget driver was ready to queue a 
> > response request, the UDC driver sent an automatic status, at which 
> > point the host sent another SETUP packet.  So the gadget driver got two 
> > ->setup() callbacks in a row with no chance to do anything in between.
> 
> What else should the gadget driver do? There's no data stage for
> wLength=0.

So when wLength is 0, dwc3 should not automatically handle the Status 
stage.  It should wait for the gadget driver to submit an explicit 
Status-stage request.  As far as I know, all the gadget drivers will do 
this.

> > > > This may be a holdover from the early days of the Gadget subsystem.  My 
> > > > memory from back then isn't very good; I vaguely recall that the first 
> > > > UDC drivers would queue their automatic Status-stage requests if wLength 
> > > > was 0 and ->setup() returned 0 (which would explain why 
> > > > USB_GADGET_DELAYED_STATUS had to be invented).  Unless I'm completely 
> > > > confused, that's not how UDC drivers are supposed to act now.
> > 
> > I did a little checking.  The USB_GADGET_DELAYED_STATUS mechanism was 
> > introduced for use by the composite framework -- not in order to make a 
> > UDC driver work properly.
> 
> Hm... perhaps we can update so that it's applicable outside of the
> composite framework. Currently dwc3 treats it as such, which may not
> work if the gadget driver does not know about USB_GADGET_DELAYED_STATUS.

I think USB_GADGET_DELAYED_STATUS belongs entirely inside the composite 
framework and it should not be used by UDC drivers at all.

That return code makes some sense in composite.c, because the composite 
framework juggles several function drivers in a single gadget.  It has 
to know when all of them are ready to complete a configuration change; 
it can't assume each function is ready when the callback returns.

An alternative approach for composite.c would be _always_ to assume that 
functions aren't ready until they have called 
usb_composite_setup_continue().  However, doing this would require 
auditing each function driver.


> dwc3 parse the SETUP data and determine whether it's a 3-state or
> 2-stage control transfer. If wLength > 0, then it must be a 3-stage
> control transfer. The dwc3 driver would not queue the status immediately
> until the data stage is completed.
> 
> To enforce the gadget driver to manually queue the status would take
> some effort to ensure all the UDC driver comply to it. We would also
> need to update the composite framework.

The composite framework already does the right thing.  And as Andrey 
said, most if not all of the other UDC drivers already behave this way.

Alan Stern