On Thu, 17 Dec 2020 15:57:02 +0100,
Lars-Peter Clausen wrote:
On 12/17/20 3:24 PM, Takashi Iwai wrote:The runtime->dma_buffer_p->bytes is assured to be page-aligned by the
On Thu, 17 Dec 2020 14:16:48 +0100,I'm not sure.
Lars-Peter Clausen wrote:
On 12/17/20 12:06 PM, Takashi Iwai wrote:That's true. On the second though, it might be better to extend that
On Thu, 17 Dec 2020 11:59:23 +0100,Ah! That memset() in hw_params is new.
Lars-Peter Clausen wrote:
On 12/17/20 10:55 AM, Takashi Iwai wrote:IIRC, we used GFP_ZERO in the past for the normal page allocations,
On Thu, 17 Dec 2020 10:43:45 +0100,What I meant was that most of the APIs that we use to allocate memory
Lars-Peter Clausen wrote:
On 12/17/20 5:15 PM, Robin Gong wrote:Hm, a good question. Basically the PCM buffer size itself shouldn't
Since mmap for userspace is based on page alignment, add page alignmentI wonder, do we also have to align size to be a multiple of PAGE_SIZE
for iram alloc from pool, otherwise, some good data located in the same
page of dmab->area maybe touched wrongly by userspace like pulseaudio.
to avoid leaking unrelated data?
be influenced by that (i.e. no hw-constraint or such is needed), but
the padding should be cleared indeed. I somehow left those to the
allocator side, but maybe it's safer to clear the whole buffer in
sound/core/memalloc.c commonly.
work on a PAGE_SIZE granularity. I.e. if you request a buffer that
where the size is not a multiple of PAGE_SIZE internally they will
still allocate a buffer that is a multiple of PAGE_SIZE and mark the
unused bytes as reserved.
But I believe that is not the case gen_pool_dma_alloc(). It will
happily allocate those extra bytes to some other allocation request.
That we need to zero out the reserved bytes even for those other APIs
is a very good additional point!
I looked at this a few years ago and I'm pretty sure that we cleared
out the allocated area, but I can't find that anymore in the current
code. Which is not so great I guess.
but this was dropped as it's no longer supported or so.
Also, we clear out the PCM buffer in hw_params call, but this is for
the requested size, not the actual allocated size, hence the padding
bytes will remain uncleared.
So I believe it's safer to add an extra memset() like my test patch.Yea, we definitely want that.
Do we care about leaking audio samples from a previous
application. I.e. application 'A' allocates a buffer plays back some
data and then closes the device again. Application 'B' then opens the
same audio devices allocates a slightly smaller buffer, so that it
still uses the same number of pages. The buffer from the previous
allocation get reused, but the remainder of the last page wont get
cleared in hw_params().
memset() in hw_params to assure clearing the whole allocated buffer.
We can check runtime->dma_buffer_p->bytes for the actual size.
Also, in the PCM memory allocator, we make sure that the allocation is
performed for page size.
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 47b155a49226..6aabad070abf 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -755,8 +755,15 @@ 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;
+
+ if (runtime->dma_buffer_p)
+ size = runtime->dma_buffer_p->bytes;
+ else
+ size = runtime->dma_bytes;
Not all drivers use snd_pcm_lib_malloc_pages() and
runtime->dma_buffer_p->bytes might not be a multiple of PAGE_SIZE.
change in pcm_memory.c in this patch. But it's true that non-standard
allocations won't cover the whole pages...
On the other hand if it is mmap-able, the underlying buffer must be aOK, how about the following instead?
multiple of PAGE_SIZE. So a simple memset(..., PAGE_ALIGN(size))
should work.
But we'd risk breaking drivers that do not reserve the remainder of
the page and use it for something else.
Maybe what we need is a check that runtime->dma_area is page aligned
and runtime->dma_bytes is a multiple of PAGE_SIZE. With a warning at
first and then turn this into a error a year later or so.
Just check SNDRV_PCM_INFO_MMAP in runtime->info; if this is set, the
buffer size must be aligned with the page size, and we are safe to
extend the size to clear.
So the revised fix is much simpler, something like below.
thanks,
Takashi
---
--- 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);