Make security drivers responsible for checking dynamic vs static labelling

The QEMU driver is doing 90% of the calls to check for static vs
dynamic labelling. Except it is forgetting todo so in many places,
in particular hotplug is mistakenly assigning disk labels. Move
all this logic into the security drivers themselves, so the HV
drivers don't have to think about it.

* src/security/security_driver.h: Add virDomainObjPtr parameter
  to virSecurityDomainRestoreHostdevLabel and to
  virSecurityDomainRestoreSavedStateLabel
* src/security/security_selinux.c, src/security/security_apparmor.c:
  Add explicit checks for VIR_DOMAIN_SECLABEL_STATIC and skip all
  chcon() code in those cases
* src/qemu/qemu_driver.c: Remove all checks for VIR_DOMAIN_SECLABEL_STATIC
  or VIR_DOMAIN_SECLABEL_DYNAMIC. Add missing checks for possibly NULL
  driver entry points.

src/qemu/qemu_driver.c

@@ -855,8 +855,7 @@ qemuReconnectDomain(void *payload, const char *name ATTRIBUTE_UNUSED, void *opaq
         goto error;
     }
 
-    if (obj->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC &&
-        driver->securityDriver &&
+    if (driver->securityDriver &&
         driver->securityDriver->domainReserveSecurityLabel &&
         driver->securityDriver->domainReserveSecurityLabel(NULL, obj) < 0)
         goto error;
@@ -2448,11 +2447,14 @@ cleanup:
 
 static int qemudDomainSetSecurityLabel(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm)
 {
-    if (vm->def->seclabel.label != NULL)
-        if (driver->securityDriver && driver->securityDriver->domainSetSecurityLabel)
-            return driver->securityDriver->domainSetSecurityLabel(conn, driver->securityDriver,
-                                                                 vm);
-    return 0;
+    int rc = 0;
+
+    if (driver->securityDriver &&
+        driver->securityDriver->domainSetSecurityLabel &&
+        driver->securityDriver->domainSetSecurityLabel(conn, driver->securityDriver, vm) < 0)
+        rc = -1;
+
+    return rc;
 }
 
 
@@ -2765,8 +2767,7 @@ static int qemudStartVMDaemon(virConnectPtr conn,
 
     /* If you are using a SecurityDriver with dynamic labelling,
        then generate a security label for isolation */
-    if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC &&
-        driver->securityDriver &&
+    if (driver->securityDriver &&
         driver->securityDriver->domainGenSecurityLabel &&
         driver->securityDriver->domainGenSecurityLabel(conn, vm) < 0)
         return -1;
@@ -3061,7 +3062,8 @@ static void qemudShutdownVMDaemon(virConnectPtr conn,
     virKillProcess(vm->pid, SIGKILL);
 
     /* Reset Security Labels */
-    if (driver->securityDriver)
+    if (driver->securityDriver &&
+        driver->securityDriver->domainRestoreSecurityLabel)
         driver->securityDriver->domainRestoreSecurityLabel(conn, vm);
 
     /* Clear out dynamically assigned labels */
@@ -4165,7 +4167,7 @@ static int qemudDomainSave(virDomainPtr dom,
 
     if (driver->securityDriver &&
         driver->securityDriver->domainRestoreSavedStateLabel &&
-        driver->securityDriver->domainRestoreSavedStateLabel(dom->conn, path) == -1)
+        driver->securityDriver->domainRestoreSavedStateLabel(dom->conn, vm, path) == -1)
         goto endjob;
 
     ret = 0;
@@ -4295,7 +4297,7 @@ static int qemudDomainCoreDump(virDomainPtr dom,
 
     if (driver->securityDriver &&
         driver->securityDriver->domainRestoreSavedStateLabel &&
-        driver->securityDriver->domainRestoreSavedStateLabel(dom->conn, path) == -1)
+        driver->securityDriver->domainRestoreSavedStateLabel(dom->conn, vm, path) == -1)
         goto endjob;
 
 endjob:
@@ -5871,6 +5873,7 @@ static int qemudDomainAttachHostDevice(virConnectPtr conn,
     if (qemuDomainSetDeviceOwnership(conn, driver, dev, 0) < 0)
         return -1;
     if (driver->securityDriver &&
+        driver->securityDriver->domainSetSecurityHostdevLabel &&
         driver->securityDriver->domainSetSecurityHostdevLabel(conn, vm, dev->data.hostdev) < 0)
         return -1;
 
@@ -5947,7 +5950,8 @@ static int qemudDomainAttachDevice(virDomainPtr dom,
         switch (dev->data.disk->device) {
         case VIR_DOMAIN_DISK_DEVICE_CDROM:
         case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
-            if (driver->securityDriver)
+            if (driver->securityDriver &&
+                driver->securityDriver->domainSetSecurityImageLabel)
                 driver->securityDriver->domainSetSecurityImageLabel(dom->conn, vm, dev->data.disk);
 
             if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 0) < 0)
@@ -5957,7 +5961,8 @@ static int qemudDomainAttachDevice(virDomainPtr dom,
             break;
 
         case VIR_DOMAIN_DISK_DEVICE_DISK:
-            if (driver->securityDriver)
+            if (driver->securityDriver &&
+                driver->securityDriver->domainSetSecurityImageLabel)
                 driver->securityDriver->domainSetSecurityImageLabel(dom->conn, vm, dev->data.disk);
 
             if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 0) < 0)
@@ -6347,6 +6352,7 @@ static int qemudDomainDetachHostDevice(virConnectPtr conn,
     }
 
     if (driver->securityDriver &&
+        driver->securityDriver->domainSetSecurityHostdevLabel &&
         driver->securityDriver->domainSetSecurityHostdevLabel(conn, vm, dev->data.hostdev) < 0)
         VIR_WARN0("Failed to restore device labelling");
 
@@ -6392,7 +6398,8 @@ static int qemudDomainDetachDevice(virDomainPtr dom,
         dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_DISK &&
         dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) {
         ret = qemudDomainDetachPciDiskDevice(dom->conn, driver, vm, dev);
-        if (driver->securityDriver)
+        if (driver->securityDriver &&
+            driver->securityDriver->domainRestoreSecurityImageLabel)
             driver->securityDriver->domainRestoreSecurityImageLabel(dom->conn, vm, dev->data.disk);
         if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 1) < 0)
             VIR_WARN0("Fail to restore disk device ownership");
@@ -6409,6 +6416,9 @@ static int qemudDomainDetachDevice(virDomainPtr dom,
         }
     } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) {
         ret = qemudDomainDetachHostDevice(dom->conn, driver, vm, dev);
+        if (driver->securityDriver &&
+            driver->securityDriver->domainRestoreSecurityHostdevLabel)
+            driver->securityDriver->domainRestoreSecurityHostdevLabel(dom->conn, vm, dev->data.hostdev);
     } else {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
                          "%s", _("This type of device cannot be hot unplugged"));

src/security/security_apparmor.c

@@ -327,6 +327,9 @@ AppArmorGenSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
     int rc = -1;
     char *profile_name = NULL;
 
+    if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC)
+        return 0;
+
     if ((vm->def->seclabel.label) ||
         (vm->def->seclabel.model) || (vm->def->seclabel.imagelabel)) {
         virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
@@ -425,7 +428,7 @@ AppArmorRestoreSecurityLabel(virConnectPtr conn, virDomainObjPtr vm)
     const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
     int rc = 0;
 
-    if (secdef->imagelabel) {
+    if (secdef->type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
         if ((rc = remove_profile(secdef->label)) != 0) {
             virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
                                    _("could not remove profile for \'%s\'"),
@@ -486,21 +489,23 @@ AppArmorRestoreSecurityImageLabel(virConnectPtr conn,
     int rc = -1;
     char *profile_name = NULL;
 
-    if (secdef->imagelabel) {
-        if ((profile_name = get_profile_name(conn, vm)) == NULL)
-            return rc;
+    if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+        return 0;
 
-        /* Update the profile only if it is loaded */
-        if (profile_loaded(secdef->imagelabel) >= 0) {
-            if (load_profile(conn, secdef->imagelabel, vm, NULL) < 0) {
-                virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
-                                       _("cannot update AppArmor profile "
-                                       "\'%s\'"),
-                                       secdef->imagelabel);
-                goto clean;
-            }
+    if ((profile_name = get_profile_name(conn, vm)) == NULL)
+        return rc;
+
+    /* Update the profile only if it is loaded */
+    if (profile_loaded(secdef->imagelabel) >= 0) {
+        if (load_profile(conn, secdef->imagelabel, vm, NULL) < 0) {
+            virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                                   _("cannot update AppArmor profile "
+                                     "\'%s\'"),
+                                   secdef->imagelabel);
+            goto clean;
         }
     }
+
     rc = 0;
   clean:
     VIR_FREE(profile_name);
@@ -517,6 +522,9 @@ AppArmorSetSecurityImageLabel(virConnectPtr conn,
     int rc = -1;
     char *profile_name;
 
+    if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+        return 0;
+
     if (!disk->src)
         return 0;
 
@@ -576,19 +584,29 @@ AppArmorReserveSecurityLabel(virConnectPtr conn ATTRIBUTE_UNUSED,
 
 static int
 AppArmorSetSecurityHostdevLabel(virConnectPtr conn ATTRIBUTE_UNUSED,
-                                virDomainObjPtr vm ATTRIBUTE_UNUSED,
+                                virDomainObjPtr vm,
                                 virDomainHostdevDefPtr dev ATTRIBUTE_UNUSED)
 
 {
+    const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
+
+    if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+        return 0;
+
     /* TODO: call load_profile with an update vm->def */
     return 0;
 }
 
 static int
 AppArmorRestoreSecurityHostdevLabel(virConnectPtr conn ATTRIBUTE_UNUSED,
+                                    virDomainObjPtr vm,
                                     virDomainHostdevDefPtr dev ATTRIBUTE_UNUSED)
 
 {
+    const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
+    if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+        return 0;
+
     /* TODO: call load_profile (needs virDomainObjPtr vm) */
     return 0;
 }

src/security/security_driver.h

@@ -38,6 +38,7 @@ typedef int (*virSecurityDomainSetImageLabel) (virConnectPtr conn,
                                                virDomainObjPtr vm,
                                                virDomainDiskDefPtr disk);
 typedef int (*virSecurityDomainRestoreHostdevLabel) (virConnectPtr conn,
+                                                     virDomainObjPtr vm,
                                                      virDomainHostdevDefPtr dev);
 typedef int (*virSecurityDomainSetHostdevLabel) (virConnectPtr conn,
                                                  virDomainObjPtr vm,
@@ -46,6 +47,7 @@ typedef int (*virSecurityDomainSetSavedStateLabel) (virConnectPtr conn,
                                                     virDomainObjPtr vm,
                                                     const char *savefile);
 typedef int (*virSecurityDomainRestoreSavedStateLabel) (virConnectPtr conn,
+                                                        virDomainObjPtr vm,
                                                         const char *savefile);
 typedef int (*virSecurityDomainGenLabel) (virConnectPtr conn,
                                           virDomainObjPtr sec);

src/security/security_selinux.c

@@ -165,6 +165,9 @@ SELinuxGenSecurityLabel(virConnectPtr conn,
     int c1 = 0;
     int c2 = 0;
 
+    if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC)
+        return 0;
+
     if (vm->def->seclabel.label ||
         vm->def->seclabel.model ||
         vm->def->seclabel.imagelabel) {
@@ -225,6 +228,9 @@ SELinuxReserveSecurityLabel(virConnectPtr conn,
     context_t ctx = NULL;
     const char *mcs;
 
+    if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC)
+        return 0;
+
     if (getpidcon(vm->pid, &pctx) == -1) {
         virReportSystemError(conn, errno,
                              _("unable to get PID %d security context"), vm->pid);
@@ -376,9 +382,14 @@ err:
 
 static int
 SELinuxRestoreSecurityImageLabel(virConnectPtr conn,
-                                 virDomainObjPtr vm ATTRIBUTE_UNUSED,
+                                 virDomainObjPtr vm,
                                  virDomainDiskDefPtr disk)
 {
+    const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
+
+    if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+        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
@@ -405,6 +416,9 @@ SELinuxSetSecurityImageLabel(virConnectPtr conn,
     const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
     const char *path;
 
+    if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+        return 0;
+
     if (!disk->src)
         return 0;
 
@@ -474,8 +488,12 @@ SELinuxSetSecurityHostdevLabel(virConnectPtr conn,
                                virDomainHostdevDefPtr dev)
 
 {
+    const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
     int ret = -1;
 
+    if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+        return 0;
+
     if (dev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
         return 0;
 
@@ -541,11 +559,16 @@ SELinuxRestoreSecurityUSBLabel(virConnectPtr conn,
 
 static int
 SELinuxRestoreSecurityHostdevLabel(virConnectPtr conn,
+                                   virDomainObjPtr vm,
                                    virDomainHostdevDefPtr dev)
 
 {
+    const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
     int ret = -1;
 
+    if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+        return 0;
+
     if (dev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
         return 0;
 
@@ -601,25 +624,28 @@ SELinuxRestoreSecurityLabel(virConnectPtr conn,
 
     VIR_DEBUG("Restoring security label on %s", vm->def->name);
 
-    if (secdef->imagelabel) {
-        for (i = 0 ; i < vm->def->nhostdevs ; i++) {
-            if (SELinuxRestoreSecurityHostdevLabel(conn, vm->def->hostdevs[i]) < 0)
-                rc = -1;
-        }
-        for (i = 0 ; i < vm->def->ndisks ; i++) {
-            if (SELinuxRestoreSecurityImageLabel(conn, vm,
-                                                 vm->def->disks[i]) < 0)
-                rc = -1;
-        }
-        VIR_FREE(secdef->model);
-        VIR_FREE(secdef->label);
-        context_t con = context_new(secdef->imagelabel);
-        if (con) {
-            mcsRemove(context_range_get(con));
-            context_free(con);
-        }
-        VIR_FREE(secdef->imagelabel);
+    if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+        return 0;
+
+    for (i = 0 ; i < vm->def->nhostdevs ; i++) {
+        if (SELinuxRestoreSecurityHostdevLabel(conn, vm, vm->def->hostdevs[i]) < 0)
+            rc = -1;
     }
+    for (i = 0 ; i < vm->def->ndisks ; i++) {
+        if (SELinuxRestoreSecurityImageLabel(conn, vm,
+                                             vm->def->disks[i]) < 0)
+            rc = -1;
+    }
+    context_t con = context_new(secdef->label);
+    if (con) {
+        mcsRemove(context_range_get(con));
+        context_free(con);
+    }
+
+    VIR_FREE(secdef->model);
+    VIR_FREE(secdef->label);
+    VIR_FREE(secdef->imagelabel);
+
     return rc;
 }
 
@@ -631,14 +657,23 @@ SELinuxSetSavedStateLabel(virConnectPtr conn,
 {
     const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
 
+    if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+        return 0;
+
     return SELinuxSetFilecon(conn, savefile, secdef->imagelabel);
 }
 
 
 static int
 SELinuxRestoreSavedStateLabel(virConnectPtr conn,
+                              virDomainObjPtr vm,
                               const char *savefile)
 {
+    const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
+
+    if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC)
+        return 0;
+
     return SELinuxRestoreSecurityFileLabel(conn, savefile);
 }
 
@@ -666,6 +701,9 @@ SELinuxSetSecurityLabel(virConnectPtr conn,
     const virSecurityLabelDefPtr secdef = &vm->def->seclabel;
     int i;
 
+    if (vm->def->seclabel.label == NULL)
+        return 0;
+
     if (!STREQ(drv->name, secdef->model)) {
         virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
                                _("security label driver mismatch: "
@@ -684,7 +722,7 @@ SELinuxSetSecurityLabel(virConnectPtr conn,
             return -1;
     }
 
-    if (secdef->imagelabel) {
+    if (secdef->type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
         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)