[PATCH v1 10/10] security_dac: Keep original label

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
---
 src/Makefile.am             |   1 +
 src/security/security_dac.c | 103 ++++++++++++++++++++++++++++++++++++++------
 2 files changed, 92 insertions(+), 12 deletions(-)

diff --git a/src/Makefile.am b/src/Makefile.am
index 90a51f6..83c8fcb 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -2631,6 +2631,7 @@ libvirt_lxc_LDADD =			\
 		libvirt_security_manager.la \
 		libvirt_conf.la \
 		libvirt_util.la \
+		libvirt_driver.la \
 		../gnulib/lib/libgnu.la
 if WITH_DTRACE_PROBES
 libvirt_lxc_LDADD += libvirt_probes.lo
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 7f69d86..2af013e 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -23,6 +23,7 @@
 #include <sys/stat.h>
 #include <fcntl.h>
 
+#include "domain_lock.h"
 #include "security_dac.h"
 #include "virerror.h"
 #include "virfile.h"
@@ -312,9 +313,56 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
                            gid_t gid)
 {
     virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
+    virLockManagerPluginPtr lockPlugin = virSecurityManagerGetLockPlugin(mgr);
+    int ret = -1;
+    bool call_chown = true;
+    bool recall_label = false;
+    struct stat sb;
+    char *original_label;
 
-    /* XXX record previous ownership */
-    return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid);
+    if (src && virStorageSourceIsLocalStorage(src))
+        path = src->path;
+
+    if (path) {
+        if (stat(path, &sb) < 0) {
+            virReportSystemError(errno,
+                                 _("unable to stat: %p"),
+                                 path);
+            goto cleanup;
+        }
+
+        if (lockPlugin) {
+            if (virAsprintf(&original_label, "+%d:%+d", sb.st_uid, sb.st_gid) < 0)
+                goto cleanup;
+
+            VIR_DEBUG("Remembering label '%s' for '%s' model on path '%s'",
+                      original_label, SECURITY_DAC_NAME, path);
+
+            if (virDomainLockRememberSeclabel(lockPlugin, path,
+                                              SECURITY_DAC_NAME,
+                                              original_label) < 0) {
+                VIR_FREE(original_label);
+                goto cleanup;
+            }
+
+            recall_label = true;
+        }
+        call_chown = sb.st_uid != uid || sb.st_gid != gid;
+    }
+
+    if (call_chown &&
+        virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid) < 0)
+        goto cleanup;
+
+    ret = 0;
+ cleanup:
+    if (ret < 0 && recall_label) {
+        /* Setting label wen't wrong, but we've remembered the label.
+         * Recall it so internal refcount stays in sync. */
+        virDomainLockRecallSeclabel(lockPlugin, path,
+                                    SECURITY_DAC_NAME, NULL);
+    }
+    return ret;
 }
 
 
@@ -324,11 +372,50 @@ virSecurityDACRestoreSecurityFileLabelInternal(virSecurityManagerPtr mgr,
                                                const char *path)
 {
     virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
+    virLockManagerPluginPtr lockPlugin = virSecurityManagerGetLockPlugin(mgr);
+    int ret = -1;
+    bool call_chown = true;
+    uid_t uid = 0;  /* By default return to root:root */
+    gid_t gid = 0;
+
     VIR_INFO("Restoring DAC user and group on '%s'",
              NULLSTR(src ? src->path : path));
 
-    /* XXX recall previous ownership */
-    return virSecurityDACSetOwnershipInternal(priv, src, path, 0, 0);
+    if (src && virStorageSourceIsLocalStorage(src))
+        path = src->path;
+
+    if (lockPlugin && path) {
+        int rc;
+        char *original_label = NULL;
+
+        if ((rc = virDomainLockRecallSeclabel(lockPlugin, path, SECURITY_DAC_NAME, &original_label)) < 0) {
+            /* Intentionally just WARN here - the label may not exist yet */
+            VIR_WARN("Unable to get original label for %s", path);
+        } else if (rc > 0) {
+            /* Label exists, but refcount is still greater than zero. Don't restore label yet. */
+            call_chown = false;
+        } else {
+            /* Hooray, label exists and we have the honour to restore the original label. */
+            if (virParseOwnershipIds(original_label, &uid, &gid) < 0) {
+                /* Error already reported by helper */
+                VIR_FREE(original_label);
+                goto cleanup;
+            }
+            VIR_FREE(original_label);
+        }
+    }
+
+    if (call_chown) {
+        VIR_DEBUG("Restoring to %u:%u",
+                  (unsigned int) uid, (unsigned int) gid);
+
+        if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid) < 0)
+            goto cleanup;
+    }
+
+    ret = 0;
+ cleanup:
+    return ret;
 }
 
 
@@ -404,14 +491,6 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr,
     if (!priv->dynamicOwnership)
         return 0;
 
-    /* Don't restore labels on readoly/shared disks, because other VMs may
-     * still be accessing these. Alternatively we could iterate over all
-     * running domains and try to figure out if it is in use, but this would
-     * not work for clustered filesystems, since we can't see running VMs using
-     * the file on other nodes. Safest bet is thus to skip the restore step. */
-    if (src->readonly || src->shared)
-        return 0;
-
     secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
     if (secdef && !secdef->relabel)
         return 0;
-- 
1.8.5.5

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list