[PATCH] ALSA: hda/core: fix memory leak in snd_hdac_bus_alloc_stream_pages()

[PATCH] ALSA: hda/core: fix memory leak in snd_hdac_bus_alloc_stream_pages()

[Zhao Dongdong]

From: Zhao Dongdong <zhaodongdong@kylinos.cn>

In snd_hdac_bus_alloc_stream_pages(), there are three memory leak paths:

  1. If snd_dma_alloc_pages() for the BDL fails at stream i, the
     previously allocated BDL DMA buffers (0..i-1) are not freed.

  2. If snd_dma_alloc_pages() for the position buffer fails, all
     previously allocated BDL DMA buffers are leaked.

  3. If snd_dma_alloc_pages() for the ring buffer fails, all
     previously allocated BDL DMA buffers and the position buffer
     are leaked.

Add proper error cleanup: free buffers that have been allocated
before each failure point.

Fixes: b2660d1ebde1 ("ALSA: hda: Move HD-audio core stuff into sound/hda/core")
Signed-off-by: Zhao Dongdong <zhaodongdong@kylinos.cn>
---
 sound/hda/core/controller.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/sound/hda/core/controller.c b/sound/hda/core/controller.c
index 69e11d62bbfa..5d8731447ef6 100644
--- a/sound/hda/core/controller.c
+++ b/sound/hda/core/controller.c
@@ -713,7 +713,7 @@ int snd_hdac_bus_alloc_stream_pages(struct hdac_bus *bus)
 					  BDL_SIZE, &s->bdl);
 		num_streams++;
 		if (err < 0)
-			return -ENOMEM;
+			goto error_bdl;
 	}
 
 	if (WARN_ON(!num_streams))
@@ -722,12 +722,24 @@ int snd_hdac_bus_alloc_stream_pages(struct hdac_bus *bus)
 	err = snd_dma_alloc_pages(dma_type, bus->dev,
 				  num_streams * 8, &bus->posbuf);
 	if (err < 0)
-		return -ENOMEM;
+		goto error_bdl;
 	list_for_each_entry(s, &bus->stream_list, list)
 		s->posbuf = (__le32 *)(bus->posbuf.area + s->index * 8);
 
 	/* single page (at least 4096 bytes) must suffice for both ringbuffes */
-	return snd_dma_alloc_pages(dma_type, bus->dev, PAGE_SIZE, &bus->rb);
+	err = snd_dma_alloc_pages(dma_type, bus->dev, PAGE_SIZE, &bus->rb);
+	if (err < 0)
+		goto error_posbuf;
+	return 0;
+
+error_posbuf:
+	snd_dma_free_pages(&bus->posbuf);
+error_bdl:
+	list_for_each_entry(s, &bus->stream_list, list) {
+		if (s->bdl.area)
+			snd_dma_free_pages(&s->bdl);
+	}
+	return -ENOMEM;
 }
 EXPORT_SYMBOL_GPL(snd_hdac_bus_alloc_stream_pages);
 
-- 
2.25.1

Re: [PATCH] ALSA: hda/core: fix memory leak in snd_hdac_bus_alloc_stream_pages()

[Takashi Iwai]

On Mon, 06 Jul 2026 09:41:10 +0200,
Zhao Dongdong wrote:
> 
> From: Zhao Dongdong <zhaodongdong@kylinos.cn>
> 
> In snd_hdac_bus_alloc_stream_pages(), there are three memory leak paths:
> 
>   1. If snd_dma_alloc_pages() for the BDL fails at stream i, the
>      previously allocated BDL DMA buffers (0..i-1) are not freed.
> 
>   2. If snd_dma_alloc_pages() for the position buffer fails, all
>      previously allocated BDL DMA buffers are leaked.
> 
>   3. If snd_dma_alloc_pages() for the ring buffer fails, all
>      previously allocated BDL DMA buffers and the position buffer
>      are leaked.
> 
> Add proper error cleanup: free buffers that have been allocated
> before each failure point.
> 
> Fixes: b2660d1ebde1 ("ALSA: hda: Move HD-audio core stuff into sound/hda/core")
> Signed-off-by: Zhao Dongdong <zhaodongdong@kylinos.cn>

They don't actually leak, as those are supposed to be released by
callers (via snd_hdac_bus_free_stream_pages() call at destructor).
That is, it's no bug, per se.

Of course, it won't be bad to clean up in the function itself, which
is more self-contained and safer.  But please rephrase the patch
subject and description, so that it won't be taken as a security issue
incorrectly.


thanks,

Takashi

Re: [PATCH] ALSA: hda/core: fix memory leak in snd_hdac_bus_alloc_stream_pages()

[Zhao Dongdong]

On Mon, 06 Jul 2026 10:11:56 +0200, Takashi Iwai <tiwai@suse.de> wrote:
>
> On Mon, 06 Jul 2026 09:41:10 +0200,
> Zhao Dongdong wrote:
> > 
> > From: Zhao Dongdong <zhaodongdong@kylinos.cn>
> > 
> > In snd_hdac_bus_alloc_stream_pages(), there are three memory leak paths:
> > 
> >   1. If snd_dma_alloc_pages() for the BDL fails at stream i, the
> >      previously allocated BDL DMA buffers (0..i-1) are not freed.
> > 
> >   2. If snd_dma_alloc_pages() for the position buffer fails, all
> >      previously allocated BDL DMA buffers are leaked.
> > 
> >   3. If snd_dma_alloc_pages() for the ring buffer fails, all
> >      previously allocated BDL DMA buffers and the position buffer
> >      are leaked.
> > 
> > Add proper error cleanup: free buffers that have been allocated
> > before each failure point.
> > 
> > Fixes: b2660d1ebde1 ("ALSA: hda: Move HD-audio core stuff into sound/hda/core")
> > Signed-off-by: Zhao Dongdong <zhaodongdong@kylinos.cn>
>
> They don't actually leak, as those are supposed to be released by
> callers (via snd_hdac_bus_free_stream_pages() call at destructor).
> That is, it's no bug, per se.
>
> Of course, it won't be bad to clean up in the function itself, which
> is more self-contained and safer.  But please rephrase the patch
> subject and description, so that it won't be taken as a security issue
> incorrectly.
>
>
> thanks,
>
> Takashi

Thanks for your review. 
You are right that snd_hdac_bus_alloc_stream_pages and snd_hdac_bus_free_stream_pages 
are paired, so the extra free call is unnecessary, please ignore this patch.


-- 
Regards,
    Zhao Dongdong

Re: [PATCH] ALSA: hda/core: fix memory leak in snd_hdac_bus_alloc_stream_pages()

[Takashi Iwai]

On Mon, 06 Jul 2026 10:51:11 +0200,
Zhao Dongdong wrote:
> 
> On Mon, 06 Jul 2026 10:11:56 +0200, Takashi Iwai <tiwai@suse.de> wrote:
> >
> > On Mon, 06 Jul 2026 09:41:10 +0200,
> > Zhao Dongdong wrote:
> > > 
> > > From: Zhao Dongdong <zhaodongdong@kylinos.cn>
> > > 
> > > In snd_hdac_bus_alloc_stream_pages(), there are three memory leak paths:
> > > 
> > >   1. If snd_dma_alloc_pages() for the BDL fails at stream i, the
> > >      previously allocated BDL DMA buffers (0..i-1) are not freed.
> > > 
> > >   2. If snd_dma_alloc_pages() for the position buffer fails, all
> > >      previously allocated BDL DMA buffers are leaked.
> > > 
> > >   3. If snd_dma_alloc_pages() for the ring buffer fails, all
> > >      previously allocated BDL DMA buffers and the position buffer
> > >      are leaked.
> > > 
> > > Add proper error cleanup: free buffers that have been allocated
> > > before each failure point.
> > > 
> > > Fixes: b2660d1ebde1 ("ALSA: hda: Move HD-audio core stuff into sound/hda/core")
> > > Signed-off-by: Zhao Dongdong <zhaodongdong@kylinos.cn>
> >
> > They don't actually leak, as those are supposed to be released by
> > callers (via snd_hdac_bus_free_stream_pages() call at destructor).
> > That is, it's no bug, per se.
> >
> > Of course, it won't be bad to clean up in the function itself, which
> > is more self-contained and safer.  But please rephrase the patch
> > subject and description, so that it won't be taken as a security issue
> > incorrectly.
> >
> >
> > thanks,
> >
> > Takashi
> 
> Thanks for your review. 
> You are right that snd_hdac_bus_alloc_stream_pages and snd_hdac_bus_free_stream_pages 
> are paired, so the extra free call is unnecessary, please ignore this patch.

Well, your code change itself is fine as a code hardening, and I'll
happily apply if the patch subject and description are updated.
So, feel free to resubmit with the updated description.


thanks,

Takashi