linux-kernel - Re: [PATCH RFC RFT] usb: hcd: Add a usb_device argument to hc_driver.endpoint

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] [day] [month] [year] [list]

Message-ID: <20250415103836.0f748d63@foxbook>
Date: Tue, 15 Apr 2025 10:38:36 +0200
From: Michał Pecio <michal.pecio@...il.com>
To: Alan Stern <stern@...land.harvard.edu>, Mathias Nyman
 <mathias.nyman@...ux.intel.com>
Cc: Mathias Nyman <mathias.nyman@...el.com>, Greg Kroah-Hartman
 <gregkh@...uxfoundation.org>, Paul Menzel <pmenzel@...gen.mpg.de>,
 linux-usb@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC RFT] usb: hcd: Add a usb_device argument to
 hc_driver.endpoint_reset()

On Wed, 9 Apr 2025 10:13:50 -0400, Alan Stern wrote:
> The core does not explicitly flush endpoints before resetting a
> device. However, it does notify the class drivers' pre_reset
> callback, which is supposed to unlink all the URBs used by that
> driver.  If a driver doesn't have a pre_reset callback, the core
> unbinds it from the device (which will unlink all its URBs).  _If_
> everything is working properly, there shouldn't be any outstanding
> URBs when the reset takes place.

Thank you for clarification. This doesn't look too bad and I currently
have no concrete cases of the mechanism failing to work.

> Either way, though, the core doesn't invoke the HCD's endpoint_reset
> or endpoint_disable callback before the reset.  If you think the core
> needs to do more, or needs to issue the callbacks in a different
> order, let me know.

The problem is a matter of mismatched expectations: the core treats
endpoint_disable() as temporary, because "classic" HCDs free their
ep->hcpriv and recreate it quietly on the next URBs submission. And
their endpoint_reset() during this time simply does nothing.

But xhci considers it more permanent, like the last thing before
drop_endpoint(). It too clears ep->hcpriv, but here hcpriv is not
the endpoint state, it's usb_device pointer saved by add_endpoint()
and required for operation. The driver drops it and it's screwed.

Moving all usb_disable_interface() calls before their corresponding
usb_hcd_alloc_bandwidth() would meet xhci expectations, but IDK if
it would work in general. As far as I see in usb_set_interface() for
example, the control request is only done after successful bandwidth
allocation and the interface swap only after a successful request.

My patch addresses the problem from xhci side, by adapting to core
expectations. As far as I see, only xhci_endpoint_reset() is broken
by this add_endpoint() -> endpoint_disable() sequence, so I fix it
by removing the dependence on ep->hcpriv. Alternatively, we could
stop clearing ep->hcpriv, but I'm not sure if it's reliable. No HCD
depends on ep->hcpriv being preserved after endpoint_disable().

I will do a v2 because Mathias expressed interest in this patch for
cleanup's sake alone, but the kernel test robot found build issues.

Regards,
Michal