lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 17 Dec 2020 16:38:22 +0100 From: Lars-Peter Clausen <lars@...afoo.de> To: Takashi Iwai <tiwai@...e.de> Cc: alsa-devel@...a-project.org, gustavoars@...nel.org, linux-kernel@...r.kernel.org, shengjiu.wang@....com, tiwai@...e.com, pierre-louis.bossart@...ux.intel.com, xiang@...nel.org, Robin Gong <yibin.gong@....com>, akpm@...ux-foundation.org Subject: Re: [PATCH v1 ] ALSA: core: memalloc: add page alignment for iram 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);
Powered by blists - more mailing lists