Re: [PATCH 1/4] ALSA: compress: fix buffer leak on repeated SET_PARAMS

[Vinod Koul <vkoul@kernel.org>]

On 07-10-26, 17:25, Péter Ujfalusi wrote:
> 
> 
> On 07/10/2026 17:10, Vinod Koul wrote:
> > On 07-10-26, 16:04, Takashi Iwai wrote:
> >> On Wed, 07 Oct 2026 15:25:06 +0200,
> >> Peter Ujfalusi wrote:
> >>>
> >>> snd_compr_allocate_buffer() unconditionally overwrites
> >>> stream->runtime->buffer with a freshly kmalloc'd buffer whenever the
> >>> driver has no ops->copy and no preallocated dma_buffer_p. SET_PARAMS
> >>> is permitted repeatedly while the stream is in the OPEN state, so a
> >>> local process can loop SNDRV_COMPRESS_SET_PARAMS and leak the
> >>> previous buffer on every call, exhausting kernel memory.
> >>>
> >>> Free any framework-owned buffer before replacing it, mirroring the
> >>> ownership check already used in snd_compr_free().
> >>>
> >>> Fixes: b21c60a4edd2 ("ALSA: core: add support for compress_offload")
> >>> Cc: stable@vger.kernel.org
> >>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
> >>> ---
> >>>  sound/core/compress_offload.c | 4 ++++
> >>>  1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> >>> index 7c397b1c9231..c0ed76e1c844 100644
> >>> --- a/sound/core/compress_offload.c
> >>> +++ b/sound/core/compress_offload.c
> >>> @@ -613,6 +613,10 @@ static int snd_compr_allocate_buffer(struct snd_compr_stream *stream,
> >>>  			return -ENOMEM;
> >>>  	}
> >>>  
> >>> +	/* a prior SET_PARAMS may have left a framework-owned buffer behind */
> >>> +	if (!stream->runtime->dma_buffer_p)
> >>> +		kfree(stream->runtime->buffer);
> >>> +
> >>
> >> I'd rather put to the else block above (or even better, if
> >> (stream->runtime->dma_buffer_p) block above that point).
> >>
> >> The code is specific to that condition, after all.
> > 
> > I would block calling snd_compr_allocate_buffer() for subsequent
> > set_params, it should not be allowed.
> 
> OK, let me see how it should be done. I'm not sure of changing other
> params are permitted either, but imagine:

I would split it up for next_track and open, so that we have clear
flows.

> the application calls set_params first then after some deliberation and
> before starting it reconsiders and wants to have bigger/smaller buffer.
> I know, they rarely do, but if such application assumes that the second
> buffer setup is valid, while we kept the initial one, things might break?

Hmmm, do we really want that. they can tear down and reopen in that
case?

If we split as above, in next_track case we can apply codec params while
ignoring buffer.. wdyt

-- 
~Vinod