Since mmap for userspace is based on page alignment, add page alignment
for iram alloc from pool, otherwise, some good data located in the same
page of dmab->area maybe touched wrongly by userspace like pulseaudio.
Signed-off-by: Robin Gong <[email protected]>
---
sound/core/memalloc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sound/core/memalloc.c b/sound/core/memalloc.c
index 0aeeb62..0f33516 100644
--- a/sound/core/memalloc.c
+++ b/sound/core/memalloc.c
@@ -77,7 +77,8 @@ static void snd_malloc_dev_iram(struct snd_dma_buffer *dmab, size_t size)
/* Assign the pool into private_data field */
dmab->private_data = pool;
- dmab->area = gen_pool_dma_alloc(pool, size, &dmab->addr);
+ dmab->area = gen_pool_dma_alloc_align(pool, size, &dmab->addr,
+ PAGE_SIZE);
}
/**
--
2.7.4
On Thu, 17 Dec 2020 17:15:47 +0100,
Robin Gong wrote:
>
> Since mmap for userspace is based on page alignment, add page alignment
> for iram alloc from pool, otherwise, some good data located in the same
> page of dmab->area maybe touched wrongly by userspace like pulseaudio.
>
> Signed-off-by: Robin Gong <[email protected]>
Thanks, applied now with Cc-to-stable.
Takashi
On 12/17/20 5:15 PM, Robin Gong wrote:
> Since mmap for userspace is based on page alignment, add page alignment
> for iram alloc from pool, otherwise, some good data located in the same
> page of dmab->area maybe touched wrongly by userspace like pulseaudio.
>
I wonder, do we also have to align size to be a multiple of PAGE_SIZE to
avoid leaking unrelated data?
On Thu, 17 Dec 2020 10:43:45 +0100,
Lars-Peter Clausen wrote:
>
> On 12/17/20 5:15 PM, Robin Gong wrote:
> > Since mmap for userspace is based on page alignment, add page alignment
> > for iram alloc from pool, otherwise, some good data located in the same
> > page of dmab->area maybe touched wrongly by userspace like pulseaudio.
> >
> I wonder, do we also have to align size to be a multiple of PAGE_SIZE
> to avoid leaking unrelated data?
Hm, a good question. Basically the PCM buffer size itself shouldn't
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.
thanks,
Takashi
On Thu, 17 Dec 2020 10:55:42 +0100,
Takashi Iwai wrote:
>
> On Thu, 17 Dec 2020 10:43:45 +0100,
> Lars-Peter Clausen wrote:
> >
> > On 12/17/20 5:15 PM, Robin Gong wrote:
> > > Since mmap for userspace is based on page alignment, add page alignment
> > > for iram alloc from pool, otherwise, some good data located in the same
> > > page of dmab->area maybe touched wrongly by userspace like pulseaudio.
> > >
> > I wonder, do we also have to align size to be a multiple of PAGE_SIZE
> > to avoid leaking unrelated data?
>
> Hm, a good question. Basically the PCM buffer size itself shouldn't
> 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.
That said, something like below (totally untested).
We might pass the pass-aligned size to dmab->bytes field instead of
keeping the original value, too.
Takashi
---
--- a/sound/core/memalloc.c
+++ b/sound/core/memalloc.c
@@ -126,6 +126,7 @@ static inline gfp_t snd_mem_get_gfp_flags(const struct device *dev,
int snd_dma_alloc_pages(int type, struct device *device, size_t size,
struct snd_dma_buffer *dmab)
{
+ size_t orig_size = size;
gfp_t gfp;
if (WARN_ON(!size))
@@ -133,6 +134,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;
@@ -177,7 +179,8 @@ int snd_dma_alloc_pages(int type, struct device *device, size_t size,
}
if (! dmab->area)
return -ENOMEM;
- dmab->bytes = size;
+ memset(dmab->area, 0, size);
+ dmab->bytes = orig_size;
return 0;
}
EXPORT_SYMBOL(snd_dma_alloc_pages);
On 12/17/20 11:14 AM, Takashi Iwai wrote:
> On Thu, 17 Dec 2020 10:55:42 +0100,
> Takashi Iwai wrote:
>> On Thu, 17 Dec 2020 10:43:45 +0100,
>> Lars-Peter Clausen wrote:
>>> On 12/17/20 5:15 PM, Robin Gong wrote:
>>>> Since mmap for userspace is based on page alignment, add page alignment
>>>> for iram alloc from pool, otherwise, some good data located in the same
>>>> page of dmab->area maybe touched wrongly by userspace like pulseaudio.
>>>>
>>> I wonder, do we also have to align size to be a multiple of PAGE_SIZE
>>> to avoid leaking unrelated data?
>> Hm, a good question. Basically the PCM buffer size itself shouldn't
>> 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.
> That said, something like below (totally untested).
> We might pass the pass-aligned size to dmab->bytes field instead of
> keeping the original value, too.
We'd need this for those APIs that also pass the size to the free()
function. Like dma_free_coherent() and free_pages_exact(), but maybe
those round up internally as well.
I had a quick look and I could not find any place were the code relies
on the requested buffer size being stored in dmab->bytes. In fact we
already reuse the buffer if there is an allocated buffer that is larger
than the requested buffer (See snd_pcm_lib_malloc_pages), so this should
be OK.
>
>
> Takashi
>
> ---
> --- a/sound/core/memalloc.c
> +++ b/sound/core/memalloc.c
> @@ -126,6 +126,7 @@ static inline gfp_t snd_mem_get_gfp_flags(const struct device *dev,
> int snd_dma_alloc_pages(int type, struct device *device, size_t size,
> struct snd_dma_buffer *dmab)
> {
> + size_t orig_size = size;
> gfp_t gfp;
>
> if (WARN_ON(!size))
> @@ -133,6 +134,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;
> @@ -177,7 +179,8 @@ int snd_dma_alloc_pages(int type, struct device *device, size_t size,
> }
> if (! dmab->area)
> return -ENOMEM;
> - dmab->bytes = size;
> + memset(dmab->area, 0, size);
> + dmab->bytes = orig_size;
> return 0;
> }
> EXPORT_SYMBOL(snd_dma_alloc_pages);
On 12/17/20 10:55 AM, Takashi Iwai wrote:
> On Thu, 17 Dec 2020 10:43:45 +0100,
> Lars-Peter Clausen wrote:
>> On 12/17/20 5:15 PM, Robin Gong wrote:
>>> Since mmap for userspace is based on page alignment, add page alignment
>>> for iram alloc from pool, otherwise, some good data located in the same
>>> page of dmab->area maybe touched wrongly by userspace like pulseaudio.
>>>
>> I wonder, do we also have to align size to be a multiple of PAGE_SIZE
>> to avoid leaking unrelated data?
> Hm, a good question. Basically the PCM buffer size itself shouldn't
> 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.
What I meant was that most of the APIs that we use to allocate memory
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.
On Thu, 17 Dec 2020 11:59:23 +0100,
Lars-Peter Clausen wrote:
>
> On 12/17/20 10:55 AM, Takashi Iwai wrote:
> > On Thu, 17 Dec 2020 10:43:45 +0100,
> > Lars-Peter Clausen wrote:
> >> On 12/17/20 5:15 PM, Robin Gong wrote:
> >>> Since mmap for userspace is based on page alignment, add page alignment
> >>> for iram alloc from pool, otherwise, some good data located in the same
> >>> page of dmab->area maybe touched wrongly by userspace like pulseaudio.
> >>>
> >> I wonder, do we also have to align size to be a multiple of PAGE_SIZE
> >> to avoid leaking unrelated data?
> > Hm, a good question. Basically the PCM buffer size itself shouldn't
> > 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.
>
> What I meant was that most of the APIs that we use to allocate memory
> 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.
IIRC, we used GFP_ZERO in the past for the normal page allocations,
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.
Takashi
On 12/17/20 12:06 PM, Takashi Iwai wrote:
> On Thu, 17 Dec 2020 11:59:23 +0100,
> Lars-Peter Clausen wrote:
>> On 12/17/20 10:55 AM, Takashi Iwai wrote:
>>> On Thu, 17 Dec 2020 10:43:45 +0100,
>>> Lars-Peter Clausen wrote:
>>>> On 12/17/20 5:15 PM, Robin Gong wrote:
>>>>> Since mmap for userspace is based on page alignment, add page alignment
>>>>> for iram alloc from pool, otherwise, some good data located in the same
>>>>> page of dmab->area maybe touched wrongly by userspace like pulseaudio.
>>>>>
>>>> I wonder, do we also have to align size to be a multiple of PAGE_SIZE
>>>> to avoid leaking unrelated data?
>>> Hm, a good question. Basically the PCM buffer size itself shouldn't
>>> 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.
>> What I meant was that most of the APIs that we use to allocate memory
>> 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.
> IIRC, we used GFP_ZERO in the past for the normal page allocations,
> 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.
Ah! That memset() in hw_params is new.
>
> 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().
On Thu, 17 Dec 2020 14:16:48 +0100,
Lars-Peter Clausen wrote:
>
> On 12/17/20 12:06 PM, Takashi Iwai wrote:
> > On Thu, 17 Dec 2020 11:59:23 +0100,
> > Lars-Peter Clausen wrote:
> >> On 12/17/20 10:55 AM, Takashi Iwai wrote:
> >>> On Thu, 17 Dec 2020 10:43:45 +0100,
> >>> Lars-Peter Clausen wrote:
> >>>> On 12/17/20 5:15 PM, Robin Gong wrote:
> >>>>> Since mmap for userspace is based on page alignment, add page alignment
> >>>>> for iram alloc from pool, otherwise, some good data located in the same
> >>>>> page of dmab->area maybe touched wrongly by userspace like pulseaudio.
> >>>>>
> >>>> I wonder, do we also have to align size to be a multiple of PAGE_SIZE
> >>>> to avoid leaking unrelated data?
> >>> Hm, a good question. Basically the PCM buffer size itself shouldn't
> >>> 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.
> >> What I meant was that most of the APIs that we use to allocate memory
> >> 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.
> > IIRC, we used GFP_ZERO in the past for the normal page allocations,
> > 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.
> Ah! That memset() in hw_params is new.
> >
> > 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().
That's true. On the second though, it might be better to extend that
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.
Below is another untested patch.
thanks,
Takashi
---
--- a/sound/core/pcm_memory.c
+++ b/sound/core/pcm_memory.c
@@ -36,6 +36,7 @@ static int do_alloc_pages(struct snd_card *card, int type, struct device *dev,
{
int err;
+ size = PAGE_ALIGN(size);
if (max_alloc_per_card &&
card->total_pcm_alloc_bytes + size > max_alloc_per_card)
return -ENOMEM;
@@ -187,7 +188,7 @@ static void snd_pcm_lib_preallocate_proc_write(struct snd_info_entry *entry,
buffer->error = -ENOMEM;
return;
}
- substream->buffer_bytes_max = size;
+ substream->buffer_bytes_max = new_dmab.bytes;
} else {
substream->buffer_bytes_max = UINT_MAX;
}
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;
+ memset(runtime->dma_area, 0, size);
+ }
snd_pcm_timer_resolution_change(substream);
snd_pcm_set_state(substream, SNDRV_PCM_STATE_SETUP);
On 12/17/20 3:24 PM, Takashi Iwai wrote:
> On Thu, 17 Dec 2020 14:16:48 +0100,
> Lars-Peter Clausen wrote:
>> On 12/17/20 12:06 PM, Takashi Iwai wrote:
>>> On Thu, 17 Dec 2020 11:59:23 +0100,
>>> Lars-Peter Clausen wrote:
>>>> On 12/17/20 10:55 AM, Takashi Iwai wrote:
>>>>> On Thu, 17 Dec 2020 10:43:45 +0100,
>>>>> Lars-Peter Clausen wrote:
>>>>>> On 12/17/20 5:15 PM, Robin Gong wrote:
>>>>>>> Since mmap for userspace is based on page alignment, add page alignment
>>>>>>> for iram alloc from pool, otherwise, some good data located in the same
>>>>>>> page of dmab->area maybe touched wrongly by userspace like pulseaudio.
>>>>>>>
>>>>>> I wonder, do we also have to align size to be a multiple of PAGE_SIZE
>>>>>> to avoid leaking unrelated data?
>>>>> Hm, a good question. Basically the PCM buffer size itself shouldn't
>>>>> 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.
>>>> What I meant was that most of the APIs that we use to allocate memory
>>>> 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.
>>> IIRC, we used GFP_ZERO in the past for the normal page allocations,
>>> 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.
>> Ah! That memset() in hw_params is new.
>>> 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().
> That's true. On the second though, it might be better to extend that
> 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;
I'm not sure.
Not all drivers use snd_pcm_lib_malloc_pages() and
runtime->dma_buffer_p->bytes might not be a multiple of PAGE_SIZE.
On the other hand if it is mmap-able, the underlying buffer must be a
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.
> + memset(runtime->dma_area, 0, size);
> + }
>
> snd_pcm_timer_resolution_change(substream);
> snd_pcm_set_state(substream, SNDRV_PCM_STATE_SETUP);
On Thu, 17 Dec 2020 15:57:02 +0100,
Lars-Peter Clausen wrote:
>
> On 12/17/20 3:24 PM, Takashi Iwai wrote:
> > On Thu, 17 Dec 2020 14:16:48 +0100,
> > Lars-Peter Clausen wrote:
> >> On 12/17/20 12:06 PM, Takashi Iwai wrote:
> >>> On Thu, 17 Dec 2020 11:59:23 +0100,
> >>> Lars-Peter Clausen wrote:
> >>>> On 12/17/20 10:55 AM, Takashi Iwai wrote:
> >>>>> On Thu, 17 Dec 2020 10:43:45 +0100,
> >>>>> Lars-Peter Clausen wrote:
> >>>>>> On 12/17/20 5:15 PM, Robin Gong wrote:
> >>>>>>> Since mmap for userspace is based on page alignment, add page alignment
> >>>>>>> for iram alloc from pool, otherwise, some good data located in the same
> >>>>>>> page of dmab->area maybe touched wrongly by userspace like pulseaudio.
> >>>>>>>
> >>>>>> I wonder, do we also have to align size to be a multiple of PAGE_SIZE
> >>>>>> to avoid leaking unrelated data?
> >>>>> Hm, a good question. Basically the PCM buffer size itself shouldn't
> >>>>> 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.
> >>>> What I meant was that most of the APIs that we use to allocate memory
> >>>> 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.
> >>> IIRC, we used GFP_ZERO in the past for the normal page allocations,
> >>> 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.
> >> Ah! That memset() in hw_params is new.
> >>> 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().
> > That's true. On the second though, it might be better to extend that
> > 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;
>
> I'm not sure.
>
> Not all drivers use snd_pcm_lib_malloc_pages() and
> runtime->dma_buffer_p->bytes might not be a multiple of PAGE_SIZE.
The runtime->dma_buffer_p->bytes is assured to be page-aligned by the
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 a
> 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.
OK, how about the following instead?
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);
On 12/17/20 4:18 PM, Takashi Iwai wrote:
> On Thu, 17 Dec 2020 15:57:02 +0100,
> Lars-Peter Clausen wrote:
>> On 12/17/20 3:24 PM, Takashi Iwai wrote:
>>> On Thu, 17 Dec 2020 14:16:48 +0100,
>>> Lars-Peter Clausen wrote:
>>>> On 12/17/20 12:06 PM, Takashi Iwai wrote:
>>>>> On Thu, 17 Dec 2020 11:59:23 +0100,
>>>>> Lars-Peter Clausen wrote:
>>>>>> On 12/17/20 10:55 AM, Takashi Iwai wrote:
>>>>>>> On Thu, 17 Dec 2020 10:43:45 +0100,
>>>>>>> Lars-Peter Clausen wrote:
>>>>>>>> On 12/17/20 5:15 PM, Robin Gong wrote:
>>>>>>>>> Since mmap for userspace is based on page alignment, add page alignment
>>>>>>>>> for iram alloc from pool, otherwise, some good data located in the same
>>>>>>>>> page of dmab->area maybe touched wrongly by userspace like pulseaudio.
>>>>>>>>>
>>>>>>>> I wonder, do we also have to align size to be a multiple of PAGE_SIZE
>>>>>>>> to avoid leaking unrelated data?
>>>>>>> Hm, a good question. Basically the PCM buffer size itself shouldn't
>>>>>>> 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.
>>>>>> What I meant was that most of the APIs that we use to allocate memory
>>>>>> 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.
>>>>> IIRC, we used GFP_ZERO in the past for the normal page allocations,
>>>>> 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.
>>>> Ah! That memset() in hw_params is new.
>>>>> 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().
>>> That's true. On the second though, it might be better to extend that
>>> 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;
>> I'm not sure.
>>
>> Not all drivers use snd_pcm_lib_malloc_pages() and
>> runtime->dma_buffer_p->bytes might not be a multiple of PAGE_SIZE.
> The runtime->dma_buffer_p->bytes is assured to be page-aligned by the
> 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 a
>> 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.
> OK, how about the following instead?
> 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.
I think this will work for the leaking data issue.
But it will not help with the original issue that
gen_pool_dma_alloc_align() does not reserve the remainder of the page
and could give it out to other allocations. We'd need a separate patch
for that.
>
>
> 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);
On Thu, 17 Dec 2020 16:38:22 +0100,
Lars-Peter Clausen wrote:
>
> >> 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.
> > OK, how about the following instead?
> > 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.
>
> I think this will work for the leaking data issue.
>
> But it will not help with the original issue that
> gen_pool_dma_alloc_align() does not reserve the remainder of the page
> and could give it out to other allocations. We'd need a separate patch
> for that.
That can be fixed by the pcm_memory.c change in the previous patch.
Recited below.
Of course it won't cover the non-standard allocation case, but then
it's rather the responsibility of such driver.
Takashi
---
--- a/sound/core/pcm_memory.c
+++ b/sound/core/pcm_memory.c
@@ -36,6 +36,7 @@ static int do_alloc_pages(struct snd_card *card, int type, struct device *dev,
{
int err;
+ size = PAGE_ALIGN(size)
if (max_alloc_per_card &&
card->total_pcm_alloc_bytes + size > max_alloc_per_card)
return -ENOMEM;
@@ -187,7 +188,7 @@ static void snd_pcm_lib_preallocate_proc_write(struct snd_info_entry *entry,
buffer->error = -ENOMEM;
return;
}
- substream->buffer_bytes_max = size;
+ substream->buffer_bytes_max = new_dmab.bytes;
} else {
substream->buffer_bytes_max = UINT_MAX;
}