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

[Mathias Nyman <mathias.nyman@linux.intel.com>]

On 10/9/26 13:15, sashiko-bot@kernel.org wrote:
> 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,
>                              ^^^^^^^^^^^^
> 

Not sure, not familiar with this. Needs to be investigated


> [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?

sg_next() could be better, but this isn't a high priority issue as we are very
far from any chained scatter-gather list limit.

xhci transfer rings usually have 2 segments. Can possibly be 4 or 8 in future if
we add ring expansion support to offloaded endpoints.

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

There are other issues with PAGE_SIZE > 4096 than this.

Wesley has a series that addresses these issues together with this client side change

I'll drop this for now.

We can take this patch later together with the rest of the > 4096 page size + custom
dma pool series.

Wesley, any idea about the virt_to_page(seg->trbs) issue?

Thanks
Mathias