[PATCH 0/2] ALSA: Clear PCM buffers more properly

[PATCH 0/2] ALSA: Clear PCM buffers more properly

[Takashi Iwai]

Hi,

here are two small fix patches for PCM buffer allocations.
Basically those address two things that have been discussed in the
recent thread [1]:
* Clear the remaining bytes in PCM buffer pages that are exposed via
  mmap
* Assure the page aligned allocations for genalloc

This doesn't cover the cases where non-standard PCM buffer allocation
is used.  Those should handle the buffer clearance by themselves.


Takashi

[1] https://lore.kernel.org/r/05c824e5-0c33-4182-26fa-b116a42b10d6@metafoo.de

===

Takashi Iwai (2):
  ALSA: memalloc: Align buffer allocations in page size
  ALSA: pcm: Clear the full allocated memory at hw_params

 sound/core/memalloc.c   | 1 +
 sound/core/pcm_native.c | 9 +++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

-- 
2.26.2

[PATCH 1/2] ALSA: memalloc: Align buffer allocations in page size

[Takashi Iwai]

Currently the standard memory allocator (snd_dma_malloc_pages*())
passes the byte size to allocate as is.  Most of the backends
allocates real pages, hence the actual allocations are aligned in page
size.  However, the genalloc doesn't seem assuring the size alignment,
hence it may result in the access outside the buffer when the whole
memory pages are exposed via mmap.

For avoiding such inconsistencies, this patch makes the allocation
size always to be aligned in page size.

Note that, after this change, snd_dma_buffer.bytes field contains the
aligned size, not the originally requested size.  This value is also
used for releasing the pages in return.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/memalloc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/core/memalloc.c b/sound/core/memalloc.c
index 0f335162f87c..966bef5acc75 100644
--- a/sound/core/memalloc.c
+++ b/sound/core/memalloc.c
@@ -133,6 +133,7 @@ int snd_dma_alloc_pages(int type, struct device *device, size_t size,
 	if (WARN_ON(!dmab))
 		return -ENXIO;
 
+	size = PAGE_ALIGN(size);
 	dmab->dev.type = type;
 	dmab->dev.dev = device;
 	dmab->bytes = 0;
-- 
2.26.2

Re: [PATCH 1/2] ALSA: memalloc: Align buffer allocations in page size

[Lars-Peter Clausen]

On 12/18/20 3:56 PM, Takashi Iwai wrote:
> Currently the standard memory allocator (snd_dma_malloc_pages*())
> passes the byte size to allocate as is.  Most of the backends
> allocates real pages, hence the actual allocations are aligned in page
> size.  However, the genalloc doesn't seem assuring the size alignment,
> hence it may result in the access outside the buffer when the whole
> memory pages are exposed via mmap.
>
> For avoiding such inconsistencies, this patch makes the allocation
> size always to be aligned in page size.
>
> Note that, after this change, snd_dma_buffer.bytes field contains the
> aligned size, not the originally requested size.  This value is also
> used for releasing the pages in return.
>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

FWIW

Reviewed-by: Lars-Peter Clausen <lars@metafoo.de>

> ---
>   sound/core/memalloc.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/sound/core/memalloc.c b/sound/core/memalloc.c
> index 0f335162f87c..966bef5acc75 100644
> --- a/sound/core/memalloc.c
> +++ b/sound/core/memalloc.c
> @@ -133,6 +133,7 @@ int snd_dma_alloc_pages(int type, struct device *device, size_t size,
>   	if (WARN_ON(!dmab))
>   		return -ENXIO;
>   
> +	size = PAGE_ALIGN(size);
>   	dmab->dev.type = type;
>   	dmab->dev.dev = device;
>   	dmab->bytes = 0;

[PATCH 2/2] ALSA: pcm: Clear the full allocated memory at hw_params

[Takashi Iwai]

The PCM hw_params core function tries to clear up the PCM buffer
before actually using for avoiding the information leak from the
previous usages or the usage before a new allocation.  It performs the
memset() with runtime->dma_bytes, but this might still leave some
remaining bytes untouched; namely, the PCM buffer size is aligned in
page size for mmap, hence runtime->dma_bytes doesn't necessarily cover
all PCM buffer pages, and the remaining bytes are exposed via mmap.

This patch changes the memory clearance to cover the all buffer pages
if the stream is supposed to be mmap-ready (that guarantees that the
buffer size is aligned in page size).

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/pcm_native.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 47b155a49226..9f3f8e953ff0 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -755,8 +755,13 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream,
 		runtime->boundary *= 2;
 
 	/* clear the buffer for avoiding possible kernel info leaks */
-	if (runtime->dma_area && !substream->ops->copy_user)
-		memset(runtime->dma_area, 0, runtime->dma_bytes);
+	if (runtime->dma_area && !substream->ops->copy_user) {
+		size_t size = runtime->dma_bytes;
+
+		if (runtime->info & SNDRV_PCM_INFO_MMAP)
+			size = PAGE_ALIGN(size);
+		memset(runtime->dma_area, 0, size);
+	}
 
 	snd_pcm_timer_resolution_change(substream);
 	snd_pcm_set_state(substream, SNDRV_PCM_STATE_SETUP);
-- 
2.26.2

Re: [PATCH 2/2] ALSA: pcm: Clear the full allocated memory at hw_params

[Lars-Peter Clausen]

On 12/18/20 3:56 PM, Takashi Iwai wrote:
> The PCM hw_params core function tries to clear up the PCM buffer
> before actually using for avoiding the information leak from the
> previous usages or the usage before a new allocation.  It performs the
> memset() with runtime->dma_bytes, but this might still leave some
> remaining bytes untouched; namely, the PCM buffer size is aligned in
> page size for mmap, hence runtime->dma_bytes doesn't necessarily cover
> all PCM buffer pages, and the remaining bytes are exposed via mmap.
>
> This patch changes the memory clearance to cover the all buffer pages
> if the stream is supposed to be mmap-ready (that guarantees that the
> buffer size is aligned in page size).
>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

Reviewed-by: Lars-Peter Clausen <lars@metafoo.de>

> ---
>   sound/core/pcm_native.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 47b155a49226..9f3f8e953ff0 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -755,8 +755,13 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream,
>   		runtime->boundary *= 2;
>   
>   	/* clear the buffer for avoiding possible kernel info leaks */
> -	if (runtime->dma_area && !substream->ops->copy_user)
> -		memset(runtime->dma_area, 0, runtime->dma_bytes);
> +	if (runtime->dma_area && !substream->ops->copy_user) {
> +		size_t size = runtime->dma_bytes;
> +
> +		if (runtime->info & SNDRV_PCM_INFO_MMAP)
> +			size = PAGE_ALIGN(size);
> +		memset(runtime->dma_area, 0, size);
> +	}
>   
>   	snd_pcm_timer_resolution_change(substream);
>   	snd_pcm_set_state(substream, SNDRV_PCM_STATE_SETUP);