[PATCH 03/15] usb: xhci: Don't skip on Stopped - Length Invalid

[PATCH 03/15] usb: xhci: Don't skip on Stopped - Length Invalid

[Mathias Nyman]

From: Michal Pecio <michal.pecio@gmail.com>

Up until commit d56b0b2ab142 ("usb: xhci: ensure skipped isoc TDs are
returned when isoc ring is stopped") in v6.11, the driver didn't skip
missed isochronous TDs when handling Stoppend and Stopped - Length
Invalid events. Instead, it erroneously cleared the skip flag, which
would cause the ring to get stuck, as future events won't match the
missed TD which is never removed from the queue until it's cancelled.

This buggy logic seems to have been in place substantially unchanged
since the 3.x series over 10 years ago, which probably speaks first
and foremost about relative rarity of this case in normal usage, but
by the spec I see no reason why it shouldn't be possible.

After d56b0b2ab142, TDs are immediately skipped when handling those
Stopped events. This poses a potential problem in case of Stopped -
Length Invalid, which occurs either on completed TDs (likely already
given back) or Link and No-Op TRBs. Such event won't be recognized
as matching any TD (unless it's the rare Link TRB inside a TD) and
will result in skipping all pending TDs, giving them back possibly
before they are done, risking isoc data loss and maybe UAF by HW.

As a compromise, don't skip and don't clear the skip flag on this
kind of event. Then the next event will skip missed TDs. A downside
of not handling Stopped - Length Invalid on a Link inside a TD is
that if the TD is cancelled, its actual length will not be updated
to account for TRBs (silently) completed before the TD was stopped.

I had no luck producing this sequence of completion events so there
is no compelling demonstration of any resulting disaster. It may be
a very rare, obscure condition. The sole motivation for this patch
is that if such unlikely event does occur, I'd rather risk reporting
a cancelled partially done isoc frame as empty than gamble with UAF.

This will be fixed more properly by looking at Stopped event's TRB
pointer when making skipping decisions, but such rework is unlikely
to be backported to v6.12, which will stay around for a few years.

Fixes: d56b0b2ab142 ("usb: xhci: ensure skipped isoc TDs are returned when isoc ring is stopped")
Cc: stable@vger.kernel.org
Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-ring.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 23cf20026359..6fb48d30ec21 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2828,6 +2828,10 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 		if (!ep_seg) {
 
 			if (ep->skip && usb_endpoint_xfer_isoc(&td->urb->ep->desc)) {
+				/* this event is unlikely to match any TD, don't skip them all */
+				if (trb_comp_code == COMP_STOPPED_LENGTH_INVALID)
+					return 0;
+
 				skip_isoc_td(xhci, td, ep, status);
 				if (!list_empty(&ep_ring->td_list))
 					continue;
-- 
2.43.0

Re: [PATCH 03/15] usb: xhci: Don't skip on Stopped - Length Invalid

[Greg KH]

On Thu, Mar 06, 2025 at 04:49:42PM +0200, Mathias Nyman wrote:
> From: Michal Pecio <michal.pecio@gmail.com>
> 
> Up until commit d56b0b2ab142 ("usb: xhci: ensure skipped isoc TDs are
> returned when isoc ring is stopped") in v6.11, the driver didn't skip
> missed isochronous TDs when handling Stoppend and Stopped - Length
> Invalid events. Instead, it erroneously cleared the skip flag, which
> would cause the ring to get stuck, as future events won't match the
> missed TD which is never removed from the queue until it's cancelled.
> 
> This buggy logic seems to have been in place substantially unchanged
> since the 3.x series over 10 years ago, which probably speaks first
> and foremost about relative rarity of this case in normal usage, but
> by the spec I see no reason why it shouldn't be possible.
> 
> After d56b0b2ab142, TDs are immediately skipped when handling those
> Stopped events. This poses a potential problem in case of Stopped -
> Length Invalid, which occurs either on completed TDs (likely already
> given back) or Link and No-Op TRBs. Such event won't be recognized
> as matching any TD (unless it's the rare Link TRB inside a TD) and
> will result in skipping all pending TDs, giving them back possibly
> before they are done, risking isoc data loss and maybe UAF by HW.
> 
> As a compromise, don't skip and don't clear the skip flag on this
> kind of event. Then the next event will skip missed TDs. A downside
> of not handling Stopped - Length Invalid on a Link inside a TD is
> that if the TD is cancelled, its actual length will not be updated
> to account for TRBs (silently) completed before the TD was stopped.
> 
> I had no luck producing this sequence of completion events so there
> is no compelling demonstration of any resulting disaster. It may be
> a very rare, obscure condition. The sole motivation for this patch
> is that if such unlikely event does occur, I'd rather risk reporting
> a cancelled partially done isoc frame as empty than gamble with UAF.
> 
> This will be fixed more properly by looking at Stopped event's TRB
> pointer when making skipping decisions, but such rework is unlikely
> to be backported to v6.12, which will stay around for a few years.
> 
> Fixes: d56b0b2ab142 ("usb: xhci: ensure skipped isoc TDs are returned when isoc ring is stopped")
> Cc: stable@vger.kernel.org
> Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>

Why is a patch cc: stable burried here in a series for linux-next?  It
will be many many weeks before it gets out to anyone else, is that
intentional?

Same for the other commit in this series tagged that way.

thanks,

greg k-h

Re: [PATCH 03/15] usb: xhci: Don't skip on Stopped - Length Invalid

[Mathias Nyman]

On 6.3.2025 16.52, Greg KH wrote:
> On Thu, Mar 06, 2025 at 04:49:42PM +0200, Mathias Nyman wrote:
> Why is a patch cc: stable burried here in a series for linux-next?  It
> will be many many weeks before it gets out to anyone else, is that
> intentional?
> 
> Same for the other commit in this series tagged that way.

These are both kind of half theoretical issues that have been
around for years without more complaints. No need to rush them to
stable. Balance between regression risk vs adding them to stable.

This patch for example states:

"I had no luck producing this sequence of completion events so there
  is no compelling demonstration of any resulting disaster. It may be
  a very rare, obscure condition. The sole motivation for this patch
  is that if such unlikely event does occur, I'd rather risk reporting
  a cancelled partially done isoc frame as empty than gamble with UA"

Thanks
Mathias

Re: [PATCH 03/15] usb: xhci: Don't skip on Stopped - Length Invalid

[Greg KH]

On Thu, Mar 06, 2025 at 05:29:30PM +0200, Mathias Nyman wrote:
> On 6.3.2025 16.52, Greg KH wrote:
> > On Thu, Mar 06, 2025 at 04:49:42PM +0200, Mathias Nyman wrote:
> > Why is a patch cc: stable burried here in a series for linux-next?  It
> > will be many many weeks before it gets out to anyone else, is that
> > intentional?
> > 
> > Same for the other commit in this series tagged that way.
> 
> These are both kind of half theoretical issues that have been
> around for years without more complaints. No need to rush them to
> stable. Balance between regression risk vs adding them to stable.
> 
> This patch for example states:
> 
> "I had no luck producing this sequence of completion events so there
>  is no compelling demonstration of any resulting disaster. It may be
>  a very rare, obscure condition. The sole motivation for this patch
>  is that if such unlikely event does occur, I'd rather risk reporting
>  a cancelled partially done isoc frame as empty than gamble with UA"

Ok, fair enough, just seeing patches languish in -next that are tagged
for stable looks odd.

thanks,

greg k-h

[PATCH 13/15] usb: xhci: Apply the link chain quirk on NEC isoc endpoints

[Mathias Nyman]

From: Michal Pecio <michal.pecio@gmail.com>

Two clearly different specimens of NEC uPD720200 (one with start/stop
bug, one without) were seen to cause IOMMU faults after some Missed
Service Errors. Faulting address is immediately after a transfer ring
segment and patched dynamic debug messages revealed that the MSE was
received when waiting for a TD near the end of that segment:

[ 1.041954] xhci_hcd: Miss service interval error for slot 1 ep 2 expected TD DMA ffa08fe0
[ 1.042120] xhci_hcd: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0005 address=0xffa09000 flags=0x0000]
[ 1.042146] xhci_hcd: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0005 address=0xffa09040 flags=0x0000]

It gets even funnier if the next page is a ring segment accessible to
the HC. Below, it reports MSE in segment at ff1e8000, plows through a
zero-filled page at ff1e9000 and starts reporting events for TRBs in
page at ff1ea000 every microframe, instead of jumping to seg ff1e6000.

[ 7.041671] xhci_hcd: Miss service interval error for slot 1 ep 2 expected TD DMA ff1e8fe0
[ 7.041999] xhci_hcd: Miss service interval error for slot 1 ep 2 expected TD DMA ff1e8fe0
[ 7.042011] xhci_hcd: WARN: buffer overrun event for slot 1 ep 2 on endpoint
[ 7.042028] xhci_hcd: All TDs skipped for slot 1 ep 2. Clear skip flag.
[ 7.042134] xhci_hcd: WARN: buffer overrun event for slot 1 ep 2 on endpoint
[ 7.042138] xhci_hcd: ERROR Transfer event TRB DMA ptr not part of current TD ep_index 2 comp_code 31
[ 7.042144] xhci_hcd: Looking for event-dma 00000000ff1ea040 trb-start 00000000ff1e6820 trb-end 00000000ff1e6820
[ 7.042259] xhci_hcd: WARN: buffer overrun event for slot 1 ep 2 on endpoint
[ 7.042262] xhci_hcd: ERROR Transfer event TRB DMA ptr not part of current TD ep_index 2 comp_code 31
[ 7.042266] xhci_hcd: Looking for event-dma 00000000ff1ea050 trb-start 00000000ff1e6820 trb-end 00000000ff1e6820

At some point completion events change from Isoch Buffer Overrun to
Short Packet and the HC finally finds cycle bit mismatch in ff1ec000.

[ 7.098130] xhci_hcd: ERROR Transfer event TRB DMA ptr not part of current TD ep_index 2 comp_code 13
[ 7.098132] xhci_hcd: Looking for event-dma 00000000ff1ecc50 trb-start 00000000ff1e6820 trb-end 00000000ff1e6820
[ 7.098254] xhci_hcd: ERROR Transfer event TRB DMA ptr not part of current TD ep_index 2 comp_code 13
[ 7.098256] xhci_hcd: Looking for event-dma 00000000ff1ecc60 trb-start 00000000ff1e6820 trb-end 00000000ff1e6820
[ 7.098379] xhci_hcd: Overrun event on slot 1 ep 2

It's possible that data from the isochronous device were written to
random buffers of pending TDs on other endpoints (either IN or OUT),
other devices or even other HCs in the same IOMMU domain.

Lastly, an error from a different USB device on another HC. Was it
caused by the above? I don't know, but it may have been. The disk
was working without any other issues and generated PCIe traffic to
starve the NEC of upstream BW and trigger those MSEs. The two HCs
shared one x1 slot by means of a commercial "PCIe splitter" board.

[ 7.162604] usb 10-2: reset SuperSpeed USB device number 3 using xhci_hcd
[ 7.178990] sd 9:0:0:0: [sdb] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x07 driverbyte=DRIVER_OK cmd_age=0s
[ 7.179001] sd 9:0:0:0: [sdb] tag#0 CDB: opcode=0x28 28 00 04 02 ae 00 00 02 00 00
[ 7.179004] I/O error, dev sdb, sector 67284480 op 0x0:(READ) flags 0x80700 phys_seg 5 prio class 0

Fortunately, it appears that this ridiculous bug is avoided by setting
the chain bit of Link TRBs on isochronous rings. Other ancient HCs are
known which also expect the bit to be set and they ignore Link TRBs if
it's not. Reportedly, 0.95 spec guaranteed that the bit is set.

The bandwidth-starved NEC HC running a 32KB/uframe UVC endpoint reports
tens of MSEs per second and runs into the bug within seconds. Chaining
Link TRBs allows the same workload to run for many minutes, many times.

No negative side effects seen in UVC recording and UAC playback with a
few devices at full speed, high speed and SuperSpeed.

The problem doesn't reproduce on the newer Renesas uPD720201/uPD720202
and on old Etron EJ168 and VIA VL805 (but the VL805 has other bug).

[shorten line length of log snippets in commit messge -Mathias]
Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci.h | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 4ee14f651d36..d9d7cd1906f3 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1760,11 +1760,20 @@ static inline void xhci_write_64(struct xhci_hcd *xhci,
 }
 
 
-/* Link TRB chain should always be set on 0.95 hosts, and AMD 0.96 ISOC rings */
+/*
+ * Reportedly, some chapters of v0.95 spec said that Link TRB always has its chain bit set.
+ * Other chapters and later specs say that it should only be set if the link is inside a TD
+ * which continues from the end of one segment to the next segment.
+ *
+ * Some 0.95 hardware was found to misbehave if any link TRB doesn't have the chain bit set.
+ *
+ * 0.96 hardware from AMD and NEC was found to ignore unchained isochronous link TRBs when
+ * "resynchronizing the pipe" after a Missed Service Error.
+ */
 static inline bool xhci_link_chain_quirk(struct xhci_hcd *xhci, enum xhci_ring_type type)
 {
 	return (xhci->quirks & XHCI_LINK_TRB_QUIRK) ||
-	       (type == TYPE_ISOC && (xhci->quirks & XHCI_AMD_0x96_HOST));
+	       (type == TYPE_ISOC && (xhci->quirks & (XHCI_AMD_0x96_HOST | XHCI_NEC_HOST)));
 }
 
 /* xHCI debugging */
-- 
2.43.0