security_manager: Don't manipulate domain XML in virDomainDefGetSecurityLabelDef

The virDomainDefGetSecurityLabelDef was modifying the domain XML.
It tried to find a seclabel corresponding to given sec driver. If the
label wasn't found, the function created one which is wrong. In fact
it's security manager which should modify this part of domain XML.

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 2b2f35c50ac419aa7473b6934eb351576d3a9a5d..f3fca7f3a5b6e72074002a63637566df3ab54621 100644 (file)
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1001,7 +1001,7 @@ virDomainGraphicsListenDefClear(virDomainGraphicsListenDefPtr def)
     return;
 }
 
-static void
+void
 virSecurityLabelDefFree(virSecurityLabelDefPtr def)
 {
     if (!def)
@@ -1014,7 +1014,7 @@ virSecurityLabelDefFree(virSecurityLabelDefPtr def)
 }
 
 
-static void
+void
 virSecurityDeviceLabelDefFree(virSecurityDeviceLabelDefPtr def)
 {
     if (!def)
@@ -16626,10 +16626,6 @@ virDomainDefGetSecurityLabelDef(virDomainDefPtr def, const char *model)
             return def->seclabels[i];
     }
 
-    seclabel = virDomainDefAddSecurityLabelDef(def, model);
-    if (seclabel)
-        seclabel->implicit = true;
-
     return seclabel;
 }
 
@@ -16664,55 +16660,31 @@ virDomainChrDefGetSecurityLabelDef(virDomainChrDefPtr def, const char *model)
 }
 
 virSecurityLabelDefPtr
-virDomainDefAddSecurityLabelDef(virDomainDefPtr def, const char *model)
+virDomainDefGenSecurityLabelDef(const char *model)
 {
     virSecurityLabelDefPtr seclabel = NULL;
 
-    if (VIR_ALLOC(seclabel) < 0)
-        goto no_memory;
-
-    if (model) {
-        seclabel->model = strdup(model);
-        if (seclabel->model == NULL)
-            goto no_memory;
+    if (VIR_ALLOC(seclabel) < 0 ||
+        (model && !(seclabel->model = strdup(model)))) {
+        virReportOOMError();
+        virSecurityLabelDefFree(seclabel);
+        seclabel = NULL;
     }
 
-    if (VIR_EXPAND_N(def->seclabels, def->nseclabels, 1) < 0)
-        goto no_memory;
-
-    def->seclabels[def->nseclabels - 1] = seclabel;
-
     return seclabel;
-
-no_memory:
-    virReportOOMError();
-    virSecurityLabelDefFree(seclabel);
-    return NULL;
 }
 
 virSecurityDeviceLabelDefPtr
-virDomainDiskDefAddSecurityLabelDef(virDomainDiskDefPtr def, const char *model)
+virDomainDiskDefGenSecurityLabelDef(const char *model)
 {
     virSecurityDeviceLabelDefPtr seclabel = NULL;
 
-    if (VIR_ALLOC(seclabel) < 0)
-        goto no_memory;
-
-    if (model) {
-        seclabel->model = strdup(model);
-        if (seclabel->model == NULL)
-            goto no_memory;
+    if (VIR_ALLOC(seclabel) < 0 ||
+        (model && !(seclabel->model = strdup(model)))) {
+        virReportOOMError();
+        virSecurityDeviceLabelDefFree(seclabel);
+        seclabel = NULL;
     }
 
-    if (VIR_EXPAND_N(def->seclabels, def->nseclabels, 1) < 0)
-        goto no_memory;
-
-    def->seclabels[def->nseclabels - 1] = seclabel;
-
     return seclabel;
-
-no_memory:
-    virReportOOMError();
-    virSecurityDeviceLabelDefFree(seclabel);
-    return NULL;
 }

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index c3b26083c5493d52b92710ada2c8a6f1e89a7392..edddf2545d2a093d00cd7285a13e4d28f1f58645 100644 (file)
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2298,10 +2298,13 @@ virSecurityDeviceLabelDefPtr
 virDomainChrDefGetSecurityLabelDef(virDomainChrDefPtr def, const char *model);
 
 virSecurityLabelDefPtr
-virDomainDefAddSecurityLabelDef(virDomainDefPtr def, const char *model);
+virDomainDefGenSecurityLabelDef(const char *model);
 
 virSecurityDeviceLabelDefPtr
-virDomainDiskDefAddSecurityLabelDef(virDomainDiskDefPtr def, const char *model);
+virDomainDiskDefGenSecurityLabelDef(const char *model);
+
+void virSecurityLabelDefFree(virSecurityLabelDefPtr def);
+void virSecurityDeviceLabelDefFree(virSecurityDeviceLabelDefPtr def);
 
 typedef const char* (*virEventActionToStringFunc)(int type);
 typedef int (*virEventActionFromStringFunc)(const char *type);

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a2c4a54c9103f7d08e653290feb4755ed06e1fd3..58121235409b2b74425bbd41529882a08c5b78c2 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -108,7 +108,6 @@ virDomainControllerTypeToString;
 virDomainCpuPlacementModeTypeFromString;
 virDomainCpuPlacementModeTypeToString;
 virDomainDefAddImplicitControllers;
-virDomainDefAddSecurityLabelDef;
 virDomainDefCheckABIStability;
 virDomainDefClearCCWAddresses;
 virDomainDefClearDeviceAliases;

diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index c621366439c57a9508c069dc630ee82e5a7384dc..5c2a95b6990012b774404fc4aa7c94007135fb86 100644 (file)
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -424,24 +424,26 @@ int virSecurityManagerRestoreSavedStateLabel(virSecurityManagerPtr mgr,
 int virSecurityManagerGenLabel(virSecurityManagerPtr mgr,
                                virDomainDefPtr vm)
 {
-    int rc = 0;
+    int ret = -1;
     size_t i;
     virSecurityManagerPtr* sec_managers = NULL;
     virSecurityLabelDefPtr seclabel;
+    bool generated = false;
 
     if (mgr == NULL || mgr->drv == NULL)
-        return -1;
+        return ret;
 
     if ((sec_managers = virSecurityManagerGetNested(mgr)) == NULL)
-        return -1;
+        return ret;
 
     virObjectLock(mgr);
     for (i = 0; sec_managers[i]; i++) {
-        seclabel = virDomainDefGetSecurityLabelDef(vm,
-                                                   sec_managers[i]->drv->name);
-        if (seclabel == NULL) {
-            rc = -1;
-            goto cleanup;
+        generated = false;
+        seclabel = virDomainDefGetSecurityLabelDef(vm, sec_managers[i]->drv->name);
+        if (!seclabel) {
+            if (!(seclabel = virDomainDefGenSecurityLabelDef(sec_managers[i]->drv->name)))
+                goto cleanup;
+            generated = seclabel->implicit = true;
         }
 
         if (seclabel->type == VIR_DOMAIN_SECLABEL_DEFAULT) {
@@ -457,23 +459,37 @@ int virSecurityManagerGenLabel(virSecurityManagerPtr mgr,
             sec_managers[i]->requireConfined) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                            _("Unconfined guests are not allowed on this host"));
-            rc = -1;
             goto cleanup;
         }
 
         if (!sec_managers[i]->drv->domainGenSecurityLabel) {
             virReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__);
         } else {
-            rc += sec_managers[i]->drv->domainGenSecurityLabel(sec_managers[i], vm);
-            if (rc)
+            /* The seclabel must be added to @vm prior calling domainGenSecurityLabel
+             * which may require seclabel to be presented already */
+
+            if (VIR_APPEND_ELEMENT(vm->seclabels, vm->nseclabels, seclabel) < 0) {
+                virReportOOMError();
+                goto cleanup;
+            }
+
+            if (sec_managers[i]->drv->domainGenSecurityLabel(sec_managers[i], vm) < 0) {
+                if (VIR_DELETE_ELEMENT(vm->seclabels,
+                                       vm->nseclabels -1, vm->nseclabels) < 0)
+                    vm->nseclabels--;
                 goto cleanup;
+            }
         }
     }
 
+    ret = 0;
+
 cleanup:
     virObjectUnlock(mgr);
+    if (generated)
+        virSecurityLabelDefFree(seclabel);
     VIR_FREE(sec_managers);
-    return rc;
+    return ret;
 }
 
 int virSecurityManagerReserveLabel(virSecurityManagerPtr mgr,

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 1e0063758acd83401019dd5e265adddcc8adfa30..60596ad757267eaad73d517429614071faa88ad4 100644 (file)
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1161,11 +1161,15 @@ virSecuritySELinuxSetSecurityFileLabel(virDomainDiskDefPtr disk,
     if (ret == 1 && !disk_seclabel) {
         /* If we failed to set a label, but virt_use_nfs let us
          * proceed anyway, then we don't need to relabel later.  */
-        disk_seclabel =
-            virDomainDiskDefAddSecurityLabelDef(disk, SECURITY_SELINUX_NAME);
+        disk_seclabel = virDomainDiskDefGenSecurityLabelDef(SECURITY_SELINUX_NAME);
         if (!disk_seclabel)
             return -1;
         disk_seclabel->norelabel = true;
+        if (VIR_APPEND_ELEMENT(disk->seclabels, disk->nseclabels, disk_seclabel) < 0) {
+            virReportOOMError();
+            virSecurityDeviceLabelDefFree(disk_seclabel);
+            return -1;
+        }
         ret = 0;
     }
     return ret;