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

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

[Peter Ujfalusi]

Hi,

Changes since v1:
- patch 1: do not free the old buffer in snd_compr_allocate_buffer().
  SET_PARAMS can only be repeated in OPEN state after ops->set_params()
  has failed, so release the framework-owned buffer on that error path
  instead. The buffer is allocated once per successful SET_PARAMS and is
  never replaced (Vinod, Takashi)
- patch 2: reworded the commit message: the buffer is set up once and the
  next_track path does not re-allocate it. The geometry check is kept
  because ops->set_params() would otherwise receive buffer parameters
  that do not match the buffer in use. tinycompress always passes the
  open-time fragment_size/fragments on next_track, so existing users are
  not affected (Vinod)
- patches 3 and 4: added Acked-by from Vinod

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

A SET_PARAMS that the driver rejects leaves the framework-owned buffer
allocated while the stream stays in the OPEN state, so a following
SET_PARAMS overwrites the pointer and leaks the buffer. During gapless
playback, SET_PARAMS can also change the buffer geometry while the byte
counters still refer to the current 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:
- releasing the framework-owned buffer when ops->set_params() fails, so
  the buffer is allocated once per successful SET_PARAMS;
- not re-allocating the buffer on the gapless next_track path and
  rejecting a buffer geometry change there;
- 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 failed 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 | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

-- 
2.56.0

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

[Peter Ujfalusi]

When the driver has neither ops->copy nor a preallocated dma_buffer_p,
snd_compr_allocate_buffer() kmallocs runtime->buffer. If
ops->set_params() then rejects the parameters, SET_PARAMS fails, the
stream stays in the OPEN state and keeps the buffer. The next
SET_PARAMS allocates a new buffer and overwrites the pointer, so
repeated SET_PARAMS calls with parameters that the driver rejects leak
one buffer per call.

Release the framework-owned buffer when ops->set_params() fails on an
OPEN stream, so that a failed SET_PARAMS leaves no buffer behind and
the buffer is allocated only once per successful setup. Use the same
ownership check as 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 | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 5c0db178d635..50301f72579e 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -702,8 +702,15 @@ snd_compr_set_params(struct snd_compr_stream *stream, unsigned long arg)
 			return -ENOMEM;
 
 		retval = stream->ops->set_params(stream, params);
-		if (retval)
+		if (retval) {
+			/* the stream stays OPEN: drop the buffer of the rejected setup */
+			if (stream->runtime->state == SNDRV_PCM_STATE_OPEN &&
+			    !stream->runtime->dma_buffer_p) {
+				kfree(stream->runtime->buffer);
+				stream->runtime->buffer = NULL;
+			}
 			return retval;
+		}
 
 		if (stream->next_track)
 			return retval;
-- 
2.56.0

[PATCH v2 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.

The buffer is set up once, with the initial SET_PARAMS, and neither the
core nor the DSP expects it to change for a gapless track transition.
Do not call snd_compr_allocate_buffer() on the next_track path and
reject a request that changes the buffer geometry, so that the codec
parameters that the driver receives match the buffer in use.

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 | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 50301f72579e..d6863336f21c 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -697,9 +697,19 @@ 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
+			 * current buffer; its geometry must not change.
+			 */
+			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 retval;
+		}
 
 		retval = stream->ops->set_params(stream, params);
 		if (retval) {
-- 
2.56.0

[PATCH v2 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>
Acked-by: Vinod Koul <vkoul@kernel.org>
---
 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 d6863336f21c..a250c6b53996 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -789,6 +789,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

[PATCH v2 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>
Acked-by: Vinod Koul <vkoul@kernel.org>
---
 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 a250c6b53996..5e5e0f78f26d 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -1206,7 +1206,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 v2 0/4] ALSA: compress: fix buffer handling and state validation

[Takashi Iwai]

On Mon, 19 Oct 2026 16:45:29 +0200,
Peter Ujfalusi wrote:
> 
> Hi,
> 
> Changes since v1:
> - patch 1: do not free the old buffer in snd_compr_allocate_buffer().
>   SET_PARAMS can only be repeated in OPEN state after ops->set_params()
>   has failed, so release the framework-owned buffer on that error path
>   instead. The buffer is allocated once per successful SET_PARAMS and is
>   never replaced (Vinod, Takashi)
> - patch 2: reworded the commit message: the buffer is set up once and the
>   next_track path does not re-allocate it. The geometry check is kept
>   because ops->set_params() would otherwise receive buffer parameters
>   that do not match the buffer in use. tinycompress always passes the
>   open-time fragment_size/fragments on next_track, so existing users are
>   not affected (Vinod)
> - patches 3 and 4: added Acked-by from Vinod
> 
> The compress offload core has several edge cases in buffer management
> and state validation.
> 
> A SET_PARAMS that the driver rejects leaves the framework-owned buffer
> allocated while the stream stays in the OPEN state, so a following
> SET_PARAMS overwrites the pointer and leaks the buffer. During gapless
> playback, SET_PARAMS can also change the buffer geometry while the byte
> counters still refer to the current 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:
> - releasing the framework-owned buffer when ops->set_params() fails, so
>   the buffer is allocated once per successful SET_PARAMS;
> - not re-allocating the buffer on the gapless next_track path and
>   rejecting a buffer geometry change there;
> - 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 failed 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

Applied all four patches now.  Thanks.


Takashi