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.

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 37ced2612dfac362baa4f3ce0b02347ad98d03b3..15bda52fffdb6bcc003ec5baae538c0533be3e8b 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/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"));

diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index 5844768667d2d67fb16be4841012a07a70ffbb46..0f9ff95e6aa93b82a98693ab4fd3c79a81f5823d 100644 (file)
--- a/src/security/security_apparmor.c
+++ b/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;
 }

diff --git a/src/security/security_driver.h b/src/security/security_driver.h
index 551496265630dcbae6716df14a6b7909c76a859a..97c900202ca2eace7871f14ae40d51bbb7b2db55 100644 (file)
--- a/src/security/security_driver.h
+++ b/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);

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index cb585ede85d93eb8e9a17d7cfbbaa8196ba1c681..c48f4c8af53872b5afb116ce34e737d41e1cbc89 100644 (file)
--- a/src/security/security_selinux.c
+++ b/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)