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@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.