Re: [PATCH 4/4] usb: xhci: Reject URBs to disconnected devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: xhci: stale rhub_port state in xhci_urb_enqueue() rejects valid URBs
--

commit 3e6930cee6e8275b8d271ab4827ba5004d45037f
Author: Michal Pecio <michal.pecio@gmail.com>

usb: xhci: Reject URBs to disconnected devices

This commit extends the port link error check in xhci_urb_enqueue() to also
reject URBs with -ENODEV when the root hub port is marked disconnected or the
endpoint has EP_DROP_PENDING set.

> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index a9e47e178c280..4a893a8438c3a 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -1665,12 +1665,6 @@ static int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
>  		goto free_priv;
>  	}
>  
> -	if (xhci->devs[slot_id]->rhub_port->link_inactive) {
> -		xhci_dbg(xhci, "Can't queue urb, port error, link inactive\n");
> -		ret = -ENODEV;
> -		goto free_priv;
> -	}
> -
>  	if (xhci->xhc_state & XHCI_STATE_DYING) {
>  		xhci_dbg(xhci, "Ep 0x%x: URB %p submitted for non-responsive xHCI host.\n",
>  			 urb->ep->desc.bEndpointAddress, urb);
> @@ -1680,6 +1674,13 @@ static int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
>  
>  	ep_state = &xhci->devs[slot_id]->eps[ep_index].ep_state;
>  
> +	if (xhci->devs[slot_id]->rhub_port->link_inactive ||
> +	    !xhci->devs[slot_id]->rhub_port->connected || *ep_state & EP_DROP_PENDING) {
> +		xhci_dbg(xhci, "Can't queue urb, link error or disconnected\n");
> +		ret = -ENODEV;
> +		goto free_priv;
> +	}

[Severity: High]
Can rhub_port->connected, rhub_port->link_inactive, or EP_DROP_PENDING remain
stale here and cause xhci_urb_enqueue() to reject valid URBs (including EP0
control transfers) during device reset, resuscitation, resume, or enumeration?

Across the xHCI driver, port->connected and port->link_inactive are only
updated in drivers/usb/host/xhci-ring.c:handle_port_status() when a hardware
TRB_PORT_STATUS event is received while PORT_RESET is not set:

drivers/usb/host/xhci-ring.c:handle_port_status() {
	...
	if (!(portsc & PORT_RESET)) {
		port->link_inactive = (pls == XDEV_INACTIVE);
		port->connected = !!(portsc & PORT_CONNECT);
	}
	...
	/*
	 * xHCI port-status-change events occur when the "or" of all the
	 * status-change bits in the portsc register changes from 0 to 1.
	 * New status changes won't cause an event if any other change
	 * bits are still set.  When an event occurs, switch over to
	 * polling to avoid losing status changes.
	 */
}

Neither xhci_get_port_status(), xhci_clear_port_change_bit(), nor
xhci_discover_or_reset_device() updates port->connected or
port->link_inactive:

drivers/usb/host/xhci-hub.c:xhci_get_port_status() {
	...
	/* common wPortStatus bits */
	if (portsc & PORT_CONNECT) {
		status |= USB_PORT_STAT_CONNECTION;
		status |= xhci_port_speed(portsc);
	}
}

drivers/usb/host/xhci.c:xhci_discover_or_reset_device() {
	...
command_cleanup:
	for (i = 0; i < EP_CTX_PER_DEV; i++)
		virt_dev->eps[i].ep_state &= ~EP_DROP_PENDING;
}

When a disconnect (PORT_CSC = 1, PORT_CONNECT = 0) or link error (PORT_PLC = 1,
pls = XDEV_INACTIVE) occurs, handle_port_status() sets port->connected = 0 or
port->link_inactive = 1, leaves the status-change bits set in PORTSC, and
switches the roothub to polling. Because the xHC only generates a
TRB_PORT_STATUS event when the logical OR of all PORTSC status-change bits
transitions from 0 to 1, subsequent transitions back to PORT_CONNECT = 1 or
XDEV_U0 will not generate a new TRB_PORT_STATUS event while any change bit
remains set in PORTSC.

This appears to leave rhub_port state (or EP_DROP_PENDING) stale in several
paths:

1. Device reset or resuscitation across a disconnect or link error via
usb_reset_device() -> usb_reset_and_verify_device() -> hub_port_init() ->
hub_port_reset(), where port_dev->status_lock is held so port_event() cannot
clear PORT_CSC or PORT_PLC. On USB3 root ports, PORT_CSC or PORT_PLC stays set
throughout the reset, suppressing TRB_PORT_STATUS when the device reconnects
and reset completes. On USB2 root ports, if the first reset completes while
PORT_CSC is set, hub_port_wait_reset() clears only C_CONNECTION and returns
-EAGAIN without clearing C_RESET (PORT_RC), so PORT_RC stays set throughout
the retry reset and suppresses TRB_PORT_STATUS when the retry completes.
Although xhci_discover_or_reset_device() clears EP_DROP_PENDING,
rhub_port->connected remains 0 (or link_inactive remains 1), causing
get_bMaxPacketSize0() or usb_get_device_descriptor() in hub_port_init() to
fail with -ENODEV.

2. Delayed connection after reset completion in
drivers/usb/core/hub.c:hub_port_wait_reset():

drivers/usb/core/hub.c:hub_port_wait_reset() {
	...
		if (!(portstatus & USB_PORT_STAT_RESET) &&
		    (portstatus & USB_PORT_STAT_CONNECTION))
			break;
}

If PORT_RESET clears (setting PORT_RC = 1) before PORT_CONNECT is asserted,
handle_port_status() runs on the PORT_RC event while PORT_CONNECT is 0 and
sets port->connected = 0. When PORT_CONNECT subsequently becomes 1 while
PORT_RC is still set, no new TRB_PORT_STATUS event fires.

3. Connect bounce during hub_port_debounce(): a bounce disconnect sets
PORT_CSC = 1 (and/or PORT_PLC = 1) and port->connected = 0. If the port
reconnects during the debounce sleep while PORT_CSC is still 1, no
TRB_PORT_STATUS event fires on reconnect, and hub_port_debounce() clears
C_CONNECTION while leaving port->connected == 0.

4. Status change during active reset: if PORT_CSC or PORT_PLC becomes 1 while
PORT_RESET is active, TRB_PORT_STATUS fires while (portsc & PORT_RESET) is
true, so handle_port_status() skips updating port->connected and
port->link_inactive, and the uncleared change bit suppresses the event when
PORT_RESET clears.

5. USB3 resuscitation without a port reset in
drivers/usb/core/hub.c:hub_port_connect_change(), or transient disconnect
during wait_for_connected() in usb_port_resume():

drivers/usb/core/hub.c:hub_port_connect_change() {
	...
		if (portstatus & USB_PORT_STAT_ENABLE) {
			/*
			 * USB-3 connections are initialized automatically by
			 * the hostcontroller hardware. Therefore check for
			 * changed device descriptors before resuscitating the
			 * device.
			 */
			descr = usb_get_device_descriptor(udev);
}

If a USB3 device briefly disconnects (setting PORT_CSC = 1,
port->connected = 0, and EP_DROP_PENDING on halted/stopped endpoints) and
reconnects to U0 while PORT_CSC is still 1, no second TRB_PORT_STATUS event
fires. When hub_port_connect_change() or finish_port_resume() then submits EP0
control URBs without calling xhci_discover_or_reset_device(), won't
xhci_urb_enqueue() reject them with -ENODEV due to stale
rhub_port->connected == 0 or stale EP_DROP_PENDING on EP0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029075409.4b203658.michal.pecio@gmail.com?part=4