Re: [PATCH 07/14] usb: xhci: sideband: fix ring sg table for sub-page TRB segments

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] usb: xhci: OOB array access and scatterlist corruption via sgt->sgl[i] direct indexing
- [High] usb: xhci: DMA API violation and potential panic via virt_to_page() on coherent DMA memory
- [High] usb: xhci: sideband: incomplete fix as qc_audio_offload ignores sg->offset
--

commit 94c9b1944dfef1bf09409dc3dc3f51bb0547b9a2
Author: Wesley Cheng <wesley.cheng@oss.qualcomm.com>

usb: xhci: sideband: fix ring sg table for sub-page TRB segments

This patch modifies xhci_ring_to_sgtable() to directly build an sg_table
instead of relying on dma_get_sgtable() and sg_alloc_table_from_pages(),
which only operate on whole PAGE_SIZE chunks. The aim is to support kernels
with larger page sizes (16K/64K) where multiple TRB segments share a page.

> diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
> index a5deeee4d5dce..beb637407e47e 100644
> --- a/drivers/usb/host/xhci-sideband.c
> +++ b/drivers/usb/host/xhci-sideband.c
[ ... ]
> @@ -9,57 +9,42 @@
>   */
>  
>  #include <linux/usb/xhci-sideband.h>
> -#include <linux/dma-direct.h>
>  
>  #include "xhci.h"
>  
>  /* sideband internal helpers */
>  static struct sg_table *
> -xhci_ring_to_sgtable(struct xhci_sideband *sb, struct xhci_ring *ring)
> +xhci_ring_to_sgtable(struct xhci_ring *ring)
>  {
[ ... ]
> +	if (sg_alloc_table(sgt, ring->num_segs, GFP_KERNEL)) {
> +		kfree(sgt);
>  		return NULL;
>  	}
>  
>  	for (i = 0; i < ring->num_segs; i++) {
> -		dma_get_sgtable(dev, sgt, seg->trbs, seg->dma,
> -				TRB_SEGMENT_SIZE);
> -		pages[i] = sg_page(sgt->sgl);
> -		sg_free_table(sgt);
> +		if (is_vmalloc_addr(seg->trbs))
> +			page = vmalloc_to_page(seg->trbs);
> +		else
> +			page = virt_to_page(seg->trbs);

[Severity: High]
Is it safe to use virt_to_page() on coherent DMA memory here?

The xHCI ring segments (seg->trbs) are allocated via dma_pool_zalloc(), which
uses dma_alloc_coherent(). The DMA API explicitly warns that memory returned
by dma_alloc_coherent() is not guaranteed to be backed by a struct page.

On architectures where DMA memory resides outside the linear direct-map
(e.g., ARM/MIPS platforms using IOMEM or CMA reserved pools), virt_to_page()
computes an invalid pointer. The previous code safely handled this by
delegating to dma_get_sgtable(), which uses architecture-specific logic.

> +
> +		sg_set_page(&sgt->sgl[i], page, TRB_SEGMENT_SIZE,
                            ^^^^^^^^^^^^

[Severity: High]
Can this array indexing corrupt the scatterlist and cause an out-of-bounds
write?

The function sg_alloc_table() returns a chained scatterlist when the
requested number of entries (ring->num_segs) exceeds SG_MAX_SINGLE_ALLOC
(typically 128).

When i reaches 127 (the chain pointer index), sg_set_page() overwrites the
chain link, severing the rest of the list. On subsequent iterations
(e.g., i = 128), &sgt->sgl[i] writes into unallocated out-of-bounds heap
memory.

Should this iterate over the allocated sg_table using the for_each_sg() macro
or sg_next() instead of flat array indexing?

[Severity: High]
Does this fix leave the issue unresolved for existing callers?

While this patch adds the correct offset to the sg_table for sub-page TRB
segments, downstream consumers currently ignore it.

For example, in sound/usb/qcom/qc_audio_offload.c:uaudio_endpoint_setup():

	pg = sg_page(sgt->sgl);
	tr_pa = page_to_phys(pg);

And similarly in uaudio_event_ring_setup():

	pg = sg_page(sgt->sgl);
	er_pa = page_to_phys(pg);

These calls extract the page via sg_page() and use page_to_phys(), which
completely strips the sg->offset. On systems with PAGE_SIZE > 4096, this maps
the start of the kernel page instead of the actual segment offset, which
could cause the audio offload hardware to read from the wrong physical
location.

Will the consumers need updates to use sg_phys() instead?

> +			    offset_in_page(seg->trbs));
>  		seg = seg->next;
>  	}

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