Re: [PATCH 13/14] usb: xhci: Fix bounce buffer overflow

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] usb: xhci: inconsistent TD Size calculation for out-of-spec bulk devices
--

commit f7283e43a6da5355da08e764e6aa96a4a118927d
Author: Michal Pecio <michal.pecio@gmail.com>
Subject: usb: xhci: Fix bounce buffer overflow

High-speed devices with out of spec 1024 byte bulk endpoints exist and
are allowed by USB core, but xhci-hcd always sets packet size to 512.
This commit fixes a bounce buffer overflow for these devices by allocating
a 512 byte bounce buffer and using the correctly clamped packet size
in xhci_align_td().

Link: https://lore.kernel.org/linux-usb/D4tcSGerkYkIV1DmaUo1t8TaR5qQElDLkidn@bugs.sh/

> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 243b1fd2b2f68..c23434001e9cb 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
[ ... ]
> @@ -3699,9 +3697,8 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
>  		if (enqd_len + trb_buff_len < full_len) {
>  			field |= TRB_CHAIN;
>  			if (trb_is_link(ring->enqueue + 1)) {
> -				if (xhci_align_td(xhci, urb, enqd_len,
> -						  &trb_buff_len,
> -						  ring->enq_seg)) {
> +				if (xhci_align_td(xhci, urb, enqd_len, &trb_buff_len,
> +						ring->enq_seg, ring->bounce_buf_len)) {

[Severity: Medium]
While xhci_align_td() is correctly updated here to use the clamped
ring->bounce_buf_len, does the xhci_td_remainder() call further down in
xhci_queue_bulk_tx() also need to be updated?

xhci_td_remainder() still relies on the raw 1024-byte max packet size from the
descriptor:

drivers/usb/host/xhci-ring.c:xhci_td_remainder() {
	...
	maxp = xhci_usb_endpoint_maxp(urb->dev, urb->ep);
	total_packet_count = DIV_ROUND_UP(td_total_len, maxp);

	/* Queueing functions don't count the current TRB into transferred */
	return (total_packet_count - ((transferred + trb_buff_len) / maxp));
}

Since the endpoint context and bounce buffer use a clamped 512-byte size, could
this mismatch cause the driver to write an incorrectly small TD Size into the
TRB? If the xHC hardware is using 512 bytes, might this mismatch lead to a
Transfer Ring Expansion Error, babble, or endpoint halt when queueing a bulk
transfer greater than 1024 bytes for a high-speed device with an out-of-spec
1024-byte max packet size?

>  					send_addr = ring->enq_seg->bounce_dma;
>  					/* TD bounced at least, and last on this seg */
>  					td->bounce_seg = ring->enq_seg;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009095834.561578-1-mathias.nyman@linux.intel.com?part=13