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

[Daniel P. Berrange]

On Tue, Jan 25, 2011 at 03:54:12PM -0500, Laine Stump wrote:
> 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! :-)

Ok, I see what you mean here. ACK to the original patch

Daniel