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, Jan 31, 2019 at 1:36 PM Phil Burk philburk@google.com wrote:
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