[PATCH 0/4] ALSA: compress: fix buffer handling and state validation

[PATCH 0/4] ALSA: compress: fix buffer handling and state validation

[Peter Ujfalusi]

Hi,

The compress offload core has several edge cases in buffer management
and state validation.

Repeated SET_PARAMS while a stream is OPEN can replace a framework-owned
buffer without freeing it. During gapless playback, SET_PARAMS can also
change the buffer geometry while the byte counters still refer to the
previous buffer. Separately, SET_METADATA can reach the driver before
SET_PARAMS, and the origin_seqno task lookup can restart a task that is
still ACTIVE.

Address these cases by:
- freeing a prior framework-owned buffer before replacing it;
- rejecting buffer geometry changes on the gapless next_track path;
- rejecting SET_METADATA while the stream is OPEN; and
- allowing origin_seqno to reuse IDLE or FINISHED tasks, but not ACTIVE
  ones.

Regards,
Peter
---
Peter Ujfalusi (4):
  ALSA: compress: fix buffer leak on repeated SET_PARAMS
  ALSA: compress: reject buffer geometry change on gapless next_track
  ALSA: compress: add missing OPEN-state gate to SET_METADATA
  ALSA: compress: reject restarting an ACTIVE task via origin_seqno

 sound/core/compress_offload.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

-- 
2.56.0

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

[Peter Ujfalusi]

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);
+
 	stream->runtime->buffer = buffer;
 	stream->runtime->buffer_size = buffer_size;
 params:
-- 
2.56.0

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

[Takashi Iwai]

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.


thanks,

Takashi

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

[Vinod Koul]

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.

-- 
~Vinod

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

[Péter Ujfalusi]

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:
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?

-- 
Péter

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

[Vinod Koul]

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

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

[Péter Ujfalusi]

On 07/10/2026 17:37, Vinod Koul wrote:
> 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

What if we just refuse consequent set_params in OPEN state?
That does not make much sense and can collapse patch 1 and 2 into one.

> 

-- 
Péter

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

[Péter Ujfalusi]

On 08/10/2026 09:25, Péter Ujfalusi wrote:
>>>> 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
> 
> What if we just refuse consequent set_params in OPEN state?
> That does not make much sense and can collapse patch 1 and 2 into one.
tried several other ways on this (patch 2 included) and at the end I
have ended up with the exact same patches.

Two separate issues and the two patch covers the points well.

The set_params in OPEN state is sticking out, but I think that allowing
it to re-allocate memory is the right thing:
this can only happen if the stream->ops->set_params(stream, params);
fails and in that case user space might want to try different memory layout.

I would leave the series unchanged.

-- 
Péter

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

[Péter Ujfalusi]

On 07/10/2026 17: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.

Yes, that is true, but then we would also need to add additional check
in there and free only if the allocation succeeded.

I think that would somehow be a bit more convoluted to grasp at once

-- 
Péter

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

[Vinod Koul]

On 07-10-26, 16:25, 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.


Logically this looks right but then I dont think we have or we would
want a case where we are allowing buffer changes on the fly. Set params
on the fly would set some params like code properties etc and not code
type or buffer ones...

> 
> 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);
> +
>  	stream->runtime->buffer = buffer;
>  	stream->runtime->buffer_size = buffer_size;
>  params:
> -- 
> 2.56.0

-- 
~Vinod

[PATCH 2/4] ALSA: compress: reject buffer geometry change on gapless next_track

[Peter Ujfalusi]

snd_compr_set_params() is invoked from the gapless next_track
re-entry path, which calls snd_compr_allocate_buffer() again and lets
buffer_size change while total_bytes_available and
total_bytes_transferred still hold values computed for the previous
buffer. snd_compr_calc_avail() then derives an avail/count value from
those stale counters against the new buffer_size, and
snd_compr_write_data() copy_from_user()s that many bytes into the
new, potentially smaller buffer, overflowing it with user-controlled
length and content.

Reject a next_track SET_PARAMS that changes the buffer geometry
instead of reallocating, since total_bytes_available/transferred are
only valid for the buffer sized during the initial SET_PARAMS.

Fixes: 9727b490e543 ("ALSA: compress: add support for gapless playback")
Cc: stable@vger.kernel.org
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 sound/core/compress_offload.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index c0ed76e1c844..e1cc34ba2c2c 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -673,9 +673,20 @@ snd_compr_set_params(struct snd_compr_stream *stream, unsigned long arg)
 		if (retval)
 			return retval;
 
-		retval = snd_compr_allocate_buffer(stream, params);
-		if (retval)
-			return -ENOMEM;
+		if (stream->next_track) {
+			/*
+			 * total_bytes_available/transferred still refer to the
+			 * buffer allocated for the current track; the geometry
+			 * must not change underneath them.
+			 */
+			if (params->buffer.fragment_size != stream->runtime->fragment_size ||
+			    params->buffer.fragments != stream->runtime->fragments)
+				return -EINVAL;
+		} else {
+			retval = snd_compr_allocate_buffer(stream, params);
+			if (retval)
+				return -ENOMEM;
+		}
 
 		retval = stream->ops->set_params(stream, params);
 		if (retval)
-- 
2.56.0

Re: [PATCH 2/4] ALSA: compress: reject buffer geometry change on gapless next_track

[Vinod Koul]

On 07-10-26, 16:25, Peter Ujfalusi wrote:
> snd_compr_set_params() is invoked from the gapless next_track
> re-entry path, which calls snd_compr_allocate_buffer() again and lets
> buffer_size change while total_bytes_available and
> total_bytes_transferred still hold values computed for the previous
> buffer. snd_compr_calc_avail() then derives an avail/count value from
> those stale counters against the new buffer_size, and
> snd_compr_write_data() copy_from_user()s that many bytes into the
> new, potentially smaller buffer, overflowing it with user-controlled
> length and content.
> 
> Reject a next_track SET_PARAMS that changes the buffer geometry
> instead of reallocating, since total_bytes_available/transferred are
> only valid for the buffer sized during the initial SET_PARAMS.
> 
> Fixes: 9727b490e543 ("ALSA: compress: add support for gapless playback")
> Cc: stable@vger.kernel.org
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
> ---
>  sound/core/compress_offload.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index c0ed76e1c844..e1cc34ba2c2c 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -673,9 +673,20 @@ snd_compr_set_params(struct snd_compr_stream *stream, unsigned long arg)
>  		if (retval)
>  			return retval;
>  
> -		retval = snd_compr_allocate_buffer(stream, params);
> -		if (retval)
> -			return -ENOMEM;
> +		if (stream->next_track) {
> +			/*
> +			 * total_bytes_available/transferred still refer to the
> +			 * buffer allocated for the current track; the geometry
> +			 * must not change underneath them.
> +			 */
> +			if (params->buffer.fragment_size != stream->runtime->fragment_size ||
> +			    params->buffer.fragments != stream->runtime->fragments)
> +				return -EINVAL;
> +		} else {
> +			retval = snd_compr_allocate_buffer(stream, params);
> +			if (retval)
> +				return -ENOMEM;
> +		}

better, i would go one step further and invoke
snd_compr_allocate_buffer() only once!
I dont think DSP expects buffers will be changed

-- 
~Vinod

[PATCH 3/4] ALSA: compress: add missing OPEN-state gate to SET_METADATA

[Peter Ujfalusi]

snd_compr_set_metadata() calls stream->ops->set_metadata() straight
after copy_from_user(), without ever checking runtime->state, even
though the function's own comment says a parameter change should only
be allowed once the stream has left the OPEN state. Every other
metadata/param entry point in this file (SET_PARAMS, WRITE, ...)
enforces that gate.

A codec driver's set_metadata callback can assume SET_PARAMS has
already run and initialised its private state; calling it while the
stream is still OPEN can operate on that uninitialised state.

Add the OPEN-state check the existing comment already documents.

Fixes: 9727b490e543 ("ALSA: compress: add support for gapless playback")
Cc: stable@vger.kernel.org
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 sound/core/compress_offload.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index e1cc34ba2c2c..bf13fa0877dd 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -759,6 +759,9 @@ snd_compr_set_metadata(struct snd_compr_stream *stream, unsigned long arg)
 	* we should allow parameter change only when stream has been
 	* opened not in other cases
 	*/
+	if (stream->runtime->state == SNDRV_PCM_STATE_OPEN)
+		return -EBADFD;
+
 	if (copy_from_user(&metadata, (void __user *)arg, sizeof(metadata)))
 		return -EFAULT;
 
-- 
2.56.0

Re: [PATCH 3/4] ALSA: compress: add missing OPEN-state gate to SET_METADATA

[Vinod Koul]

On 07-10-26, 16:25, Peter Ujfalusi wrote:
> snd_compr_set_metadata() calls stream->ops->set_metadata() straight
> after copy_from_user(), without ever checking runtime->state, even
> though the function's own comment says a parameter change should only
> be allowed once the stream has left the OPEN state. Every other
> metadata/param entry point in this file (SET_PARAMS, WRITE, ...)
> enforces that gate.
> 
> A codec driver's set_metadata callback can assume SET_PARAMS has
> already run and initialised its private state; calling it while the
> stream is still OPEN can operate on that uninitialised state.
> 
> Add the OPEN-state check the existing comment already documents.

Acked-by: Vinod Koul <vkoul@kernel.org>

-- 
~Vinod

[PATCH 4/4] ALSA: compress: reject restarting an ACTIVE task via origin_seqno

[Peter Ujfalusi]

snd_compr_task_start_prepare() only rejects tasks with
state >= SND_COMPRESS_TASK_STATE_FINISHED. Since the enum is
IDLE(0) < ACTIVE(1) < FINISHED(2), this accepts ACTIVE tasks and
rejects FINISHED ones, the opposite of what the origin_seqno
mechanism needs: origin_seqno is meant to let a caller reuse a
FINISHED task's buffers for a new job, not to restart a task that is
still queued with the driver.

The seqno lookup path in snd_compr_task_start() has its own explicit
"task->state != IDLE" guard, but the origin_seqno lookup path relies
solely on this function, so it can restart an ACTIVE task, causing
ops->task_start() to be called twice for the same task and
runtime->active_tasks to be incremented twice without a matching
finish/stop.

Accept IDLE and FINISHED, reject ACTIVE, on both entry paths.

Fixes: 04177158cf98 ("ALSA: compress_offload: introduce accel operation mode")
Cc: stable@vger.kernel.org
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 sound/core/compress_offload.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index bf13fa0877dd..27bc490aa3ff 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -1154,7 +1154,9 @@ static int snd_compr_task_start_prepare(struct snd_compr_task_runtime *task,
 {
 	if (task == NULL)
 		return -EINVAL;
-	if (task->state >= SND_COMPRESS_TASK_STATE_FINISHED)
+	/* only an unqueued or already finished task may be (re)started */
+	if (task->state != SND_COMPRESS_TASK_STATE_IDLE &&
+	    task->state != SND_COMPRESS_TASK_STATE_FINISHED)
 		return -EBUSY;
 	if (utask->input_size > task->input->size)
 		return -EINVAL;
-- 
2.56.0

Re: [PATCH 4/4] ALSA: compress: reject restarting an ACTIVE task via origin_seqno

[Vinod Koul]

On 07-10-26, 16:25, Peter Ujfalusi wrote:
> snd_compr_task_start_prepare() only rejects tasks with
> state >= SND_COMPRESS_TASK_STATE_FINISHED. Since the enum is
> IDLE(0) < ACTIVE(1) < FINISHED(2), this accepts ACTIVE tasks and
> rejects FINISHED ones, the opposite of what the origin_seqno
> mechanism needs: origin_seqno is meant to let a caller reuse a
> FINISHED task's buffers for a new job, not to restart a task that is
> still queued with the driver.
> 
> The seqno lookup path in snd_compr_task_start() has its own explicit
> "task->state != IDLE" guard, but the origin_seqno lookup path relies
> solely on this function, so it can restart an ACTIVE task, causing
> ops->task_start() to be called twice for the same task and
> runtime->active_tasks to be incremented twice without a matching
> finish/stop.
> 
> Accept IDLE and FINISHED, reject ACTIVE, on both entry paths.

Acked-by: Vinod Koul <vkoul@kernel.org>

-- 
~Vinod