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

[PATCH] 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.

This is reachable with a USB mass storage device behind xHCI backing a
dm-verity target with 512 byte hash blocks. 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 the first bounced segment in td->bounce_seg and, on completion,
walk the segments the TD covers from there up to td->end_seg, handling
every segment that still has a pending bounce.

Fixes: f9c589e142d0 ("xhci: TD-fragment, align the unsplittable case with a bounce buffer")
Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
Cc: stable@vger.kernel.org
Signed-off-by: Arthur Gautier <baloo@superbaloo.net>
---
 drivers/usb/host/xhci-ring.c | 49 +++++++++++++++++++++++++++++-------
 1 file changed, 40 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 4f98d8269625..4154e84420ac 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,37 @@ 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;
+	unsigned int i;
+
+	if (!ring || !td->bounce_seg || !td->urb)
+		return;
+
+	/*
+	 * A TD that spans more than two ring segments crosses several link
+	 * TRBs, and may have been aligned with a bounce buffer at each of
+	 * them. Every bounce buffer lives on the segment whose link TRB it
+	 * was needed for, so walk all segments the TD covers, starting at
+	 * the first one that was bounced.
+	 */
+	seg = td->bounce_seg;
+	for (i = 0; i < ring->num_segs; i++) {
+		if (seg->bounce_len)
+			xhci_unmap_one_bounce_buffer(xhci, ring, td, seg);
+		if (seg == td->end_seg)
+			break;
+		seg = seg->next;
+	}
+}
+
 static void xhci_td_cleanup(struct xhci_hcd *xhci, struct xhci_td *td,
 			    struct xhci_ring *ep_ring, int status)
 {
@@ -3674,8 +3698,15 @@ 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 */
-					td->bounce_seg = ring->enq_seg;
+					/*
+					 * A TD spanning several segments can be
+					 * bounced once per segment boundary it
+					 * crosses. Remember the first bounced
+					 * segment, the rest are found by walking
+					 * the TD's segments on completion.
+					 */
+					if (!td->bounce_seg)
+						td->bounce_seg = ring->enq_seg;
 				}
 			}
 		}
-- 
2.55.0

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

[Michal Pecio]

On Tue, 11 Aug 2026 03:35:08 +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.
> 
> This is reachable with a USB mass storage device behind xHCI backing a
> dm-verity target with 512 byte hash blocks. 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

Quite nasty, and not everybody uses dm-verify in particular. It seems
corruption could also affect rare FAT filesystems with tiny clusters. 

> 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 the first bounced segment in td->bounce_seg and, on completion,
> walk the segments the TD covers from there up to td->end_seg, handling
> every segment that still has a pending bounce.
> 
> Fixes: f9c589e142d0 ("xhci: TD-fragment, align the unsplittable case with a bounce buffer")
> Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
> Cc: stable@vger.kernel.org

Note: you don't need to list here the driver maintainer you are sending
this email to. And no need to *actually* send email to the stable list.

> Signed-off-by: Arthur Gautier <baloo@superbaloo.net>
> ---
>  drivers/usb/host/xhci-ring.c | 49 +++++++++++++++++++++++++++++-------
>  1 file changed, 40 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 4f98d8269625..4154e84420ac 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,37 @@ 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;
> +	unsigned int i;
> +
> +	if (!ring || !td->bounce_seg || !td->urb)
> +		return;

Tiny optimization nit: !td->bounce_seg is by far the most likely case,
so it could be first. The others guard against bugs and "never happen".

> +
> +	/*
> +	 * A TD that spans more than two ring segments crosses several link
> +	 * TRBs, and may have been aligned with a bounce buffer at each of
> +	 * them. Every bounce buffer lives on the segment whose link TRB it
> +	 * was needed for, so walk all segments the TD covers, starting at
> +	 * the first one that was bounced.
> +	 */

Hmm, AI patch? I think most people would write it like below :)

/* we have the first bounced seg, find and unmap all of them */

The code looks correct, though I would do it differently: store the
last bounce_seg and run the loop from td->start_seg to td->bounce_seg.
This avoids adding the 'if' during enqueue, and still works correctly
if the TD wraps around the whole ring so that end_seg == start_seg.

The driver is never supposed to create such TDs (they break the ring
expansion procedure) but I prefer more robust code if it costs nothing.
Bugs happen, or expansion could theoretically become more flexible.

Regards,
Michal

> +	seg = td->bounce_seg;
> +	for (i = 0; i < ring->num_segs; i++) {
> +		if (seg->bounce_len)
> +			xhci_unmap_one_bounce_buffer(xhci, ring, td, seg);
> +		if (seg == td->end_seg)
> +			break;
> +		seg = seg->next;
> +	}
> +}
> +
>  static void xhci_td_cleanup(struct xhci_hcd *xhci, struct xhci_td *td,
>  			    struct xhci_ring *ep_ring, int status)
>  {
> @@ -3674,8 +3698,15 @@ 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 */
> -					td->bounce_seg = ring->enq_seg;
> +					/*
> +					 * A TD spanning several segments can be
> +					 * bounced once per segment boundary it
> +					 * crosses. Remember the first bounced
> +					 * segment, the rest are found by walking
> +					 * the TD's segments on completion.
> +					 */
> +					if (!td->bounce_seg)
> +						td->bounce_seg = ring->enq_seg;
>  				}
>  			}
>  		}
> -- 
> 2.55.0
> 

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

[Michal Pecio]

On Tue, 11 Aug 2026 10:42:37 +0200, Michal Pecio wrote:
> The code looks correct, though I would do it differently: store the
> last bounce_seg and run the loop from td->start_seg to td->bounce_seg.
> This avoids adding the 'if' during enqueue, and still works correctly
> if the TD wraps around the whole ring so that end_seg == start_seg.
> 
> The driver is never supposed to create such TDs (they break the ring
> expansion procedure) but I prefer more robust code if it costs nothing.
> Bugs happen, or expansion could theoretically become more flexible.
> 
> Regards,
> Michal
> 
> > +	seg = td->bounce_seg;
> > +	for (i = 0; i < ring->num_segs; i++) {
> > +		if (seg->bounce_len)
> > +			xhci_unmap_one_bounce_buffer(xhci, ring, td, seg);
> > +		if (seg == td->end_seg)
> > +			break;
> > +		seg = seg->next;
> > +	}

Oops, sorry, Mathias is right, this loop may incorrectly unmap a buffer
belonging to a later TD which starts somewhere in td->end_seg.

I think the alternative procedure I suggested is free of this problem.

- if td has bounce_seg, then it surely spans beyond start_seg, so we
  can safely unmap start_seg's bounce buffer
- if start_seg == bounce_seg then this was the last bounce to unmap,
  because it's impossible for end_trb to also be a bonuce.
- otherwise, we can continue to blindly unmap until reachng bounce_seg
- after unmapping bounce_seg, there is nothing more left to unmap

Regards,
Michal

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

[Arthur Gautier]

On Tue, Aug 11, 2026 at 8:42 AM Michal Pecio <michal.pecio@gmail.com> wrote:
> >   device-mapper: verity: 8:2: metadata block 10850 is corrupted
>
> Quite nasty, and not everybody uses dm-verify in particular. It seems
> corruption could also affect rare FAT filesystems with tiny clusters.

I've triggered it very reliably via the dm-verity prefetcher. For the
last 6 months,
my workaround to the crash was disabling the prefetcher, to the cost
of performance.

See https://bugzilla.kernel.org/show_bug.cgi?id=220861

> > Fixes: f9c589e142d0 ("xhci: TD-fragment, align the unsplittable case with a bounce buffer")
> > Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
> > Cc: stable@vger.kernel.org
>
> Note: you don't need to list here the driver maintainer you are sending
> this email to. And no need to *actually* send email to the stable list.

I didn't mean to CC the maintainer here, that CC was for the author of
the commit I was fixing.
The cc just got parsed by git-send-email and sent it automatically.

> > +     if (!ring || !td->bounce_seg || !td->urb)
> > +             return;
>
> Tiny optimization nit: !td->bounce_seg is by far the most likely case,
> so it could be first. The others guard against bugs and "never happen".

Noted.

> Hmm, AI patch? I think most people would write it like below :)

I used AI to walk me through the trb/td logic I was unfamiliar with. This was
a copy paste of one output.

> The code looks correct, though I would do it differently: store the
> last bounce_seg and run the loop from td->start_seg to td->bounce_seg.
> This avoids adding the 'if' during enqueue, and still works correctly
> if the TD wraps around the whole ring so that end_seg == start_seg.

That makes the patch significantly smaller. See the V2.
(https://lore.kernel.org/linux-usb/20260812002554.1166712-1-baloo@superbaloo.net/T/#u)

Thank you both!

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

[Mathias Nyman]

On 8/11/26 06:35, 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.
> 
> This is reachable with a USB mass storage device behind xHCI backing a
> dm-verity target with 512 byte hash blocks. 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 the first bounced segment in td->bounce_seg and, on completion,
> walk the segments the TD covers from there up to td->end_seg, handling
> every segment that still has a pending bounce.
> 

Thanks, nice catch.
I didn't expect bulk TDs spanning three segments.

> Fixes: f9c589e142d0 ("xhci: TD-fragment, align the unsplittable case with a bounce buffer")
> Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Arthur Gautier <baloo@superbaloo.net>
> ---
>   drivers/usb/host/xhci-ring.c | 49 +++++++++++++++++++++++++++++-------
>   1 file changed, 40 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 4f98d8269625..4154e84420ac 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,37 @@ 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;
> +	unsigned int i;
> +
> +	if (!ring || !td->bounce_seg || !td->urb)
> +		return;
> +
> +	/*
> +	 * A TD that spans more than two ring segments crosses several link
> +	 * TRBs, and may have been aligned with a bounce buffer at each of
> +	 * them. Every bounce buffer lives on the segment whose link TRB it
> +	 * was needed for, so walk all segments the TD covers, starting at
> +	 * the first one that was bounced.
> +	 */
> +	seg = td->bounce_seg;
> +	for (i = 0; i < ring->num_segs; i++) {
> +		if (seg->bounce_len)
> +			xhci_unmap_one_bounce_buffer(xhci, ring, td, seg);
> +		if (seg == td->end_seg)
> +			break;
> +		seg = seg->next;

The ordering above needs tuning.

This could call xhci_unmap_one_bounce_buffer() for a segment bounce buffer that
belongs to a later TD, not the one we are currently handling
  
The solution idea looks good otherwise

Thanks
Mathias