[PATCH v2] xhci: fix lost bounce buffers on TDs spanning several ring segments

[PATCH v2] xhci: fix lost bounce buffers on TDs spanning several ring segments

[Arthur Gautier]

When a TD reaches a link TRB with data that is not aligned to the
endpoint's wMaxPacketSize, xhci_align_td() stages the unalignable tail
through the bounce buffer of the ring segment holding that link TRB.
xhci_unmap_td_bounce_buffer() later unmaps it and, for IN transfers,
copies the data back into the URB's buffer.

The enqueue path records the segment that was bounced in td->bounce_seg,
under the assumption that a TD never spans more than two ring segments.
That assumption does not hold: a TD large enough to span three or more
segments crosses several link TRBs and can be bounced at each of them.
Only the last one survives in td->bounce_seg, so every earlier bounce
buffer is neither copied back nor DMA unmapped.

The URB still completes with actual_length equal to the requested length
and no error, so the transfer looks successful while a wMaxPacketSize
sized hole in the destination buffer silently keeps its previous
contents. It also leaks a DMA mapping per dropped bounce.

Any sufficiently large and fragmented bulk transfer can hit this. It was
found with a USB mass storage device behind xHCI backing a dm-verity
target with 512 byte hash blocks, where the stale data is detected rather
than silently consumed. The device enumerates as SuperSpeed, so
wMaxPacketSize is 1024, while dm-bufio issues one 512 byte bio per hash
block. verity_prefetch_io() makes the block layer merge hundreds of them
into a single request of up to 512 scatterlist entries of 512 bytes each.
At 256 TRBs per ring segment such a TD spans three segments, and every
segment boundary falls on an odd multiple of 512, i.e. unaligned to
wMaxPacketSize. dm-bufio then caches a hash block holding stale data and
dm-verity declares the metadata block corrupted:

  device-mapper: verity: 8:2: metadata block 10850 is corrupted

A reproducer running this under qemu is available at
https://github.com/baloo/xhci-verity

The bounce state (bounce_buf, bounce_dma, bounce_len, bounce_offs)
already lives on the ring segment, so there is nothing extra to track.
Keep recording the last bounced segment in td->bounce_seg and, on
completion, walk the segments from td->start_seg up to it, unmapping
every segment that still has a pending bounce.

Stopping at td->bounce_seg rather than td->end_seg matters: a bounce
implies the TD continues past that segment's link TRB, so bounce_seg is
always strictly before end_seg, and a later TD may already have started
in end_seg and been bounced there. Walking that far would copy a foreign
bounce buffer into this URB and unmap it twice. It also keeps the walk
correct if a TD ever wraps the whole ring so that end_seg == start_seg.

Changes since v1:
- Walk td->start_seg -> td->bounce_seg instead of td->bounce_seg ->
  td->end_seg. end_seg can hold the start of a later TD which may
  already have been bounced there, so the v1 walk could copy a
  foreign bounce buffer into this URB and unmap it twice.
  (caught by Mathias and Michal)
- Keep recording the last bounced segment. Also handles a TD
  wrapping the whole ring.  (suggested by Michal)
- Test !td->bounce_seg first, it is the common case. (Michal)

v1: https://patchwork.kernel.org/project/linux-usb/patch/20260811033508.1050148-1-baloo@superbaloo.net/

Fixes: f9c589e142d0 ("xhci: TD-fragment, align the unsplittable case with a bounce buffer")
Suggested-by: Michal Pecio <michal.pecio@gmail.com>
Signed-off-by: Arthur Gautier <baloo@superbaloo.net>
---
 drivers/usb/host/xhci-ring.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 4f98d8269625..1772cdf9ffc5 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -842,21 +842,18 @@ static void xhci_giveback_urb_in_irq(struct xhci_hcd *xhci,
 	usb_hcd_giveback_urb(hcd, urb, status);
 }
 
-static void xhci_unmap_td_bounce_buffer(struct xhci_hcd *xhci,
-		struct xhci_ring *ring, struct xhci_td *td)
+static void xhci_unmap_one_bounce_buffer(struct xhci_hcd *xhci,
+		struct xhci_ring *ring, struct xhci_td *td,
+		struct xhci_segment *seg)
 {
 	struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
-	struct xhci_segment *seg = td->bounce_seg;
 	struct urb *urb = td->urb;
 	size_t len;
 
-	if (!ring || !seg || !urb)
-		return;
-
 	if (usb_urb_dir_out(urb)) {
 		dma_unmap_single(dev, seg->bounce_dma, ring->bounce_buf_len,
 				 DMA_TO_DEVICE);
-		return;
+		goto done;
 	}
 
 	dma_unmap_single(dev, seg->bounce_dma, ring->bounce_buf_len,
@@ -872,10 +869,28 @@ static void xhci_unmap_td_bounce_buffer(struct xhci_hcd *xhci,
 		memcpy(urb->transfer_buffer + seg->bounce_offs, seg->bounce_buf,
 		       seg->bounce_len);
 	}
+done:
 	seg->bounce_len = 0;
 	seg->bounce_offs = 0;
 }
 
+static void xhci_unmap_td_bounce_buffer(struct xhci_hcd *xhci,
+		struct xhci_ring *ring, struct xhci_td *td)
+{
+	struct xhci_segment *seg;
+
+	if (!td->bounce_seg || !ring || !td->urb)
+		return;
+
+	/* td->bounce_seg is the last one bounced, unmap them all */
+	for (seg = td->start_seg; ; seg = seg->next) {
+		if (seg->bounce_len)
+			xhci_unmap_one_bounce_buffer(xhci, ring, td, seg);
+		if (seg == td->bounce_seg)
+			break;
+	}
+}
+
 static void xhci_td_cleanup(struct xhci_hcd *xhci, struct xhci_td *td,
 			    struct xhci_ring *ep_ring, int status)
 {
@@ -3674,7 +3689,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
 						  &trb_buff_len,
 						  ring->enq_seg)) {
 					send_addr = ring->enq_seg->bounce_dma;
-					/* assuming TD won't span 2 segs */
+					/* a TD may be bounced in every seg it spans */
 					td->bounce_seg = ring->enq_seg;
 				}
 			}
-- 
2.55.0

Re: [PATCH v2] xhci: fix lost bounce buffers on TDs spanning several ring segments

[Michal Pecio]

On Wed, 12 Aug 2026 00:25:54 +0000, Arthur Gautier wrote:
> When a TD reaches a link TRB with data that is not aligned to the
> endpoint's wMaxPacketSize, xhci_align_td() stages the unalignable tail
> through the bounce buffer of the ring segment holding that link TRB.
> xhci_unmap_td_bounce_buffer() later unmaps it and, for IN transfers,
> copies the data back into the URB's buffer.
> 
> The enqueue path records the segment that was bounced in td->bounce_seg,
> under the assumption that a TD never spans more than two ring segments.
> That assumption does not hold: a TD large enough to span three or more
> segments crosses several link TRBs and can be bounced at each of them.
> Only the last one survives in td->bounce_seg, so every earlier bounce
> buffer is neither copied back nor DMA unmapped.
> 
> The URB still completes with actual_length equal to the requested length
> and no error, so the transfer looks successful while a wMaxPacketSize
> sized hole in the destination buffer silently keeps its previous
> contents. It also leaks a DMA mapping per dropped bounce.
> 
> Any sufficiently large and fragmented bulk transfer can hit this. It was
> found with a USB mass storage device behind xHCI backing a dm-verity
> target with 512 byte hash blocks, where the stale data is detected rather
> than silently consumed. The device enumerates as SuperSpeed, so
> wMaxPacketSize is 1024, while dm-bufio issues one 512 byte bio per hash
> block. verity_prefetch_io() makes the block layer merge hundreds of them
> into a single request of up to 512 scatterlist entries of 512 bytes each.
> At 256 TRBs per ring segment such a TD spans three segments, and every
> segment boundary falls on an odd multiple of 512, i.e. unaligned to
> wMaxPacketSize. dm-bufio then caches a hash block holding stale data and
> dm-verity declares the metadata block corrupted:
> 
>   device-mapper: verity: 8:2: metadata block 10850 is corrupted
> 
> A reproducer running this under qemu is available at
> https://github.com/baloo/xhci-verity
> 
> The bounce state (bounce_buf, bounce_dma, bounce_len, bounce_offs)
> already lives on the ring segment, so there is nothing extra to track.
> Keep recording the last bounced segment in td->bounce_seg and, on
> completion, walk the segments from td->start_seg up to it, unmapping
> every segment that still has a pending bounce.
> 
> Stopping at td->bounce_seg rather than td->end_seg matters: a bounce
> implies the TD continues past that segment's link TRB, so bounce_seg is
> always strictly before end_seg, and a later TD may already have started
> in end_seg and been bounced there. Walking that far would copy a foreign
> bounce buffer into this URB and unmap it twice. It also keeps the walk
> correct if a TD ever wraps the whole ring so that end_seg == start_seg.
> 
> Changes since v1:
> - Walk td->start_seg -> td->bounce_seg instead of td->bounce_seg ->
>   td->end_seg. end_seg can hold the start of a later TD which may
>   already have been bounced there, so the v1 walk could copy a
>   foreign bounce buffer into this URB and unmap it twice.
>   (caught by Mathias and Michal)
> - Keep recording the last bounced segment. Also handles a TD
>   wrapping the whole ring.  (suggested by Michal)
> - Test !td->bounce_seg first, it is the common case. (Michal)
> 
> v1: https://patchwork.kernel.org/project/linux-usb/patch/20260811033508.1050148-1-baloo@superbaloo.net/

This should go below the --- line.
Patch revision log is *not* meant to go into the kernel changelog

> Fixes: f9c589e142d0 ("xhci: TD-fragment, align the unsplittable case with a bounce buffer")
> Suggested-by: Michal Pecio <michal.pecio@gmail.com>
> Signed-off-by: Arthur Gautier <baloo@superbaloo.net>

This should still have the Cc: stable line, just no need to actually
send email there. OTOH, this email should be addressed to Mathias Nyman
and Greg KH, not only linux-usb (but don't list them here).

I admit that I send patches manually like a caveman, so I don't know
how to configure git send-email to get it right...

That being said, I downloaded and applied this patch and it does seem
to work. The bug is easy to repro by setting TRB_MAX_BUFF_SIZE to 512.
Before the patch, reading a 1GB partition gives different md5sum each
time. With the patch, multiple readings are correct.

And I think we can agree that the risk of unmapping a later TD's bounce
buffer no longer exists with the revised loop. I know I said the same
about v1, but I think this time it should be good for real.

> +	for (seg = td->start_seg; ; seg = seg->next) {
> +		if (seg->bounce_len)
> +			xhci_unmap_one_bounce_buffer(xhci, ring, td, seg);
> +		if (seg == td->bounce_seg)
> +			break;
> +	}

You have removed protection from infinite looping. I think nowadays the
driver has more loops without such protection and everytihng is fine,
but I'm not sure how it was in the past, and this patch goes to stable.

Maybe let's see what Mathias thinks about it.

> @@ -3674,7 +3689,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
>  						  &trb_buff_len,
>  						  ring->enq_seg)) {
>  					send_addr = ring->enq_seg->bounce_dma;
> -					/* assuming TD won't span 2 segs */
> +					/* a TD may be bounced in every seg it spans */
>  					td->bounce_seg = ring->enq_seg;

This comment looks a bit odd: you know that this may execute multiple
times, but you overwrite all previous pointers. Perhaps make it clear
that this was intentional:

		/* remember the last segment bounced in this TD */

Regards,
Michal

Re: [PATCH v2] xhci: fix lost bounce buffers on TDs spanning several ring segments

[Mathias Nyman]

On 8/18/26 01:48, Michal Pecio wrote:
> On Wed, 12 Aug 2026 00:25:54 +0000, Arthur Gautier wrote:
>> When a TD reaches a link TRB with data that is not aligned to the
>> endpoint's wMaxPacketSize, xhci_align_td() stages the unalignable tail
>> through the bounce buffer of the ring segment holding that link TRB.
>> xhci_unmap_td_bounce_buffer() later unmaps it and, for IN transfers,
>> copies the data back into the URB's buffer.
>>
>> The enqueue path records the segment that was bounced in td->bounce_seg,
>> under the assumption that a TD never spans more than two ring segments.
>> That assumption does not hold: a TD large enough to span three or more
>> segments crosses several link TRBs and can be bounced at each of them.
>> Only the last one survives in td->bounce_seg, so every earlier bounce
>> buffer is neither copied back nor DMA unmapped.
>>
>> The URB still completes with actual_length equal to the requested length
>> and no error, so the transfer looks successful while a wMaxPacketSize
>> sized hole in the destination buffer silently keeps its previous
>> contents. It also leaks a DMA mapping per dropped bounce.
>>
>> Any sufficiently large and fragmented bulk transfer can hit this. It was
>> found with a USB mass storage device behind xHCI backing a dm-verity
>> target with 512 byte hash blocks, where the stale data is detected rather
>> than silently consumed. The device enumerates as SuperSpeed, so
>> wMaxPacketSize is 1024, while dm-bufio issues one 512 byte bio per hash
>> block. verity_prefetch_io() makes the block layer merge hundreds of them
>> into a single request of up to 512 scatterlist entries of 512 bytes each.
>> At 256 TRBs per ring segment such a TD spans three segments, and every
>> segment boundary falls on an odd multiple of 512, i.e. unaligned to
>> wMaxPacketSize. dm-bufio then caches a hash block holding stale data and
>> dm-verity declares the metadata block corrupted:
>>
>>    device-mapper: verity: 8:2: metadata block 10850 is corrupted
>>
>> A reproducer running this under qemu is available at
>> https://github.com/baloo/xhci-verity
>>
>> The bounce state (bounce_buf, bounce_dma, bounce_len, bounce_offs)
>> already lives on the ring segment, so there is nothing extra to track.
>> Keep recording the last bounced segment in td->bounce_seg and, on
>> completion, walk the segments from td->start_seg up to it, unmapping
>> every segment that still has a pending bounce.
>>
>> Stopping at td->bounce_seg rather than td->end_seg matters: a bounce
>> implies the TD continues past that segment's link TRB, so bounce_seg is
>> always strictly before end_seg, and a later TD may already have started
>> in end_seg and been bounced there. Walking that far would copy a foreign
>> bounce buffer into this URB and unmap it twice. It also keeps the walk
>> correct if a TD ever wraps the whole ring so that end_seg == start_seg.
>>
>> Changes since v1:
>> - Walk td->start_seg -> td->bounce_seg instead of td->bounce_seg ->
>>    td->end_seg. end_seg can hold the start of a later TD which may
>>    already have been bounced there, so the v1 walk could copy a
>>    foreign bounce buffer into this URB and unmap it twice.
>>    (caught by Mathias and Michal)
>> - Keep recording the last bounced segment. Also handles a TD
>>    wrapping the whole ring.  (suggested by Michal)
>> - Test !td->bounce_seg first, it is the common case. (Michal)
>>
>> v1: https://patchwork.kernel.org/project/linux-usb/patch/20260811033508.1050148-1-baloo@superbaloo.net/
> 
> This should go below the --- line.
> Patch revision log is *not* meant to go into the kernel changelog
> 
>> Fixes: f9c589e142d0 ("xhci: TD-fragment, align the unsplittable case with a bounce buffer")
>> Suggested-by: Michal Pecio <michal.pecio@gmail.com>
>> Signed-off-by: Arthur Gautier <baloo@superbaloo.net>
> 
> This should still have the Cc: stable line, just no need to actually
> send email there. OTOH, this email should be addressed to Mathias Nyman
> and Greg KH, not only linux-usb (but don't list them here).
> 
> I admit that I send patches manually like a caveman, so I don't know
> how to configure git send-email to get it right...
> 
> That being said, I downloaded and applied this patch and it does seem
> to work. The bug is easy to repro by setting TRB_MAX_BUFF_SIZE to 512.
> Before the patch, reading a 1GB partition gives different md5sum each
> time. With the patch, multiple readings are correct.
> 
> And I think we can agree that the risk of unmapping a later TD's bounce
> buffer no longer exists with the revised loop. I know I said the same
> about v1, but I think this time it should be good for real.
> 
>> +	for (seg = td->start_seg; ; seg = seg->next) {
>> +		if (seg->bounce_len)
>> +			xhci_unmap_one_bounce_buffer(xhci, ring, td, seg);
>> +		if (seg == td->bounce_seg)
>> +			break;
>> +	}
> 
> You have removed protection from infinite looping. I think nowadays the
> driver has more loops without such protection and everytihng is fine,
> but I'm not sure how it was in the past, and this patch goes to stable.
> 
> Maybe let's see what Mathias thinks about it.

Thanks for adding me back to the loop (cc)

The infinite loop risk could be prevented by using xhci_for_each_ring_seg():

xhci_for_each_ring_seg(td->start_seg, seg) {
	if (seg->bounce_len)
		xhci_unmap_one_bounce_buffer(xhci, ring, td, seg);
	if (seg == td->bounce_seg)
		break;
}

Maybe one more patch revision fixing both this, and the details Michal pointed out
earlier would make sense

Thanks
Mathias

Re: [PATCH v2] xhci: fix lost bounce buffers on TDs spanning several ring segments

[Michal Pecio]

On Tue, 18 Aug 2026 10:29:41 +0300, Mathias Nyman wrote:
> On 8/18/26 01:48, Michal Pecio wrote: 
> > You have removed protection from infinite looping. I think nowadays
> > the driver has more loops without such protection and everytihng is
> > fine, but I'm not sure how it was in the past, and this patch goes
> > to stable.
> > 
> > Maybe let's see what Mathias thinks about it.  
> 
> Thanks for adding me back to the loop (cc)
> 
> The infinite loop risk could be prevented by using
> xhci_for_each_ring_seg():
> 
> xhci_for_each_ring_seg(td->start_seg, seg) {
> 	if (seg->bounce_len)
> 		xhci_unmap_one_bounce_buffer(xhci, ring, td, seg);
> 	if (seg == td->bounce_seg)
> 		break;
> }

That's tricky to backport; the macro doesn't exist in linux-6.6.y or
earlier and the commit which added it includes many other changes.

And it only prevents infinite loop if start_seg is reachable from
itself (so not if start_seg->next->next == start_seg->next). Similarly,
v2 is good enough as long as bounce_seg is reachable from start_seg.
It surely was reachable at the time of enqueue, so only a botched ring
expansion *later* could break this.

I actually think that chances of such bugs existing and being unnoticed
for years are practically zero, so maybe just don't worry about it.

I only mentioned it because:
- v1 included a safety counter checked against ring->num_segs
- it's something that people used to worry about a lot in the past
  for some reason; maybe just to aid debugging during development

Regards,
Michal