From 5214b2f1a3f06ac329424134979edde2bf988146 Mon Sep 17 00:00:00 2001
From: Michal Privoznik <mprivozn@redhat.com>
Date: Tue, 15 Jan 2019 11:15:19 +0100
Subject: [PATCH] security: Don't skip label restore on file systems lacking
 XATTRs
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The way that virSecurityDACRecallLabel is currently written is
that if XATTRs are not supported for given path to the caller
this is not different than if the path is still in use. The value
of 1 is returned which makes secdrivers skip label restore.
This is clearly a bug as we are not restoring labels on say NFS
even though previously we were.

Strictly speaking, changes to virSecurityDACRememberLabel are not
needed, but they are done anyway so that getter and setter behave
in the same fashion.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/security/security_dac.c     | 18 ++++++++++++------
 src/security/security_selinux.c | 21 +++++++++++++++------
 src/security/security_util.c    |  6 ++++--
 3 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 3c21dbbddb..300c383dd5 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -458,10 +458,11 @@ virSecurityDACRecallLabel(virSecurityDACDataPtr priv ATTRIBUTE_UNUSED,
 {
     char *label;
     int ret = -1;
+    int rv;
 
-    if (virSecurityGetRememberedLabel(SECURITY_DAC_NAME,
-                                      path, &label) < 0)
-        goto cleanup;
+    rv = virSecurityGetRememberedLabel(SECURITY_DAC_NAME, path, &label);
+    if (rv < 0)
+        return rv;
 
     if (!label)
         return 1;
@@ -760,7 +761,9 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
         }
 
         refcount = virSecurityDACRememberLabel(priv, path, sb.st_uid, sb.st_gid);
-        if (refcount < 0) {
+        if (refcount == -2) {
+            /* Not supported. Don't error though. */
+        } else if (refcount < 0) {
             return -1;
         } else if (refcount > 1) {
             /* Refcount is greater than 1 which means that there
@@ -827,10 +830,13 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr,
 
     if (recall && path) {
         rv = virSecurityDACRecallLabel(priv, path, &uid, &gid);
-        if (rv < 0)
+        if (rv == -2) {
+            /* Not supported. Don't error though. */
+        } else if (rv < 0) {
             return -1;
-        if (rv > 0)
+        } else if (rv > 0) {
             return 0;
+        }
     }
 
     VIR_INFO("Restoring DAC user and group on '%s' to %ld:%ld",
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index ec8d407351..ff54d47e23 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -207,9 +207,11 @@ static int
 virSecuritySELinuxRecallLabel(const char *path,
                               security_context_t *con)
 {
-    if (virSecurityGetRememberedLabel(SECURITY_SELINUX_NAME,
-                                      path, con) < 0)
-        return -1;
+    int rv;
+
+    rv = virSecurityGetRememberedLabel(SECURITY_SELINUX_NAME, path, con);
+    if (rv < 0)
+        return rv;
 
     if (!*con)
         return 1;
@@ -1337,7 +1339,9 @@ virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr,
 
         if (econ) {
             refcount = virSecuritySELinuxRememberLabel(path, econ);
-            if (refcount < 0) {
+            if (refcount == -2) {
+                /* Not supported. Don't error though. */
+            } else if (refcount < 0) {
                 goto cleanup;
             } else if (refcount > 1) {
                 /* Refcount is greater than 1 which means that there
@@ -1485,13 +1489,18 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr,
     }
 
     if (recall) {
-        if ((rc = virSecuritySELinuxRecallLabel(newpath, &fcon)) < 0) {
+        rc = virSecuritySELinuxRecallLabel(newpath, &fcon);
+        if (rc == -2) {
+            /* Not supported. Lookup the default label below. */
+        } else if (rc < 0) {
             goto cleanup;
         } else if (rc > 0) {
             ret = 0;
             goto cleanup;
         }
-    } else {
+    }
+
+    if (!recall || rc == -2) {
         if (stat(newpath, &buf) != 0) {
             VIR_WARN("cannot stat %s: %s", newpath,
                      virStrerror(errno, ebuf, sizeof(ebuf)));
diff --git a/src/security/security_util.c b/src/security/security_util.c
index f09a18a623..3c24d7cded 100644
--- a/src/security/security_util.c
+++ b/src/security/security_util.c
@@ -105,6 +105,7 @@ virSecurityGetRefCountAttrName(const char *name ATTRIBUTE_UNUSED)
  * zero) and returns zero.
  *
  * Returns: 0 on success,
+ *         -2 if underlying file system doesn't support XATTRs,
  *         -1 otherwise (with error reported)
  */
 int
@@ -125,7 +126,7 @@ virSecurityGetRememberedLabel(const char *name,
 
     if (virFileGetXAttrQuiet(path, ref_name, &value) < 0) {
         if (errno == ENOSYS || errno == ENODATA || errno == ENOTSUP) {
-            ret = 0;
+            ret = -2;
         } else {
             virReportSystemError(errno,
                                  _("Unable to get XATTR %s on %s"),
@@ -192,6 +193,7 @@ virSecurityGetRememberedLabel(const char *name,
  * See also virSecurityGetRememberedLabel.
  *
  * Returns: the new refcount value on success,
+ *         -2 if underlying file system doesn't support XATTRs,
  *         -1 otherwise (with error reported)
  */
 int
@@ -210,7 +212,7 @@ virSecuritySetRememberedLabel(const char *name,
 
     if (virFileGetXAttrQuiet(path, ref_name, &value) < 0) {
         if (errno == ENOSYS || errno == ENOTSUP) {
-            ret = 0;
+            ret = -2;
             goto cleanup;
         } else if (errno != ENODATA) {
             virReportSystemError(errno,
-- 
GitLab