[pulseaudio-discuss] [PATCH 0/5] Add access control to protocol-native

[Ahmed S. Darwish]

Hi,

On Fri, Jul 01, 2016 at 08:00:03PM +0300, Tanu Kaskinen wrote:
> On Fri, 2016-07-01 at 10:22 +0200, David Henningsson wrote:
> > On 2016-07-01 03:30, Ahmed S. Darwish wrote:
> > > On Fri, Jun 24, 2016 at 05:21:53PM +0200, David Henningsson wrote:
> > > > Suppose the following chain of events:
> > > > 
> > > >   1. Protocol-native loads
> > > >   2. Access control module loads, puts hooks into existing protocol-native
> > > > instance
> > > >   3. Protocol-native unloads
> > > >  4. Another Protocol-native loads
> > > > 
> > > > At this point the new protocol-native is unprotected, which is probably not
> > > > what you want?
> > > > 
> > > > Also we have to make sure we don't get use-after-free errors, no matter
> > > > which order the modules are unloaded. But that's certainly solvable, just an
> > > > additional thing to remember.
> > > 
> > > Can we solve the above two points by by introducing a pulse module
> > > dependencies feature, just like what the linux kernel does? [1]
> > > 
> > > If so, we can do the following:
> > > 
> > > (a) Introduce module dependencies
> > > 
> > > (b) Put the access-control hooks inside protocol-native itself,
> > >     and thus remove the 1-to-1 mappings [2] and avoid making core
> > >     dependent on protocol-native (even implicitly)
> > > 
> > > (c) Make all policy modules, which contains all the hooks
> > >     implementations, hard-dependent on protocol-native
> > > 
> > > In the first point raised, to unload protocol-native, user will
> > > have to unload the dependent protocol-native-policy-X first. So if
> > > the user loads protocol-native again, he/she will know that the
> > > policy module will need to be reloaded as well ..
> > > 
> > > In the second point raised, hopefully module dependencies will
> > > simplify the relationship: use-after-free errors can be eliminated
> > > given proper cleanup by the policy modules ..
> > > 
> > > Any thoughts on this?
> > > 
> > 
> > Just looking at core.h there seems to be both PA_CORE_HOOK_MODULE_NEW 
> > and PA_CORE_HOOK_MODULE_UNLINK hooks, perhaps it could be as simple as 
> > just requiring any access control module to listen to these and take the 
> > appropriate action?
> >

Yup, that's much easier indeed.

> > As for whether or not a generic "module dependencies" feature would be 
> > good or bad: I would find it more user friendly myself if the user would 
> > not have to manually reload access control modules, but since I'm not 
> > that involved anymore, I don't think I should have decision power on 
> > that matter. :-)
> 
> I agree that using MODULE_NEW and MODULE_UNLINK would be preferable to
> a module dependency system if there actually was any danger of use-
> after-free problems, but that issue doesn't exist. The access policy
> module presumably holds a reference to the pa_native_protocol object,
> which is a reference counted singleton, which will not be freed as long
> as someone holds a reference. There's no 1:1 mapping to module-native-
> protocol instances - there may be multiple instances loaded, but only
> one pa_native_protocol object will ever exist.
>

Good point!

I guess MODULE_NEW and MODULE_UNLINK can still have another good
use in the policy module for fixing the earlier-mentioned case
of load policy, unload protocol-native, then load it again, thus
leaving the new protocol-native unsecured. Or will loading the
new protocol-native be irrelevant since the older singleton object
didn't get removed due to the reference count? will investigate ..

In all cases, to keep everyone updated: we were disussing two
proposed designs for implementing access-control in the latest
weekly meeting. Option #1 represents a separation of concerns,
proposed by Arun over IRC, and option #2 integrates access
control with protocol-native as done in this patch series. [1]

After some (amazingly quick!) working prototypes by Arun during
the weekly, [2] it seems there are no major roadblocks to the
first option. I've also had some initial code in the weekend, and
the more code I've put into it, the more elegant this solution
felt over design option #2.

As a result of the above, we've agreed – in a quick discussion
today's morning with Arun – to build access control using the
first "separation of concerns" option. [1]

In case I'll face a _major_ roadblock during the implementation
of option #1, we will favor back option #2 where the hooks are
coupled inside protocol native. At that point the issue of
protocol-native being reference-counted, as shown by Tanu above,
will be revisited.

Thanks a lot everyone,

[1] https://www.freedesktop.org/wiki/Software/PulseAudio/Documentation/Developer/AccessControl/#proposeddesigns

[2] https://cgit.freedesktop.org/~arun/pulseaudio/log/

-- 
Darwish
http://darwish.chasingpointers.com