Re: [PATCH] xhci: sideband: fix ring sg table pages leak

[Michal Pecio <michal.pecio@gmail.com>]

On Fri, 10 Jul 2026 15:51:17 +0200, Greg KH wrote:
> On Tue, Jun 02, 2026 at 11:43:27AM +0800, Xu Rao wrote:
> > xhci_ring_to_sgtable() allocates a temporary pages array and
> > uses it to build the returned sg_table with
> > sg_alloc_table_from_pages().
> > 
> > The error paths free the pages array, but the success path
> > returns the sg_table without freeing it. This leaks the temporary
> > array every time a sideband client gets an endpoint or event ring
> > buffer.
> > 
> > Free the pages array after sg_alloc_table_from_pages() succeeds.
> > The returned sg_table has its own scatterlist entries and does not
> > depend on the temporary array after construction.
> > 
> > Fixes: de66754e9f80 ("xhci: sideband: add initial api to register a secondary interrupter entity")
> > 
> > Signed-off-by: Xu Rao <raoxu@uniontech.com>
> > ---
> >  drivers/usb/host/xhci-sideband.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
> > index 23153e136d4b..a5deeee4d5dc 100644
> > --- a/drivers/usb/host/xhci-sideband.c
> > +++ b/drivers/usb/host/xhci-sideband.c
> > @@ -58,6 +58,8 @@ xhci_ring_to_sgtable(struct xhci_sideband *sb, struct xhci_ring *ring)
> >  	if (sg_alloc_table_from_pages(sgt, pages, n_pages, 0, sz, GFP_KERNEL))
> >  		goto err;
> > 
> > +	kvfree(pages);  
> 
> This looks very odd, how was this tested?  If
> sg_alloc_table_from_pages() succeeds, then the table has the pages in
> it, you don't want to free them again, right?

This functions copies pointers to long-lived allocations from private
driver data structures into a standard sg_table.

'pages' is an array temporarily holding references to ring segments,
under the optimistic assumption that segments are exactly page sized.
If they aren't, the returned sg_table is wrong, and if page size > 4K
then 'pages' is allocated too short and used out of bounds.

AFAIU, sg_alloc_table_from_pages() converts those page pointers into
addresses stored in scatterlist nodes, so freeing 'pages' is right.
But see above, the whole implementation is questionable.

> And if this is such a major bug, why hasn't anyone noticed the leak
> yet?

The leak is a few bytes per offloaded device.