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

[Michal Privoznik]

On 05.12.2016 15:25, Daniel P. Berrange wrote:
> On Mon, Dec 05, 2016 at 03:14:50PM +0100, Michal Privoznik wrote:
>> 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.
> 
> Hmm, good point. fork() is fairly fast on Linux, but it is still a
> non-negligble overhead in our startup codepath.
> 
> Avoiding the fork would mean having some persistent child and RPC
> to it which is also likely to add just as much overhead.
> 
> I guess we'll just have to bear in mind the limitations for security
> managers in future and go with what you've done.

The other option might be to:

1) dig out code that gets you the most concrete seclabel for a device
2) chown() & setfilecon_raw() devices at the time we are creating /dev
entry (for this we need the function from 1.)
3) have a filter callback (that would be set & unset just like you
propose above) that for any /dev prefixed path returns 'success' without
any actual chown() or setfilecon_raw() performed.

This way we would not need any additional fork() nor we would have to
worry about modifying internal state of secdriver in a separate process.
However:

a) I'm a bit worried as this is going behind secdriver's back,
b) It's a lot more code than this patch.

Therefore I'd suggest to go with this patch for now and save the work
for a follow up patch.

Michal