Refactor setup & cleanup of security labels in security driver

The current security driver architecture has the following
split of logic

 * domainGenSecurityLabel

    Allocate the unique label for the domain about to be started

 * domainGetSecurityLabel

    Retrieve the current live security label for a process

 * domainSetSecurityLabel

    Apply the previously allocated label to the current process
    Setup all disk image / device labelling

 * domainRestoreSecurityLabel

    Restore the original disk image / device labelling.
    Release the unique label for the domain

The 'domainSetSecurityLabel' method is special because it runs
in the context of the child process between the fork + exec.

This is require in order to set the process label. It is not
required in order to label disks/devices though. Having the
disk labelling code run in the child process limits what it
can do.

In particularly libvirtd would like to remember the current
disk image label, and only change shared image labels for the
first VM to start. This requires use & update of global state
in the libvirtd daemon, and thus cannot run in the child
process context.

The solution is to split domainSetSecurityLabel into two parts,
one applies process label, and the other handles disk image
labelling. At the same time domainRestoreSecurityLabel is
similarly split, just so that it matches the style. Thus the
previous 4 methods are replaced by the following 6 new methods

 * domainGenSecurityLabel

    Allocate the unique label for the domain about to be started
    No actual change here.

 * domainReleaseSecurityLabel

   Release the unique label for the domain

 * domainGetSecurityProcessLabel

   Retrieve the current live security label for a process
   Merely renamed for clarity.

 * domainSetSecurityProcessLabel

   Apply the previously allocated label to the current process

 * domainRestoreSecurityAllLabel

    Restore the original disk image / device labelling.

 * domainSetSecurityAllLabel

    Setup all disk image / device labelling

The SELinux and AppArmour drivers are then updated to comply with
this new spec. Notice that the AppArmour driver was actually a
little different. It was creating its profile for the disk image
and device labels in the 'domainGenSecurityLabel' method, where as
the SELinux driver did it in 'domainSetSecurityLabel'. With the
new method split, we can have consistency, with both drivers doing
that in the domainSetSecurityAllLabel method.

NB, the AppArmour changes here haven't been compiled so may not
build.

src/qemu/qemu_driver.c

@@ -2450,8 +2450,14 @@ static int qemudDomainSetSecurityLabel(virConnectPtr conn, struct qemud_driver *
     int rc = 0;
 
     if (driver->securityDriver &&
-        driver->securityDriver->domainSetSecurityLabel &&
+        driver->securityDriver->domainSetSecurityAllLabel &&
-        driver->securityDriver->domainSetSecurityLabel(conn, driver->securityDriver, vm) < 0)
+        driver->securityDriver->domainSetSecurityAllLabel(conn, vm) < 0)
+        rc = -1;
+
+    if (rc == 0 &&
+        driver->securityDriver &&
+        driver->securityDriver->domainSetSecurityProcessLabel &&
+        driver->securityDriver->domainSetSecurityProcessLabel(conn, driver->securityDriver, vm) < 0)
         rc = -1;
 
     return rc;
@@ -3063,8 +3069,11 @@ static void qemudShutdownVMDaemon(virConnectPtr conn,
 
     /* Reset Security Labels */
     if (driver->securityDriver &&
-        driver->securityDriver->domainRestoreSecurityLabel)
+        driver->securityDriver->domainRestoreSecurityAllLabel)
-        driver->securityDriver->domainRestoreSecurityLabel(conn, vm);
+        driver->securityDriver->domainRestoreSecurityAllLabel(conn, vm);
+    if (driver->securityDriver &&
+        driver->securityDriver->domainReleaseSecurityLabel)
+        driver->securityDriver->domainReleaseSecurityLabel(conn, vm);
 
     /* Clear out dynamically assigned labels */
     if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
@@ -4632,8 +4641,8 @@ static int qemudDomainGetSecurityLabel(virDomainPtr dom, virSecurityLabelPtr sec
      *   QEMU monitor hasn't seen SIGHUP/ERR on poll().
      */
     if (virDomainObjIsActive(vm)) {
-        if (driver->securityDriver && driver->securityDriver->domainGetSecurityLabel) {
+        if (driver->securityDriver && driver->securityDriver->domainGetSecurityProcessLabel) {
-            if (driver->securityDriver->domainGetSecurityLabel(dom->conn, vm, seclabel) == -1) {
+            if (driver->securityDriver->domainGetSecurityProcessLabel(dom->conn, vm, seclabel) == -1) {
                 qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
                                  "%s", _("Failed to get security label"));
                 goto cleanup;

src/security/security_apparmor.c

@@ -341,16 +341,6 @@ AppArmorGenSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
     if ((profile_name = get_profile_name(conn, vm)) == NULL)
         return rc;
 
-    /* if the profile is not already loaded, then load one */
-    if (profile_loaded(profile_name) < 0) {
-        if (load_profile(conn, profile_name, vm, NULL) < 0) {
-            virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
-                                   _("cannot generate AppArmor profile "
-                                   "\'%s\'"), profile_name);
-            goto clean;
-        }
-    }
-
     vm->def->seclabel.label = strndup(profile_name, strlen(profile_name));
     if (!vm->def->seclabel.label) {
         virReportOOMError(NULL);
@@ -375,7 +365,6 @@ AppArmorGenSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
     goto clean;
 
   err:
-    remove_profile(profile_name);
     VIR_FREE(vm->def->seclabel.label);
     VIR_FREE(vm->def->seclabel.imagelabel);
     VIR_FREE(vm->def->seclabel.model);
@@ -386,12 +375,33 @@ AppArmorGenSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
     return rc;
 }
 
+static int
+AppArmorSetSecurityAllLabel(virConnectPtr conn, virDomainObjPtr vm)
+{
+    int rc = -1;
+
+    if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC)
+        return 0;
+
+    /* if the profile is not already loaded, then load one */
+    if (profile_loaded(vm->def->seclabel.label) < 0) {
+        if (load_profile(conn, vm->def->seclabel.label, vm, NULL) < 0) {
+            virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                                   _("cannot generate AppArmor profile "
+                                   "\'%s\'"), vm->def->seclabel.label);
+            return -1;
+        }
+    }
+
+    return 0;
+}
+
 /* Seen with 'virsh dominfo <vm>'. This function only called if the VM is
  * running.
  */
 static int
-AppArmorGetSecurityLabel(virConnectPtr conn,
+AppArmorGetSecurityProcessLabel(virConnectPtr conn,
-                         virDomainObjPtr vm, virSecurityLabelPtr sec)
+                                virDomainObjPtr vm, virSecurityLabelPtr sec)
 {
     int rc = -1;
     char *profile_name = NULL;
@@ -423,7 +433,20 @@ AppArmorGetSecurityLabel(virConnectPtr conn,
  * more details. Currently called via qemudShutdownVMDaemon.
  */
 static int
-AppArmorRestoreSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
+AppArmorReleaseSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
+{
+    const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
+
+    VIR_FREE(secdef->model);
+    VIR_FREE(secdef->label);
+    VIR_FREE(secdef->imagelabel);
+
+    return 0;
+}
+
+
+static int
+AppArmorRestoreSecurityAllLabel(virConnectPtr conn, virDomainObjPtr vm)
 {
     const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
     int rc = 0;
@@ -434,9 +457,6 @@ AppArmorRestoreSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
                                    _("could not remove profile for \'%s\'"),
                                    secdef->label);
         }
-        VIR_FREE(secdef->model);
-        VIR_FREE(secdef->label);
-        VIR_FREE(secdef->imagelabel);
     }
     return rc;
 }
@@ -445,8 +465,8 @@ AppArmorRestoreSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
  * LOCAL_STATE_DIR/log/libvirt/qemu/<vm name>.log
  */
 static int
-AppArmorSetSecurityLabel(virConnectPtr conn,
+AppArmorSetSecurityProcessLabel(virConnectPtr conn,
-                         virSecurityDriverPtr drv, virDomainObjPtr vm)
+                                virSecurityDriverPtr drv, virDomainObjPtr vm)
 {
     const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
     int rc = -1;
@@ -620,9 +640,11 @@ virSecurityDriver virAppArmorSecurityDriver = {
     .domainRestoreSecurityImageLabel = AppArmorRestoreSecurityImageLabel,
     .domainGenSecurityLabel = AppArmorGenSecurityLabel,
     .domainReserveSecurityLabel = AppArmorReserveSecurityLabel,
-    .domainGetSecurityLabel = AppArmorGetSecurityLabel,
+    .domainReleaseSecurityLabel = AppArmorReleaseSecurityLabel,
-    .domainRestoreSecurityLabel = AppArmorRestoreSecurityLabel,
+    .domainGetSecurityProcessLabel = AppArmorGetSecurityProcessLabel,
-    .domainSetSecurityLabel = AppArmorSetSecurityLabel,
+    .domainSetSecurityProcessLabel = AppArmorSetSecurityProcessLabel,
+    .domainRestoreSecurityAllLabel = AppArmorRestoreSecurityAllLabel,
+    .domainSetSecurityAllLabel = AppArmorSetSecurityAllLabel,
     .domainSetSecurityHostdevLabel = AppArmorSetSecurityHostdevLabel,
     .domainRestoreSecurityHostdevLabel = AppArmorRestoreSecurityHostdevLabel,
 };

src/security/security_driver.h

@@ -52,15 +52,19 @@ typedef int (*virSecurityDomainRestoreSavedStateLabel) (virConnectPtr conn,
 typedef int (*virSecurityDomainGenLabel) (virConnectPtr conn,
                                           virDomainObjPtr sec);
 typedef int (*virSecurityDomainReserveLabel) (virConnectPtr conn,
-                                           virDomainObjPtr sec);
+                                              virDomainObjPtr sec);
-typedef int (*virSecurityDomainGetLabel) (virConnectPtr conn,
+typedef int (*virSecurityDomainReleaseLabel) (virConnectPtr conn,
-                                          virDomainObjPtr vm,
+                                              virDomainObjPtr sec);
-                                          virSecurityLabelPtr sec);
+typedef int (*virSecurityDomainSetAllLabel) (virConnectPtr conn,
-typedef int (*virSecurityDomainRestoreLabel) (virConnectPtr conn,
+                                             virDomainObjPtr sec);
-                                              virDomainObjPtr vm);
+typedef int (*virSecurityDomainRestoreAllLabel) (virConnectPtr conn,
-typedef int (*virSecurityDomainSetLabel) (virConnectPtr conn,
+                                                 virDomainObjPtr vm);
-                                          virSecurityDriverPtr drv,
+typedef int (*virSecurityDomainGetProcessLabel) (virConnectPtr conn,
-                                          virDomainObjPtr vm);
+                                                 virDomainObjPtr vm,
+                                                 virSecurityLabelPtr sec);
+typedef int (*virSecurityDomainSetProcessLabel) (virConnectPtr conn,
+                                                 virSecurityDriverPtr drv,
+                                                 virDomainObjPtr vm);
 typedef int (*virSecurityDomainSecurityVerify) (virConnectPtr conn,
                                                 virDomainDefPtr def);
 
@@ -73,9 +77,11 @@ struct _virSecurityDriver {
     virSecurityDomainSetImageLabel domainSetSecurityImageLabel;
     virSecurityDomainGenLabel domainGenSecurityLabel;
     virSecurityDomainReserveLabel domainReserveSecurityLabel;
-    virSecurityDomainGetLabel domainGetSecurityLabel;
+    virSecurityDomainReleaseLabel domainReleaseSecurityLabel;
-    virSecurityDomainSetLabel domainSetSecurityLabel;
+    virSecurityDomainGetProcessLabel domainGetSecurityProcessLabel;
-    virSecurityDomainRestoreLabel domainRestoreSecurityLabel;
+    virSecurityDomainSetProcessLabel domainSetSecurityProcessLabel;
+    virSecurityDomainSetAllLabel domainSetSecurityAllLabel;
+    virSecurityDomainRestoreAllLabel domainRestoreSecurityAllLabel;
     virSecurityDomainRestoreHostdevLabel domainRestoreSecurityHostdevLabel;
     virSecurityDomainSetHostdevLabel domainSetSecurityHostdevLabel;
     virSecurityDomainSetSavedStateLabel domainSetSavedStateLabel;

src/security/security_selinux.c

@@ -277,9 +277,9 @@ SELinuxSecurityDriverOpen(virConnectPtr conn, virSecurityDriverPtr drv)
 }
 
 static int
-SELinuxGetSecurityLabel(virConnectPtr conn,
+SELinuxGetSecurityProcessLabel(virConnectPtr conn,
-                        virDomainObjPtr vm,
+                               virDomainObjPtr vm,
-                        virSecurityLabelPtr sec)
+                               virSecurityLabelPtr sec)
 {
     security_context_t ctx;
 
@@ -615,8 +615,8 @@ done:
 }
 
 static int
-SELinuxRestoreSecurityLabel(virConnectPtr conn,
+SELinuxRestoreSecurityAllLabel(virConnectPtr conn,
-                            virDomainObjPtr vm)
+                               virDomainObjPtr vm)
 {
     const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
     int i;
@@ -636,6 +636,19 @@ SELinuxRestoreSecurityLabel(virConnectPtr conn,
                                              vm->def->disks[i]) < 0)
             rc = -1;
     }
+
+    return rc;
+}
+
+static int
+SELinuxReleaseSecurityLabel(virConnectPtr conn ATTRIBUTE_UNUSED,
+                            virDomainObjPtr vm)
+{
+    const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
+
+    if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+        return 0;
+
     context_t con = context_new(secdef->label);
     if (con) {
         mcsRemove(context_range_get(con));
@@ -646,7 +659,7 @@ SELinuxRestoreSecurityLabel(virConnectPtr conn,
     VIR_FREE(secdef->label);
     VIR_FREE(secdef->imagelabel);
 
-    return rc;
+    return 0;
 }
 
 
@@ -693,13 +706,12 @@ SELinuxSecurityVerify(virConnectPtr conn, virDomainDefPtr def)
 }
 
 static int
-SELinuxSetSecurityLabel(virConnectPtr conn,
+SELinuxSetSecurityProcessLabel(virConnectPtr conn,
-                        virSecurityDriverPtr drv,
+                               virSecurityDriverPtr drv,
-                        virDomainObjPtr vm)
+                               virDomainObjPtr vm)
 {
     /* TODO: verify DOI */
     const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
-    int i;
 
     if (vm->def->seclabel.label == NULL)
         return 0;
@@ -722,18 +734,32 @@ SELinuxSetSecurityLabel(virConnectPtr conn,
             return -1;
     }
 
-    if (secdef->type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
+    return 0;
-        for (i = 0 ; i < vm->def->ndisks ; i++) {
+}
-            /* XXX fixme - we need to recursively label the entriy tree :-( */
+
-            if (vm->def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_DIR)
+static int
-                continue;
+SELinuxSetSecurityAllLabel(virConnectPtr conn,
-            if (SELinuxSetSecurityImageLabel(conn, vm, vm->def->disks[i]) < 0)
+                           virDomainObjPtr vm)
-                return -1;
+{
-        }
+    const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
-        for (i = 0 ; i < vm->def->nhostdevs ; i++) {
+    int i;
-            if (SELinuxSetSecurityHostdevLabel(conn, vm, vm->def->hostdevs[i]) < 0)
+
-                return -1;
+    if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+        return 0;
+
+    for (i = 0 ; i < vm->def->ndisks ; i++) {
+        /* XXX fixme - we need to recursively label the entire tree :-( */
+        if (vm->def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_DIR) {
+            VIR_WARN("Unable to relabel directory tree %s for disk %s",
+                     vm->def->disks[i]->src, vm->def->disks[i]->dst);
+            continue;
         }
+        if (SELinuxSetSecurityImageLabel(conn, vm, vm->def->disks[i]) < 0)
+            return -1;
+    }
+    for (i = 0 ; i < vm->def->nhostdevs ; i++) {
+        if (SELinuxSetSecurityHostdevLabel(conn, vm, vm->def->hostdevs[i]) < 0)
+            return -1;
     }
 
     return 0;
@@ -748,9 +774,11 @@ virSecurityDriver virSELinuxSecurityDriver = {
     .domainRestoreSecurityImageLabel = SELinuxRestoreSecurityImageLabel,
     .domainGenSecurityLabel     = SELinuxGenSecurityLabel,
     .domainReserveSecurityLabel     = SELinuxReserveSecurityLabel,
-    .domainGetSecurityLabel     = SELinuxGetSecurityLabel,
+    .domainReleaseSecurityLabel     = SELinuxReleaseSecurityLabel,
-    .domainRestoreSecurityLabel = SELinuxRestoreSecurityLabel,
+    .domainGetSecurityProcessLabel     = SELinuxGetSecurityProcessLabel,
-    .domainSetSecurityLabel     = SELinuxSetSecurityLabel,
+    .domainSetSecurityProcessLabel     = SELinuxSetSecurityProcessLabel,
+    .domainRestoreSecurityAllLabel = SELinuxRestoreSecurityAllLabel,
+    .domainSetSecurityAllLabel     = SELinuxSetSecurityAllLabel,
     .domainSetSecurityHostdevLabel = SELinuxSetSecurityHostdevLabel,
     .domainRestoreSecurityHostdevLabel = SELinuxRestoreSecurityHostdevLabel,
     .domainSetSavedStateLabel = SELinuxSetSavedStateLabel,