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

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

[Xu Rao]

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);
+
 	/*
 	 * Save first segment dma address to sg dma_address field for the sideband
 	 * client to have access to the IOVA of the ring.
--
2.50.1

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

[Greg KH]

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?

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

thanks,

greg k-h

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

[Michal Pecio]

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.