[libvirt] [PATCH v1 16/21] qemu: Enter the namespace on relabelling

[Michal Privoznik]

On 05.12.2016 14:40, Daniel P. Berrange wrote:
> On Thu, Nov 24, 2016 at 03:48:05PM +0100, Michal Privoznik wrote:
>> Instead of trying to fix our security drivers, we can use a
>> simple trick to relabel paths in both namespace and the host.
>> I mean, if we enter the namespace some paths are still shared
>> with the host so any change done to them is visible from the host
>> too.
>> Therefore, we can just enter the namespace and call
>> SetAllLabel()/RestoreAllLabel() from there. Yes, it has slight
>> overhead because we have to fork in order to enter the namespace.
>> But on the other hand, no complexity is added to our code.
> 
> I'm a little concerned that this may be storing problems for us
> at a later date. If the security manager classes have any state
> they update such changes are invisible to the main libvirt
> process now. Also stuff running between fork+exec is restricted
> to async signal safe functions only. I think it is hard for us
> to be entirely confident about that safety when we're running
> the entire security driver labelling code in that region.
> 
> Ultimately the only bit of the security drivers that needs to
> run in the namespace is the setfilecon_raw() or chown() system
> calls.
> 
> So I wonder if we should make it possible to provide a namespace
> helper callback to the security drivers that they'd use only
> for the setfilecon_raw/chown calls.
> 
> eg, we could do something like
> 
>   
>   typedef (*virSecurityManagerNamespaceHelperFunc)(void *opaque)
>   typedef (*virSecurityManagerNamespaceHelper)(virSecurityManagerNamespaceHelperFunc func, void *opaque)
> 
>   virSecurityManagerSetNamespacehelper(mgr, runInNamespaceHelper)
> 
>    ....do the labelling...
> 
>   virSecurityManagerSetNamespacehelper(mgr, NULL)
> 
> the namespace helper would have to be stored in a thread local
> since we need to cope with some VMs not having separate namespaces
> 
> The security manager code would need todo
> 
>   struct SELinuxNamespaceData {
>       const char *path;
>       security_context_t ctx;
>   }
> 
> Now instead of
> 
> 
>    if (setfilecon_raw(path, tcon) < 0)
>        ...
> 
> it would do
> 
>    static int SELinuxNamespaceHelperFunc(void *opaque) {
>        struct SELinuxNamespaceData *data = opaque
> 
>        return setfilecon_raw(data->path, data->tcon)
>    }
> 
>    struct SELinuxNamespaceData data = { path, tcon}
> 
>    if (namespacehelper(SELinuxNamespaceHelperFunc, &data) < 0)
>       ...
> 

I was thinking about this too, but I see two problems with this approach:

1) fork() on every chown() and every sefilecon_raw(). That's too much IMO.

2) thread safeness. We can't have the sec manager wide callback. But you
already covered that - and thread local variable might in fact work just
fine!

Michal