Re: [libvirt] [PATCH v1 16/21] qemu: Enter the namespace on relabelling
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