linux-kernel - [PATCH 1/1] usb: xhci: Queue URB_ZERO

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: <20250908130217.3896fd48.michal.pecio@gmail.com>
Date: Mon, 8 Sep 2025 13:02:17 +0200
From: Michal Pecio <michal.pecio@...il.com>
To: Mathias Nyman <mathias.nyman@...ux.intel.com>
Cc: linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: [PATCH 1/1] usb: xhci: Queue URB_ZERO_PACKET as one TD

This feature was implemented with zero consideration of how the event
handler will deal with halting multi-TD URBs. Sadly, it doesn't expect
such things at all. The only other multi-TD URBs which can halt are
control, but they are represented as one xhci_td and produce one event,
so either the whole URB succeeds, or it is wholly thrown out.

With URB_ZERO_PACKET, the handler is aware of two distinct TDs and if
the initial data transfer TD halts, it tries the zero packet TD next.
Only when that also halts or completes is the URB given back, and with
status and actual_length determined solely by the latter TD.

Fixing the event handler is possible: cancel all TDs of a halted URB
and ensure it will be given back with correct status regardless of how
and when are the individual TDs given back.

Or we can eliminate the problem using the control URB's trick, namely:

1. no longer allocate two xhci_td for those URBs
2. count the zero packet TD as part of the data transfer TD
3. clear IOC on the data transfer TD when a zero packet TD follows it

On success, we get one event on the zero packet TD and give back the
URB normally, confident that the whole transfer worked out.

On errors or Stopped events, usual partial transfer calculations yield
actual transfer size, including cases of events on the zero packet TD.
The whole URB is cancelled at once and given back with correct status.

The latter approach seems advantageous for a few reasons:
* It doesn't require the event handler to deal with an obscure and
  evidently poorly tested corner case.
* We have no real need for the data transfer completion event and it
  only wastes CPU cycles. Same situation in control URBs, so it seems
  unlikely that we will ever want to convert them to multi-TD model.
* Event handling code evolves a lot, while URB_ZERO_PACKET does not.
  This patch can be easily backported to stable kernels.

Issue found by code review, but reproduced and tested. Few drivers use
this flag, but we can patch any (e.g. usbserial) to set it needlessly.
Then it's a matter of sending wMaxPacketSize bytes while the USB link
is broken, for example interrupted D- line of a full speed device.

Signed-off-by: Michal Pecio <michal.pecio@...il.com>
---
 drivers/usb/host/xhci-ring.c | 23 +++++++++++++----------
 drivers/usb/host/xhci.c      |  5 -----
 2 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index a879c569c2f2..9a963db11167 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3664,6 +3664,14 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
 		addr = (u64) urb->transfer_dma;
 		block_len = full_len;
 	}
+	/* Deal with URB_ZERO_PACKET - need one more td/trb */
+	if (urb->transfer_flags & URB_ZERO_PACKET &&
+	    usb_endpoint_is_bulk_out(&urb->ep->desc) &&
+	    urb->transfer_buffer_length > 0 &&
+	    !(urb->transfer_buffer_length % usb_endpoint_maxp(&urb->ep->desc))) {
+		need_zero_pkt = true;
+		num_trbs++;
+	}
 	ret = prepare_transfer(xhci, xhci->devs[slot_id],
 			ep_index, urb->stream_id,
 			num_trbs, urb, 0, mem_flags);
@@ -3672,10 +3680,6 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
 
 	urb_priv = urb->hcpriv;
 
-	/* Deal with URB_ZERO_PACKET - need one more td/trb */
-	if (urb->transfer_flags & URB_ZERO_PACKET && urb_priv->num_tds > 1)
-		need_zero_pkt = true;
-
 	td = &urb_priv->td[0];
 
 	/*
@@ -3724,7 +3728,9 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
 		}
 		if (enqd_len + trb_buff_len >= full_len) {
 			field &= ~TRB_CHAIN;
-			field |= TRB_IOC;
+			/* zero packet TRB isn't chained, but it steals our IOC */
+			if (!need_zero_pkt)
+				field |= TRB_IOC;
 			more_trbs_coming = false;
 			td->end_trb = ring->enqueue;
 			td->end_seg = ring->enq_seg;
@@ -3772,11 +3778,8 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
 	}
 
 	if (need_zero_pkt) {
-		ret = prepare_transfer(xhci, xhci->devs[slot_id],
-				       ep_index, urb->stream_id,
-				       1, urb, 1, mem_flags);
-		urb_priv->td[1].end_trb = ring->enqueue;
-		urb_priv->td[1].end_seg = ring->enq_seg;
+		td->end_trb = ring->enqueue;
+		td->end_seg = ring->enq_seg;
 		field = TRB_TYPE(TRB_NORMAL) | ring->cycle_state | TRB_IOC;
 		queue_trb(xhci, ring, 0, 0, 0, TRB_INTR_TARGET(0), field);
 	}
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index d0662f9a0c2e..75d73d2add1a 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1626,11 +1626,6 @@ static int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 
 	if (usb_endpoint_xfer_isoc(&urb->ep->desc))
 		num_tds = urb->number_of_packets;
-	else if (usb_endpoint_is_bulk_out(&urb->ep->desc) &&
-	    urb->transfer_buffer_length > 0 &&
-	    urb->transfer_flags & URB_ZERO_PACKET &&
-	    !(urb->transfer_buffer_length % usb_endpoint_maxp(&urb->ep->desc)))
-		num_tds = 2;
 	else
 		num_tds = 1;
 
-- 
2.48.1