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]

Date:   Wed, 23 Aug 2023 02:05:37 +0000
From:   Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To:     Alan Stern <stern@...land.harvard.edu>
CC:     Andrey Konovalov <andreyknvl@...il.com>,
        Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        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 Mon, Aug 21, 2023, Alan Stern wrote:
> On Mon, Aug 21, 2023 at 06:13:05PM +0200, Andrey Konovalov wrote:
> > Out of curiosity, I also did some digging around USB_GADGET_DELAYED_STATUS.
> > 
> > 1. USB_GADGET_DELAYED_STATUS was introduced in 1b9ba000177 ("usb:
> > gadget: composite: Allow function drivers to pause control
> > transfers"). It looks like it was indeed initially intended for the
> > composite framework, as nor that commit nor the directly following
> > commits use USB_GADGET_DELAYED_STATUS in any UDC drivers. However,
> > this commit had an unintended (?) side-effect of returning
> > USB_GADGET_DELAYED_STATUS from the ->setup() call of the composite
> > framework gadget driver.
> > 
> > 2. In 5bdb1dcc6330 ("usb: dwc3: ep0: handle delayed_status again"),
> > the dwc3 driver was the first one to start relying on
> > USB_GADGET_DELAYED_STATUS to decide when to avoid auto-completing the
> > Status stage (+Sebastian). The commit description mentions some
> > previous rework of the driver that made it lose the ability of handle
> > delayed Status stage handling, but I couldn't figure out the exact
> > commit it refers to.
> > 
> > 3. Following that, a few other UDC drivers also started using
> > USB_GADGET_DELAYED_STATUS, potentially using the dwc3 behavior as a
> > reference.
> > 
> > Interestingly, in 946ef68ad4e4 ("usb: gadget: ffs: Let setup() return
> > USB_GADGET_DELAYED_STATUS"), the FunctionFS composite driver had to
> > add USB_GADGET_DELAYED_STATUS to specifically avoid failures when dwc3
> > is used. This is the same "fix" that worked for me with Raw Gadget.
> > 
> > Right now dwc2, dwc3, mtu3, cdns3, bdc, and renesas have special
> > handling for USB_GADGET_DELAYED_STATUS. Surprisingly, dwc2 works with
> > Raw Gadget as is nevertheless. dwc3 fails as I described. For the
> > others, I don't have the hardware to test them.
> > 
> > I guess the proper solution would be to contain
> > USB_GADGET_DELAYED_STATUS within the composite framework and make all
> > UDCs not to handle the Status stage on their own. However, this would
> > require a collaborative effort from the maintainers of the UDC drivers
> > I mentioned.
> 
> Most if not all of the UDC drivers you listed are actively maintained.  
> It shouldn't be too hard to get people to remove the special treatment 
> of USB_GADGET_DELAYED_STATUS in them.
> 
> The necessary changes should be pretty small: Whenever wLength is 0, 
> treat any non-negative return value from ->setup() as if it were 
> USB_GADGET_DELAYED_STATUS.  This would probably end up shrinking the 
> UDC driver code a little.  :-)

I don't think a simple removal of a few lines in dwc3 and all will be
fine. This will impact some users (including us and the tools we use for
validation). Also, this will require quite a bit of reviewing and
testing.

Nonetheless, this is noted.

> 
> > An alternative would to declare USB_GADGET_DELAYED_STATUS to be usable
> > outside of the composite framework and leave everything as is
> > otherwise (but change Raw Gadget to return USB_GADGET_DELAYED_STATUS).
> > The downside is the discrepancy in the interface of different UDCs
> > (some require USB_GADGET_DELAYED_STATUS, others imply), but perhaps
> > it's not that bad provided that this discrepancy is documented.
> 
> This alternative is less desirable, because the legacy gadgets (some of 
> which don't use the composite framework) may not be compatible with it.
> 
> And as a matter of general principle, allowing UDC drivers to start 
> automatically send Status replies to 0-length control transfers is a 
> step in the wrong direction.  What we really should focus our energy on 
> is getting them to _stop_ sending automatic Status replies to 
> non-zero-length control transfers!
> 

Thanks,
Thinh