[PATCH v2] usb: core: hub: Re-guess ep0 maxpacket size on reset

[Pengfei Lv]

Per USB 2.0 spec section 5.5.3, the ep0 maxpacket size is fixed for
all device speeds except full-speed devices, where it can be 8, 16,
32, or 64 bytes and can only be determined by reading the device
descriptor.  During the initial enumeration of a full-speed device,
the USB core guesses 64 bytes (hub_port_init() with initial=true);
after reading the descriptor, the real value (e.g. 8) is stored in
udev->ep0.desc.wMaxPacketSize and propagated to the xHCI ep0 context
via an Evaluate Context command.

After a port reset (usb_reset_and_verify_device() -> hub_port_init()
with initial=false), the "if (initial)" guard added by commit
ff33299ec8bb ("USB: core: Fix race by not overwriting udev->descriptor in hub_port_init()")
skips the ep0 maxpacket guess block, so
udev->ep0.desc.wMaxPacketSize retains the real value (e.g. 8).  The
xHCI Reset Device command preserves the ep0 context as well, so the
xHC ep0 max_packet also stays at 8.

When the subsequent re-enumeration then uses the "new scheme" and
issues a 64-byte GET_DESCRIPTOR (get_bMaxPacketSize0 with size=64),
the xHC queues a Data TRB of length 64 but applies ep0 max_packet=8
for short-packet detection.  If the device's first data packet is
exactly 8 bytes and it sends no ZLP, the xHC does not recognise a
short packet, keeps issuing IN tokens for the remaining 56 bytes, and
the control transfer hangs in the Data stage until URB timeout.

This is not merely theoretical.  It was observed in production on a
6.6 LTS kernel with a Full-Speed USB banking token (bMaxPacketSize0
= 8) in a virtual-machine USB passthrough scenario; the reset
re-enumeration hangs 100% of the time after the VM issues
USBDEVFS_RESET.  Bus captures with an Ellisys analyzer confirm that
the device sends exactly one 8-byte packet for the 64-byte request
and then stops responding to IN tokens, relying on the host's
short-packet detection with max_packet=64 to terminate the transfer.
For the subsequent 18-byte GET_DESCRIPTOR (usb_get_device_descriptor,
wLength=18) the same device sends the full 18 bytes as 8 + 8 + 2;
the final 2-byte packet is always a short packet regardless of
max_packet, so that transfer succeeds on both kernels.  This is why
the device completes initial enumeration yet hangs on reset
re-enumeration.

The device is not strictly compliant (it should continue answering
IN tokens for the 64-byte request), but this class of USB banking
token is widely deployed in enterprise environments, and the
regression introduced by commit ff33299ec8bb ("USB: core: Fix race by not overwriting udev->descriptor in hub_port_init()") breaks a configuration that
previously worked.

Fix this by removing the "if (initial)" guard so that the ep0 maxpacket
size is re-guessed from udev->speed on every reset, and by calling
usb_ep0_reinit() afterwards when !initial so that the new software
value is propagated to the xHCI ep0 context via an Evaluate Context
command before the GET_DESCRIPTOR data stage.

The usb_ep0_reinit() call is necessary because commit e34900f46cd6
("xhci: Reconfigure endpoint 0 max packet size only during endpoint reset")
moved the ep0 maxpacket sync point from xhci_urb_enqueue()
(xhci_check_maxpacket, lazy per-URB) to xhci_endpoint_reset()
(xhci_check_ep0_maxpacket, eager), reachable only via
usb_ep0_reinit() -> usb_enable_endpoint(reset_ep=true) ->
usb_hcd_reset_endpoint() -> xhci_endpoint_reset().  Without the
explicit usb_ep0_reinit() after the re-guess, no Evaluate Context
command is issued on the reset path: the pre-existing
usb_ep0_reinit() in usb_reset_and_verify_device() runs before
hub_port_init() and thus syncs the stale value, and the subsequent
hub_enable_device() -> xhci_setup_device(SETUP_CONTEXT_ONLY) returns
early when the slot is already in SLOT_STATE_DEFAULT.

The !initial guard on the new usb_ep0_reinit() call ensures that
initial enumeration is unaffected: on first enumeration the ep0
context is initialised by the Address Device command, so no extra
Evaluate Context is needed.  For high-speed, low-speed, and
SuperSpeed-or-faster devices, xhci_endpoint_reset() returns early
(speed != USB_SPEED_FULL), so the call is a no-op on xHCI, and on
non-xHCI host controllers endpoint_reset for control endpoints is
likewise a no-op (e.g. ehci_endpoint_reset only handles bulk and
interrupt endpoints).  The descriptor-immutability checks (!initial
&& maxp0 != udev->descriptor.bMaxPacketSize0) remain unchanged;
udev->ep0.desc is not part of udev->descriptor and is not the field
involved in the read_descriptors() race that commit ff33299ec8bb ("USB: core: Fix race by not overwriting udev->descriptor in hub_port_init()") fixed, so
restoring the wMaxPacketSize re-guess does not reopen that race.

Fixes: ff33299ec8bb ("USB: core: Fix race by not overwriting udev->descriptor in hub_port_init()")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Pengfei Lv <lvpengfei20@xxxxxxxxxx>
---

Changes in v2:
- Add usb_ep0_reinit(udev) after the ep0 maxpacket re-guess block,
  guarded by !initial, to propagate the new wMaxPacketSize to the
  xHCI ep0 context.  v1 only removed the if(initial) guard, which
  is insufficient after commit e34900f46cd6 ("xhci: Reconfigure endpoint 0 max packet size only during endpoint reset")
  moved the xHCI ep0
  maxpacket sync point from xhci_urb_enqueue() (xhci_check_maxpacket,
  lazy per-URB) to xhci_endpoint_reset() (xhci_check_ep0_maxpacket,
  eager), reachable only via usb_ep0_reinit().  The !initial guard
  avoids a redundant Evaluate Context on first enumeration, where the
  Address Device command already initialises the ep0 context to 64.
  (Sashiko AI review)
- Expand commit message with observed-in-production evidence and the
  wLength=64 vs wLength=18 device behavior distinction, per Alan
  Stern's review.
- Note: on kernels before e34900f46cd6 (e.g. 6.6 LTS), the xHCI ep0
  maxpacket sync happens lazily in xhci_urb_enqueue() via
  xhci_check_maxpacket(), so removing the if(initial) guard alone is
  sufficient and the extra usb_ep0_reinit() call is a no-op for ep0
  maxpacket sync (xhci_endpoint_reset returns early for control
  endpoints on those kernels).  This patch targets current mainline
  where e34900f46cd6 is already present.  Would the maintainers
  prefer a separate, simpler patch (without the usb_ep0_reinit()
  addition) for stable branches predating e34900f46cd6, or is this
  single patch acceptable for backport given that the
  usb_ep0_reinit() call is harmless on those kernels?
---
 drivers/usb/core/hub.c | 49 +++++++++++++++++++++---------------------
 1 file changed, 25 insertions(+), 24 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 199d3f535a89..a6c260d39094 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4969,33 +4969,34 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
 	}
 	oldspeed = udev->speed;
 
-	if (initial) {
-		/* USB 2.0 section 5.5.3 talks about ep0 maxpacket ...
-		 * it's fixed size except for full speed devices.
+	/* USB 2.0 section 5.5.3 talks about ep0 maxpacket ...
+	 * it's fixed size except for full speed devices.
+	 */
+	switch (udev->speed) {
+	case USB_SPEED_SUPER_PLUS:
+	case USB_SPEED_SUPER:
+		udev->ep0.desc.wMaxPacketSize = cpu_to_le16(512);
+		break;
+	case USB_SPEED_HIGH:		/* fixed at 64 */
+		udev->ep0.desc.wMaxPacketSize = cpu_to_le16(64);
+		break;
+	case USB_SPEED_FULL:		/* 8, 16, 32, or 64 */
+		/* to determine the ep0 maxpacket size, try to read
+		 * the device descriptor to get bMaxPacketSize0 and
+		 * then correct our initial guess.
 		 */
-		switch (udev->speed) {
-		case USB_SPEED_SUPER_PLUS:
-		case USB_SPEED_SUPER:
-			udev->ep0.desc.wMaxPacketSize = cpu_to_le16(512);
-			break;
-		case USB_SPEED_HIGH:		/* fixed at 64 */
-			udev->ep0.desc.wMaxPacketSize = cpu_to_le16(64);
-			break;
-		case USB_SPEED_FULL:		/* 8, 16, 32, or 64 */
-			/* to determine the ep0 maxpacket size, try to read
-			 * the device descriptor to get bMaxPacketSize0 and
-			 * then correct our initial guess.
-			 */
-			udev->ep0.desc.wMaxPacketSize = cpu_to_le16(64);
-			break;
-		case USB_SPEED_LOW:		/* fixed at 8 */
-			udev->ep0.desc.wMaxPacketSize = cpu_to_le16(8);
-			break;
-		default:
-			goto fail;
-		}
+		udev->ep0.desc.wMaxPacketSize = cpu_to_le16(64);
+		break;
+	case USB_SPEED_LOW:		/* fixed at 8 */
+		udev->ep0.desc.wMaxPacketSize = cpu_to_le16(8);
+		break;
+	default:
+		goto fail;
 	}
 
+	if (!initial)
+		usb_ep0_reinit(udev);
+
 	speed = usb_speed_string(udev->speed);
 
 	/*
-- 
2.34.1