Re: [PATCH v4 3/5] usb: xhci: sideband: allocate sideband ring segments from a dedicated pool

[Wesley Cheng <wesley.cheng@oss.qualcomm.com>]

On 10/13/2026 2:11 AM, Michal Pecio wrote:
> On Mon, 12 Oct 2026 18:01:50 -0700, Wesley Cheng wrote:
>> On 10/5/2026 4:03 AM, Mathias Nyman wrote:
>>> xhci is the one allocating the ring when adding an endpoint, and
>>> telling xHC hardware to write to that ring. It seems more
>>> approriate that xhci should be the one freeing the ring after it
>>> told xHC hardware to drop the endpoint, and there no longer is a
>>> risk on hardware accessing it.
>>
>> Hi Mathias,
>>
>> Sorry for the late response.  Was out last week.
>>
>> Specifically in the disconnect path (while audio offload is active),
>> when we utilize the usb_set_interface() path, it'll return early due
>> to the iface->unregistering check.  This would lead to the
>> drop_endpoint not to be called, and rely on the xhci_free_dev() to
>> handle the transfer ring cleanup.  This wouldn't work in the offload
>> situation, because qc_audio_offload is the only entity managing the
>> DMA pool, and that would be freed during the .disconnect callback,
>> which happens BEFORE xhci_free_dev().
>>
>> Would you be more inclined to prefer a solution where we can defer
>> the DMA pool free'ing to the xHCI core during xhci_free_dev, or maybe
>> adding another sideband notifier (sb->notify_client) enum to tell the
>> client driver that it can release the pool?
> 
> I'm not Mathias but my opinion is the same as always: none of these
> problems would exist if the sideband client allocated and freed all
> transfer/event ring memory, and xhci-sideband only owned a mapping.
> 
> The mapping would be freed by xhci_sideband_unregister() or similar
> calls, which should ensure that the xHC stops using the rings, and
> memory would only be freed by the client driver after unregistering
> the sideband and stopping the coprocessor. Nothing is ever UAF or
> leaked, and ensuring this is *easy*. And if xhci-hcd randomly wants
> to get rid of some endpoint, it simply stops using the mapping and
> lets the sideband client clean it up two milliseconds later.
> 

Hi Michal

Your feedback is always welcomed.

I guess my intention was to see if we could leverage the existing memory 
operations within xHCI as much as we could.  The only concern I would have 
with having the client own all the memory operations/allocations is that we 
would need to carefully maintain the transitional phases between offloaded 
and non-offloaded scenarios to be handled properly.  At least in the audio 
offload case, we're only offloading data to the DSP when audio is active, 
which can be for a short duration, and we have proper checks and locking 
already within the xHCI mem/core layers.

> One thing I'm not entirely sure about is how we could create this
> mapping. Using bare iommu_map() creates a hard dependency on IOMMU.
> Documentation/core-api/dma-attributes.rst mentions possibility of
> using generic dma_map_() helpers on multiple devices, so maybe this
> can work, but I haven't seen existing examples of such use.
> 
> And there seems to be one more potential problem case, which hasn't
> been mentioned in this thread AFAIK: if xHC resume failes, xhci-hcd
> frees its private data and tell USB core to reset-resume devices.
> Looks like another opportunity for UAF.
> 

Since I did take a look at the other issue that probably reminded you of 
this, without getting on to a tangent subject, it really broke down to two 
scenarios:
1.  reset_resume during bus resume
2.  usb_device_reset for recovery

In both situations there are requirements that when those happen/trigger, 
technically, the bus should be completely idle.  For #1, since we're coming 
from a bus suspend transition, there should be no pending URBs in flight, 
so if reset_resume happened, xfer rings aren't being utilized.  For #2, 
there were some changes that weren't properly handling the pre and post 
reset callbacks, leading to problems in that situation.  I think that 
series showed us that there are corner cases where referencing vdev could 
potentially be unreliable.  I'm going to assume this has no relationship to 
the client driver owning memory, please correct me if I'm wrong. :)

Thanks
Wesley Cheng