From 46e8dc710a03312e2744fec324937c00effaeec8 Mon Sep 17 00:00:00 2001
From: Laine Stump <laine@laine.org>
Date: Tue, 27 Sep 2011 14:04:53 -0400
Subject: [PATCH] security: properly chown/label bidirectional and
 unidirectional fifos

This patch fixes the regression with using named pipes for qemu serial
devices noted in:

  https://bugzilla.redhat.com/show_bug.cgi?id=740478

The problem was that, while new code in libvirt looks for a single
bidirectional fifo of the name given in the config, then relabels that
and continues without looking for / relabelling the two unidirectional
fifos named ${name}.in and ${name}.out, qemu looks in the opposite
order. So if the user had naively created all three fifos, libvirt
would relabel the bidirectional fifo to allow qemu access, but qemu
would attempt to use the two unidirectional fifos and fail (because it
didn't have proper permissions/rights).

This patch changes the order that libvirt looks for the fifos to match
what qemu does - first it looks for the dual fifos, then it looks for
the single bidirectional fifo. If it finds the dual unidirectional
fifos first, it labels/chowns them and ignores any possible
bidirectional fifo.

(Note commit d37c6a3a (which first appeared in libvirt-0.9.2) added
the code that checked for a bidirectional fifo. Prior to that commit,
bidirectional fifos for serial devices didn't work because libvirt
always required the ${name}.(in|out) fifos to exist, and qemu would
always prefer those.
---
 src/security/security_dac.c     | 30 ++++++++++++++++++------------
 src/security/security_selinux.c | 29 +++++++++++++++++------------
 2 files changed, 35 insertions(+), 24 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index af02236121..0e75319f8f 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -406,18 +406,19 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
         break;
 
     case VIR_DOMAIN_CHR_TYPE_PIPE:
-        if (virFileExists(dev->data.file.path)) {
-            if (virSecurityDACSetOwnership(dev->data.file.path, priv->user, priv->group) < 0)
-                goto done;
-        } else {
-            if ((virAsprintf(&in, "%s.in", dev->data.file.path) < 0) ||
-                (virAsprintf(&out, "%s.out", dev->data.file.path) < 0)) {
-                virReportOOMError();
+        if ((virAsprintf(&in, "%s.in", dev->data.file.path) < 0) ||
+            (virAsprintf(&out, "%s.out", dev->data.file.path) < 0)) {
+            virReportOOMError();
+            goto done;
+        }
+        if (virFileExists(in) && virFileExists(out)) {
+            if ((virSecurityDACSetOwnership(in, priv->user, priv->group) < 0) ||
+                (virSecurityDACSetOwnership(out, priv->user, priv->group) < 0)) {
                 goto done;
             }
-            if ((virSecurityDACSetOwnership(in, priv->user, priv->group) < 0) ||
-                (virSecurityDACSetOwnership(out, priv->user, priv->group) < 0))
-                goto done;
+        } else if (virSecurityDACSetOwnership(dev->data.file.path,
+                                              priv->user, priv->group) < 0) {
+            goto done;
         }
         ret = 0;
         break;
@@ -452,9 +453,14 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
             virReportOOMError();
             goto done;
         }
-        if ((virSecurityDACRestoreSecurityFileLabel(out) < 0) ||
-            (virSecurityDACRestoreSecurityFileLabel(in) < 0))
+        if (virFileExists(in) && virFileExists(out)) {
+            if ((virSecurityDACRestoreSecurityFileLabel(out) < 0) ||
+                (virSecurityDACRestoreSecurityFileLabel(in) < 0)) {
             goto done;
+            }
+        } else if (virSecurityDACRestoreSecurityFileLabel(dev->data.file.path) < 0) {
+            goto done;
+        }
         ret = 0;
         break;
 
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 0807a34c63..e1a257d183 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -806,18 +806,18 @@ SELinuxSetSecurityChardevLabel(virDomainObjPtr vm,
         break;
 
     case VIR_DOMAIN_CHR_TYPE_PIPE:
-        if (virFileExists(dev->data.file.path)) {
-            if (SELinuxSetFilecon(dev->data.file.path, secdef->imagelabel) < 0)
-                goto done;
-        } else {
-            if ((virAsprintf(&in, "%s.in", dev->data.file.path) < 0) ||
-                (virAsprintf(&out, "%s.out", dev->data.file.path) < 0)) {
-                virReportOOMError();
+        if ((virAsprintf(&in, "%s.in", dev->data.file.path) < 0) ||
+            (virAsprintf(&out, "%s.out", dev->data.file.path) < 0)) {
+            virReportOOMError();
+            goto done;
+        }
+        if (virFileExists(in) && virFileExists(out)) {
+            if ((SELinuxSetFilecon(in, secdef->imagelabel) < 0) ||
+                (SELinuxSetFilecon(out, secdef->imagelabel) < 0)) {
                 goto done;
             }
-            if ((SELinuxSetFilecon(in, secdef->imagelabel) < 0) ||
-                (SELinuxSetFilecon(out, secdef->imagelabel) < 0))
-                goto done;
+        } else if (SELinuxSetFilecon(dev->data.file.path, secdef->imagelabel) < 0) {
+            goto done;
         }
         ret = 0;
         break;
@@ -858,9 +858,14 @@ SELinuxRestoreSecurityChardevLabel(virDomainObjPtr vm,
             virReportOOMError();
             goto done;
         }
-        if ((SELinuxRestoreSecurityFileLabel(out) < 0) ||
-            (SELinuxRestoreSecurityFileLabel(in) < 0))
+        if (virFileExists(in) && virFileExists(out)) {
+            if ((SELinuxRestoreSecurityFileLabel(out) < 0) ||
+                (SELinuxRestoreSecurityFileLabel(in) < 0)) {
+                goto done;
+            }
+        } else if (SELinuxRestoreSecurityFileLabel(dev->data.file.path) < 0) {
             goto done;
+        }
         ret = 0;
         break;