[PATCH 1/6] ALSA: compress: remove illegal state mutation in poll()

[PATCH 1/6] ALSA: compress: remove illegal state mutation in poll()

[Omer Cohen]

snd_compr_poll() transitions runtime->state from DRAINING to SETUP
when it observes a drained stream.  This write is unsynchronized with
snd_compress_wait_for_drain(), which reads runtime->state in its
wait_event condition after the device mutex is dropped.

KCSAN flags this on an arm64 system:

  BUG: KCSAN: data-race in snd_compr_poll / snd_compress_wait_for_drain

  write to 0xffff0000c75c8200 of 4 bytes by task 282 on cpu 3:
   snd_compr_poll+0x29c/0x2c8

  read to 0xffff0000c75c8200 of 4 bytes by task 279 on cpu 1:
   snd_compress_wait_for_drain+0xa4/0x270

  value changed: 0x00000005 -> 0x00000001

poll() is a query operation and should not mutate stream state.  The
DRAINING to SETUP transition is already performed by drivers via
snd_compr_drain_notify() when drain actually completes.  The redundant
transition in poll() has been present since the initial compress
offload implementation but only manifests when poll() and drain run
concurrently from different threads.

Remove the state mutation from poll().  When the stream is DRAINING,
report it as ready so userspace can proceed without altering state.

Reproducer (requires any compress offload device, CONFIG_KCSAN=y,
CONFIG_KCSAN_STRICT=y):

  /* 4 threads on the same compress fd, ~2000 iterations to trigger */
  static int compr_fd;

  void *poll_thread(void *arg) {
      struct pollfd pfd = { .fd = compr_fd, .events = POLLOUT };
      while (!stop) poll(&pfd, 1, 10);
      return NULL;
  }
  void *drain_thread(void *arg) {
      while (!stop) ioctl(compr_fd, SNDRV_COMPRESS_DRAIN);
      return NULL;
  }
  void *stop_thread(void *arg) {
      while (!stop) { usleep(500); ioctl(compr_fd, SNDRV_COMPRESS_STOP); }
      return NULL;
  }
  /* main: open compress dev, SET_PARAMS, write data, START,
   * then spawn all 3 threads + repeat setup in a loop.
   * KCSAN fires within seconds. */

  Full reproducer source available on request.

Fixes: b21c60a4edd2 ("ALSA: core: add support for compress_offload")
Cc: stable@vger.kernel.org
Reported-by: Omer Cohen <nevergfx1@gmail.com>
Signed-off-by: Omer Cohen <nevergfx1@gmail.com>
---
 sound/core/compress_offload.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index fd63d219bf86..XXXXXXXXXXXX 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -478,11 +478,8 @@ static __poll_t snd_compr_poll(struct file *f, poll_table *wait)
 	/* check if we have at least one fragment to fill */
 	switch (runtime->state) {
 	case SNDRV_PCM_STATE_DRAINING:
-		/* stream has been woken up after drain is complete
-		 * draining done so set stream state to stopped
-		 */
+		/* drain completed or completing, report ready */
 		retval = snd_compr_get_poll(stream);
-		runtime->state = SNDRV_PCM_STATE_SETUP;
 		break;
 	case SNDRV_PCM_STATE_RUNNING:
 	case SNDRV_PCM_STATE_PREPARED:
--
2.43.0

[PATCH 2/6] ALSA: compress: fix buffer leak on set_params driver failure

[Omer Cohen]

snd_compr_set_params() calls snd_compr_allocate_buffer() which stores
the new buffer pointer in runtime->buffer, then calls
ops->set_params().  If the driver callback fails, the function returns
the error but runtime->buffer already points to the new allocation.
On a subsequent set_params retry, snd_compr_allocate_buffer() overwrites
runtime->buffer with a fresh allocation without freeing the previous
one, leaking kernel memory.

An unprivileged user with access to a compress offload device can
trigger this repeatedly to exhaust kernel memory.

Fix by saving the old buffer state before allocation and restoring it
if the driver callback fails.

Fixes: b21c60a4edd2 ("ALSA: core: add support for compress_offload")
Cc: stable@vger.kernel.org
Reported-by: Omer Cohen <nevergfx1@gmail.com>
Signed-off-by: Omer Cohen <nevergfx1@gmail.com>
---
 sound/core/compress_offload.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index fd63d219bf86..XXXXXXXXXXXX 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -665,14 +665,28 @@ snd_compr_set_params(struct snd_compr_stream *stream, unsigned long arg)
 		if (retval)
 			return retval;

+		void *old_buffer = stream->runtime->buffer;
+		size_t old_size = stream->runtime->buffer_size;
+		unsigned int old_frag_size = stream->runtime->fragment_size;
+		unsigned int old_fragments = stream->runtime->fragments;
+
 		retval = snd_compr_allocate_buffer(stream, params);
 		if (retval)
 			return -ENOMEM;

 		retval = stream->ops->set_params(stream, params);
-		if (retval)
+		if (retval) {
+			/* Restore old buffer to avoid leak on retry.
+			 * Free the newly allocated one if it differs.
+			 */
+			if (stream->runtime->buffer != old_buffer &&
+			    !stream->runtime->dma_buffer_p)
+				kfree(stream->runtime->buffer);
+			stream->runtime->buffer = old_buffer;
+			stream->runtime->buffer_size = old_size;
+			stream->runtime->fragment_size = old_frag_size;
+			stream->runtime->fragments = old_fragments;
 			return retval;
+		}

 		if (stream->next_track)
 			return retval;
--
2.43.0

[PATCH 3/6] ALSA: timer: fix lockless tu->tread read in snd_timer_user_read()

[Omer Cohen]

snd_timer_user_read() reads tu->tread without holding ioctl_lock to
determine the entry size (unit), then reads it again under ioctl_lock
to select the copy format.  A concurrent SNDRV_TIMER_IOCTL_TREAD64
can change tu->tread between the two reads, causing a mismatch between
the entry size used for the read loop and the format used for the
actual copy.

Move the initial tu->tread read inside the ioctl_lock and cache the
value in a local variable for consistent use throughout the function.

Fixes: d11662f4f798 ("ALSA: timer: Fix race between read and ioctl")
Cc: stable@vger.kernel.org
Reported-by: Omer Cohen <nevergfx1@gmail.com>
Signed-off-by: Omer Cohen <nevergfx1@gmail.com>
---
 sound/core/timer.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/sound/core/timer.c b/sound/core/timer.c
index 51c6ac4df9f4..XXXXXXXXXXXX 100644
--- a/sound/core/timer.c
+++ b/sound/core/timer.c
@@ -2390,12 +2390,15 @@ static ssize_t snd_timer_user_read(struct file *file, char __user *buffer,
 	struct snd_timer_tread32 tread32;
 	struct snd_timer_user *tu;
 	long result = 0, unit;
+	int tread_format;
 	int qhead;
 	int err = 0;

 	tu = file->private_data;
-	switch (tu->tread) {
-	case TREAD_FORMAT_TIME64:
+	mutex_lock(&tu->ioctl_lock);
+	tread_format = tu->tread;
+	switch (tread_format) {
+	case TREAD_FORMAT_TIME64:
 		unit = sizeof(struct snd_timer_tread64);
 		break;
 	case TREAD_FORMAT_TIME32:
@@ -2407,8 +2410,8 @@ static ssize_t snd_timer_user_read(struct file *file, char __user *buffer,
 	default:
 		WARN_ONCE(1, "Corrupt snd_timer_user\n");
+		mutex_unlock(&tu->ioctl_lock);
 		return -ENOTSUPP;
 	}

-	mutex_lock(&tu->ioctl_lock);
 	spin_lock_irq(&tu->qlock);
 	while ((long)count - result >= unit) {
@@ -2453,7 +2456,7 @@ static ssize_t snd_timer_user_read(struct file *file, char __user *buffer,

 		tread = &tu->tqueue[qhead];

-		switch (tu->tread) {
+		switch (tread_format) {
 		case TREAD_FORMAT_TIME64:
 			if (copy_to_user(buffer, tread,
 					 sizeof(struct snd_timer_tread64)))
--
2.43.0

[PATCH 4/6] ALSA: timer: copy queue entries to local buffer before copy_to_user

[Omer Cohen]

snd_timer_user_read() dequeues an entry from the timer event queue
under qlock, then drops the spinlock and calls copy_to_user() directly
from the queue slot (tread = &tu->tqueue[qhead]).

While the lock is dropped, timer interrupt callbacks
(snd_timer_user_tinterrupt) can write new entries to the queue.  If
enough events arrive to wrap qtail back to the dequeued slot, the
callback overwrites data that copy_to_user() is still reading.

This is the same class of bug fixed for the non-tread queue path in
CVE-2017-1000380, but the tread queue (tu->tqueue) and the NONE
format queue (tu->queue) were left with the same pattern.

Copy dequeued entries to local stack variables under the spinlock
before dropping it, then use the local copies for copy_to_user().

Fixes: d11662f4f798 ("ALSA: timer: Fix race between read and ioctl")
Cc: stable@vger.kernel.org
Reported-by: Omer Cohen <nevergfx1@gmail.com>
Signed-off-by: Omer Cohen <nevergfx1@gmail.com>
---
 sound/core/timer.c | 19 +++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/sound/core/timer.c b/sound/core/timer.c
index 51c6ac4df9f4..XXXXXXXXXXXX 100644
--- a/sound/core/timer.c
+++ b/sound/core/timer.c
@@ -2390,6 +2390,7 @@ static ssize_t snd_timer_user_read(struct file *file, char __user *buffer,
-	struct snd_timer_tread64 *tread;
+	struct snd_timer_tread64 tread_local;
 	struct snd_timer_tread32 tread32;
+	struct snd_timer_read read_local;
 	struct snd_timer_user *tu;
 	long result = 0, unit;
 	int tread_format;
 	int qhead;
@@ -2452,23 +2454,23 @@ static ssize_t snd_timer_user_read(struct file *file, char __user *buffer,
 		qhead = tu->qhead++;
 		tu->qhead %= tu->queue_size;
 		tu->qused--;
+		tread_local = tu->tqueue[qhead];
+		read_local = tu->queue[qhead];
 		spin_unlock_irq(&tu->qlock);

-		tread = &tu->tqueue[qhead];
-
 		switch (tread_format) {
 		case TREAD_FORMAT_TIME64:
-			if (copy_to_user(buffer, tread,
+			if (copy_to_user(buffer, &tread_local,
 					 sizeof(struct snd_timer_tread64)))
 				err = -EFAULT;
 			break;
 		case TREAD_FORMAT_TIME32:
 			memset(&tread32, 0, sizeof(tread32));
 			tread32 = (struct snd_timer_tread32) {
-				.event = tread->event,
-				.tstamp_sec = tread->tstamp_sec,
-				.tstamp_nsec = tread->tstamp_nsec,
-				.val = tread->val,
+				.event = tread_local.event,
+				.tstamp_sec = tread_local.tstamp_sec,
+				.tstamp_nsec = tread_local.tstamp_nsec,
+				.val = tread_local.val,
 			};

 			if (copy_to_user(buffer, &tread32, sizeof(tread32)))
@@ -2470,6 +2474,6 @@ static ssize_t snd_timer_user_read(struct file *file, char __user *buffer,
 			break;
 		case TREAD_FORMAT_NONE:
-			if (copy_to_user(buffer, &tu->queue[qhead],
+			if (copy_to_user(buffer, &read_local,
 					 sizeof(struct snd_timer_read)))
 				err = -EFAULT;
 			break;
--
2.43.0

[PATCH 5/6] ALSA: pcm: use locked state read in snd_pcm_drop()

[Omer Cohen]

snd_pcm_drop() reads runtime->state without the stream lock at lines
2274-2275 to check for OPEN and DISCONNECTED states.  The stream lock
is acquired shortly after at line 2278.

Commit 7bc02ab446d3 ("ALSA: pcm: Fix unlocked state reads in
read/write file ops") fixed this exact pattern in the read and write
paths but missed snd_pcm_drop().

Use snd_pcm_get_state() which acquires the stream lock for the read.

Fixes: f0061c18c169 ("ALSA: pcm: Avoid reference to status->state")
Cc: stable@vger.kernel.org
Reported-by: Omer Cohen <nevergfx1@gmail.com>
Signed-off-by: Omer Cohen <nevergfx1@gmail.com>
---
 sound/core/pcm_native.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index XXXXXXXXXXXX..XXXXXXXXXXXX 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -2265,13 +2265,14 @@ static int snd_pcm_drain(struct snd_pcm_substream *substream)
 static int snd_pcm_drop(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime;
+	snd_pcm_state_t state;
 	int result = 0;

 	if (PCM_RUNTIME_CHECK(substream))
 		return -ENXIO;
 	runtime = substream->runtime;

-	if (runtime->state == SNDRV_PCM_STATE_OPEN ||
-	    runtime->state == SNDRV_PCM_STATE_DISCONNECTED)
+	state = snd_pcm_get_state(substream);
+	if (state == SNDRV_PCM_STATE_OPEN ||
+	    state == SNDRV_PCM_STATE_DISCONNECTED)
 		return -EBADFD;

 	guard(pcm_stream_lock_irq)(substream);
--
2.43.0

[PATCH 6/6] ALSA: rawmidi: propagate resize_runtime_buffer() error in input_params

[Omer Cohen]

snd_rawmidi_input_params() calls resize_runtime_buffer() but ignores
its return value, unconditionally returning 0.  This silently swallows
-ENOMEM (allocation failure), -EINVAL (invalid parameters), and -EBUSY
(buffer in use).

When resize fails, the framing and clock_type parameters are still
applied to the substream, but the buffer remains at its old size.
This leaves the stream in an inconsistent state where the configured
framing mode does not match the buffer layout.

Return the error from resize_runtime_buffer().

Fixes: 08fdced60ca0 ("ALSA: rawmidi: Add framing mode")
Cc: stable@vger.kernel.org
Reported-by: Omer Cohen <nevergfx1@gmail.com>
Signed-off-by: Omer Cohen <nevergfx1@gmail.com>
---
 sound/core/rawmidi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
index XXXXXXXXXXXX..XXXXXXXXXXXX 100644
--- a/sound/core/rawmidi.c
+++ b/sound/core/rawmidi.c
@@ -784,7 +784,7 @@ int snd_rawmidi_input_params(struct snd_rawmidi_substream *substream,
 		substream->framing = framing;
 		substream->clock_type = clock_type;
 	}
-	return 0;
+	return err;
 }
 EXPORT_SYMBOL(snd_rawmidi_input_params);
--
2.43.0