| Message ID | 20190130124139.10439-1-perex@perex.cz (mailing list archive) |
|---|---|
| Headers | show |
| Series | ALSA: pcm: implement the anonymous dup v3 | expand |
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.
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. 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. 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. >
Hi Phil & all, On Wed, Jan 30, 2019 at 04:45:04PM -0800, 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. > > 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. Thanks for the inputs. I went through discussions, I'd like to divide the work into two mainly parts: - The first part is to use dmabuf to dynamically import buffer to sound device; so the buffer is not bound to sound device at the initialization phase; - The second part is to use dmabuf to export buffer with anon_inode; thus it can meet the security requirement. I go through Jaroslav implementation (thanks a lot for the quick moving for this part!), it tries to implement the second part, but it misses the first part support for dynamically binding audio buffer; and as Mark/Phil mentioned, Jaroslav patch tries to use the same one FD for both sound device and audio buffer. I think it's good to firstly use one test case to demonstrate to dynamically import buffer to sound device, then this buffer can be exported with anon_inode for user space. Is this doable? Thanks, Leo Yan > 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. > >
On Wed, 30 Jan 2019 23:32:37 +0100, Mark Brown 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. The patch 2 implements exactly that kind of access restriction, so that the passed fd won't do anything else than wished. If we want to be super-conservative, the implementation could be even simpler -- instead of filtering, we may pass a minimum fd ops that contains only mmap and release for the anon-dup fd... thanks, Takashi
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. > >
Dne 31.1.2019 v 01:45 Phil Burk napsal(a): > 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. Hello Phil, My point is that the dma-buf -> sound pcm buffer maping interface is more complex, error prone and the code review/audit expensive than reusing the current code without any functionality or security benefits. We can nicely restrict the file operations to allow to mmap only the pcm sound buffer and eventually, if we are too much paranoid (to bypass the the bitmap like permission checking as I suggested), we can create a special case for the Android usage to return the file descriptor with very restricted 'struct file_operations' with just the mmap and release callbacks. We can also change the name for this file descriptor to distinguish it from the "anon_inode:snd-pcm" (for example "anon_inode:snd-pcm-paranoid") to let SELinux do it's work properly. The mmap implementation for the sound driver is few lines of the code (for the standard devices - very easy to review), so we cannot speak about security holes at all. If there is a problem with the kernel page allocation/management in the sound driver, there will be problem with dmabuf -> sound pcm buffer mapping, too (plus other problems caused by the concurrent access to the buffer which is managed /alloc/free/ by the sound driver - not dma-buf). > 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. Thanks. Let us know the result. Eventually, your security expert can freely join to our conversation here. Jaroslav
On Thu, Jan 31, 2019 at 09:08:04AM +0100, Takashi Iwai wrote: > Mark Brown wrote: > > 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. > The patch 2 implements exactly that kind of access restriction, so > that the passed fd won't do anything else than wished. Yeah. > If we want to be super-conservative, the implementation could be even > simpler -- instead of filtering, we may pass a minimum fd ops that > contains only mmap and release for the anon-dup fd... I think that'd definitely help address the concerns.
Dne 31.1.2019 v 13:26 Mark Brown napsal(a): > On Thu, Jan 31, 2019 at 09:08:04AM +0100, Takashi Iwai wrote: >> Mark Brown wrote: > >>> 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. > >> The patch 2 implements exactly that kind of access restriction, so >> that the passed fd won't do anything else than wished. > > Yeah. > >> If we want to be super-conservative, the implementation could be even >> simpler -- instead of filtering, we may pass a minimum fd ops that >> contains only mmap and release for the anon-dup fd... > > I think that'd definitely help address the concerns. A possible implementation: http://git.alsa-project.org/?p=alsa-kernel.git;a=commitdiff;h=ca15bc69a984cc0eae2c43d0a49c66a20c937f39 Jaroslav
+Zach Riggle <riggle@google.com> Hello, > Eventually, your security expert can freely join to our conversation here. Thanks. The biggest unanswered question is whether Android security will allow the file descriptor to be passed to an app. So I have added our security person, Zach Riggle, who originally requested the anon_inode:dmabuf FD. If Zach is happy then I am happy. We will need two file descriptors, one with full permissions for the HAL, and one with only PCM access for the app to use. It seems we are considering two options for the app's FD: 1) provide an anon_inode:dmabuf that never has CONTROL permissions, which seems safe, but requires more changes to the driver and is a bit of a hack 2) provide an anon_inode:snd-pcm that has CONTROL permissions turned off, which seems seems less safe, but requires fewer changes and fits with the design Which one is actually better for security? Here is an earlier argument for snd-pcm from Jaroslav: My point is that the dma-buf -> sound pcm buffer maping interface is > more complex, error prone and the code review/audit expensive than > reusing the current code without any functionality or security benefits. > We can nicely restrict the file operations to allow to mmap only the pcm > sound buffer and eventually, if we are too much paranoid (to bypass the > the bitmap like permission checking as I suggested), we can create a > special case for the Android usage to return the file descriptor with > very restricted 'struct file_operations' with just the mmap and release > callbacks. We can also change the name for this file descriptor to > distinguish it from the "anon_inode:snd-pcm" (for example > "anon_inode:snd-pcm-paranoid") to let SELinux do it's work properly. > The mmap implementation for the sound driver is few lines of the code > (for the standard devices - very easy to review), so we cannot speak > about security holes at all. If there is a problem with the kernel page > allocation/management in the sound driver, there will be problem with > dmabuf -> sound pcm buffer mapping, too (plus other problems caused by > the concurrent access to the buffer which is managed /alloc/free/ by the > sound driver - not dma-buf). Also note that my emails bounce off the alsa-project mail list. Phil Burk On Thu, Jan 31, 2019 at 5:30 AM Jaroslav Kysela <perex@perex.cz> wrote: > Dne 31.1.2019 v 13:26 Mark Brown napsal(a): > > On Thu, Jan 31, 2019 at 09:08:04AM +0100, Takashi Iwai wrote: > >> Mark Brown wrote: > > > >>> 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. > > > >> The patch 2 implements exactly that kind of access restriction, so > >> that the passed fd won't do anything else than wished. > > > > Yeah. > > > >> If we want to be super-conservative, the implementation could be even > >> simpler -- instead of filtering, we may pass a minimum fd ops that > >> contains only mmap and release for the anon-dup fd... > > > > I think that'd definitely help address the concerns. > > A possible implementation: > > > http://git.alsa-project.org/?p=alsa-kernel.git;a=commitdiff;h=ca15bc69a984cc0eae2c43d0a49c66a20c937f39 > > Jaroslav > > -- > Jaroslav Kysela <perex@perex.cz> > Linux Sound Maintainer; ALSA Project; Red Hat, Inc. >
Mark and Zach and I talked. Zach said that "dmabuf" is not a hard requirement. Another "anon_inode" would probably be OK as long as the app cannot turn on any permissions besides PCM access. Our security team will just need to review the changes. So I think you should proceed with the "anon_inode:snd-pcm" if you think that will be more secure. Thanks for proposing this. Zach has some notes in his initial review of Jaroslav's code. Zach? One thing Zach mentioned is that the API should only allow *removing* permissions and not adding permissions. What permissions would be set on the FD given to the app? Also Mark mentioned that the FD app would have PCM access and "close" permission. What flag allows close? What else is permitted under that flag? Or is close permission just a generic "FD" permission unrelated to ALSA? Thanks for all your work on this. Sorry if I caused alarm. I just wanted to make sure we could use the solution you provide. Phil Burk
The big concerns on our end are: (1) Can the buffer be mremap()ed with a different *offset* into the buffer? This was a concern in the past and the reason for the anon_inode stuff at all. I believe that as long as the *size* of the mapping doesn't change, Linux mm will gladly permit mremap() without informing the driver. (2) Can we ensure that permissions can only ever be dropped? (new_perms = old_perms & requested_perms) . It's probably useful to throw an error code if new permissions are requested. (3) It looks like the code for snd_pcm_anonymous_dup in the patchset takes a *perm* argument from user-space and discards it. Is this intended? (4) I'm not familiar with the lifecycle of all of the objects, and introducing a custom *dup* routine might cause unexpected problems (e.g. use-after-free, double-free). I'm not well-versed-enough in how the driver-specific stuff is handled underneath e.g. snd_card_file_add / snd_card_file_remove to be sure about it. Zach Riggle | Android Security | riggle@google.com | Austin, TX
On Thu, 31 Jan 2019 20:54:33 +0100, Zach Riggle
Dne 31.1.2019 v 21:32 Takashi Iwai napsal(a): > On Thu, 31 Jan 2019 20:54:33 +0100, > Zach Riggle
On Thu, Jan 31, 2019 at 09:32:27PM +0100, Takashi Iwai wrote: > On Thu, 31 Jan 2019 20:54:33 +0100, > Zach Riggle
On Fri, Feb 01, 2019 at 10:55:24AM +0100, Jaroslav Kysela wrote: > I agree. We can have just two modes for the beginning: > a) full one (useful for testing) > b) buffer only (allow just sound data mmap) > The question, if we should use different names (line anon_inode:snd-pcm > and anon_inode:snd-pcm-buffer) for the anonymous inodes remains. I > believe it might be good to distinguish this to allow the proper SELinux > audit. I agree that the separte names seems better, it gives more flexibility and control to people writing policies.
Thank you all for sorting this out. It seems like we are moving in a really good direction. > I agree. We can have just two modes for the beginning: > a) full one (useful for testing) > b) buffer only (allow just sound data mmap) The full one is would also be used by the HAL for querying the DSP position. > if we should use different names (like anon_inode:snd-pcm and anon_inode:snd-pcm-buffer) That would be helpful. I have attached a revised requirements doc. The original doc was more of a HowTo for OEMs to create the "anon_inode:dmabuf" FD.. This clarifies the requirements and allows for the use of "anon_inode:snd-pcm*". It should match what we have arrived at by discussion. Let me know if it makes sense. Thanks, Phil Burk On Fri, Feb 1, 2019 at 5:01 AM Mark Brown <broonie@kernel.org> wrote: > On Fri, Feb 01, 2019 at 10:55:24AM +0100, Jaroslav Kysela wrote: > > > I agree. We can have just two modes for the beginning: > > > a) full one (useful for testing) > > b) buffer only (allow just sound data mmap) > > > The question, if we should use different names (line anon_inode:snd-pcm > > and anon_inode:snd-pcm-buffer) for the anonymous inodes remains. I > > believe it might be good to distinguish this to allow the proper SELinux > > audit. > > I agree that the separte names seems better, it gives more flexibility > and control to people writing policies. >
Dne 1.2.2019 v 16:31 Phil Burk napsal(a): > Thank you all for sorting this out. It seems like we are moving in a > really good direction. > >> I agree. We can have just two modes for the beginning: >> a) full one (useful for testing) >> b) buffer only (allow just sound data mmap) > > The full one is would also be used by the HAL for querying the DSP position. > >> if we should use different names (like anon_inode:snd-pcm and > anon_inode:snd-pcm-buffer) > > That would be helpful. > > I have attached a revised requirements doc. The original doc was more of > a HowTo for OEMs to create the "anon_inode:dmabuf" FD.. This clarifies > the requirements and allows for the use of "anon_inode:snd-pcm*". It > should match what we have arrived at by discussion. Let me know if it > makes sense. It looks fine, but the HAL will use probably the standard sound device open (/dev/snd/), doesn't? So: fd1 - /dev/snd/pcm (HAL) - standard sound device inode (no restrictions) fd2 - anon_inode:snd-pcm-buffer (for the EXCLUSIVE access) With modes, I talked about the anonymous dup ioctl parameter. If there's another resource manager above HAL, the situation might be: fd1 - /dev/snd/pcm (resource manager) - standard sound device inode fd2 - anon_inode:snd-pcm (access to the full pcm sound API using the anonymous inode) fd3 - anon_inode:snd-pcm-buffer (for the EXCLUSIVE access) Perhaps you have different layers in Android. Thanks, Jaroslav
Thanks for the clarification. I was thinking the anon_inode:snd-pcm FD replaced the /dev/snd/pcm FD. > fd1 - /dev/snd/pcm (HAL) - standard sound device inode (no restrictions) > fd2 - anon_inode:snd-pcm-buffer (for the EXCLUSIVE access) That sounds right. Thanks again for this work. ALSA is very important for Android Audio. Phil Burk On Fri, Feb 1, 2019 at 8:28 AM Jaroslav Kysela <perex@perex.cz> wrote: > Dne 1.2.2019 v 16:31 Phil Burk napsal(a): > > Thank you all for sorting this out. It seems like we are moving in a > > really good direction. > > > >> I agree. We can have just two modes for the beginning: > >> a) full one (useful for testing) > >> b) buffer only (allow just sound data mmap) > > > > The full one is would also be used by the HAL for querying the DSP > position. > > > >> if we should use different names (like anon_inode:snd-pcm and > > anon_inode:snd-pcm-buffer) > > > > That would be helpful. > > > > I have attached a revised requirements doc. The original doc was more of > > a HowTo for OEMs to create the "anon_inode:dmabuf" FD.. This clarifies > > the requirements and allows for the use of "anon_inode:snd-pcm*". It > > should match what we have arrived at by discussion. Let me know if it > > makes sense. > > It looks fine, but the HAL will use probably the standard sound device > open (/dev/snd/), doesn't? So: > > fd1 - /dev/snd/pcm (HAL) - standard sound device inode (no restrictions) > fd2 - anon_inode:snd-pcm-buffer (for the EXCLUSIVE access) > > With modes, I talked about the anonymous dup ioctl parameter. If there's > another resource manager above HAL, the situation might be: > > fd1 - /dev/snd/pcm (resource manager) - standard sound device inode > fd2 - anon_inode:snd-pcm (access to the full pcm sound API using the > anonymous inode) > fd3 - anon_inode:snd-pcm-buffer (for the EXCLUSIVE access) > > Perhaps you have different layers in Android. > > Thanks, > Jaroslav > > > -- > Jaroslav Kysela <perex@perex.cz> > Linux Sound Maintainer; ALSA Project; Red Hat, Inc. >