[PATCH] ALSA: pcm: Don't release the DMA buffer while it is mmapped

[PATCH] ALSA: pcm: Don't release the DMA buffer while it is mmapped

[bo cao]

From: Bo Cao <treebacker@foxmail.com>

The hw_params state check refuses a re-configuration while the data
buffer is mmapped, but it carries an exemption for OSS emulation
streams.  The OSS flag is kept on the shared substream, and a native
ALSA fd attaching to an already-open substream through an O_APPEND
open inherits that flag, so the exemption can be reached without ever
touching the OSS emulation layer itself.

An unprivileged user can exploit this combination by opening /dev/dsp
to mark the substream as OSS, attaching to the same substream with a
native O_APPEND open, running hw_params with a small buffer size and
mmapping it, and then re-running hw_params: either with a larger size
(making snd_pcm_lib_malloc_pages() release the old DMA buffer pages)
or with parameters that fail refinement (making the hw_params error
path release them).  In both cases the pages are freed while the
userspace PTEs remain valid and writable.

The stale mapping of the freed page never expires and can be turned
into arbitrary physical memory read/write (e.g. by reclaiming the page
as a page table).  On arm64 this was demonstrated to escalate an
audio-group user to full root via a modprobe_path overwrite.

Refuse the buffer release while the buffer is still mmapped:

  - snd_pcm_lib_malloc_pages() returns -EBADFD instead of freeing
    the old buffer when the new size does not fit it and the buffer
    is still mmapped (same-size re-configuration, which is what the
    OSS emulation layer itself does while mmapped, keeps working),

  - snd_pcm_lib_free_pages() refuses to release a buffer that is
    still mmapped, which also covers the hw_params error path and
    drivers freeing through it,

  - a buffer retained this way by a failed hw_params is released at
    the final close of the substream instead of leaking.

Fixes: 0df63e44c3e3 ("[ALSA] Add O_APPEND flag support to PCM")
Cc: stable@vger.kernel.org
Reported-by: Bo Cao <treebacker@foxmail.com>
Tested-by: Bo Cao <treebacker@foxmail.com>
Signed-off-by: Bo Cao <treebacker@foxmail.com>

Hi Johan,

thanks for the quick response.  Below is a proposed fix as discussed.
It enforces a single invariant at the release sites -- the DMA buffer
is never released while it is still mmapped -- which covers both the
buffer-growth path my PoC uses and the hw_params error path that
frees the buffer on failed refinement while the OSS exemption lets
the re-configuration through.  The close path releases a buffer that
was retained by a failed hw_params, so the fix does not trade the
UAF for a leak.

One caveat: I am not a professional kernel developer -- the analysis
and PoCs come from an independent security research effort -- so
please treat the patch as a starting point rather than a finished
fix.  In particular I would appreciate review of the
snd_pcm_release_substream() hunk (needed so that a buffer retained
by a failed hw_params does not leak) and of any OSS emulation
behavior I may have missed.  I am happy to iterate on a v2 with
whatever approach the maintainers prefer.

I verified on the affected setup (7.4-rc1, QEMU arm64, snd-intel8x0)
that the PoC now fails cleanly: the second hw_params returns -EBADFD
and the mmapped pages are not freed.

Best regards,
Bo Cao

---
 sound/core/pcm_memory.c | 8 ++++++++
 sound/core/pcm_native.c | 6 ++++++
 2 files changed, 14 insertions(+)

diff --git a/sound/core/pcm_memory.c b/sound/core/pcm_memory.c
index cfe9f10e7359..ed3da74e2cf5 100644
--- a/sound/core/pcm_memory.c
+++ b/sound/core/pcm_memory.c
@@ -439,6 +439,11 @@ int snd_pcm_lib_malloc_pages(struct snd_pcm_substream *substream, size_t size)
 			runtime->dma_bytes = size;
 			return 0;	/* ok, do not change */
 		}
+		/* the old buffer may still be mmapped: refuse instead of
+		 * freeing it behind the userspace mapping
+		 */
+		if (atomic_read(&substream->mmap_count))
+			return -EBADFD;
 		snd_pcm_lib_free_pages(substream);
 	}
 	if (substream->dma_buffer.area != NULL &&
@@ -488,6 +493,9 @@ int snd_pcm_lib_free_pages(struct snd_pcm_substream *substream)
 	runtime = substream->runtime;
 	if (runtime->dma_area == NULL)
 		return 0;
+	/* the buffer may still be mmapped: refuse to release it */
+	if (atomic_read(&substream->mmap_count))
+		return -EBADFD;
 	if (runtime->dma_buffer_p != &substream->dma_buffer) {
 		struct snd_card *card = substream->pcm->card;
 
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 5cbf88da7626..8e337983f2ec 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -2851,6 +2851,12 @@ void snd_pcm_release_substream(struct snd_pcm_substream *substream)
 		snd_refcount_sync(&substream->runtime->refcnt);
 		if (substream->runtime->state != SNDRV_PCM_STATE_OPEN)
 			do_hw_free(substream);
+		else if (substream->managed_buffer_alloc)
+			/* a failed hw_params may have left the buffer
+			 * allocated (it was still mmapped at the time);
+			 * release it now that the mapping is gone
+			 */
+			snd_pcm_lib_free_pages(substream);
 		substream->ops->close(substream);
 		substream->hw_opened = 0;
 	}
-- 
2.39.5

[PATCH] ALSA: pcm: Don't release the DMA buffer while it is mmapped

[bo cao]

From: Bo Cao <treebacker@foxmail.com>

The hw_params state check refuses a re-configuration while the data
buffer is mmapped, but it carries an exemption for OSS emulation
streams.  The OSS flag is kept on the shared substream, and a native
ALSA fd attaching to an already-open substream through an O_APPEND
open inherits that flag, so the exemption can be reached without ever
touching the OSS emulation layer itself.

When such a stream is re-configured with a larger buffer size,
snd_pcm_lib_malloc_pages() releases the old DMA buffer pages, and a
failed re-configuration releases them through the hw_params error
path.  Neither path checks whether the buffer is still mmapped, so
the pages can be freed while the userspace mapping remains in place,
resulting in a use-after-free of the mapped pages, which is
exploitable for privilege escalation.

Refuse the buffer release while the buffer is still mmapped:

  - snd_pcm_lib_malloc_pages() returns -EBADFD instead of freeing
    the old buffer when the new size does not fit it and the buffer
    is still mmapped (same-size re-configuration, which is what the
    OSS emulation layer itself does while mmapped, keeps working),

  - snd_pcm_lib_free_pages() refuses to release a buffer that is
    still mmapped, which also covers the hw_params error path and
    drivers freeing through it,

  - a buffer retained this way by a failed hw_params is released at
    the final close of the substream instead of leaking.

Fixes: 0df63e44c3e3 ("[ALSA] Add O_APPEND flag support to PCM")
Cc: stable@vger.kernel.org
Reported-by: Bo Cao <treebacker@foxmail.com>
Tested-by: Bo Cao <treebacker@foxmail.com>
Signed-off-by: Bo Cao <treebacker@foxmail.com>

---
Note for the list: this issue was originally reported privately to
security@kernel.org; an accidental CC let this patch reach the stable
list early, so it is now submitted through the normal process at the
security team's request.

A word of caution on my part: I am not a professional kernel
developer -- the analysis comes from an independent security research
effort -- so please treat this as a starting point rather than a
finished fix.  In particular, review of the snd_pcm_release_substream()
hunk (which ensures a buffer retained by a failed hw_params does not
leak) and of any OSS emulation behavior I may have missed would be
appreciated.  I am happy to iterate on a v2 with whatever approach is
preferred.

Tested on linux 7.4-rc1 (arm64 QEMU, snd-intel8x0): a reproducer
fails cleanly after the fix -- the second hw_params returns -EBADFD
and the mmapped pages are not freed.

 sound/core/pcm_memory.c | 8 ++++++++
 sound/core/pcm_native.c | 6 ++++++
 2 files changed, 14 insertions(+)

diff --git a/sound/core/pcm_memory.c b/sound/core/pcm_memory.c
index cfe9f10e7359..ed3da74e2cf5 100644
--- a/sound/core/pcm_memory.c
+++ b/sound/core/pcm_memory.c
@@ -439,6 +439,11 @@ int snd_pcm_lib_malloc_pages(struct snd_pcm_substream *substream, size_t size)
 			runtime->dma_bytes = size;
 			return 0;	/* ok, do not change */
 		}
+		/* the old buffer may still be mmapped: refuse instead of
+		 * freeing it behind the userspace mapping
+		 */
+		if (atomic_read(&substream->mmap_count))
+			return -EBADFD;
 		snd_pcm_lib_free_pages(substream);
 	}
 	if (substream->dma_buffer.area != NULL &&
@@ -488,6 +493,9 @@ int snd_pcm_lib_free_pages(struct snd_pcm_substream *substream)
 	runtime = substream->runtime;
 	if (runtime->dma_area == NULL)
 		return 0;
+	/* the buffer may still be mmapped: refuse to release it */
+	if (atomic_read(&substream->mmap_count))
+		return -EBADFD;
 	if (runtime->dma_buffer_p != &substream->dma_buffer) {
 		struct snd_card *card = substream->pcm->card;
 
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 5cbf88da7626..8e337983f2ec 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -2851,6 +2851,12 @@ void snd_pcm_release_substream(struct snd_pcm_substream *substream)
 		snd_refcount_sync(&substream->runtime->refcnt);
 		if (substream->runtime->state != SNDRV_PCM_STATE_OPEN)
 			do_hw_free(substream);
+		else if (substream->managed_buffer_alloc)
+			/* a failed hw_params may have left the buffer
+			 * allocated (it was still mmapped at the time);
+			 * release it now that the mapping is gone
+			 */
+			snd_pcm_lib_free_pages(substream);
 		substream->ops->close(substream);
 		substream->hw_opened = 0;
 	}
-- 
2.39.5

Re: [PATCH] ALSA: pcm: Don't release the DMA buffer while it is mmapped

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ALSA: pcm: leak in snd_pcm_release_substream() for non-managed buffers
- [High] ALSA: pcm: state desync in snd_pcm_hw_params() when hw_free() runs mmapped
--

commit d25a1b1c86ceb79fd02fea330e439e2906af4528
Author: Bo Cao <treebacker@foxmail.com>

ALSA: pcm: Don't release the DMA buffer while it is mmapped

This commit prevents snd_pcm_lib_malloc_pages() and snd_pcm_lib_free_pages()
from freeing an active PCM DMA buffer while substream->mmap_count is non-zero,
and releases any retained managed buffer when the substream is closed in
snd_pcm_release_substream().

> diff --git a/sound/core/pcm_memory.c b/sound/core/pcm_memory.c
> index cfe9f10e7359d..ed3da74e2cf55 100644
> --- a/sound/core/pcm_memory.c
> +++ b/sound/core/pcm_memory.c
> @@ -439,6 +439,11 @@ int snd_pcm_lib_malloc_pages(struct snd_pcm_substream *substream, size_t size)
>  			runtime->dma_bytes = size;
>  			return 0;	/* ok, do not change */
>  		}
> +		/* the old buffer may still be mmapped: refuse instead of
> +		 * freeing it behind the userspace mapping
> +		 */
> +		if (atomic_read(&substream->mmap_count))
> +			return -EBADFD;
>  		snd_pcm_lib_free_pages(substream);
>  	}
>  	if (substream->dma_buffer.area != NULL &&
> @@ -488,6 +493,9 @@ int snd_pcm_lib_free_pages(struct snd_pcm_substream *substream)
>  	runtime = substream->runtime;
>  	if (runtime->dma_area == NULL)
>  		return 0;
> +	/* the buffer may still be mmapped: refuse to release it */
> +	if (atomic_read(&substream->mmap_count))
> +		return -EBADFD;

[Severity: High]
Can returning -EBADFD here while substream->ops->hw_free() still runs on the
snd_pcm_hw_params() error path desynchronize driver state from
runtime->dma_buffer_p?

When snd_pcm_hw_params() fails while substream->mmap_count is non-zero (for
instance, when a larger buffer size causes snd_pcm_lib_malloc_pages() to
return -EBADFD, or when snd_pcm_hw_refine() fails), the _error path still
calls substream->ops->hw_free() before snd_pcm_lib_free_pages():

sound/core/pcm_native.c:snd_pcm_hw_params() {
	...
 _error:
	if (substream->runtime) {
		/* hardware might be unusable from this time,
		   so we force application to retry to set
		   the correct hardware parameter settings */
		snd_pcm_set_state(substream, SNDRV_PCM_STATE_OPEN);
		if (substream->ops->hw_free != NULL)
			substream->ops->hw_free(substream);
		if (substream->managed_buffer_alloc)
			snd_pcm_lib_free_pages(substream);
	}
}

With this change, substream->ops->hw_free() tears down driver-side hardware
mappings or memory blocks (such as voice->memblk in snd_trident_hw_free(),
pcmp->memory in snd_gf1_pcm_playback_hw_free(), or epcm->memblk in
snd_emu10k1_playback_hw_free()), while snd_pcm_lib_free_pages() returns
-EBADFD and leaves runtime->dma_buffer_p intact.

If a subsequent snd_pcm_hw_params() call requests a buffer size that fits in
the retained runtime->dma_buffer_p->bytes, snd_pcm_lib_malloc_pages() takes
the fast path and returns 0, setting runtime->buffer_changed = 0:

sound/core/pcm_native.c:snd_pcm_hw_params() {
	...
	if (substream->managed_buffer_alloc) {
		err = snd_pcm_lib_malloc_pages(substream,
					       params_buffer_bytes(params));
		if (err < 0)
			goto _error;
		runtime->buffer_changed = err > 0;
	}
}

Drivers that only allocate their hardware memory blocks when
runtime->buffer_changed is non-zero or when snd_pcm_lib_malloc_pages()
returns > 0 then skip reallocating the block that ops->hw_free() freed,
leaving voice->memblk, pcmp->memory, or epcm->memblk unallocated in
SNDRV_PCM_STATE_SETUP:

sound/pci/trident/trident_main.c:snd_trident_allocate_pcm_mem() {
	...
	if (trident->tlb.entries) {
		if (runtime->buffer_changed) {
			if (voice->memblk)
				snd_trident_free_pages(trident, voice->memblk);
			voice->memblk = snd_trident_alloc_pages(trident, substream);
			if (voice->memblk == NULL)
				return -ENOMEM;
		}
	}
}

In addition, snd_cs46xx_playback_hw_free() and snd_cs46xx_capture_hw_free()
ignore the -EBADFD return from snd_pcm_lib_free_pages() and unconditionally
clear runtime->dma_buffer_p and runtime->dma_area to NULL:

sound/pci/cs46xx/cs46xx_lib.c:snd_cs46xx_playback_hw_free() {
	...
	if (runtime->dma_area != cpcm->hw_buf.area)
		snd_pcm_lib_free_pages(substream);

	snd_pcm_set_runtime_buffer(substream, NULL);
	...
}

Wouldn't clearing runtime->dma_buffer_p while the buffer is still mmapped
cause a subsequent snd_pcm_lib_malloc_pages() call to see
runtime->dma_buffer_p == NULL and bypass the new mmap_count check?

> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 5cbf88da7626d..8e337983f2ec6 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -2851,6 +2851,12 @@ void snd_pcm_release_substream(struct snd_pcm_substream *substream)
>  		snd_refcount_sync(&substream->runtime->refcnt);
>  		if (substream->runtime->state != SNDRV_PCM_STATE_OPEN)
>  			do_hw_free(substream);
> +		else if (substream->managed_buffer_alloc)
> +			/* a failed hw_params may have left the buffer
> +			 * allocated (it was still mmapped at the time);
> +			 * release it now that the mapping is gone
> +			 */
> +			snd_pcm_lib_free_pages(substream);

[Severity: High]
Does gating this cleanup on substream->managed_buffer_alloc leak
runtime->dma_buffer_p and card->total_pcm_alloc_bytes for drivers that use
snd_pcm_lib_malloc_pages() and snd_pcm_lib_free_pages() without setting
substream->managed_buffer_alloc?

Drivers that preallocate via snd_pcm_lib_preallocate_pages() or
snd_pcm_lib_preallocate_pages_for_all() (such as sound/pci/emu10k1/emupcm.c,
sound/pci/rme9652/hdspm.c, and sound/pci/cs46xx/cs46xx_lib.c) have
substream->managed_buffer_alloc == 0 and call snd_pcm_lib_malloc_pages() from
substream->ops->hw_params() and snd_pcm_lib_free_pages() from
substream->ops->hw_free():

sound/pci/emu10k1/emupcm.c:snd_emu10k1_playback_hw_free() {
	...
	snd_pcm_lib_free_pages(substream);
	return 0;
}

When a second snd_pcm_hw_params() call fails while the buffer is mmapped, the
_error path sets substream->runtime->state to SNDRV_PCM_STATE_OPEN and calls
substream->ops->hw_free(), where snd_pcm_lib_free_pages() now returns -EBADFD
and retains runtime->dma_buffer_p.

When the mapping is later unmapped and the substream is closed,
snd_pcm_release_substream() sees substream->runtime->state ==
SNDRV_PCM_STATE_OPEN, skips do_hw_free(), and skips snd_pcm_lib_free_pages()
because substream->managed_buffer_alloc is 0, leaking runtime->dma_buffer_p
and its card->total_pcm_alloc_bytes quota when snd_pcm_detach_substream()
frees substream->runtime.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_E90BB4F086165FAA3313723567A5C675EA06@qq.com?part=1