[PATCH 0/2] thunderbolt: Fix lost ring interrupts and an XDomain use-after-free

[PATCH 0/2] thunderbolt: Fix lost ring interrupts and an XDomain use-after-free

[Sven Neuhaus]

Hi folks,

here are two independent fixes found while running an out-of-tree 
USB4 service driver between two hosts with AMD Strix Halo USB4 host 
routers (1022:158d, 1022:158e). The driver exchanges XDomain requests 
and responses in both directions and moves data over its own rings.

Patch 1 stops tb_xdomain_match() from matching received packets to the
requests that tb_xdomain_response() queues. Those requests have no
response buffer, and a match after their work has run makes the work
run again after the request was freed (refcount underflow in
tb_cfg_request_work()).

Patch 2 reads back the NHI after the ring's interrupt status is cleared
in ring_msix() on hosts without QUIRK_AUTO_CLEAR_INT. Without the read
the posted clear can take effect after the next frame has completed
and clear its status too, so that frame raises no interrupt. On these
hosts that left an XDomain request unprocessed until the sender timed
out (about 75 ms in 3-7% of request/reply exchanges), and data ring
completions were lost in the same way.

Each patch says how it was tested and what was not tested. Both apply
to the fixes branch (f1ae606eb404) and to 7.3.3. They were built on
7.3.3 with W=1 and add no warnings; they were not built on the fixes
branch. The analysis, the fixes and the tests were done
with an AI coding assistant.

Sven Neuhaus (2):
  thunderbolt: Do not match XDomain responses to received packets
  thunderbolt: Flush the ring interrupt status clear

 drivers/thunderbolt/nhi.c     | 8 ++++++++
 drivers/thunderbolt/xdomain.c | 7 +++++++
 2 files changed, 15 insertions(+)


base-commit: f1ae606eb404bca2498b9ad69b923073655556e4
-- 
2.47.3

[PATCH 1/2] thunderbolt: Do not match XDomain responses to received packets

[Sven Neuhaus]

What happens is that __tb_xdomain_response() queues a request that has
no response buffer. tb_cfg_request() schedules its work right after
sending it, and the request stays on ctl->request_queue until that work
dequeues it.

Until then tb_xdomain_match() can match a received error packet, or an
XDomain response with the same route and UUID, to this request. The
packet may really belong to a tb_xdomain_request() queued behind it.
tb_ctl_rx_callback() then calls schedule_work() again. If the first run
has already started, the work runs a second time after the request has
been released:

  refcount_t: underflow; use-after-free.
  WARNING: lib/refcount.c:28 at refcount_warn_saturate+0x59/0x90
  Workqueue: events tb_cfg_request_work [thunderbolt]
  Call Trace:
   tb_cfg_request_put+0x5a/0x70 [thunderbolt]
   tb_cfg_request_work+0x2e/0x50 [thunderbolt]
   process_one_work+0x19d/0x380

The match also reads req->request, which points to the caller's buffer.
That is often on the stack and gone by the time the packet arrives.

Such a request does not wait for anything, so never match it.

I've seen this 7.2.15 between two hosts connected with USB4 cables,
where an out-of-tree service driver exchanges XDomain requests and
responses in both directions. tb_xdomain_match() and
tb_cfg_request_work() are the same in mainline.

To test it, I added counters to ctl.c and ran ib_write_bw over that
driver in both directions. In 60 seconds a received packet matched a
request without a response buffer 527 times, and 6 times the work of
such a request was queued again after its callback had run and then
ran a second time. The refcount warning above is printed only once per
boot, so the log shows just the first one. With this patch and the
same load for 120 seconds, none of the counters moved, even with a
2-3 ms sleep added before tb_cfg_request_dequeue() to widen the window.

The patch was tested on 7.2.15 and build-tested on 7.3.3.
tb_xdomain_match() is unchanged in the thunderbolt.git fixes branch,
where the patch applies as is. Not tested: peers other than Linux and
the XDomain discovery protocol responses.

The analysis, the fix and the tests were done with an AI coding assistant.

Fixes: d1ff70241a27 ("thunderbolt: Add support for XDomain discovery protocol")
Cc: stable@vger.kernel.org
Signed-off-by: Sven Neuhaus <sven-kernel@sven.de>
---
 drivers/thunderbolt/xdomain.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/thunderbolt/xdomain.c b/drivers/thunderbolt/xdomain.c
index 563a8b65c854..64e2842b8b3f 100644
--- a/drivers/thunderbolt/xdomain.c
+++ b/drivers/thunderbolt/xdomain.c
@@ -90,6 +90,13 @@ bool tb_is_xdomain_enabled(void)
 static bool tb_xdomain_match(const struct tb_cfg_request *req,
 			     const struct ctl_pkg *pkg)
 {
+	/*
+	 * A request without a response buffer is complete once it has
+	 * been sent, and req->request may no longer be valid.
+	 */
+	if (!req->response)
+		return false;
+
 	switch (pkg->frame.eof) {
 	case TB_CFG_PKG_ERROR:
 		return true;
-- 
2.47.3

[PATCH 2/2] thunderbolt: Flush the ring interrupt status clear

[Sven Neuhaus]

On hosts without QUIRK_AUTO_CLEAR_INT, ring_msix() clears the ring's
bit in REG_RING_INT_CLEAR before it schedules ring_work(). This is a
posted write and nothing is read from the NHI afterwards, so the clear
can take effect after the next frame on the ring has completed. It
then clears the status of that frame as well. The descriptor stays
completed and the interrupt stays enabled, but no MSI-X is raised, and
the frame waits until another interrupt on the ring runs ring_work().

Before commit 468c49f44759 ("thunderbolt: Disable interrupt auto clear
for rings") the status was cleared by reading REG_RING_NOTIFY_BASE,
and a read is not posted.

Read back REG_RING_INTERRUPT_BASE after the write, so the clear has
taken effect before ring_work() looks at the descriptors. A frame that
completed before that point is seen by ring_work(), and a later one
raises a new interrupt.

This was seen between two hosts with AMD Strix Halo USB4 host routers
(1022:158d, 1022:158e) connected by USB4 cables, running an
out-of-tree service driver that exchanges XDomain requests and
responses and moves data over its own rings. When one host sent a
response and a request back to back, the request sometimes stayed in
the peer's control RX ring until tb_xdomain_request() timed out and
sent it again. In a request/reply test with 300 us between exchanges,
3-7% of the exchanges took the whole request timeout (about 75 ms).
A debug build that checked the control RX ring every 500 us found
these frames with RING_DESC_COMPLETED set, the ring's bit set in
REG_RING_INTERRUPT_BASE and its bit clear in REG_RING_NOTIFY_BASE.
The data rings of the driver lost completions the same way, which
showed up as retransmissions after the driver's ack timeout.

With this patch, 6000 exchanges of 8-64 KiB had none above 1.7 ms
(99th percentile 0.35-0.58 ms, before about 75 ms), no request was
sent twice, and the retransmissions went away (84 in 1000 exchanges
before, none after). ib_write_bw at 4 and 8 MiB stayed within
run-to-run noise.

Tested on 7.3.3; ring_clear_msix() is unchanged in the thunderbolt.git
fixes branch, where the patch applies as is. Not tested: other AMD
host routers. Intel hosts set QUIRK_AUTO_CLEAR_INT and return before
the write, so they are not affected.

The analysis, the fix and the tests were done with an AI coding
assistant.

Fixes: 468c49f44759 ("thunderbolt: Disable interrupt auto clear for rings")
Cc: stable@vger.kernel.org
Cc: Mario Limonciello <superm1@kernel.org>
Signed-off-by: Sven Neuhaus <sven-kernel@sven.de>
---
 drivers/thunderbolt/nhi.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
index e99a3fcc4a29..dd109f6ac70f 100644
--- a/drivers/thunderbolt/nhi.c
+++ b/drivers/thunderbolt/nhi.c
@@ -533,6 +533,14 @@ static void ring_clear_msix(const struct tb_ring *ring)
 	else
 		iowrite32(BIT(bit), ring->nhi->iobase + REG_RING_INT_CLEAR +
 			  4 * (ring->nhi->hop_count / 32));
+
+	/*
+	 * Flush the posted write before ring_work() looks at the
+	 * descriptors. Otherwise the clear can take effect after the next
+	 * frame has completed and clear its status too, and that frame
+	 * never raises an interrupt.
+	 */
+	ioread32(ring->nhi->iobase + REG_RING_INTERRUPT_BASE);
 }
 
 irqreturn_t ring_msix(int irq, void *data)
-- 
2.47.3

Re: [PATCH 1/2] thunderbolt: Do not match XDomain responses to received packets

[sashiko-bot]

> What happens is that __tb_xdomain_response() queues a request that has
> no response buffer. tb_cfg_request() schedules its work right after
> sending it, and the request stays on ctl->request_queue until that work
> dequeues it.
> 
> Until then tb_xdomain_match() can match a received error packet, or an
> XDomain response with the same route and UUID, to this request. The
> packet may really belong to a tb_xdomain_request() queued behind it.
> tb_ctl_rx_callback() then calls schedule_work() again. If the first run
> has already started, the work runs a second time after the request has
> been released:
> 
>   refcount_t: underflow; use-after-free.
>   WARNING: lib/refcount.c:28 at refcount_warn_saturate+0x59/0x90
>   Workqueue: events tb_cfg_request_work [thunderbolt]
> [ ... ]
> The analysis, the fix and the tests were done with an AI coding assistant.
> 
> Fixes: d1ff70241a27 ("thunderbolt: Add support for XDomain discovery protocol")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sven Neuhaus <sven-kernel@sven.de>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/1794520303-5758-1-git-send-email-sven-kernel@sven.de?part=1

Re: [PATCH 2/2] thunderbolt: Flush the ring interrupt status clear

[sashiko-bot]

> On hosts without QUIRK_AUTO_CLEAR_INT, ring_msix() clears the ring's
> bit in REG_RING_INT_CLEAR before it schedules ring_work(). This is a
> posted write and nothing is read from the NHI afterwards, so the clear
> can take effect after the next frame on the ring has completed. It
> then clears the status of that frame as well. The descriptor stays
> completed and the interrupt stays enabled, but no MSI-X is raised, and
> the frame waits until another interrupt on the ring runs ring_work().
> 
> Before commit 468c49f44759 ("thunderbolt: Disable interrupt auto clear
> for rings") the status was cleared by reading REG_RING_NOTIFY_BASE,
> and a read is not posted.
> 
> Read back REG_RING_INTERRUPT_BASE after the write, so the clear has
> taken effect before ring_work() looks at the descriptors. A frame that
> completed before that point is seen by ring_work(), and a later one
> [ ... ]
> 
> Fixes: 468c49f44759 ("thunderbolt: Disable interrupt auto clear for rings")
> Cc: stable@vger.kernel.org
> Cc: Mario Limonciello <superm1@kernel.org>
> Signed-off-by: Sven Neuhaus <sven-kernel@sven.de>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/1794520303-5758-1-git-send-email-sven-kernel@sven.de?part=2