Re: [libvirt] [PATCH 01/12] 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 | 40 ++++++++++++------- src/security/security_apparmor.c | 46 +++++++++++++++------- src/security/security_driver.h | 2 + src/security/security_selinux.c | 78 ++++++++++++++++++++++++++++---------- 4 files changed, 117 insertions(+), 49 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2d80774..446f6ff 100644 --- 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; @@ -2441,11 +2440,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; } @@ -2758,8 +2760,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; @@ -3054,7 +3055,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 */ @@ -4166,7 +4168,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; @@ -4296,7 +4298,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 5844768..0f9ff95 100644 --- 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 5514962..97c9002 100644 --- 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 cb585ed..c48f4c8 100644 --- 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)