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: <3f52eb94-1d8c-4233-86ac-bbc78d4efce7@rowland.harvard.edu>
Date:   Mon, 21 Aug 2023 13:25:23 -0400
From:   Alan Stern <stern@...land.harvard.edu>
To:     Andrey Konovalov <andreyknvl@...il.com>
Cc:     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 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.  :-)

> 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!

Alan Stern