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

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/[email protected]/
> 
> 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 <[email protected]>
>> Signed-off-by: Arthur Gautier <[email protected]>
> 
> 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