[libvirt] [PATCH 2/3] Set SELinux context label of pipes used for qemu migration

[Laine Stump]

On 01/25/2011 12:49 PM, Daniel P. Berrange wrote:
> On Tue, Jan 25, 2011 at 04:24:19AM -0500, Laine Stump wrote:
>> This patch is a partial resolution to the following bug:
>>
>>     https://bugzilla.redhat.com/show_bug.cgi?id=667756
>>
>> (to complete the fix, an updated selinux-policy package is required,
>> to add the policy that allows libvirt to set the context of a fifo,
>> which was previously not allowed).
>>
>> Explanation : When an incoming migration is over a pipe (for example,
>> if the image was compressed and is being fed through gzip, or was on a
>> root-squash nfs server, so needed to be opened by a child process
>> running as a different uid), qemu cannot read it unless the selinux
>> context label for the pipe has been set properly.
>>
>> The solution is to check the fd used as the source of the migration
>> just before passing it to qemu; if it's a fifo (implying that it's a
>> pipe), we call the newly added virSecurityManagerSetFDLabel() function
>> to set the context properly.
>> ---
>>   src/qemu/qemu_driver.c |   18 ++++++++++++++++++
>>   1 files changed, 18 insertions(+), 0 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 34cc29f..985b062 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -2667,6 +2667,24 @@ static int qemudStartVMDaemon(virConnectPtr conn,
>>                                         vm, stdin_path)<  0)
>>           goto cleanup;
>>
>> +    if (stdin_fd != -1) {
>> +        /* if there's an fd to migrate from, and it's a pipe, put the
>> +         * proper security label on it
>> +         */
>> +        struct stat stdin_sb;
>> +
>> +        DEBUG0("setting security label on pipe used for migration");
>> +
>> +        if (fstat(stdin_fd,&stdin_sb)<  0) {
>> +            virReportSystemError(errno,
>> +                                 _("cannot stat fd %d"), stdin_fd);
>> +            goto cleanup;
>> +        }
>> +        if (S_ISFIFO(stdin_sb.st_mode)&&
>> +            virSecurityManagerSetFDLabel(driver->securityManager, vm, stdin_fd)<  0)
>> +            goto cleanup;
>> +    }
> This feels like the wrong place to put this call. The callers
> of qemudStartVMDaemon() which opened 'stdin_fd' in the first
> place will already know if it is a pipe or not. If we put
> the virSecurityManagerSetFDLabel call in the appropriate
> callers, then the fstat() complexity is avoided.

That was my first intent too. However, in the case of an image on 
root-squashed NFS, the knowledge about whether to directly open, or open 
via a pipe to a child process, is made in qemudOpenAsUID(), which 
doesn't have access to the domainObj, so cannot call the security driver.

In a broader view, qemudOpenAsUID() is really a potentially general 
purpose function that could be used outside of this context some day, 
but gumming it up with application-specific things like a domainObj 
would lock it into being specific to domain-related functions.

More specifically (and importantly), the domainObj hasn't even been 
constructed until after qemudOpenAsUID() is finished and returned, since 
it's going to be created by the caller from the header of the file that 
qemudOpenAsUID() has just opened. So by the time we have the domainObj, 
we no longer have the knowledge that the fd is actually the read side of 
a pipe - we would still have to call fstat (or clutter up the calling 
sequence to pass back an "is_fifo" bool or something, which sounds even 
less right).

Compared to that, putting the call to SetFDLabel() in a single place, 
qualified by fstat() to see if the fd was a fifo, seemed like a much 
less intrusive change to the code. (The other instance of a pipe being 
created (for compression) is less problematic, as everything we need is 
already there. However, since we're already doing the fstat() for the 
root-squash case, and since doing two FDSetLabels() would be superfluous 
(in the case of a compressed image stored on a root-squashed share), I 
figured we might as well have a single call in a common place (which, by 
the way, is strategically located as late as possible, so that any 
future additions of pipes will automatically be caught and accounted for).)

However, If anyone has a suggestion for dealing with the chicken-egg 
problem of domainObj vs fifo that is less ugly, please speak up, and 
I'll be happy to implement it! :-)

(NB: I have an idea in the back of my head that all files in libvirt 
could be opened by a VIR_OPEN() that would automatically handle "the 
root squash problem", possibly in some other manner than 
qemudOpenAsUID() (using SCM_RIGHTS maybe?), and in that case we wouldn't 
have the option of passing the security label down to the innards of the 
utility function even if we had it (unless we wanted to wire up every 
single VIR_OPEN() with an extra NULL arg or something).