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

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