Skip to contentSkip to conversation

usb: xhci: Make usb_host_endpoint.hcpriv survive endpoint_disable()

PATCH In review Michal Pecio 2 participants 4 messages Mar 30 – Apr 1

Subsystems (2)
USB USB xHCI driver
lore
Share & tools
lore

Michal Pecio

author
#0 lore raw

PATCH 1/1 usb: xhci: Make usb_host_endpoint.hcpriv survive endpoint_disable()

xHCI hardware maintains its endpoint state between add_endpoint() and drop_endpoint() calls followed by successful check_bandwidth(). So does the driver.

Core may call endpoint_disable() during xHCI endpoint life, so don't clear host_ep->hcpriv then, because this breaks endpoint_reset().

If a driver calls usb_set_interface(), submits URBs which make host sequence state non-zero and calls usb_clear_halt(), the device clears its sequence state but xhci_endpoint_reset() bails out. The next URB
malfunctions: USB2 loses one packet, USB3 gets Transaction Error or may not complete at all on some (buggy?) HCs from ASMedia and AMD. This is triggered by uvcvideo on bulk video devices.

The code was copied from ehci_endpoint_disable() but it isn't needed here - hcpriv should only be NULL on emulated root hub endpoints. It might prevent resetting and inadvertently enabling a disabled and dropped endpoint, but core shouldn't try to reset dropped endpoints.

Document xhci requirements regarding hcpriv. They are currently met.

Fixes 18b74067ac78 ("xhci: Fix use-after-free regression in xhci clear hub TT implementation")
Cc [email protected]
Signed-off-by Michal Pecio <[email protected]>
---
 drivers/usb/host/xhci.c | 1 -
 include/linux/usb.h     | 3 ++-
 2 files changed, 2 insertions(+), 2 deletions(-)
drivers/usb/host/xhci.c +0 -1
index 2f7e6544e5ae..849a568d0e63 100644
@@ -3353,7 +3353,6 @@ static void xhci_endpoint_disable(struct usb_hcd *hcd,
 		xhci_dbg(xhci, "endpoint disable with ep_state 0x%x\n",
 			 ep->ep_state);
 done:
-	host_ep->hcpriv = NULL;
 	spin_unlock_irqrestore(&xhci->lock, flags);
 }
include/linux/usb.h +2 -1
index 04277af4bb9d..27e95eade121 100644
@@ -54,7 +54,8 @@ struct ep_device;
  * @eusb2_isoc_ep_comp: eUSB2 isoc companion descriptor for this endpoint
  * @urb_list: urbs queued to this endpoint; maintained by usbcore
  * @hcpriv: for use by HCD; typically holds hardware dma queue head (QH)
- *	with one or more transfer descriptors (TDs) per urb
+ *	with one or more transfer descriptors (TDs) per urb; must be preserved
+ *	by core while BW is allocated for the endpoint
  * @ep_dev: ep_device for sysfs info
  * @extra: descriptors following this endpoint in the configuration
  * @extralen: how many bytes of "extra" are valid
Signature · 1 line
2.48.1
#1 lore raw
Quoted 7 lines · Michal Pecio

xHCI hardware maintains its endpoint state between add_endpoint() and drop_endpoint() calls followed by successful check_bandwidth(). So does the driver.

Core may call endpoint_disable() during xHCI endpoint life, so don't clear host_ep->hcpriv then, because this breaks endpoint_reset().

Quoted 6 lines

If a driver calls usb_set_interface(), submits URBs which make host sequence state non-zero and calls usb_clear_halt(), the device clears its sequence state but xhci_endpoint_reset() bails out. The next URB
malfunctions: USB2 loses one packet, USB3 gets Transaction Error or may not complete at all on some (buggy?) HCs from ASMedia and AMD. This is triggered by uvcvideo on bulk video devices.

Were you able to trigger a usb_clear_halt() called with ep->hcpriv == NULL, causing a toggle/seq mismatch?

The ep->hcpriv should be set back correctly in usb_set_interface():

usb_set_interface()
usb_hcd_alloc_bandwidth()

    hcd->driver->add_endpoint()
      xhci_add_endpoint()
        ep->hcpriv = udev;

I'm not against this patch, but would like to understand how we end here

Thanks
Mathias

author
#2 lore raw
Quoted 18 lines · Mathias Nyman
Quoted 7 lines · Michal Pecio

xHCI hardware maintains its endpoint state between add_endpoint() and drop_endpoint() calls followed by successful check_bandwidth(). So does the driver.

Core may call endpoint_disable() during xHCI endpoint life, so don't clear host_ep->hcpriv then, because this breaks endpoint_reset().

Quoted 6 lines

If a driver calls usb_set_interface(), submits URBs which make host sequence state non-zero and calls usb_clear_halt(), the device clears its sequence state but xhci_endpoint_reset() bails out. The next URB
malfunctions: USB2 loses one packet, USB3 gets Transaction Error or may not complete at all on some (buggy?) HCs from ASMedia and AMD. This is triggered by uvcvideo on bulk video devices.

Were you able to trigger a usb_clear_halt() called with ep->hcpriv == NULL, causing a toggle/seq mismatch?

Quoted 7 lines

The ep->hcpriv should be set back correctly in usb_set_interface():

usb_set_interface()
usb_hcd_alloc_bandwidth()

     hcd->driver->add_endpoint()
       xhci_add_endpoint()
         ep->hcpriv = udev;

right, and later:

usb_disable_interface(dev, iface, true)
usb_disable_endpoint(dev, ..., true)
usb_hcd_disable_endpoint(dev, ep)
hcd->driver->endpoint_disable(hcd, ep)
usb_enable_interface(dev, iface, true)
usb_enable_endpoint(dev, ..., true)
usb_hcd_reset_endpoint(dev, ep)
hcd->driver->endpoint_reset(hcd, ep)

So it seems set_interface() is broken and doesn't actually reset host sequence state (while the device is supposed to reset its own).

This alone is rarely a problem because the endpoint is usually "fresh".

But uvcvideo calls usb_clear_halt() *after* stopping a bulk stream, because that's what Windows does. Then sequence state is random and gets cleared only on the device, because hcpriv is still NULL.

The next URB gets Transaction Error, the host endpoint is reset and another URB succeeds (because we restart the endpoint unconditionally).

Regards,
Michal

#3 lore raw
Quoted 29 lines · Michal Pecio
Quoted 18 lines · Mathias Nyman
Quoted 7 lines · Michal Pecio

xHCI hardware maintains its endpoint state between add_endpoint() and drop_endpoint() calls followed by successful check_bandwidth(). So does the driver.

Core may call endpoint_disable() during xHCI endpoint life, so don't clear host_ep->hcpriv then, because this breaks endpoint_reset().

Quoted 6 lines

If a driver calls usb_set_interface(), submits URBs which make host sequence state non-zero and calls usb_clear_halt(), the device clears its sequence state but xhci_endpoint_reset() bails out. The next URB
malfunctions: USB2 loses one packet, USB3 gets Transaction Error or may not complete at all on some (buggy?) HCs from ASMedia and AMD. This is triggered by uvcvideo on bulk video devices.

Were you able to trigger a usb_clear_halt() called with ep->hcpriv == NULL, causing a toggle/seq mismatch?

Quoted 7 lines

The ep->hcpriv should be set back correctly in usb_set_interface():

usb_set_interface()

    usb_hcd_alloc_bandwidth()
      hcd->driver->add_endpoint()
        xhci_add_endpoint()
          ep->hcpriv = udev;

right, and later:

Quoted 9 lines

usb_disable_interface(dev, iface, true)
usb_disable_endpoint(dev, ..., true)
usb_hcd_disable_endpoint(dev, ep)
hcd->driver->endpoint_disable(hcd, ep)
usb_enable_interface(dev, iface, true)
usb_enable_endpoint(dev, ..., true)
usb_hcd_reset_endpoint(dev, ep)
hcd->driver->endpoint_reset(hcd, ep)

True, thanks, usb_set_interface() calls usb_disable_interface() twice. First time just to flush pending URBs, second time with reset_hardware flag set.

Adding patch to queue

-Mathias