[PATCH] ALSA: pcm: Fix race between concurrent START and hw_params/hw_free

[Aohan Mei <ljp1205831794@gmail.com>]

From: Aohan Mei <henrymei@tencent.com>

snd_pcm_hw_params() and snd_pcm_hw_free() validate the stream state
only once, under the stream lock at entry, and then free/realloc the
DMA buffer (snd_pcm_lib_free_pages() -> snd_pcm_set_runtime_buffer(NULL),
followed by the sleeping allocation in snd_pcm_lib_malloc_pages()) and
update the runtime fields without holding the stream lock.  The state,
however, stays SNDRV_PCM_STATE_PREPARED until the final
snd_pcm_set_state() at the very end of the operation.

A concurrent SNDRV_PCM_IOCTL_START takes only the stream lock and
merely requires state == PREPARED in snd_pcm_pre_start(), so it can
slip into that window: snd_pcm_post_start() sets the state RUNNING,
arms the driver data plane and fills the initial silence through
snd_pcm_playback_silence().  The data plane then keeps operating on
the buffer that hw_params/hw_free is tearing down concurrently; once
snd_pcm_set_runtime_buffer(NULL) has cleared runtime->dma_area, the
silence fill in fill_silence() -> get_dma_ptr() dereferences a NULL
pointer, and on real hardware the device may additionally keep
DMA-ing into the freed pages.

Close the race by leaving the PREPARED state atomically with the
entry state check, inside the same stream lock critical section: a
START that already completed makes the hw_params/hw_free state check
fail with -EBADFD, and a later START observes SETUP and fails in
snd_pcm_pre_start() with -EBADFD as well, so the data plane can never
be armed while the buffer is being freed or reallocated.  Both
functions impose SETUP as their resulting state anyway, hence this
does not change the state machine semantics visible to user space.

The buffer_mutex/buffer_accessing serialization introduced by the
earlier fixes for the prepare-vs-hw_params races cannot simply be
extended to the START path: the trigger action has to run under the
IRQ-off stream spinlock for atomic PCMs, and failing START with
-EBUSY whenever a read/write transfer is in flight would be a user
visible regression.  Transitioning the state atomically at entry
avoids both problems.

Cc: stable@vger.kernel.org
Assisted-by: CodeBuddy:Kimi-K3
Signed-off-by: Aohan Mei <henrymei@tencent.com>
---
 sound/core/pcm_native.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 62324282fcae..6040eb752873 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -805,6 +805,17 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream,
 			err = -EBADFD;
 			break;
 		}
+		/*
+		 * The buffer free/realloc and the runtime field updates
+		 * below run without the stream lock, while a concurrent
+		 * SNDRV_PCM_IOCTL_START only requires state == PREPARED
+		 * under that lock.  Leave the PREPARED state atomically
+		 * with the check above so that a racing START (and the
+		 * data plane it arms) can no longer slip in and operate
+		 * on the buffer while it is being freed or reallocated.
+		 */
+		if (!err && runtime->state == SNDRV_PCM_STATE_PREPARED)
+			__snd_pcm_set_state(runtime, SNDRV_PCM_STATE_SETUP);
 	}
 	if (err)
 		goto unlock;
@@ -966,6 +977,13 @@ static int snd_pcm_hw_free(struct snd_pcm_substream *substream)
 			result = -EBADFD;
 			break;
 		}
+		/* Same race as in snd_pcm_hw_params(): leave the PREPARED
+		 * state atomically with the check above, so that a racing
+		 * START cannot arm the data plane while do_hw_free() tears
+		 * down the buffer without the stream lock.
+		 */
+		if (!result && runtime->state == SNDRV_PCM_STATE_PREPARED)
+			__snd_pcm_set_state(runtime, SNDRV_PCM_STATE_SETUP);
 	}
 	if (result)
 		goto unlock;
-- 
2.43.7