[alsa-devel] [PATCH 0/2] ALSA: pcm: implement the anonymous dup v3

[Takashi Iwai]

On Thu, 31 Jan 2019 01:45:04 +0100,
Phil Burk wrote:
> 
> Hello Mark,
> 
> Our security team was very concerned about the old ALSA FD. It provided too
> much access to the guts of ALSA. 
> 
> I assume they will not like anything other than a plain "anon_inode:dmabuf".
> If it is a new FD, then the code would have to be reviewed. Even if it looked
> OK there might be some holes that we don't find. So it would probably be
> rejected.

The review is appreciated, sure! ;)

Above all, it'd be helpful if you can tell us exactly which feature is
requested and exactly what have to be avoided.  Jaroslav's patchset
tries to provide the generic implementation, and this can do more than
you guys needed.  But it can restrict the permissions well, too.

> I cannot speak for our security team so I am working on setting up a meeting
> or conversation between Mark and Zach, our security expert.
> 
> Adding the anon_inode:snd-pcm might be great for ALSA. That could be used by
> the HAL for STATUS and CONTROL. But it is likely that we will need an
> additional anon_inode:dmabuf FD that is only associated with the PCM buffer.
> It can then be safely passed to an Android app.

I find it fine to add a dma-buf implementation, too -- but only if
it's really safely, sanely and simply implemented.  The suggested
implementation so far has a way too many holes, unfortunately, and it
can easily lead to kernel Oops when something wrong happens in the
master stream side.  And we need to cover some corner cases (e.g. a
hardware buffer setup that isn't supposed to be shared) that are
overlooked, too.


thanks,

Takashi

> 
> Thanks,
> Phil Burk
> 
> On Wed, Jan 30, 2019 at 2:32 PM Mark Brown <broonie at kernel.org> wrote:
> 
>     On Wed, Jan 30, 2019 at 01:41:37PM +0100, Jaroslav Kysela wrote:
>     > This patchset contains the anonymous dup implementation with permissions
>     > checking for the ALSA's PCM interface in kernel to enable the restricted
>     > DMA sound buffer sharing for the restricted tasks.
>     >
>     > The code was tested through qemu and it seems to be pretty stable.
>     >
>     > The initial tinyalsa implementation can be found here:
>     >
>     >   https://github.com/perexg/tinyalsa/commits/anondup
>     >
>     > The filtering might be refined. It depends on the real requirements.
>     > Perhaps, we may create more ioctl groups. Any comments are more than
>     > welcome.
>    
>     My understanding based on some off-list discussion is that the Android
>     security people are going to see anything that involves passing more
>     than a block of memory (and in particular anything that gives access to
>     the sound APIs) as a problem.  That's obviously going to be an issue for
>     anything O_APPEND based.  My understanding is that this is fundamentally
>     a risk mitigation thing - by not having any of the sound kernel
>     interfaces available to the applications affected there's no possibility
>     that any problems in the sound code can cause security issues.
> 
>