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

[Takashi Iwai]

On Thu, 31 Jan 2019 20:54:33 +0100,
Zach Riggle 🖖 wrote:
> 
> 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.

Could you elaborate which perspective of mremap() can be a big
problem?  The driver interface does nothing but a standard mmap for
now.

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

The permission is bound with each fd, and determined only at creation
time, and unchangeable.

Also, the patches Jaroslav posted can become even simpler / safer;
i.e. we don't need to introduce the perms bits as a start.  IMO, we
can begin with the minimum, mmap-only file ops.  The API should be
defined properly from the beginning (e.g. passing perms argument,
etc), of course.  Then, if any request comes up, we may extend later.

> (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?

My understanding is that it's the design to be simple.  But we don't
need to stick with it, if you can suggest any better interface.

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

That's the advantage of anon-dup implementation; the PCM stream
handling with O_APPEND has been heavily used by alsa-lib dmix PCM
implementations.  So it already survives over decades.


thanks,

Takashi

> Zach Riggle |  Android Security |  riggle at google.com |  Austin, TX 🇨🇱
> 
> On Thu, Jan 31, 2019 at 1:36 PM Phil Burk <philburk at 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
> 
>