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: <CA+fCnZcmb78DMaffb3cq2JeCNxcGBeyt8hxeJq3SaTTkbZ3ewA@mail.gmail.com>
Date:   Mon, 21 Aug 2023 18:13:05 +0200
From:   Andrey Konovalov <andreyknvl@...il.com>
To:     Alan Stern <stern@...land.harvard.edu>,
        Thinh Nguyen <thinh.nguyen@...opsys.com>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:     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 Sun, Aug 20, 2023 at 4:20 PM Alan Stern <stern@...land.harvard.edu> wrote:
>
> > > > > 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.

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.

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.

Thank you!