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: <20230823171757.vegbktpkk23eg34z@synopsys.com>
Date:   Wed, 23 Aug 2023 17:18:02 +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 Wed, Aug 23, 2023, Alan Stern wrote:
> On Wed, Aug 23, 2023 at 04:30:23AM +0200, Andrey Konovalov wrote:
> > I started looking into reworking the UDC drivers to drop the special
> > case for USB_GADGET_DELAYED_STATUS, but this seems more complicated.
> > 
> > First, I noticed that some of the UDC drivers only expect to handle a
> > delayed Status stage for SET_CONFIGURATION requests. (Which is

Just want to clarify that dwc3 would expect a delayed status for any
request returned with USB_GADGET_DELAYED_STATUS. The issue is that dwc3
assumes the _first_ delayed request would be SET_CONFIGURATION. Any
subsequence control request with delayed request, it assumes the device
is already in configured state.

> 
> That expectation is wrong; gadget drivers can also want to delay the 
> Status stage for a SET_INTERFACE request.  And in theory they might want 
> to delay any control-OUT transfer.

Agree. Thanks Andrey and Alan for looking into dwc3.

Regarding SET_INTERFACE, it should be fine because it should be done
while it's already in configured state, which is after
SET_CONFIGURATION. But it's true that dwc3 needs to fix this assumption
here.

> 
> > reasonable, as they were developed assuming that only the composite
> > framework might request to delay the Status stage.) In particular,
> > dwc3, cdns2, and cdns3 set the gadget state to USB_STATE_CONFIGURED
> > when handling a delayed Status stage:
> > 
> > dwc3/ep0.c:136: usb_gadget_set_state(dwc->gadget, USB_STATE_CONFIGURED);
> > cdns3/cdns3-ep0.c:739: usb_gadget_set_state(&priv_dev->gadget,
> > USB_STATE_CONFIGURED);
> > gadget/udc/cdns2/cdns2-ep0.c:572: usb_gadget_set_state(&pdev->gadget,
> > USB_STATE_CONFIGURED);
> 
> This is also wrong.  SET_CONFIGURATION can tell a gadget to install 
> config 0, in which case the state should be changed to 
> USB_STATE_ADDRESS.

Added on my TODO list.

> 
> For that matter, a gadget can undergo many state changes other than the 
> change into the CONFIGURED state.  It doesn't look like many of the UDC 
> drivers are careful about reporting them.
> 
> > So I believe an additional check for whether the request was indeed
> > SET_CONFIGURATION is required. (cdns2 and cdns3 also do other things
> > besides setting the state to USB_STATE_CONFIGURED, but it should be
> > possible to hide that under the same check.)
> > 
> > I also looked into how other UDC drivers change the gadget state to
> > USB_STATE_CONFIGURED:
> > 
> > 1. isp1760, mtu3, and bdc immediately set USB_STATE_CONFIGURED once
> > they receive a SET_CONFIGURATION request, before calling ->setup() for
> > the gadget driver;
> > 2. gr and mv_u3d do that after the ->setup() call;
> > 3. tegra does it after the first non-control endpoint is enabled;
> > 4. dwc3, cdns2, and cdns3 appear to not set USB_STATE_CONFIGURED if
> > the Status stage is not delayed;
> > 5. dwc2, cdnsp, and all other UDCs don't set USB_STATE_CONFIGURED at all.
> > 
> > I'm guessing the UDCs in #4 and #5 expect the gadget driver to set
> > USB_STATE_CONFIGURED.
> > 
> > I see that the composite framework sets the gadget state to
> > USB_STATE_CONFIGURED even if some of the functions request a delayed
> > Status stage via USB_GADGET_DELAYED_STATUS. And GadgetFS also sets the
> > state to USB_STATE_CONFIGURED before delegating the SET_CONFIGURATION
> > request to userspace. However, Raw Gadget expects the userspace to
> > issue an ioctl that sets USB_STATE_CONFIGURED before completing the
> > delayed SET_CONFIGURATION request.
> > 
> > So I am wondering: when is proper time to set USB_STATE_CONFIGURED?
> > And should this be handled by the UDC driver or the gadget driver?
> 
> The proper time isn't really well defined.  As far as the gadget driver 
> is concerned, it's when the configuration change is completed (when it 
> tells the composite framework to stop delaying the status stage).  But 
> as far as the host is concerned, it's when the Status stage completes 
> successfully.
> 
> If the Status stage of the control transfer gets corrupted, it's 
> possible to end up in a situation where the gadget believes it is 
> configured and the host believes it isn't.  Luckily this doesn't 
> happen very often, and if it does then the host should reissue the 
> transfer.
> 
> All the other state changes are (or should be) handled by the UDC 
> drivers.  I guess they can handle the changes to/from the CONFIGURED 
> state as well, although they will have to be more careful about it than 
> they are now.
> 

The dwc3 tries to handle these state changes. However, as pointed out,
it needs to be audited. We got a little over reliant on USB CV tests to
audit these state changes for us.

Thanks,
Thinh