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

Arthur Gautier <[email protected]>
Newsgroups org.kernel.vger.linux-usb,org.kernel.vger.stable
Message-ID <[email protected]>
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 <[email protected]>
Cc: [email protected]
Signed-off-by: Arthur Gautier <[email protected]>
---
 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
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.