[libvirt] [PATCH 0/12] Improve security driver handling & QEMU DAC management

older
[libvirt] [PATCH] storage: Replace...

Daniel P. Berrange

20 Jan 2010 20 Jan '10

3:14 p.m.

This patch series does some work on te security drivers, and the QEMU code for managing DAC permissions on files. The core goal is to turn the QEMU driver DAC file management code into a security driver. Instead of QEMU calling into the SELinux/AppArmour drivers directly, a stacked driver module is introduced. This delegates all operations to first the QEMU DAC driver, and then the main SELinux/AppArmour driver. The end result is that all the permissions management code is removed from the QEMU driver, and we're left with just simple security driver calls. In the process of this a number of flaws in the current hotplug code were found, and code was generally tidied up with a view to making it easier to manage. Finally, we add the ability to turn off the QEMU DAC file managment code, and also deal gracefully with failures to change ownership (eg on NFS with root squash, or readonly FS).

Show replies by date

Daniel P. Berrange

20 Jan 20 Jan

3:14 p.m.

New subject: [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) -- 1.6.5.2

Daniel Veillard

3:43 p.m.

New subject: [libvirt] [PATCH 01/12] Make security drivers responsible for checking dynamic vs static labelling

On Wed, Jan 20, 2010 at 03:14:58PM +0000, Daniel P. Berrange wrote:

...

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)

ACK, also adds some missing check for some driver->securityDriver entry point. My only concern is dereferencing vm->def->seclabel without any check for NULL, I would feel better if that extra safety belt was added, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

Daniel Veillard

3:59 p.m.

New subject: [libvirt] [PATCH 01/12] Make security drivers responsible for checking dynamic vs static labelling

On Wed, Jan 20, 2010 at 03:14:58PM +0000, Daniel P. Berrange wrote:

...

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)

Daniel P. Berrange

3:14 p.m.

New subject: [libvirt] [PATCH 02/12] 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 | 21 ++++++++--- src/security/security_apparmor.c | 66 ++++++++++++++++++++++----------- src/security/security_driver.h | 30 +++++++++------ src/security/security_selinux.c | 75 +++++++++++++++++++++++++------------- 4 files changed, 127 insertions(+), 65 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 446f6ff..b67abf1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2443,8 +2443,14 @@ static int qemudDomainSetSecurityLabel(virConnectPtr conn, struct qemud_driver * int rc = 0; if (driver->securityDriver && - driver->securityDriver->domainSetSecurityLabel && - driver->securityDriver->domainSetSecurityLabel(conn, driver->securityDriver, vm) < 0) + driver->securityDriver->domainSetSecurityAllLabel && + 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; @@ -3056,8 +3062,11 @@ static void qemudShutdownVMDaemon(virConnectPtr conn, /* Reset Security Labels */ if (driver->securityDriver && - driver->securityDriver->domainRestoreSecurityLabel) - driver->securityDriver->domainRestoreSecurityLabel(conn, vm); + driver->securityDriver->domainRestoreSecurityAllLabel) + 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) { @@ -4633,8 +4642,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->domainGetSecurityLabel(dom->conn, vm, seclabel) == -1) { + if (driver->securityDriver && driver->securityDriver->domainGetSecurityProcessLabel) { + 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; diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 0f9ff95..f288645 100644 --- a/src/security/security_apparmor.c +++ b/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, - virDomainObjPtr vm, virSecurityLabelPtr sec) +AppArmorGetSecurityProcessLabel(virConnectPtr conn, + 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, - virSecurityDriverPtr drv, virDomainObjPtr vm) +AppArmorSetSecurityProcessLabel(virConnectPtr conn, + 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, - .domainRestoreSecurityLabel = AppArmorRestoreSecurityLabel, - .domainSetSecurityLabel = AppArmorSetSecurityLabel, + .domainReleaseSecurityLabel = AppArmorReleaseSecurityLabel, + .domainGetSecurityProcessLabel = AppArmorGetSecurityProcessLabel, + .domainSetSecurityProcessLabel = AppArmorSetSecurityProcessLabel, + .domainRestoreSecurityAllLabel = AppArmorRestoreSecurityAllLabel, + .domainSetSecurityAllLabel = AppArmorSetSecurityAllLabel, .domainSetSecurityHostdevLabel = AppArmorSetSecurityHostdevLabel, .domainRestoreSecurityHostdevLabel = AppArmorRestoreSecurityHostdevLabel, }; diff --git a/src/security/security_driver.h b/src/security/security_driver.h index 97c9002..5d2446d 100644 --- a/src/security/security_driver.h +++ b/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); -typedef int (*virSecurityDomainGetLabel) (virConnectPtr conn, - virDomainObjPtr vm, - virSecurityLabelPtr sec); -typedef int (*virSecurityDomainRestoreLabel) (virConnectPtr conn, - virDomainObjPtr vm); -typedef int (*virSecurityDomainSetLabel) (virConnectPtr conn, - virSecurityDriverPtr drv, - virDomainObjPtr vm); + virDomainObjPtr sec); +typedef int (*virSecurityDomainReleaseLabel) (virConnectPtr conn, + virDomainObjPtr sec); +typedef int (*virSecurityDomainSetAllLabel) (virConnectPtr conn, + virDomainObjPtr sec); +typedef int (*virSecurityDomainRestoreAllLabel) (virConnectPtr conn, + virDomainObjPtr vm); +typedef int (*virSecurityDomainGetProcessLabel) (virConnectPtr conn, + 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; - virSecurityDomainSetLabel domainSetSecurityLabel; - virSecurityDomainRestoreLabel domainRestoreSecurityLabel; + virSecurityDomainReleaseLabel domainReleaseSecurityLabel; + virSecurityDomainGetProcessLabel domainGetSecurityProcessLabel; + virSecurityDomainSetProcessLabel domainSetSecurityProcessLabel; + virSecurityDomainSetAllLabel domainSetSecurityAllLabel; + virSecurityDomainRestoreAllLabel domainRestoreSecurityAllLabel; virSecurityDomainRestoreHostdevLabel domainRestoreSecurityHostdevLabel; virSecurityDomainSetHostdevLabel domainSetSecurityHostdevLabel; virSecurityDomainSetSavedStateLabel domainSetSavedStateLabel; diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index c48f4c8..847a522 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -277,9 +277,9 @@ SELinuxSecurityDriverOpen(virConnectPtr conn, virSecurityDriverPtr drv) } static int -SELinuxGetSecurityLabel(virConnectPtr conn, - virDomainObjPtr vm, - virSecurityLabelPtr sec) +SELinuxGetSecurityProcessLabel(virConnectPtr conn, + virDomainObjPtr vm, + virSecurityLabelPtr sec) { security_context_t ctx; @@ -615,8 +615,8 @@ done: } static int -SELinuxRestoreSecurityLabel(virConnectPtr conn, - virDomainObjPtr vm) +SELinuxRestoreSecurityAllLabel(virConnectPtr conn, + 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, - virSecurityDriverPtr drv, - virDomainObjPtr vm) +SELinuxSetSecurityProcessLabel(virConnectPtr conn, + virSecurityDriverPtr drv, + virDomainObjPtr vm) { /* TODO: verify DOI */ const virSecurityLabelDefPtr secdef = &vm->def->seclabel; - int i; if (vm->def->seclabel.label == NULL) return 0; @@ -722,18 +734,29 @@ SELinuxSetSecurityLabel(virConnectPtr conn, return -1; } - 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) - 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; +} + +static int +SELinuxSetSecurityAllLabel(virConnectPtr conn, + virDomainObjPtr vm) +{ + const virSecurityLabelDefPtr secdef = &vm->def->seclabel; + int i; + + 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 entriy tree :-( */ + if (vm->def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_DIR) + 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 +771,11 @@ virSecurityDriver virSELinuxSecurityDriver = { .domainRestoreSecurityImageLabel = SELinuxRestoreSecurityImageLabel, .domainGenSecurityLabel = SELinuxGenSecurityLabel, .domainReserveSecurityLabel = SELinuxReserveSecurityLabel, - .domainGetSecurityLabel = SELinuxGetSecurityLabel, - .domainRestoreSecurityLabel = SELinuxRestoreSecurityLabel, - .domainSetSecurityLabel = SELinuxSetSecurityLabel, + .domainReleaseSecurityLabel = SELinuxReleaseSecurityLabel, + .domainGetSecurityProcessLabel = SELinuxGetSecurityProcessLabel, + .domainSetSecurityProcessLabel = SELinuxSetSecurityProcessLabel, + .domainRestoreSecurityAllLabel = SELinuxRestoreSecurityAllLabel, + .domainSetSecurityAllLabel = SELinuxSetSecurityAllLabel, .domainSetSecurityHostdevLabel = SELinuxSetSecurityHostdevLabel, .domainRestoreSecurityHostdevLabel = SELinuxRestoreSecurityHostdevLabel, .domainSetSavedStateLabel = SELinuxSetSavedStateLabel, -- 1.6.5.2

Daniel Veillard

4:19 p.m.

New subject: [libvirt] [PATCH 02/12] Refactor setup & cleanup of security labels in security driver

On Wed, Jan 20, 2010 at 03:14:59PM +0000, Daniel P. Berrange wrote:

...

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.

A bit complex, thanks for the explanations !

...

NB, the AppArmour changes here haven't been compiled so may not build. --- src/qemu/qemu_driver.c | 21 ++++++++--- src/security/security_apparmor.c | 66 ++++++++++++++++++++++----------- src/security/security_driver.h | 30 +++++++++------ src/security/security_selinux.c | 75 +++++++++++++++++++++++++------------- 4 files changed, 127 insertions(+), 65 deletions(-) [...] +static int +SELinuxSetSecurityAllLabel(virConnectPtr conn, + virDomainObjPtr vm) +{ + const virSecurityLabelDefPtr secdef = &vm->def->seclabel; + int i; + + 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 entriy tree :-( */

while we're are there let's fix the entriy/entire typo

...

+ if (vm->def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_DIR) + 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; }

Okay, ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

Stephen Smalley

5:41 p.m.

New subject: [libvirt] [PATCH 02/12] Refactor setup & cleanup of security labels in security driver

On Wed, 2010-01-20 at 17:19 +0100, Daniel Veillard wrote:

...

On Wed, Jan 20, 2010 at 03:14:59PM +0000, Daniel P. Berrange wrote:

...
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.

A bit complex, thanks for the explanations !

...
NB, the AppArmour changes here haven't been compiled so may not build. --- src/qemu/qemu_driver.c | 21 ++++++++--- src/security/security_apparmor.c | 66 ++++++++++++++++++++++----------- src/security/security_driver.h | 30 +++++++++------ src/security/security_selinux.c | 75 +++++++++++++++++++++++++------------- 4 files changed, 127 insertions(+), 65 deletions(-) [...] +static int +SELinuxSetSecurityAllLabel(virConnectPtr conn, + virDomainObjPtr vm) +{ + const virSecurityLabelDefPtr secdef = &vm->def->seclabel; + int i; + + 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 entriy tree :-( */

while we're are there let's fix the entriy/entire typo

Not sure what this is supposed to mean, but you could exec chcon -R on the directory tree if you want to recursively relabel it. -- Stephen Smalley National Security Agency

Daniel P. Berrange

5:42 p.m.

New subject: [libvirt] [PATCH 02/12] Refactor setup & cleanup of security labels in security driver

On Wed, Jan 20, 2010 at 12:41:29PM -0500, Stephen Smalley wrote:

...

On Wed, 2010-01-20 at 17:19 +0100, Daniel Veillard wrote:

...
On Wed, Jan 20, 2010 at 03:14:59PM +0000, Daniel P. Berrange wrote:

...
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.

A bit complex, thanks for the explanations !

...
NB, the AppArmour changes here haven't been compiled so may not build. --- src/qemu/qemu_driver.c | 21 ++++++++--- src/security/security_apparmor.c | 66 ++++++++++++++++++++++----------- src/security/security_driver.h | 30 +++++++++------ src/security/security_selinux.c | 75 +++++++++++++++++++++++++------------- 4 files changed, 127 insertions(+), 65 deletions(-) [...] +static int +SELinuxSetSecurityAllLabel(virConnectPtr conn, + virDomainObjPtr vm) +{ + const virSecurityLabelDefPtr secdef = &vm->def->seclabel; + int i; + + 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 entriy tree :-( */

while we're are there let's fix the entriy/entire typo

Not sure what this is supposed to mean, but you could exec chcon -R on the directory tree if you want to recursively relabel it.

It is tricky, because part of the this refactoring work is to allow me to maintain a DB of the original contexts, so that the restore step at VM shutdown can put back the original context, rather than resetting to the defaults using matchpathcon(). So we need to handle the recursive chcon directly in libvirt, rather than spawning an external command. Fortunately, I don't expect anyone to ever use the VVFAT driver that this comment in referring to, so I'm not particularly worrying about fixing this any time soon. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Daniel P. Berrange

3:15 p.m.

New subject: [libvirt] [PATCH 03/12] Fix leak of allocated security label

If a VM fails to start, we can't simply free the security label strings, we must call the domainReleaseSecurityLabel() method otherwise the reserved 'mcs' level will be leaked in SElinux * src/qemu/qemu_driver.c: Invoke domainReleaseSecurityLabel() when domain fails to start --- src/qemu/qemu_driver.c | 8 +++----- 1 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b67abf1..6c66b22 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2978,11 +2978,9 @@ cleanup: /* We jump here if we failed to start the VM for any reason * XXX investigate if we can kill this block and safely call * qemudShutdownVMDaemon even though no PID is running */ - if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC) { - VIR_FREE(vm->def->seclabel.model); - VIR_FREE(vm->def->seclabel.label); - VIR_FREE(vm->def->seclabel.imagelabel); - } + if (driver->securityDriver && + driver->securityDriver->domainReleaseSecurityLabel) + driver->securityDriver->domainReleaseSecurityLabel(conn, vm); qemuRemoveCgroup(conn, driver, vm, 0); if ((vm->def->ngraphics == 1) && vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC && -- 1.6.5.2

Daniel Veillard

4:20 p.m.

New subject: [libvirt] [PATCH 03/12] Fix leak of allocated security label

On Wed, Jan 20, 2010 at 03:15:00PM +0000, Daniel P. Berrange wrote:

...

If a VM fails to start, we can't simply free the security label strings, we must call the domainReleaseSecurityLabel() method otherwise the reserved 'mcs' level will be leaked in SElinux

* src/qemu/qemu_driver.c: Invoke domainReleaseSecurityLabel() when domain fails to start --- src/qemu/qemu_driver.c | 8 +++----- 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b67abf1..6c66b22 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2978,11 +2978,9 @@ cleanup: /* We jump here if we failed to start the VM for any reason * XXX investigate if we can kill this block and safely call * qemudShutdownVMDaemon even though no PID is running */ - if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC) { - VIR_FREE(vm->def->seclabel.model); - VIR_FREE(vm->def->seclabel.label); - VIR_FREE(vm->def->seclabel.imagelabel); - } + if (driver->securityDriver && + driver->securityDriver->domainReleaseSecurityLabel) + driver->securityDriver->domainReleaseSecurityLabel(conn, vm); qemuRemoveCgroup(conn, driver, vm, 0); if ((vm->def->ngraphics == 1) && vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC &&

ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

Daniel P. Berrange

3:15 p.m.

New subject: [libvirt] [PATCH 04/12] Pull initial disk labelling out into libvirtd instead of exec hook

Pulling the disk labelling code out of the exec hook, and into libvirtd will allow it to access shared state in the daemon. It will also make debugging & error reporting easier / more reliable. * qemu/qemu_driver.c: Move initial disk labelling calls up into libvirtd. Add cleanup of disk labels upon failure --- src/qemu/qemu_driver.c | 14 ++++++++------ 1 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6c66b22..8195b74 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2443,12 +2443,6 @@ static int qemudDomainSetSecurityLabel(virConnectPtr conn, struct qemud_driver * int rc = 0; if (driver->securityDriver && - driver->securityDriver->domainSetSecurityAllLabel && - 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; @@ -2771,6 +2765,11 @@ static int qemudStartVMDaemon(virConnectPtr conn, driver->securityDriver->domainGenSecurityLabel(conn, vm) < 0) return -1; + if (driver->securityDriver && + driver->securityDriver->domainSetSecurityAllLabel && + driver->securityDriver->domainSetSecurityAllLabel(conn, vm) < 0) + goto cleanup; + /* Ensure no historical cgroup for this VM is lieing around bogus settings */ qemuRemoveCgroup(conn, driver, vm, 1); @@ -2979,6 +2978,9 @@ cleanup: * XXX investigate if we can kill this block and safely call * qemudShutdownVMDaemon even though no PID is running */ if (driver->securityDriver && + driver->securityDriver->domainRestoreSecurityAllLabel) + driver->securityDriver->domainRestoreSecurityAllLabel(conn, vm); + if (driver->securityDriver && driver->securityDriver->domainReleaseSecurityLabel) driver->securityDriver->domainReleaseSecurityLabel(conn, vm); qemuRemoveCgroup(conn, driver, vm, 0); -- 1.6.5.2

Daniel Veillard

4:24 p.m.

New subject: [libvirt] [PATCH 04/12] Pull initial disk labelling out into libvirtd instead of exec hook

On Wed, Jan 20, 2010 at 03:15:01PM +0000, Daniel P. Berrange wrote:

...

Pulling the disk labelling code out of the exec hook, and into libvirtd will allow it to access shared state in the daemon. It will also make debugging & error reporting easier / more reliable.

* qemu/qemu_driver.c: Move initial disk labelling calls up into libvirtd. Add cleanup of disk labels upon failure --- src/qemu/qemu_driver.c | 14 ++++++++------ 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6c66b22..8195b74 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2443,12 +2443,6 @@ static int qemudDomainSetSecurityLabel(virConnectPtr conn, struct qemud_driver * int rc = 0;

if (driver->securityDriver && - driver->securityDriver->domainSetSecurityAllLabel && - 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; @@ -2771,6 +2765,11 @@ static int qemudStartVMDaemon(virConnectPtr conn, driver->securityDriver->domainGenSecurityLabel(conn, vm) < 0) return -1;

+ if (driver->securityDriver && + driver->securityDriver->domainSetSecurityAllLabel && + driver->securityDriver->domainSetSecurityAllLabel(conn, vm) < 0) + goto cleanup; + /* Ensure no historical cgroup for this VM is lieing around bogus settings */ qemuRemoveCgroup(conn, driver, vm, 1);

@@ -2979,6 +2978,9 @@ cleanup: * XXX investigate if we can kill this block and safely call * qemudShutdownVMDaemon even though no PID is running */ if (driver->securityDriver && + driver->securityDriver->domainRestoreSecurityAllLabel) + driver->securityDriver->domainRestoreSecurityAllLabel(conn, vm); + if (driver->securityDriver && driver->securityDriver->domainReleaseSecurityLabel) driver->securityDriver->domainReleaseSecurityLabel(conn, vm); qemuRemoveCgroup(conn, driver, vm, 0);

Daniel P. Berrange

3:15 p.m.

New subject: [libvirt] [PATCH 05/12] Introduce a stacked security driver impl for QEMU

* qemu/qemu_conf.h: Add securityPrimaryDriver and securitySecondaryDriver fields to 'struct qemud_driver' * Makefile.am: Add new files * qemu/qemu_security_stacked.c, qemu/qemu_security_stacked.h: A simple stacked security driver --- src/Makefile.am | 4 +- src/qemu/qemu_conf.h | 2 + src/qemu/qemu_security_stacked.c | 353 ++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_security_stacked.h | 22 +++ 4 files changed, 380 insertions(+), 1 deletions(-) create mode 100644 src/qemu/qemu_security_stacked.c create mode 100644 src/qemu/qemu_security_stacked.h diff --git a/src/Makefile.am b/src/Makefile.am index 713cbda..0fb6dba 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -198,7 +198,9 @@ QEMU_DRIVER_SOURCES = \ qemu/qemu_monitor_json.h \ qemu/qemu_driver.c qemu/qemu_driver.h \ qemu/qemu_bridge_filter.c \ - qemu/qemu_bridge_filter.h + qemu/qemu_bridge_filter.h \ + qemu/qemu_security_stacked.h \ + qemu/qemu_security_stacked.c UML_DRIVER_SOURCES = \ uml/uml_conf.c uml/uml_conf.h \ diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index ed2d32b..678bc6f 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -134,6 +134,8 @@ struct qemud_driver { char *securityDriverName; virSecurityDriverPtr securityDriver; + virSecurityDriverPtr securityPrimaryDriver; + virSecurityDriverPtr securitySecondaryDriver; char *saveImageFormat; diff --git a/src/qemu/qemu_security_stacked.c b/src/qemu/qemu_security_stacked.c new file mode 100644 index 0000000..60acb4c --- /dev/null +++ b/src/qemu/qemu_security_stacked.c @@ -0,0 +1,353 @@ +/* + * Copyright (C) 2010 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * QEMU stacked security driver + */ + +#include <config.h> +#include <selinux/selinux.h> +#include <selinux/context.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> + +#include "qemu_security_stacked.h" + +#include "qemu_conf.h" +#include "datatypes.h" +#include "virterror_internal.h" +#include "util.h" +#include "memory.h" +#include "logging.h" +#include "pci.h" +#include "hostusb.h" +#include "storage_file.h" + +#define VIR_FROM_THIS VIR_FROM_QEMU + + +static struct qemud_driver *driver; + +void qemuSecurityStackedSetDriver(struct qemud_driver *newdriver) +{ + driver = newdriver; +} + + +static int +qemuSecurityStackedVerify(virConnectPtr conn, + virDomainDefPtr def) +{ + int rc = 0; + + if (driver->securitySecondaryDriver && + driver->securitySecondaryDriver->domainSecurityVerify && + driver->securitySecondaryDriver->domainSecurityVerify(conn, def) < 0) + rc = -1; + + if (driver->securityPrimaryDriver && + driver->securityPrimaryDriver->domainSecurityVerify && + driver->securityPrimaryDriver->domainSecurityVerify(conn, def) < 0) + rc = -1; + + return rc; +} + + +static int +qemuSecurityStackedGenLabel(virConnectPtr conn, + virDomainObjPtr vm) +{ + int rc = 0; + + if (driver->securitySecondaryDriver && + driver->securitySecondaryDriver->domainGenSecurityLabel && + driver->securitySecondaryDriver->domainGenSecurityLabel(conn, vm) < 0) + rc = -1; + + if (driver->securityPrimaryDriver && + driver->securityPrimaryDriver->domainGenSecurityLabel && + driver->securityPrimaryDriver->domainGenSecurityLabel(conn, vm) < 0) + rc = -1; + + return rc; +} + + +static int +qemuSecurityStackedReleaseLabel(virConnectPtr conn, + virDomainObjPtr vm) +{ + int rc = 0; + + if (driver->securitySecondaryDriver && + driver->securitySecondaryDriver->domainReleaseSecurityLabel && + driver->securitySecondaryDriver->domainReleaseSecurityLabel(conn, vm) < 0) + rc = -1; + + if (driver->securityPrimaryDriver && + driver->securityPrimaryDriver->domainReleaseSecurityLabel && + driver->securityPrimaryDriver->domainReleaseSecurityLabel(conn, vm) < 0) + rc = -1; + + return rc; +} + + +static int +qemuSecurityStackedReserveLabel(virConnectPtr conn, + virDomainObjPtr vm) +{ + int rc = 0; + + if (driver->securitySecondaryDriver && + driver->securitySecondaryDriver->domainReserveSecurityLabel && + driver->securitySecondaryDriver->domainReserveSecurityLabel(conn, vm) < 0) + rc = -1; + + if (driver->securityPrimaryDriver && + driver->securityPrimaryDriver->domainReserveSecurityLabel && + driver->securityPrimaryDriver->domainReserveSecurityLabel(conn, vm) < 0) + rc = -1; + + return rc; +} + + +static int +qemuSecurityStackedSetSecurityImageLabel(virConnectPtr conn, + virDomainObjPtr vm, + virDomainDiskDefPtr disk) +{ + int rc = 0; + + if (driver->securitySecondaryDriver && + driver->securitySecondaryDriver->domainSetSecurityImageLabel && + driver->securitySecondaryDriver->domainSetSecurityImageLabel(conn, vm, disk) < 0) + rc = -1; + + if (driver->securityPrimaryDriver && + driver->securityPrimaryDriver->domainSetSecurityImageLabel && + driver->securityPrimaryDriver->domainSetSecurityImageLabel(conn, vm, disk) < 0) + rc = -1; + + return rc; +} + + +static int +qemuSecurityStackedRestoreSecurityImageLabel(virConnectPtr conn, + virDomainObjPtr vm, + virDomainDiskDefPtr disk) +{ + int rc = 0; + + if (driver->securitySecondaryDriver && + driver->securitySecondaryDriver->domainRestoreSecurityImageLabel && + driver->securitySecondaryDriver->domainRestoreSecurityImageLabel(conn, vm, disk) < 0) + rc = -1; + + if (driver->securityPrimaryDriver && + driver->securityPrimaryDriver->domainRestoreSecurityImageLabel && + driver->securityPrimaryDriver->domainRestoreSecurityImageLabel(conn, vm, disk) < 0) + rc = -1; + + return rc; +} + + +static int +qemuSecurityStackedSetSecurityHostdevLabel(virConnectPtr conn, + virDomainObjPtr vm, + virDomainHostdevDefPtr dev) + +{ + int rc = 0; + + if (driver->securitySecondaryDriver && + driver->securitySecondaryDriver->domainSetSecurityHostdevLabel && + driver->securitySecondaryDriver->domainSetSecurityHostdevLabel(conn, vm, dev) < 0) + rc = -1; + + if (driver->securityPrimaryDriver && + driver->securityPrimaryDriver->domainSetSecurityHostdevLabel && + driver->securityPrimaryDriver->domainSetSecurityHostdevLabel(conn, vm, dev) < 0) + rc = -1; + + return rc; +} + + +static int +qemuSecurityStackedRestoreSecurityHostdevLabel(virConnectPtr conn, + virDomainObjPtr vm, + virDomainHostdevDefPtr dev) + +{ + int rc = 0; + + if (driver->securitySecondaryDriver && + driver->securitySecondaryDriver->domainRestoreSecurityHostdevLabel && + driver->securitySecondaryDriver->domainRestoreSecurityHostdevLabel(conn, vm, dev) < 0) + rc = -1; + + if (driver->securityPrimaryDriver && + driver->securityPrimaryDriver->domainRestoreSecurityHostdevLabel && + driver->securityPrimaryDriver->domainRestoreSecurityHostdevLabel(conn, vm, dev) < 0) + rc = -1; + + return rc; +} + + +static int +qemuSecurityStackedSetSecurityAllLabel(virConnectPtr conn, + virDomainObjPtr vm) +{ + int rc = 0; + + if (driver->securitySecondaryDriver && + driver->securitySecondaryDriver->domainSetSecurityAllLabel && + driver->securitySecondaryDriver->domainSetSecurityAllLabel(conn, vm) < 0) + rc = -1; + + if (driver->securityPrimaryDriver && + driver->securityPrimaryDriver->domainSetSecurityAllLabel && + driver->securityPrimaryDriver->domainSetSecurityAllLabel(conn, vm) < 0) + rc = -1; + + return rc; +} + + +static int +qemuSecurityStackedRestoreSecurityAllLabel(virConnectPtr conn, + virDomainObjPtr vm) +{ + int rc = 0; + + if (driver->securitySecondaryDriver && + driver->securitySecondaryDriver->domainRestoreSecurityAllLabel && + driver->securitySecondaryDriver->domainRestoreSecurityAllLabel(conn, vm) < 0) + rc = -1; + + if (driver->securityPrimaryDriver && + driver->securityPrimaryDriver->domainRestoreSecurityAllLabel && + driver->securityPrimaryDriver->domainRestoreSecurityAllLabel(conn, vm) < 0) + rc = -1; + + return rc; +} + + +static int +qemuSecurityStackedSetSavedStateLabel(virConnectPtr conn, + virDomainObjPtr vm, + const char *savefile) +{ + int rc = 0; + + if (driver->securitySecondaryDriver && + driver->securitySecondaryDriver->domainSetSavedStateLabel && + driver->securitySecondaryDriver->domainSetSavedStateLabel(conn, vm, savefile) < 0) + rc = -1; + + if (driver->securityPrimaryDriver && + driver->securityPrimaryDriver->domainSetSavedStateLabel && + driver->securityPrimaryDriver->domainSetSavedStateLabel(conn, vm, savefile) < 0) + rc = -1; + + return rc; +} + + +static int +qemuSecurityStackedRestoreSavedStateLabel(virConnectPtr conn, + virDomainObjPtr vm, + const char *savefile) +{ + int rc = 0; + + if (driver->securitySecondaryDriver && + driver->securitySecondaryDriver->domainRestoreSavedStateLabel && + driver->securitySecondaryDriver->domainRestoreSavedStateLabel(conn, vm, savefile) < 0) + rc = -1; + + if (driver->securityPrimaryDriver && + driver->securityPrimaryDriver->domainRestoreSavedStateLabel && + driver->securityPrimaryDriver->domainRestoreSavedStateLabel(conn, vm, savefile) < 0) + rc = -1; + + return rc; +} + + +static int +qemuSecurityStackedSetProcessLabel(virConnectPtr conn, + virSecurityDriverPtr drv ATTRIBUTE_UNUSED, + virDomainObjPtr vm) +{ + int rc = 0; + + if (driver->securitySecondaryDriver && + driver->securitySecondaryDriver->domainSetSecurityProcessLabel && + driver->securitySecondaryDriver->domainSetSecurityProcessLabel(conn, + driver->securitySecondaryDriver, + vm) < 0) + rc = -1; + + if (driver->securityPrimaryDriver && + driver->securityPrimaryDriver->domainSetSecurityProcessLabel && + driver->securityPrimaryDriver->domainSetSecurityProcessLabel(conn, + driver->securityPrimaryDriver, + vm) < 0) + rc = -1; + + return rc; +} + +static int +qemuSecurityStackedGetProcessLabel(virConnectPtr conn, + virDomainObjPtr vm, + virSecurityLabelPtr seclabel) +{ + int rc = 0; + + if (driver->securityPrimaryDriver && + driver->securityPrimaryDriver->domainGetSecurityProcessLabel && + driver->securityPrimaryDriver->domainGetSecurityProcessLabel(conn, + vm, + seclabel) < 0) + rc = -1; + + return rc; +} + +virSecurityDriver qemuStackedSecurityDriver = { + .name = "qemuStacked", + .domainSecurityVerify = qemuSecurityStackedVerify, + + .domainGenSecurityLabel = qemuSecurityStackedGenLabel, + .domainReleaseSecurityLabel = qemuSecurityStackedReleaseLabel, + .domainReserveSecurityLabel = qemuSecurityStackedReserveLabel, + + .domainGetSecurityProcessLabel = qemuSecurityStackedGetProcessLabel, + .domainSetSecurityProcessLabel = qemuSecurityStackedSetProcessLabel, + + .domainSetSecurityImageLabel = qemuSecurityStackedSetSecurityImageLabel, + .domainRestoreSecurityImageLabel = qemuSecurityStackedRestoreSecurityImageLabel, + + .domainSetSecurityAllLabel = qemuSecurityStackedSetSecurityAllLabel, + .domainRestoreSecurityAllLabel = qemuSecurityStackedRestoreSecurityAllLabel, + + .domainSetSecurityHostdevLabel = qemuSecurityStackedSetSecurityHostdevLabel, + .domainRestoreSecurityHostdevLabel = qemuSecurityStackedRestoreSecurityHostdevLabel, + + .domainSetSavedStateLabel = qemuSecurityStackedSetSavedStateLabel, + .domainRestoreSavedStateLabel = qemuSecurityStackedRestoreSavedStateLabel, +}; diff --git a/src/qemu/qemu_security_stacked.h b/src/qemu/qemu_security_stacked.h new file mode 100644 index 0000000..d67a5f1 --- /dev/null +++ b/src/qemu/qemu_security_stacked.h @@ -0,0 +1,22 @@ +/* + * Copyright (C) 2010 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * QEMU stacked security driver + */ + +#include "security/security_driver.h" +#include "qemu_conf.h" + +#ifndef __QEMU_SECURITY_STACKED +#define __QEMU_SECURITY_STACKED + +extern virSecurityDriver qemuStackedSecurityDriver; + +void qemuSecurityStackedSetDriver(struct qemud_driver *driver); + +#endif /* __QEMU_SECURITY_DAC */ -- 1.6.5.2

Daniel Veillard

5 p.m.

New subject: [libvirt] [PATCH 05/12] Introduce a stacked security driver impl for QEMU

On Wed, Jan 20, 2010 at 03:15:02PM +0000, Daniel P. Berrange wrote:

...

* qemu/qemu_conf.h: Add securityPrimaryDriver and securitySecondaryDriver fields to 'struct qemud_driver' * Makefile.am: Add new files * qemu/qemu_security_stacked.c, qemu/qemu_security_stacked.h: A simple stacked security driver --- src/Makefile.am | 4 +- src/qemu/qemu_conf.h | 2 + src/qemu/qemu_security_stacked.c | 353 ++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_security_stacked.h | 22 +++ 4 files changed, 380 insertions(+), 1 deletions(-) create mode 100644 src/qemu/qemu_security_stacked.c create mode 100644 src/qemu/qemu_security_stacked.h [...] +++ b/src/qemu/qemu_security_stacked.c @@ -0,0 +1,353 @@ +/* + * Copyright (C) 2010 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * QEMU stacked security driver + */ + +#include <config.h> +#include <selinux/selinux.h> +#include <selinux/context.h>

Why do we need to include SELinux headers, the module content seems to be agnostic on the underlying framework (SELinux/Apparmor)

...

+static int +qemuSecurityStackedGenLabel(virConnectPtr conn, + virDomainObjPtr vm) +{ + int rc = 0; + + if (driver->securitySecondaryDriver && + driver->securitySecondaryDriver->domainGenSecurityLabel && + driver->securitySecondaryDriver->domainGenSecurityLabel(conn, vm) < 0) + rc = -1; + + if (driver->securityPrimaryDriver && + driver->securityPrimaryDriver->domainGenSecurityLabel && + driver->securityPrimaryDriver->domainGenSecurityLabel(conn, vm) < 0) + rc = -1; + + return rc; +}

Okay, so the general pattern is that any stacked driver call will fail if any of the subdriver entry point fail, a missing entry point not being a failure. I'm still surprized that if both entry point are missing we return 0 i.e. success...

...

+virSecurityDriver qemuStackedSecurityDriver = { + .name = "qemuStacked", + .domainSecurityVerify = qemuSecurityStackedVerify, + + .domainGenSecurityLabel = qemuSecurityStackedGenLabel, + .domainReleaseSecurityLabel = qemuSecurityStackedReleaseLabel, + .domainReserveSecurityLabel = qemuSecurityStackedReserveLabel, + + .domainGetSecurityProcessLabel = qemuSecurityStackedGetProcessLabel, + .domainSetSecurityProcessLabel = qemuSecurityStackedSetProcessLabel, + + .domainSetSecurityImageLabel = qemuSecurityStackedSetSecurityImageLabel, + .domainRestoreSecurityImageLabel = qemuSecurityStackedRestoreSecurityImageLabel, + + .domainSetSecurityAllLabel = qemuSecurityStackedSetSecurityAllLabel, + .domainRestoreSecurityAllLabel = qemuSecurityStackedRestoreSecurityAllLabel, + + .domainSetSecurityHostdevLabel = qemuSecurityStackedSetSecurityHostdevLabel, + .domainRestoreSecurityHostdevLabel = qemuSecurityStackedRestoreSecurityHostdevLabel, + + .domainSetSavedStateLabel = qemuSecurityStackedSetSavedStateLabel, + .domainRestoreSavedStateLabel = qemuSecurityStackedRestoreSavedStateLabel, +};

I tend to prefer full initialization to see if there is missing entry points. Just minor remarks except maybe for the return value in case both subdrivers misses the entry point. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

Daniel P. Berrange

5:05 p.m.

New subject: [libvirt] [PATCH 05/12] Introduce a stacked security driver impl for QEMU

On Wed, Jan 20, 2010 at 06:00:06PM +0100, Daniel Veillard wrote:

...

On Wed, Jan 20, 2010 at 03:15:02PM +0000, Daniel P. Berrange wrote:

...
* qemu/qemu_conf.h: Add securityPrimaryDriver and securitySecondaryDriver fields to 'struct qemud_driver' * Makefile.am: Add new files * qemu/qemu_security_stacked.c, qemu/qemu_security_stacked.h: A simple stacked security driver --- src/Makefile.am | 4 +- src/qemu/qemu_conf.h | 2 + src/qemu/qemu_security_stacked.c | 353 ++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_security_stacked.h | 22 +++ 4 files changed, 380 insertions(+), 1 deletions(-) create mode 100644 src/qemu/qemu_security_stacked.c create mode 100644 src/qemu/qemu_security_stacked.h [...] +++ b/src/qemu/qemu_security_stacked.c @@ -0,0 +1,353 @@ +/* + * Copyright (C) 2010 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * QEMU stacked security driver + */ + +#include <config.h> +#include <selinux/selinux.h> +#include <selinux/context.h>

Why do we need to include SELinux headers, the module content seems to be agnostic on the underlying framework (SELinux/Apparmor)

Copy & paste error :-)

...

...
+static int +qemuSecurityStackedGenLabel(virConnectPtr conn, + virDomainObjPtr vm) +{ + int rc = 0; + + if (driver->securitySecondaryDriver && + driver->securitySecondaryDriver->domainGenSecurityLabel && + driver->securitySecondaryDriver->domainGenSecurityLabel(conn, vm) < 0) + rc = -1; + + if (driver->securityPrimaryDriver && + driver->securityPrimaryDriver->domainGenSecurityLabel && + driver->securityPrimaryDriver->domainGenSecurityLabel(conn, vm) < 0) + rc = -1; + + return rc; +}

Okay, so the general pattern is that any stacked driver call will fail if any of the subdriver entry point fail, a missing entry point not being a failure. I'm still surprized that if both entry point are missing we return 0 i.e. success...

If the operation being performed doesn't make sense for a driver it is fine to skip it without error. This is actually the same semantics we had before this refactoring, because the QEMU driver always checks for NULL entry point & skips it without error. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Daniel P. Berrange

3:15 p.m.

New subject: [libvirt] [PATCH 06/12] Introduce a new DAC security driver for QEMU

This new security driver is responsible for managing UID/GID changes to the QEMU process, and any files/disks/devices assigned to it. * qemu/qemu_conf.h: Add flag for disabling automatic file permission changes * qemu/qemu_security_dac.h, qemu/qemu_security_dac.c: New DAC driver for QEMU guests * Makefile.am: Add new files --- po/POTFILES.in | 1 + src/Makefile.am | 4 +- src/qemu/qemu_conf.h | 1 + src/qemu/qemu_security_dac.c | 458 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_security_dac.h | 22 ++ 5 files changed, 485 insertions(+), 1 deletions(-) create mode 100644 src/qemu/qemu_security_dac.c create mode 100644 src/qemu/qemu_security_dac.h diff --git a/po/POTFILES.in b/po/POTFILES.in index 3b82a74..18f5243 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -36,6 +36,7 @@ src/qemu/qemu_driver.c src/qemu/qemu_monitor.c src/qemu/qemu_monitor_json.c src/qemu/qemu_monitor_text.c +src/qemu/qemu_security_dac.c src/remote/remote_driver.c src/secret/secret_driver.c src/security/security_driver.c diff --git a/src/Makefile.am b/src/Makefile.am index 0fb6dba..3232256 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -200,7 +200,9 @@ QEMU_DRIVER_SOURCES = \ qemu/qemu_bridge_filter.c \ qemu/qemu_bridge_filter.h \ qemu/qemu_security_stacked.h \ - qemu/qemu_security_stacked.c + qemu/qemu_security_stacked.c \ + qemu/qemu_security_dac.h \ + qemu/qemu_security_dac.c UML_DRIVER_SOURCES = \ uml/uml_conf.c uml/uml_conf.h \ diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 678bc6f..e4a10bb 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -90,6 +90,7 @@ struct qemud_driver { uid_t user; gid_t group; + int dynamicOwnership; unsigned int qemuVersion; int nextvmid; diff --git a/src/qemu/qemu_security_dac.c b/src/qemu/qemu_security_dac.c new file mode 100644 index 0000000..6b7aab5 --- /dev/null +++ b/src/qemu/qemu_security_dac.c @@ -0,0 +1,458 @@ +/* + * Copyright (C) 2010 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * QEMU POSIX DAC security driver + */ +#include <config.h> +#include <selinux/selinux.h> +#include <selinux/context.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> + +#include "qemu_security_dac.h" +#include "qemu_conf.h" +#include "datatypes.h" +#include "virterror_internal.h" +#include "util.h" +#include "memory.h" +#include "logging.h" +#include "pci.h" +#include "hostusb.h" +#include "storage_file.h" + +#define VIR_FROM_THIS VIR_FROM_QEMU + +static struct qemud_driver *driver; + +void qemuSecurityDACSetDriver(struct qemud_driver *newdriver) +{ + driver = newdriver; +} + + +static int +qemuSecurityDACSetOwnership(virConnectPtr conn, const char *path, int uid, int gid) +{ + VIR_INFO("Setting DAC context on '%s' to '%d:%d'", path, uid, gid); + + if (chown(path, uid, gid) < 0) { + struct stat sb; + int chown_errno = errno; + + if (stat(path, &sb) >= 0) { + if (sb.st_uid == uid && + sb.st_gid == gid) { + /* It's alright, there's nothing to change anyway. */ + return 0; + } + } + + /* if the error complaint is related to an image hosted on + * an nfs mount, or a usbfs/sysfs filesystem not supporting + * labelling, then just ignore it & hope for the best. + * The user hopefully set one of the necessary qemuSecurityDAC + * virt_use_{nfs,usb,pci} boolean tunables to allow it... + */ + if (chown_errno == EOPNOTSUPP) { + VIR_INFO("Setting security context '%d:%d' on '%s' not supported by filesystem", + uid, gid, path); + } else if (chown_errno == EPERM) { + VIR_INFO("Setting security context '%d:%d' on '%s' not permitted", + uid, gid, path); + } else if (chown_errno == EROFS) { + VIR_INFO("Setting security context '%d:%d' on '%s' not possible on readonly filesystem", + uid, gid, path); + } else { + virReportSystemError(conn, chown_errno, + _("unable to set security context '%d:%d' on '%s'"), + uid, gid, path); + return -1; + } + } + return 0; +} + +static int +qemuSecurityDACRestoreSecurityFileLabel(virConnectPtr conn, + const char *path) +{ + struct stat buf; + security_context_t fcon = NULL; + int rc = -1; + int err; + char *newpath = NULL; + + VIR_INFO("Restoring DAC context on '%s'", path); + + if ((err = virFileResolveLink(path, &newpath)) < 0) { + virReportSystemError(conn, err, + _("cannot resolve symlink %s"), path); + goto err; + } + + if (stat(newpath, &buf) != 0) + goto err; + + /* XXX record previous ownership */ + rc = qemuSecurityDACSetOwnership(conn, newpath, 0, 0); + +err: + VIR_FREE(fcon); + VIR_FREE(newpath); + return rc; +} + + +static int +qemuSecurityDACSetSecurityImageLabel(virConnectPtr conn, + virDomainObjPtr vm ATTRIBUTE_UNUSED, + virDomainDiskDefPtr disk) + +{ + const char *path; + + if (!driver->privileged || !driver->dynamicOwnership) + return 0; + + if (!disk->src) + return 0; + + path = disk->src; + do { + virStorageFileMetadata meta; + int ret; + + memset(&meta, 0, sizeof(meta)); + + ret = virStorageFileGetMetadata(conn, path, &meta); + + if (path != disk->src) + VIR_FREE(path); + path = NULL; + + if (ret < 0) + return -1; + + if (meta.backingStore != NULL && + qemuSecurityDACSetOwnership(conn, meta.backingStore, + driver->user, driver->group) < 0) { + VIR_FREE(meta.backingStore); + return -1; + } + + path = meta.backingStore; + } while (path != NULL); + + return qemuSecurityDACSetOwnership(conn, disk->src, driver->user, driver->group); +} + + +static int +qemuSecurityDACRestoreSecurityImageLabel(virConnectPtr conn, + virDomainObjPtr vm ATTRIBUTE_UNUSED, + virDomainDiskDefPtr disk) +{ + if (!driver->privileged || !driver->dynamicOwnership) + 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 + * domains and try to figure out if it is in use, but + * this would not work for clustered filesystems, since + * we can't see running VMs using the file on other nodes + * Safest bet is thus to skip the restore step. + */ + if (disk->readonly || disk->shared) + return 0; + + if (!disk->src) + return 0; + + return qemuSecurityDACRestoreSecurityFileLabel(conn, disk->src); +} + + +static int +qemuSecurityDACSetSecurityPCILabel(virConnectPtr conn, + pciDevice *dev ATTRIBUTE_UNUSED, + const char *file, + void *opaque ATTRIBUTE_UNUSED) +{ + return qemuSecurityDACSetOwnership(conn, file, driver->user, driver->group); +} + + +static int +qemuSecurityDACSetSecurityUSBLabel(virConnectPtr conn, + usbDevice *dev ATTRIBUTE_UNUSED, + const char *file, + void *opaque ATTRIBUTE_UNUSED) +{ + return qemuSecurityDACSetOwnership(conn, file, driver->user, driver->group); +} + + +static int +qemuSecurityDACSetSecurityHostdevLabel(virConnectPtr conn, + virDomainObjPtr vm, + virDomainHostdevDefPtr dev) + +{ + int ret = -1; + + if (!driver->privileged || !driver->dynamicOwnership) + return 0; + + if (dev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) + return 0; + + switch (dev->source.subsys.type) { + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: { + usbDevice *usb = usbGetDevice(conn, + dev->source.subsys.u.usb.bus, + dev->source.subsys.u.usb.device, + dev->source.subsys.u.usb.vendor, + dev->source.subsys.u.usb.product); + + if (!usb) + goto done; + + ret = usbDeviceFileIterate(conn, usb, qemuSecurityDACSetSecurityUSBLabel, vm); + usbFreeDevice(conn, usb); + break; + } + + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: { + pciDevice *pci = pciGetDevice(conn, + dev->source.subsys.u.pci.domain, + dev->source.subsys.u.pci.bus, + dev->source.subsys.u.pci.slot, + dev->source.subsys.u.pci.function); + + if (!pci) + goto done; + + ret = pciDeviceFileIterate(conn, pci, qemuSecurityDACSetSecurityPCILabel, vm); + pciFreeDevice(conn, pci); + + break; + } + + default: + ret = 0; + break; + } + +done: + return ret; +} + + +static int +qemuSecurityDACRestoreSecurityPCILabel(virConnectPtr conn, + pciDevice *dev ATTRIBUTE_UNUSED, + const char *file, + void *opaque ATTRIBUTE_UNUSED) +{ + return qemuSecurityDACRestoreSecurityFileLabel(conn, file); +} + + +static int +qemuSecurityDACRestoreSecurityUSBLabel(virConnectPtr conn, + usbDevice *dev ATTRIBUTE_UNUSED, + const char *file, + void *opaque ATTRIBUTE_UNUSED) +{ + return qemuSecurityDACRestoreSecurityFileLabel(conn, file); +} + + +static int +qemuSecurityDACRestoreSecurityHostdevLabel(virConnectPtr conn, + virDomainObjPtr vm ATTRIBUTE_UNUSED, + virDomainHostdevDefPtr dev) + +{ + int ret = -1; + + if (!driver->privileged || !driver->dynamicOwnership) + return 0; + + if (dev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) + return 0; + + switch (dev->source.subsys.type) { + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: { + usbDevice *usb = usbGetDevice(conn, + dev->source.subsys.u.usb.bus, + dev->source.subsys.u.usb.device, + dev->source.subsys.u.usb.vendor, + dev->source.subsys.u.usb.product); + + if (!usb) + goto done; + + ret = usbDeviceFileIterate(conn, usb, qemuSecurityDACRestoreSecurityUSBLabel, NULL); + usbFreeDevice(conn, usb); + + break; + } + + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: { + pciDevice *pci = pciGetDevice(conn, + dev->source.subsys.u.pci.domain, + dev->source.subsys.u.pci.bus, + dev->source.subsys.u.pci.slot, + dev->source.subsys.u.pci.function); + + if (!pci) + goto done; + + ret = pciDeviceFileIterate(conn, pci, qemuSecurityDACRestoreSecurityPCILabel, NULL); + pciFreeDevice(conn, pci); + + break; + } + + default: + ret = 0; + break; + } + +done: + return ret; +} + + +static int +qemuSecurityDACRestoreSecurityAllLabel(virConnectPtr conn, + virDomainObjPtr vm) +{ + int i; + int rc = 0; + + if (!driver->privileged || !driver->dynamicOwnership) + return 0; + + VIR_DEBUG("Restoring security label on %s", vm->def->name); + + for (i = 0 ; i < vm->def->nhostdevs ; i++) { + if (qemuSecurityDACRestoreSecurityHostdevLabel(conn, vm, + vm->def->hostdevs[i]) < 0) + rc = -1; + } + for (i = 0 ; i < vm->def->ndisks ; i++) { + if (qemuSecurityDACRestoreSecurityImageLabel(conn, vm, + vm->def->disks[i]) < 0) + rc = -1; + } + return rc; +} + + +static int +qemuSecurityDACSetSecurityAllLabel(virConnectPtr conn, + virDomainObjPtr vm) +{ + int i; + + if (!driver->privileged || !driver->dynamicOwnership) + 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) + continue; + if (qemuSecurityDACSetSecurityImageLabel(conn, vm, vm->def->disks[i]) < 0) + return -1; + } + for (i = 0 ; i < vm->def->nhostdevs ; i++) { + if (qemuSecurityDACSetSecurityHostdevLabel(conn, vm, vm->def->hostdevs[i]) < 0) + return -1; + } + + return 0; +} + + +static int +qemuSecurityDACSetSavedStateLabel(virConnectPtr conn, + virDomainObjPtr vm ATTRIBUTE_UNUSED, + const char *savefile) +{ + if (!driver->privileged || !driver->dynamicOwnership) + return 0; + + return qemuSecurityDACSetOwnership(conn, savefile, driver->user, driver->group); +} + + +static int +qemuSecurityDACRestoreSavedStateLabel(virConnectPtr conn, + virDomainObjPtr vm ATTRIBUTE_UNUSED, + const char *savefile) +{ + if (!driver->privileged || !driver->dynamicOwnership) + return 0; + + return qemuSecurityDACRestoreSecurityFileLabel(conn, savefile); +} + + +static int +qemuSecurityDACSetProcessLabel(virConnectPtr conn ATTRIBUTE_UNUSED, + virSecurityDriverPtr drv ATTRIBUTE_UNUSED, + virDomainObjPtr vm ATTRIBUTE_UNUSED) +{ + DEBUG("Dropping privileges of VM to %d:%d", driver->user, driver->group); + + if (!driver->privileged) + return 0; + + if (driver->group) { + if (setregid(driver->group, driver->group) < 0) { + virReportSystemError(NULL, errno, + _("cannot change to '%d' group"), + driver->group); + return -1; + } + } + if (driver->user) { + if (setreuid(driver->user, driver->user) < 0) { + virReportSystemError(NULL, errno, + _("cannot change to '%d' user"), + driver->user); + return -1; + } + } + + return 0; +} + + + +virSecurityDriver virqemuSecurityDACSecurityDriver = { + .name = "qemuDAC", + + .domainSetSecurityProcessLabel = qemuSecurityDACSetProcessLabel, + + .domainSetSecurityImageLabel = qemuSecurityDACSetSecurityImageLabel, + .domainRestoreSecurityImageLabel = qemuSecurityDACRestoreSecurityImageLabel, + + .domainSetSecurityAllLabel = qemuSecurityDACSetSecurityAllLabel, + .domainRestoreSecurityAllLabel = qemuSecurityDACRestoreSecurityAllLabel, + + .domainSetSecurityHostdevLabel = qemuSecurityDACSetSecurityHostdevLabel, + .domainRestoreSecurityHostdevLabel = qemuSecurityDACRestoreSecurityHostdevLabel, + + .domainSetSavedStateLabel = qemuSecurityDACSetSavedStateLabel, + .domainRestoreSavedStateLabel = qemuSecurityDACRestoreSavedStateLabel, +}; diff --git a/src/qemu/qemu_security_dac.h b/src/qemu/qemu_security_dac.h new file mode 100644 index 0000000..246ebe2 --- /dev/null +++ b/src/qemu/qemu_security_dac.h @@ -0,0 +1,22 @@ +/* + * Copyright (C) 2010 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * QEMU POSIX DAC security driver + */ + +#include "security/security_driver.h" +#include "qemu_conf.h" + +#ifndef __QEMU_SECURITY_DAC +#define __QEMU_SECURITY_DAC + +extern virSecurityDriver qemuDACSecurityDriver; + +void qemuSecurityDACSetDriver(struct qemud_driver *driver); + +#endif /* __QEMU_SECURITY_DAC */ -- 1.6.5.2

Daniel Veillard

5:19 p.m.

New subject: [libvirt] [PATCH 06/12] Introduce a new DAC security driver for QEMU

On Wed, Jan 20, 2010 at 03:15:03PM +0000, Daniel P. Berrange wrote:

...

This new security driver is responsible for managing UID/GID changes to the QEMU process, and any files/disks/devices assigned to it.

* qemu/qemu_conf.h: Add flag for disabling automatic file permission changes * qemu/qemu_security_dac.h, qemu/qemu_security_dac.c: New DAC driver for QEMU guests * Makefile.am: Add new files --- po/POTFILES.in | 1 + src/Makefile.am | 4 +- src/qemu/qemu_conf.h | 1 + src/qemu/qemu_security_dac.c | 458 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_security_dac.h | 22 ++ 5 files changed, 485 insertions(+), 1 deletions(-) create mode 100644 src/qemu/qemu_security_dac.c create mode 100644 src/qemu/qemu_security_dac.h [...] + * QEMU POSIX DAC security driver + */ +#include <config.h> +#include <selinux/selinux.h> +#include <selinux/context.h>

SELinux includes not needed there either I think Otherwise looks fine, ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

Daniel P. Berrange

3:15 p.m.

New subject: [libvirt] [PATCH 07/12] Switch QEMU driver over to use the DAC security driver

Remove all the QEMU driver calls for setting file ownership and process uid/gid. Instead wire in the QEMU DAC security driver, stacking it ontop of the primary SELinux/AppArmour driver. * qemu/qemu_driver.c: Switch over to new DAC security driver --- src/qemu/qemu_driver.c | 311 ++---------------------------------------- src/qemu/qemu_security_dac.c | 2 +- 2 files changed, 13 insertions(+), 300 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8195b74..9554852 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -69,7 +69,8 @@ #include "pci.h" #include "hostusb.h" #include "processinfo.h" -#include "security/security_driver.h" +#include "qemu_security_stacked.h" +#include "qemu_security_dac.h" #include "cgroup.h" #include "libvirt_internal.h" #include "xml.h" @@ -936,7 +937,12 @@ qemudSecurityInit(struct qemud_driver *qemud_drv) return 0; } - qemud_drv->securityDriver = security_drv; + qemuSecurityStackedSetDriver(qemud_drv); + qemuSecurityDACSetDriver(qemud_drv); + + qemud_drv->securityPrimaryDriver = security_drv; + qemud_drv->securitySecondaryDriver = &qemuDACSecurityDriver; + qemud_drv->securityDriver = &qemuStackedSecurityDriver; VIR_INFO("Initialized security driver %s", security_drv->name); @@ -2438,225 +2444,6 @@ cleanup: } -static int qemudDomainSetSecurityLabel(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm) -{ - int rc = 0; - - if (driver->securityDriver && - driver->securityDriver->domainSetSecurityProcessLabel && - driver->securityDriver->domainSetSecurityProcessLabel(conn, driver->securityDriver, vm) < 0) - rc = -1; - - return rc; -} - - -#ifdef __linux__ -struct qemuFileOwner { - uid_t uid; - gid_t gid; -}; - -static int qemuDomainSetHostdevUSBOwnershipActor(virConnectPtr conn, - usbDevice *dev ATTRIBUTE_UNUSED, - const char *file, void *opaque) -{ - struct qemuFileOwner *owner = opaque; - - VIR_DEBUG("Setting ownership on %s to %d:%d", file, owner->uid, owner->gid); - - if (chown(file, owner->uid, owner->gid) < 0) { - virReportSystemError(conn, errno, _("cannot set ownership on %s"), file); - return -1; - } - - return 0; -} - -static int qemuDomainSetHostdevUSBOwnership(virConnectPtr conn, - virDomainHostdevDefPtr def, - uid_t uid, gid_t gid) -{ - struct qemuFileOwner owner = { uid, gid }; - int ret = -1; - - usbDevice *dev = usbGetDevice(conn, - def->source.subsys.u.usb.bus, - def->source.subsys.u.usb.device, - def->source.subsys.u.usb.vendor, - def->source.subsys.u.usb.product); - - if (!dev) - goto cleanup; - - ret = usbDeviceFileIterate(conn, dev, - qemuDomainSetHostdevUSBOwnershipActor, &owner); - - usbFreeDevice(conn, dev); -cleanup: - return ret; -} - -static int qemuDomainSetHostdevPCIOwnershipActor(virConnectPtr conn, - pciDevice *dev ATTRIBUTE_UNUSED, - const char *file, void *opaque) -{ - struct qemuFileOwner *owner = opaque; - - VIR_DEBUG("Setting ownership on %s to %d:%d", file, owner->uid, owner->gid); - - if (chown(file, owner->uid, owner->gid) < 0) { - virReportSystemError(conn, errno, _("cannot set ownership on %s"), file); - return -1; - } - - return 0; -} - -static int qemuDomainSetHostdevPCIOwnership(virConnectPtr conn, - virDomainHostdevDefPtr def, - uid_t uid, gid_t gid) -{ - struct qemuFileOwner owner = { uid, gid }; - int ret = -1; - - pciDevice *dev = pciGetDevice(conn, - def->source.subsys.u.pci.domain, - def->source.subsys.u.pci.bus, - def->source.subsys.u.pci.slot, - def->source.subsys.u.pci.function); - - if (!dev) - goto cleanup; - - ret = pciDeviceFileIterate(conn, dev, - qemuDomainSetHostdevPCIOwnershipActor, &owner); - - pciFreeDevice(conn, dev); -cleanup: - return ret; -} -#endif - - -static int qemuDomainSetHostdevOwnership(virConnectPtr conn, - virDomainHostdevDefPtr def, - uid_t uid, gid_t gid) -{ - if (def->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) - return 0; - -#ifdef __linux__ - switch (def->source.subsys.type) { - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: - return qemuDomainSetHostdevUSBOwnership(conn, def, uid, gid); - - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: - return qemuDomainSetHostdevPCIOwnership(conn, def, uid, gid); - - } - return 0; -#else - qemudReportError(conn, NULL, NULL, VIR_ERR_NO_SUPPORT, "%s", - _("unable to set host device ownership on this platform")); - return -1; -#endif - -} - -static int qemuDomainSetFileOwnership(virConnectPtr conn, - const char *path, - uid_t uid, gid_t gid) -{ - - if (!path) - return 0; - - VIR_DEBUG("Setting ownership on %s to %d:%d", path, uid, gid); - if (chown(path, uid, gid) < 0) { - virReportSystemError(conn, errno, _("cannot set ownership on %s"), - path); - return -1; - } - return 0; -} - -static int qemuDomainSetDeviceOwnership(virConnectPtr conn, - struct qemud_driver *driver, - virDomainDeviceDefPtr def, - int restore) -{ - uid_t uid; - gid_t gid; - - if (!driver->privileged) - return 0; - - /* short circuit case of root:root */ - if (!driver->user && !driver->group) - return 0; - - uid = restore ? 0 : driver->user; - gid = restore ? 0 : driver->group; - - switch (def->type) { - case VIR_DOMAIN_DEVICE_DISK: - if (restore && - (def->data.disk->readonly || def->data.disk->shared)) - return 0; - - return qemuDomainSetFileOwnership(conn, def->data.disk->src, uid, gid); - - case VIR_DOMAIN_DEVICE_HOSTDEV: - return qemuDomainSetHostdevOwnership(conn, def->data.hostdev, uid, gid); - } - - return 0; -} - -static int qemuDomainSetAllDeviceOwnership(virConnectPtr conn, - struct qemud_driver *driver, - virDomainDefPtr def, - int restore) -{ - int i; - uid_t uid; - gid_t gid; - - if (!driver->privileged) - return 0; - - /* short circuit case of root:root */ - if (!driver->user && !driver->group) - return 0; - - uid = restore ? 0 : driver->user; - gid = restore ? 0 : driver->group; - - if (qemuDomainSetFileOwnership(conn, def->os.kernel, uid, gid) < 0 || - qemuDomainSetFileOwnership(conn, def->os.initrd, uid, gid) < 0) - return -1; - - for (i = 0 ; i < def->ndisks ; i++) { - if (restore && - (def->disks[i]->readonly || def->disks[i]->shared)) - continue; - - if (qemuDomainSetFileOwnership(conn, def->disks[i]->src, uid, gid) < 0) - return -1; - } - - for (i = 0 ; i < def->nhostdevs ; i++) { - if (qemuDomainSetHostdevOwnership(conn, def->hostdevs[i], uid, gid) < 0) - return -1; - } - - return 0; -} - -static virDomainPtr qemudDomainLookupByName(virConnectPtr conn, - const char *name); - struct qemudHookData { virConnectPtr conn; virDomainObjPtr vm; @@ -2669,33 +2456,11 @@ static int qemudSecurityHook(void *data) { if (qemuAddToCgroup(h->driver, h->vm->def) < 0) return -1; - if (qemudDomainSetSecurityLabel(h->conn, h->driver, h->vm) < 0) + if (h->driver->securityDriver && + h->driver->securityDriver->domainSetSecurityProcessLabel && + h->driver->securityDriver->domainSetSecurityProcessLabel(h->conn, h->driver->securityDriver, h->vm) < 0) return -1; - if (h->driver->privileged) { - if (qemuDomainSetAllDeviceOwnership(h->conn, h->driver, h->vm->def, 0) < 0) - return -1; - - DEBUG("Dropping privileges of VM to %d:%d", h->driver->user, h->driver->group); - - if (h->driver->group) { - if (setregid(h->driver->group, h->driver->group) < 0) { - virReportSystemError(NULL, errno, - _("cannot change to '%d' group"), - h->driver->group); - return -1; - } - } - if (h->driver->user) { - if (setreuid(h->driver->user, h->driver->user) < 0) { - virReportSystemError(NULL, errno, - _("cannot change to '%d' user"), - h->driver->user); - return -1; - } - } - } - return 0; } @@ -3078,10 +2843,6 @@ static void qemudShutdownVMDaemon(virConnectPtr conn, virDomainDefClearPCIAddresses(vm->def); virDomainDefClearDeviceAliases(vm->def); - if (qemuDomainSetAllDeviceOwnership(conn, driver, vm->def, 1) < 0) - VIR_WARN("Failed to restore all device ownership for %s", - vm->def->name); - qemuDomainReAttachHostDevices(conn, driver, vm->def); retry: @@ -4132,14 +3893,6 @@ static int qemudDomainSave(virDomainPtr dom, } fd = -1; - if (driver->privileged && - chown(path, driver->user, driver->group) < 0) { - virReportSystemError(NULL, errno, - _("unable to set ownership of '%s' to user %d:%d"), - path, driver->user, driver->group); - goto endjob; - } - if (driver->securityDriver && driver->securityDriver->domainSetSavedStateLabel && driver->securityDriver->domainSetSavedStateLabel(dom->conn, vm, path) == -1) @@ -4167,14 +3920,6 @@ static int qemudDomainSave(virDomainPtr dom, if (rc < 0) goto endjob; - if (driver->privileged && - chown(path, 0, 0) < 0) { - virReportSystemError(NULL, errno, - _("unable to set ownership of '%s' to user %d:%d"), - path, 0, 0); - goto endjob; - } - if (driver->securityDriver && driver->securityDriver->domainRestoreSavedStateLabel && driver->securityDriver->domainRestoreSavedStateLabel(dom->conn, vm, path) == -1) @@ -4262,14 +4007,6 @@ static int qemudDomainCoreDump(virDomainPtr dom, goto endjob; } - if (driver->privileged && - chown(path, driver->user, driver->group) < 0) { - virReportSystemError(NULL, errno, - _("unable to set ownership of '%s' to user %d:%d"), - path, driver->user, driver->group); - goto endjob; - } - if (driver->securityDriver && driver->securityDriver->domainSetSavedStateLabel && driver->securityDriver->domainSetSavedStateLabel(dom->conn, vm, path) == -1) @@ -4297,14 +4034,6 @@ static int qemudDomainCoreDump(virDomainPtr dom, qemuDomainObjExitMonitor(vm); paused |= (ret == 0); - if (driver->privileged && - chown(path, 0, 0) < 0) { - virReportSystemError(NULL, errno, - _("unable to set ownership of '%s' to user %d:%d"), - path, 0, 0); - goto endjob; - } - if (driver->securityDriver && driver->securityDriver->domainRestoreSavedStateLabel && driver->securityDriver->domainRestoreSavedStateLabel(dom->conn, vm, path) == -1) @@ -5879,8 +5608,6 @@ static int qemudDomainAttachHostDevice(virConnectPtr conn, return -1; } - if (qemuDomainSetDeviceOwnership(conn, driver, dev, 0) < 0) - return -1; if (driver->securityDriver && driver->securityDriver->domainSetSecurityHostdevLabel && driver->securityDriver->domainSetSecurityHostdevLabel(conn, vm, dev->data.hostdev) < 0) @@ -5963,9 +5690,6 @@ static int qemudDomainAttachDevice(virDomainPtr dom, driver->securityDriver->domainSetSecurityImageLabel) driver->securityDriver->domainSetSecurityImageLabel(dom->conn, vm, dev->data.disk); - if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 0) < 0) - goto endjob; - ret = qemudDomainChangeEjectableMedia(dom->conn, driver, vm, dev); break; @@ -5974,9 +5698,6 @@ static int qemudDomainAttachDevice(virDomainPtr dom, driver->securityDriver->domainSetSecurityImageLabel) driver->securityDriver->domainSetSecurityImageLabel(dom->conn, vm, dev->data.disk); - if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 0) < 0) - goto endjob; - if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_USB) { ret = qemudDomainAttachUsbMassstorageDevice(dom->conn, driver, vm, dev); } else if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) { @@ -6032,11 +5753,8 @@ cleanup: if (cgroup) virCgroupFree(&cgroup); - if (ret < 0 && dev != NULL) { - if (qemuDomainSetDeviceOwnership(dom->conn, driver, dev, 1) < 0) - VIR_WARN0("Fail to restore disk device ownership"); + if (ret < 0 && dev != NULL) virDomainDeviceDefFree(dev); - } if (vm) virDomainObjUnlock(vm); qemuDriverUnlock(driver); @@ -6365,9 +6083,6 @@ static int qemudDomainDetachHostDevice(virConnectPtr conn, driver->securityDriver->domainSetSecurityHostdevLabel(conn, vm, dev->data.hostdev) < 0) VIR_WARN0("Failed to restore device labelling"); - if (qemuDomainSetDeviceOwnership(conn, driver, dev, 1) < 0) - VIR_WARN0("Failed to restore device ownership"); - return ret; } @@ -6410,8 +6125,6 @@ static int qemudDomainDetachDevice(virDomainPtr dom, 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"); } else if (dev->type == VIR_DOMAIN_DEVICE_NET) { ret = qemudDomainDetachNetDevice(dom->conn, driver, vm, dev); } else if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) { diff --git a/src/qemu/qemu_security_dac.c b/src/qemu/qemu_security_dac.c index 6b7aab5..623aa55 100644 --- a/src/qemu/qemu_security_dac.c +++ b/src/qemu/qemu_security_dac.c @@ -439,7 +439,7 @@ qemuSecurityDACSetProcessLabel(virConnectPtr conn ATTRIBUTE_UNUSED, -virSecurityDriver virqemuSecurityDACSecurityDriver = { +virSecurityDriver qemuDACSecurityDriver = { .name = "qemuDAC", .domainSetSecurityProcessLabel = qemuSecurityDACSetProcessLabel, -- 1.6.5.2

Daniel Veillard

5:21 p.m.

New subject: [libvirt] [PATCH 07/12] Switch QEMU driver over to use the DAC security driver

On Wed, Jan 20, 2010 at 03:15:04PM +0000, Daniel P. Berrange wrote:

...

Remove all the QEMU driver calls for setting file ownership and process uid/gid. Instead wire in the QEMU DAC security driver, stacking it ontop of the primary SELinux/AppArmour driver.

* qemu/qemu_driver.c: Switch over to new DAC security driver --- src/qemu/qemu_driver.c | 311 ++---------------------------------------- src/qemu/qemu_security_dac.c | 2 +-

Daniel P. Berrange

3:15 p.m.

New subject: [libvirt] [PATCH 08/12] Add missing call to re-attach host devices if VM startup fails

If there is a problem with VM startup, PCI devices may be left assigned to pci-stub / pci-back. Adding a call to reattach host devices in the cleanup path is required. * qemu/qemu_driver.c: qemuDomainReAttachHostDevices() when VM startup fails --- src/qemu/qemu_driver.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9554852..22b6adc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2742,6 +2742,8 @@ cleanup: /* We jump here if we failed to start the VM for any reason * XXX investigate if we can kill this block and safely call * qemudShutdownVMDaemon even though no PID is running */ + qemuDomainReAttachHostDevices(conn, driver, vm->def); + if (driver->securityDriver && driver->securityDriver->domainRestoreSecurityAllLabel) driver->securityDriver->domainRestoreSecurityAllLabel(conn, vm); -- 1.6.5.2

Daniel Veillard

5:21 p.m.

New subject: [libvirt] [PATCH 08/12] Add missing call to re-attach host devices if VM startup fails

On Wed, Jan 20, 2010 at 03:15:05PM +0000, Daniel P. Berrange wrote:

...

If there is a problem with VM startup, PCI devices may be left assigned to pci-stub / pci-back. Adding a call to reattach host devices in the cleanup path is required.

* qemu/qemu_driver.c: qemuDomainReAttachHostDevices() when VM startup fails --- src/qemu/qemu_driver.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9554852..22b6adc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2742,6 +2742,8 @@ cleanup: /* We jump here if we failed to start the VM for any reason * XXX investigate if we can kill this block and safely call * qemudShutdownVMDaemon even though no PID is running */ + qemuDomainReAttachHostDevices(conn, driver, vm->def); + if (driver->securityDriver && driver->securityDriver->domainRestoreSecurityAllLabel) driver->securityDriver->domainRestoreSecurityAllLabel(conn, vm);

Daniel P. Berrange

3:15 p.m.

New subject: [libvirt] [PATCH 09/12] Fix security driver calls in hotplug cleanup paths

The hotplug code was not correctly invoking the security driver in error paths. If a hotplug attempt failed, the device would be left with VM permissions applied, rather than restored to the original permissions. Also, a CDROM media that is ejected was not restored to original permissions. Finally there was a bogus call to set hostdev permissions in the hostdev unplug code * qemu/qemu_driver.c: Fix security driver usage in hotplug/unplug --- src/qemu/qemu_driver.c | 177 +++++++++++++++++++++++++++++++++--------------- 1 files changed, 123 insertions(+), 54 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 22b6adc..5054bcf 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5126,6 +5126,11 @@ static int qemudDomainChangeEjectableMedia(virConnectPtr conn, return -1; } + if (driver->securityDriver && + driver->securityDriver->domainSetSecurityImageLabel && + driver->securityDriver->domainSetSecurityImageLabel(conn, vm, newdisk) < 0) + return -1; + qemuDomainObjPrivatePtr priv = vm->privateData; qemuDomainObjEnterMonitorWithDriver(driver, vm); if (newdisk->src) { @@ -5144,14 +5149,27 @@ static int qemudDomainChangeEjectableMedia(virConnectPtr conn, } qemuDomainObjExitMonitorWithDriver(driver, vm); - if (ret == 0) { - VIR_FREE(origdisk->src); - origdisk->src = newdisk->src; - newdisk->src = NULL; - origdisk->type = newdisk->type; - } + if (ret < 0) + goto error; + + if (driver->securityDriver && + driver->securityDriver->domainRestoreSecurityImageLabel && + driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm, origdisk) < 0) + VIR_WARN("Unable to restore security label on ejected image %s", origdisk->src); + + VIR_FREE(origdisk->src); + origdisk->src = newdisk->src; + newdisk->src = NULL; + origdisk->type = newdisk->type; return ret; + +error: + if (driver->securityDriver && + driver->securityDriver->domainRestoreSecurityImageLabel && + driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm, newdisk) < 0) + VIR_WARN("Unable to restore security label on new media %s", newdisk->src); + return -1; } @@ -5173,9 +5191,14 @@ static int qemudDomainAttachPciDiskDevice(virConnectPtr conn, } } + if (driver->securityDriver && + driver->securityDriver->domainSetSecurityImageLabel && + driver->securityDriver->domainSetSecurityImageLabel(conn, vm, dev->data.disk) < 0) + return -1; + if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0) { virReportOOMError(conn); - return -1; + goto error; } qemuDomainObjEnterMonitorWithDriver(driver, vm); @@ -5185,13 +5208,22 @@ static int qemudDomainAttachPciDiskDevice(virConnectPtr conn, &guestAddr); qemuDomainObjExitMonitorWithDriver(driver, vm); - if (ret == 0) { - dev->data.disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; - memcpy(&dev->data.disk->info.addr.pci, &guestAddr, sizeof(guestAddr)); - virDomainDiskInsertPreAlloced(vm->def, dev->data.disk); - } + if (ret < 0) + goto error; - return ret; + dev->data.disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + memcpy(&dev->data.disk->info.addr.pci, &guestAddr, sizeof(guestAddr)); + virDomainDiskInsertPreAlloced(vm->def, dev->data.disk); + + return 0; + +error: + if (driver->securityDriver && + driver->securityDriver->domainRestoreSecurityImageLabel && + driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm, dev->data.disk) < 0) + VIR_WARN("Unable to restore security label on %s", dev->data.disk->src); + + return -1; } static int qemudDomainAttachPciControllerDevice(virConnectPtr conn, @@ -5285,15 +5317,21 @@ static int qemudDomainAttachSCSIDisk(virConnectPtr conn, if (STREQ(vm->def->disks[i]->dst, dev->dst)) { qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, _("target %s already exists"), dev->dst); - goto cleanup; + return -1; } } + + if (driver->securityDriver && + driver->securityDriver->domainSetSecurityImageLabel && + driver->securityDriver->domainSetSecurityImageLabel(conn, vm, dev) < 0) + return -1; + /* This func allocates the bus/unit IDs so must be before * we search for controller */ if (!(drivestr = qemuBuildDriveStr(dev, 0, qemuCmdFlags))) - goto cleanup; + goto error; /* We should have an adddress now, so make sure */ @@ -5301,24 +5339,24 @@ static int qemudDomainAttachSCSIDisk(virConnectPtr conn, qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("unexpected disk address type %s"), virDomainDeviceAddressTypeToString(dev->info.type)); - goto cleanup; + goto error; } for (i = 0 ; i < dev->info.addr.drive.controller ; i++) { cont = qemuDomainFindOrCreateSCSIDiskController(conn, driver, vm, i); if (!cont) - goto cleanup; + goto error; } if (cont->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("SCSI controller %d was missing its PCI address"), cont->idx); - goto cleanup; + goto error; } if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0) { virReportOOMError(conn); - goto cleanup; + goto error; } qemuDomainObjEnterMonitorWithDriver(driver, vm); @@ -5326,20 +5364,28 @@ static int qemudDomainAttachSCSIDisk(virConnectPtr conn, drivestr, &cont->info.addr.pci, &driveAddr); - qemuDomainObjExitMonitorWithDriver(driver, vm); - if (ret == 0) { - /* XXX we should probably validate that the addr matches - * our existing defined addr instead of overwriting */ - dev->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; - memcpy(&dev->info.addr.drive, &driveAddr, sizeof(driveAddr)); - virDomainDiskInsertPreAlloced(vm->def, dev); - } + if (ret < 0) + goto error; -cleanup: + /* XXX we should probably validate that the addr matches + * our existing defined addr instead of overwriting */ + dev->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; + memcpy(&dev->info.addr.drive, &driveAddr, sizeof(driveAddr)); + virDomainDiskInsertPreAlloced(vm->def, dev); VIR_FREE(drivestr); - return ret; + + return 0; + +error: + VIR_FREE(drivestr); + if (driver->securityDriver && + driver->securityDriver->domainRestoreSecurityImageLabel && + driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm, dev) < 0) + VIR_WARN("Unable to restore security label on %s", dev->src); + + return -1; } @@ -5359,27 +5405,43 @@ static int qemudDomainAttachUsbMassstorageDevice(virConnectPtr conn, } } + if (driver->securityDriver && + driver->securityDriver->domainSetSecurityImageLabel && + driver->securityDriver->domainSetSecurityImageLabel(conn, vm, dev->data.disk) < 0) + return -1; + if (!dev->data.disk->src) { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, "%s", _("disk source path is missing")); - return -1; + goto error; } if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0) { virReportOOMError(conn); - return -1; + goto error; } qemuDomainObjEnterMonitorWithDriver(driver, vm); ret = qemuMonitorAddUSBDisk(priv->mon, dev->data.disk->src); qemuDomainObjExitMonitorWithDriver(driver, vm); - if (ret == 0) - virDomainDiskInsertPreAlloced(vm->def, dev->data.disk); + if (ret < 0) + goto error; - return ret; + virDomainDiskInsertPreAlloced(vm->def, dev->data.disk); + + return 0; + +error: + if (driver->securityDriver && + driver->securityDriver->domainRestoreSecurityImageLabel && + driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm, dev->data.disk) < 0) + VIR_WARN("Unable to restore security label on %s", dev->data.disk->src); + + return -1; } + static int qemudDomainAttachNetDevice(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, @@ -5617,15 +5679,31 @@ static int qemudDomainAttachHostDevice(virConnectPtr conn, switch (hostdev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: - return qemudDomainAttachHostPciDevice(conn, driver, vm, dev); + if (qemudDomainAttachHostPciDevice(conn, driver, vm, dev) < 0) + goto error; + break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: - return qemudDomainAttachHostUsbDevice(conn, driver, vm, dev); + if (qemudDomainAttachHostUsbDevice(conn, driver, vm, dev) < 0) + goto error; + break; + default: qemudReportError(conn, dom, NULL, VIR_ERR_NO_SUPPORT, _("hostdev subsys type '%s' not supported"), virDomainHostdevSubsysTypeToString(hostdev->source.subsys.type)); - return -1; + goto error; } + + return 0; + +error: + if (driver->securityDriver && + driver->securityDriver->domainRestoreSecurityHostdevLabel && + driver->securityDriver->domainRestoreSecurityHostdevLabel(conn, vm, dev->data.hostdev) < 0) + VIR_WARN0("Unable to restore host device labelling on hotplug fail"); + + return -1; } static int qemudDomainAttachDevice(virDomainPtr dom, @@ -5688,18 +5766,10 @@ 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 && - driver->securityDriver->domainSetSecurityImageLabel) - driver->securityDriver->domainSetSecurityImageLabel(dom->conn, vm, dev->data.disk); - ret = qemudDomainChangeEjectableMedia(dom->conn, driver, vm, dev); break; case VIR_DOMAIN_DISK_DEVICE_DISK: - if (driver->securityDriver && - driver->securityDriver->domainSetSecurityImageLabel) - driver->securityDriver->domainSetSecurityImageLabel(dom->conn, vm, dev->data.disk); - if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_USB) { ret = qemudDomainAttachUsbMassstorageDevice(dom->conn, driver, vm, dev); } else if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) { @@ -5815,6 +5885,11 @@ static int qemudDomainDetachPciDiskDevice(virConnectPtr conn, } virDomainDiskDefFree(detach); + if (driver->securityDriver && + driver->securityDriver->domainRestoreSecurityImageLabel && + driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm, dev->data.disk) < 0) + VIR_WARN("Unable to restore security label on %s", dev->data.disk->src); + ret = 0; cleanup: @@ -6081,9 +6156,9 @@ 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"); + driver->securityDriver->domainRestoreSecurityHostdevLabel && + driver->securityDriver->domainRestoreSecurityHostdevLabel(conn, vm, dev->data.hostdev) < 0) + VIR_WARN0("Failed to restore host device labelling"); return ret; } @@ -6124,9 +6199,6 @@ 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 && - driver->securityDriver->domainRestoreSecurityImageLabel) - driver->securityDriver->domainRestoreSecurityImageLabel(dom->conn, vm, dev->data.disk); } else if (dev->type == VIR_DOMAIN_DEVICE_NET) { ret = qemudDomainDetachNetDevice(dom->conn, driver, vm, dev); } else if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) { @@ -6140,9 +6212,6 @@ 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")); -- 1.6.5.2

Daniel Veillard

5:37 p.m.

New subject: [libvirt] [PATCH 09/12] Fix security driver calls in hotplug cleanup paths

On Wed, Jan 20, 2010 at 03:15:06PM +0000, Daniel P. Berrange wrote:

...

The hotplug code was not correctly invoking the security driver in error paths. If a hotplug attempt failed, the device would be left with VM permissions applied, rather than restored to the original permissions. Also, a CDROM media that is ejected was not restored to original permissions. Finally there was a bogus call to set hostdev permissions in the hostdev unplug code

* qemu/qemu_driver.c: Fix security driver usage in hotplug/unplug --- src/qemu/qemu_driver.c | 177 +++++++++++++++++++++++++++++++++--------------- 1 files changed, 123 insertions(+), 54 deletions(-)

...

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 22b6adc..5054bcf 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5126,6 +5126,11 @@ static int qemudDomainChangeEjectableMedia(virConnectPtr conn,

Okay the refactoring for error handling of the functions is not trivial to follow but looks fine, ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

Daniel P. Berrange

3:15 p.m.

New subject: [libvirt] [PATCH 10/12] Add configuration option to turn off dynamic permissions management

Add the ability to turn off dynamic management of file permissions for libvirt guests. * qemu/libvirtd_qemu.aug: Support 'dynamic_ownership' flag * qemu/qemu.conf: Document 'dynamic_ownership' flag. * qemu/qemu_conf.c: Load 'dynamic_ownership' flag * qemu/test_libvirtd_qemu.aug: Test 'dynamic_ownership' flag --- src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 5 +++++ src/qemu/qemu_conf.c | 13 ++++++++++--- src/qemu/test_libvirtd_qemu.aug | 4 ++++ 4 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index f0b2a5e..b71e4cf 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -32,6 +32,7 @@ module Libvirtd_qemu = | str_entry "security_driver" | str_entry "user" | str_entry "group" + | bool_entry "dynamic_ownership" | str_array_entry "cgroup_controllers" | str_array_entry "cgroup_device_acl" | str_entry "save_image_format" diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 2129bae..c662893 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -96,6 +96,11 @@ # The group ID for QEMU processes run by the system instance #group = "root" +# Whether libvirt should dynamically change file ownership +# to match the configured user/group above. Defaults to 1. +# Set to 0 to disable file ownership changes. +#dynamic_ownership = 1 + # What cgroup controllers to make use of with QEMU guests # diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 2cefa6a..34c6fdb 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -102,7 +102,9 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, char *group; int i; - /* Setup 2 critical defaults */ + /* Setup critical defaults */ + driver->dynamicOwnership = 1; + if (!(driver->vncListen = strdup("127.0.0.1"))) { virReportOOMError(NULL); return -1; @@ -224,6 +226,7 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, } VIR_FREE(user); + p = virConfGetValue (conf, "group"); CHECK_TYPE ("group", VIR_CONF_STRING); if (!(group = strdup(p && p->str ? p->str : QEMU_GROUP))) { @@ -231,8 +234,6 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, virConfFree(conf); return -1; } - - if (virGetGroupID(NULL, group, &driver->group) < 0) { VIR_FREE(group); virConfFree(conf); @@ -240,6 +241,12 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, } VIR_FREE(group); + + p = virConfGetValue (conf, "dynamic_ownership"); + CHECK_TYPE ("dynamic_ownership", VIR_CONF_LONG); + if (p) driver->dynamicOwnership = p->l; + + p = virConfGetValue (conf, "cgroup_controllers"); CHECK_TYPE ("cgroup_controllers", VIR_CONF_LIST); if (p) { diff --git a/src/qemu/test_libvirtd_qemu.aug b/src/qemu/test_libvirtd_qemu.aug index ac89438..3ce47f3 100644 --- a/src/qemu/test_libvirtd_qemu.aug +++ b/src/qemu/test_libvirtd_qemu.aug @@ -86,6 +86,8 @@ user = \"root\" group = \"root\" +dynamic_ownership = 1 + cgroup_controllers = [ \"cpu\", \"devices\" ] cgroup_device_acl = [ \"/dev/null\", \"/dev/full\", \"/dev/zero\" ] @@ -182,6 +184,8 @@ hugetlbfs_mount = \"/dev/hugepages\" { "#empty" } { "group" = "root" } { "#empty" } +{ "dynamic_ownership" = "1" } +{ "#empty" } { "cgroup_controllers" { "1" = "cpu" } { "2" = "devices" } -- 1.6.5.2

Daniel Veillard

5:42 p.m.

New subject: [libvirt] [PATCH 10/12] Add configuration option to turn off dynamic permissions management

On Wed, Jan 20, 2010 at 03:15:07PM +0000, Daniel P. Berrange wrote:

...

Add the ability to turn off dynamic management of file permissions for libvirt guests.

* qemu/libvirtd_qemu.aug: Support 'dynamic_ownership' flag * qemu/qemu.conf: Document 'dynamic_ownership' flag. * qemu/qemu_conf.c: Load 'dynamic_ownership' flag * qemu/test_libvirtd_qemu.aug: Test 'dynamic_ownership' flag --- src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 5 +++++ src/qemu/qemu_conf.c | 13 ++++++++++--- src/qemu/test_libvirtd_qemu.aug | 4 ++++ 4 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index f0b2a5e..b71e4cf 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -32,6 +32,7 @@ module Libvirtd_qemu = | str_entry "security_driver" | str_entry "user" | str_entry "group" + | bool_entry "dynamic_ownership" | str_array_entry "cgroup_controllers" | str_array_entry "cgroup_device_acl" | str_entry "save_image_format" diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 2129bae..c662893 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -96,6 +96,11 @@ # The group ID for QEMU processes run by the system instance #group = "root"

+# Whether libvirt should dynamically change file ownership +# to match the configured user/group above. Defaults to 1. +# Set to 0 to disable file ownership changes. +#dynamic_ownership = 1 +

# What cgroup controllers to make use of with QEMU guests # diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 2cefa6a..34c6fdb 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -102,7 +102,9 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, char *group; int i;

- /* Setup 2 critical defaults */ + /* Setup critical defaults */ + driver->dynamicOwnership = 1; + if (!(driver->vncListen = strdup("127.0.0.1"))) { virReportOOMError(NULL); return -1; @@ -224,6 +226,7 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, } VIR_FREE(user);

+ p = virConfGetValue (conf, "group"); CHECK_TYPE ("group", VIR_CONF_STRING); if (!(group = strdup(p && p->str ? p->str : QEMU_GROUP))) { @@ -231,8 +234,6 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, virConfFree(conf); return -1; } - - if (virGetGroupID(NULL, group, &driver->group) < 0) { VIR_FREE(group); virConfFree(conf); @@ -240,6 +241,12 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, } VIR_FREE(group);

+ + p = virConfGetValue (conf, "dynamic_ownership"); + CHECK_TYPE ("dynamic_ownership", VIR_CONF_LONG); + if (p) driver->dynamicOwnership = p->l; +

Hum, I was wondering about the value space for dynamicOwnership, it's defined as an int but so far we just use the !driver->dynamicOwnership test. So that looks fine, but I wonder if in the future we may not extend this to take more values for example depending on the type of devices. ACK Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

Daniel P. Berrange

3:15 p.m.

New subject: [libvirt] [PATCH 11/12] Fix leak in hotplug code in QEMU driver

The hotplug code in QEMU was leaking memory because although the inner device object was being moved into the main virDomainDefPtr config object, the outer container virDomainDeviceDefPtr was not. * src/qemu/qemu_driver.c: Clarify code to show that the inner device object is owned by the main domain config upon successfull attach. --- src/qemu/qemu_driver.c | 190 ++++++++++++++++++++++++++---------------------- 1 files changed, 103 insertions(+), 87 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5054bcf..cb6fe86 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5088,17 +5088,16 @@ cleanup: static int qemudDomainChangeEjectableMedia(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, - virDomainDeviceDefPtr dev) + virDomainDiskDefPtr disk) { - virDomainDiskDefPtr origdisk = NULL, newdisk; + virDomainDiskDefPtr origdisk = NULL; int i; int ret; origdisk = NULL; - newdisk = dev->data.disk; for (i = 0 ; i < vm->def->ndisks ; i++) { - if (vm->def->disks[i]->bus == newdisk->bus && - STREQ(vm->def->disks[i]->dst, newdisk->dst)) { + if (vm->def->disks[i]->bus == disk->bus && + STREQ(vm->def->disks[i]->dst, disk->dst)) { origdisk = vm->def->disks[i]; break; } @@ -5107,8 +5106,8 @@ static int qemudDomainChangeEjectableMedia(virConnectPtr conn, if (!origdisk) { qemudReportError(conn, dom, NULL, VIR_ERR_INTERNAL_ERROR, _("No device with bus '%s' and target '%s'"), - virDomainDiskBusTypeToString(newdisk->bus), - newdisk->dst); + virDomainDiskBusTypeToString(disk->bus), + disk->dst); return -1; } @@ -5122,28 +5121,28 @@ static int qemudDomainChangeEjectableMedia(virConnectPtr conn, origdisk->device != VIR_DOMAIN_DISK_DEVICE_CDROM) { qemudReportError(conn, dom, NULL, VIR_ERR_INTERNAL_ERROR, _("Removable media not supported for %s device"), - virDomainDiskDeviceTypeToString(newdisk->device)); + virDomainDiskDeviceTypeToString(disk->device)); return -1; } if (driver->securityDriver && driver->securityDriver->domainSetSecurityImageLabel && - driver->securityDriver->domainSetSecurityImageLabel(conn, vm, newdisk) < 0) + driver->securityDriver->domainSetSecurityImageLabel(conn, vm, disk) < 0) return -1; qemuDomainObjPrivatePtr priv = vm->privateData; qemuDomainObjEnterMonitorWithDriver(driver, vm); - if (newdisk->src) { + if (disk->src) { const char *format = NULL; - if (newdisk->type != VIR_DOMAIN_DISK_TYPE_DIR) { - if (newdisk->driverType) - format = newdisk->driverType; + if (disk->type != VIR_DOMAIN_DISK_TYPE_DIR) { + if (disk->driverType) + format = disk->driverType; else if (origdisk->driverType) format = origdisk->driverType; } ret = qemuMonitorChangeMedia(priv->mon, origdisk->info.alias, - newdisk->src, format); + disk->src, format); } else { ret = qemuMonitorEjectMedia(priv->mon, origdisk->info.alias); } @@ -5158,17 +5157,19 @@ static int qemudDomainChangeEjectableMedia(virConnectPtr conn, VIR_WARN("Unable to restore security label on ejected image %s", origdisk->src); VIR_FREE(origdisk->src); - origdisk->src = newdisk->src; - newdisk->src = NULL; - origdisk->type = newdisk->type; + origdisk->src = disk->src; + disk->src = NULL; + origdisk->type = disk->type; + + virDomainDiskDefFree(disk); return ret; error: if (driver->securityDriver && driver->securityDriver->domainRestoreSecurityImageLabel && - driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm, newdisk) < 0) - VIR_WARN("Unable to restore security label on new media %s", newdisk->src); + driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm, disk) < 0) + VIR_WARN("Unable to restore security label on new media %s", disk->src); return -1; } @@ -5176,24 +5177,24 @@ error: static int qemudDomainAttachPciDiskDevice(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, - virDomainDeviceDefPtr dev) + virDomainDiskDefPtr disk) { int i, ret; - const char* type = virDomainDiskBusTypeToString(dev->data.disk->bus); + const char* type = virDomainDiskBusTypeToString(disk->bus); qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDevicePCIAddress guestAddr; for (i = 0 ; i < vm->def->ndisks ; i++) { - if (STREQ(vm->def->disks[i]->dst, dev->data.disk->dst)) { + if (STREQ(vm->def->disks[i]->dst, disk->dst)) { qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, - _("target %s already exists"), dev->data.disk->dst); + _("target %s already exists"), disk->dst); return -1; } } if (driver->securityDriver && driver->securityDriver->domainSetSecurityImageLabel && - driver->securityDriver->domainSetSecurityImageLabel(conn, vm, dev->data.disk) < 0) + driver->securityDriver->domainSetSecurityImageLabel(conn, vm, disk) < 0) return -1; if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0) { @@ -5203,7 +5204,7 @@ static int qemudDomainAttachPciDiskDevice(virConnectPtr conn, qemuDomainObjEnterMonitorWithDriver(driver, vm); ret = qemuMonitorAddPCIDisk(priv->mon, - dev->data.disk->src, + disk->src, type, &guestAddr); qemuDomainObjExitMonitorWithDriver(driver, vm); @@ -5211,36 +5212,37 @@ static int qemudDomainAttachPciDiskDevice(virConnectPtr conn, if (ret < 0) goto error; - dev->data.disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; - memcpy(&dev->data.disk->info.addr.pci, &guestAddr, sizeof(guestAddr)); - virDomainDiskInsertPreAlloced(vm->def, dev->data.disk); + disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + memcpy(&disk->info.addr.pci, &guestAddr, sizeof(guestAddr)); + virDomainDiskInsertPreAlloced(vm->def, disk); return 0; error: if (driver->securityDriver && driver->securityDriver->domainRestoreSecurityImageLabel && - driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm, dev->data.disk) < 0) - VIR_WARN("Unable to restore security label on %s", dev->data.disk->src); + driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm, disk) < 0) + VIR_WARN("Unable to restore security label on %s", disk->src); return -1; } + static int qemudDomainAttachPciControllerDevice(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, - virDomainControllerDefPtr def) + virDomainControllerDefPtr controller) { int i, ret; - const char* type = virDomainControllerTypeToString(def->type); + const char* type = virDomainControllerTypeToString(controller->type); qemuDomainObjPrivatePtr priv = vm->privateData; for (i = 0 ; i < vm->def->ncontrollers ; i++) { - if ((vm->def->controllers[i]->type == def->type) && - (vm->def->controllers[i]->idx == def->idx)) { + if ((vm->def->controllers[i]->type == controller->type) && + (vm->def->controllers[i]->idx == controller->idx)) { qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, _("target %s:%d already exists"), - type, def->idx); + type, controller->idx); return -1; } } @@ -5253,17 +5255,18 @@ static int qemudDomainAttachPciControllerDevice(virConnectPtr conn, qemuDomainObjEnterMonitorWithDriver(driver, vm); ret = qemuMonitorAttachPCIDiskController(priv->mon, type, - &def->info.addr.pci); + &controller->info.addr.pci); qemuDomainObjExitMonitorWithDriver(driver, vm); if (ret == 0) { - def->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; - virDomainControllerInsertPreAlloced(vm->def, def); + controller->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + virDomainControllerInsertPreAlloced(vm->def, controller); } return ret; } + static virDomainControllerDefPtr qemuDomainFindOrCreateSCSIDiskController(virConnectPtr conn, struct qemud_driver *driver, @@ -5300,10 +5303,11 @@ qemuDomainFindOrCreateSCSIDiskController(virConnectPtr conn, return cont; } + static int qemudDomainAttachSCSIDisk(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, - virDomainDiskDefPtr dev, + virDomainDiskDefPtr disk, int qemuCmdFlags) { int i; @@ -5314,9 +5318,9 @@ static int qemudDomainAttachSCSIDisk(virConnectPtr conn, int ret = -1; for (i = 0 ; i < vm->def->ndisks ; i++) { - if (STREQ(vm->def->disks[i]->dst, dev->dst)) { + if (STREQ(vm->def->disks[i]->dst, disk->dst)) { qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED, - _("target %s already exists"), dev->dst); + _("target %s already exists"), disk->dst); return -1; } } @@ -5324,25 +5328,25 @@ static int qemudDomainAttachSCSIDisk(virConnectPtr conn, if (driver->securityDriver && driver->securityDriver->domainSetSecurityImageLabel && - driver->securityDriver->domainSetSecurityImageLabel(conn, vm, dev) < 0) + driver->securityDriver->domainSetSecurityImageLabel(conn, vm, disk) < 0) return -1; /* This func allocates the bus/unit IDs so must be before * we search for controller */ - if (!(drivestr = qemuBuildDriveStr(dev, 0, qemuCmdFlags))) + if (!(drivestr = qemuBuildDriveStr(disk, 0, qemuCmdFlags))) goto error; /* We should have an adddress now, so make sure */ - if (dev->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { + if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, _("unexpected disk address type %s"), - virDomainDeviceAddressTypeToString(dev->info.type)); + virDomainDeviceAddressTypeToString(disk->info.type)); goto error; } - for (i = 0 ; i < dev->info.addr.drive.controller ; i++) { + for (i = 0 ; i < disk->info.addr.drive.controller ; i++) { cont = qemuDomainFindOrCreateSCSIDiskController(conn, driver, vm, i); if (!cont) goto error; @@ -5371,9 +5375,9 @@ static int qemudDomainAttachSCSIDisk(virConnectPtr conn, /* XXX we should probably validate that the addr matches * our existing defined addr instead of overwriting */ - dev->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; - memcpy(&dev->info.addr.drive, &driveAddr, sizeof(driveAddr)); - virDomainDiskInsertPreAlloced(vm->def, dev); + disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; + memcpy(&disk->info.addr.drive, &driveAddr, sizeof(driveAddr)); + virDomainDiskInsertPreAlloced(vm->def, disk); VIR_FREE(drivestr); return 0; @@ -5382,8 +5386,8 @@ error: VIR_FREE(drivestr); if (driver->securityDriver && driver->securityDriver->domainRestoreSecurityImageLabel && - driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm, dev) < 0) - VIR_WARN("Unable to restore security label on %s", dev->src); + driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm, disk) < 0) + VIR_WARN("Unable to restore security label on %s", disk->src); return -1; } @@ -5392,25 +5396,25 @@ error: static int qemudDomainAttachUsbMassstorageDevice(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, - virDomainDeviceDefPtr dev) + virDomainDiskDefPtr disk) { qemuDomainObjPrivatePtr priv = vm->privateData; int i, ret; for (i = 0 ; i < vm->def->ndisks ; i++) { - if (STREQ(vm->def->disks[i]->dst, dev->data.disk->dst)) { + if (STREQ(vm->def->disks[i]->dst, disk->dst)) { qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, - _("target %s already exists"), dev->data.disk->dst); + _("target %s already exists"), disk->dst); return -1; } } if (driver->securityDriver && driver->securityDriver->domainSetSecurityImageLabel && - driver->securityDriver->domainSetSecurityImageLabel(conn, vm, dev->data.disk) < 0) + driver->securityDriver->domainSetSecurityImageLabel(conn, vm, disk) < 0) return -1; - if (!dev->data.disk->src) { + if (!disk->src) { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, "%s", _("disk source path is missing")); goto error; @@ -5422,21 +5426,21 @@ static int qemudDomainAttachUsbMassstorageDevice(virConnectPtr conn, } qemuDomainObjEnterMonitorWithDriver(driver, vm); - ret = qemuMonitorAddUSBDisk(priv->mon, dev->data.disk->src); + ret = qemuMonitorAddUSBDisk(priv->mon, disk->src); qemuDomainObjExitMonitorWithDriver(driver, vm); if (ret < 0) goto error; - virDomainDiskInsertPreAlloced(vm->def, dev->data.disk); + virDomainDiskInsertPreAlloced(vm->def, disk); return 0; error: if (driver->securityDriver && driver->securityDriver->domainRestoreSecurityImageLabel && - driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm, dev->data.disk) < 0) - VIR_WARN("Unable to restore security label on %s", dev->data.disk->src); + driver->securityDriver->domainRestoreSecurityImageLabel(conn, vm, disk) < 0) + VIR_WARN("Unable to restore security label on %s", disk->src); return -1; } @@ -5445,10 +5449,9 @@ error: static int qemudDomainAttachNetDevice(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, - virDomainDeviceDefPtr dev, + virDomainNetDefPtr net, unsigned int qemuCmdFlags) { - virDomainNetDefPtr net = dev->data.net; qemuDomainObjPrivatePtr priv = vm->privateData; char *tapfd_name = NULL; int i, tapfd = -1; @@ -5572,13 +5575,13 @@ no_memory: goto cleanup; } + static int qemudDomainAttachHostPciDevice(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, - virDomainDeviceDefPtr dev) + virDomainHostdevDefPtr hostdev) { qemuDomainObjPrivatePtr priv = vm->privateData; - virDomainHostdevDefPtr hostdev = dev->data.hostdev; pciDevice *pci; int ret; virDomainDevicePCIAddress guestAddr; @@ -5627,10 +5630,11 @@ error: return -1; } + static int qemudDomainAttachHostUsbDevice(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, - virDomainDeviceDefPtr dev) + virDomainHostdevDefPtr hostdev) { int ret; qemuDomainObjPrivatePtr priv = vm->privateData; @@ -5641,30 +5645,29 @@ static int qemudDomainAttachHostUsbDevice(virConnectPtr conn, } qemuDomainObjEnterMonitorWithDriver(driver, vm); - if (dev->data.hostdev->source.subsys.u.usb.vendor) { + if (hostdev->source.subsys.u.usb.vendor) { ret = qemuMonitorAddUSBDeviceMatch(priv->mon, - dev->data.hostdev->source.subsys.u.usb.vendor, - dev->data.hostdev->source.subsys.u.usb.product); + hostdev->source.subsys.u.usb.vendor, + hostdev->source.subsys.u.usb.product); } else { ret = qemuMonitorAddUSBDeviceExact(priv->mon, - dev->data.hostdev->source.subsys.u.usb.bus, - dev->data.hostdev->source.subsys.u.usb.device); + hostdev->source.subsys.u.usb.bus, + hostdev->source.subsys.u.usb.device); } qemuDomainObjExitMonitorWithDriver(driver, vm); - if (ret != -1) - vm->def->hostdevs[vm->def->nhostdevs++] = dev->data.hostdev; + if (ret == 0) + vm->def->hostdevs[vm->def->nhostdevs++] = hostdev; return ret; } + static int qemudDomainAttachHostDevice(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, - virDomainDeviceDefPtr dev) + virDomainHostdevDefPtr hostdev) { - virDomainHostdevDefPtr hostdev = dev->data.hostdev; - if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { qemudReportError(conn, dom, NULL, VIR_ERR_NO_SUPPORT, _("hostdev mode '%s' not supported"), @@ -5674,17 +5677,17 @@ static int qemudDomainAttachHostDevice(virConnectPtr conn, if (driver->securityDriver && driver->securityDriver->domainSetSecurityHostdevLabel && - driver->securityDriver->domainSetSecurityHostdevLabel(conn, vm, dev->data.hostdev) < 0) + driver->securityDriver->domainSetSecurityHostdevLabel(conn, vm, hostdev) < 0) return -1; switch (hostdev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: - if (qemudDomainAttachHostPciDevice(conn, driver, vm, dev) < 0) + if (qemudDomainAttachHostPciDevice(conn, driver, vm, hostdev) < 0) goto error; break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: - if (qemudDomainAttachHostUsbDevice(conn, driver, vm, dev) < 0) + if (qemudDomainAttachHostUsbDevice(conn, driver, vm, hostdev) < 0) goto error; break; @@ -5700,14 +5703,16 @@ static int qemudDomainAttachHostDevice(virConnectPtr conn, error: if (driver->securityDriver && driver->securityDriver->domainRestoreSecurityHostdevLabel && - driver->securityDriver->domainRestoreSecurityHostdevLabel(conn, vm, dev->data.hostdev) < 0) + driver->securityDriver->domainRestoreSecurityHostdevLabel(conn, vm, hostdev) < 0) VIR_WARN0("Unable to restore host device labelling on hotplug fail"); return -1; } + static int qemudDomainAttachDevice(virDomainPtr dom, - const char *xml) { + const char *xml) +{ struct qemud_driver *driver = dom->conn->privateData; virDomainObjPtr vm; virDomainDeviceDefPtr dev = NULL; @@ -5766,16 +5771,24 @@ static int qemudDomainAttachDevice(virDomainPtr dom, switch (dev->data.disk->device) { case VIR_DOMAIN_DISK_DEVICE_CDROM: case VIR_DOMAIN_DISK_DEVICE_FLOPPY: - ret = qemudDomainChangeEjectableMedia(dom->conn, driver, vm, dev); + ret = qemudDomainChangeEjectableMedia(dom->conn, driver, vm, dev->data.disk); + if (ret == 0) + dev->data.disk = NULL; break; case VIR_DOMAIN_DISK_DEVICE_DISK: if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_USB) { - ret = qemudDomainAttachUsbMassstorageDevice(dom->conn, driver, vm, dev); + ret = qemudDomainAttachUsbMassstorageDevice(dom->conn, driver, vm, dev->data.disk); + if (ret == 0) + dev->data.disk = NULL; } else if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) { - ret = qemudDomainAttachPciDiskDevice(dom->conn, driver, vm, dev); + ret = qemudDomainAttachPciDiskDevice(dom->conn, driver, vm, dev->data.disk); + if (ret == 0) + dev->data.disk = NULL; } else if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) { ret = qemudDomainAttachSCSIDisk(dom->conn, driver, vm, dev->data.disk, qemuCmdFlags); + if (ret == 0) + dev->data.disk = NULL; } else { qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT, _("disk bus '%s' cannot be hotplugged."), @@ -5804,9 +5817,13 @@ static int qemudDomainAttachDevice(virDomainPtr dom, /* fallthrough */ } } else if (dev->type == VIR_DOMAIN_DEVICE_NET) { - ret = qemudDomainAttachNetDevice(dom->conn, driver, vm, dev, qemuCmdFlags); + ret = qemudDomainAttachNetDevice(dom->conn, driver, vm, dev->data.net, qemuCmdFlags); + if (ret == 0) + dev->data.net = NULL; } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) { - ret = qemudDomainAttachHostDevice(dom->conn, driver, vm, dev); + ret = qemudDomainAttachHostDevice(dom->conn, driver, vm, dev->data.hostdev); + if (ret == 0) + dev->data.hostdev = NULL; } else { qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT, _("device type '%s' cannot be attached"), @@ -5825,8 +5842,7 @@ cleanup: if (cgroup) virCgroupFree(&cgroup); - if (ret < 0 && dev != NULL) - virDomainDeviceDefFree(dev); + virDomainDeviceDefFree(dev); if (vm) virDomainObjUnlock(vm); qemuDriverUnlock(driver); -- 1.6.5.2

Daniel Veillard

5:49 p.m.

New subject: [libvirt] [PATCH 11/12] Fix leak in hotplug code in QEMU driver

On Wed, Jan 20, 2010 at 03:15:08PM +0000, Daniel P. Berrange wrote:

...

The hotplug code in QEMU was leaking memory because although the inner device object was being moved into the main virDomainDefPtr config object, the outer container virDomainDeviceDefPtr was not.

* src/qemu/qemu_driver.c: Clarify code to show that the inner device object is owned by the main domain config upon successfull attach. --- src/qemu/qemu_driver.c | 190 ++++++++++++++++++++++++++---------------------- 1 files changed, 103 insertions(+), 87 deletions(-)

Okay, that results in a fairly large refactoring ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

Daniel P. Berrange

3:15 p.m.

New subject: [libvirt] [PATCH 12/12] Fix off-by-1 in SCSI drive hotplug

The loop looking for the controller associated with a SCI drive had an off by one, causing it to miss the last controller. * src/qemu/qemu_driver.c: Fix off-by-1 in searching for SCSI drive hotplug --- src/qemu/qemu_driver.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cb6fe86..d546975 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5346,7 +5346,7 @@ static int qemudDomainAttachSCSIDisk(virConnectPtr conn, goto error; } - for (i = 0 ; i < disk->info.addr.drive.controller ; i++) { + for (i = 0 ; i <= disk->info.addr.drive.controller ; i++) { cont = qemuDomainFindOrCreateSCSIDiskController(conn, driver, vm, i); if (!cont) goto error; -- 1.6.5.2

Daniel Veillard

5:55 p.m.

New subject: [libvirt] [PATCH 12/12] Fix off-by-1 in SCSI drive hotplug

On Wed, Jan 20, 2010 at 03:15:09PM +0000, Daniel P. Berrange wrote:

...

The loop looking for the controller associated with a SCI drive had an off by one, causing it to miss the last controller.

* src/qemu/qemu_driver.c: Fix off-by-1 in searching for SCSI drive hotplug --- src/qemu/qemu_driver.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cb6fe86..d546975 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5346,7 +5346,7 @@ static int qemudDomainAttachSCSIDisk(virConnectPtr conn, goto error; }

- for (i = 0 ; i < disk->info.addr.drive.controller ; i++) { + for (i = 0 ; i <= disk->info.addr.drive.controller ; i++) { cont = qemuDomainFindOrCreateSCSIDiskController(conn, driver, vm, i); if (!cont) goto error;

Ah, yes, it's initialized by def->info.addr.drive.controller = idx / 7; so 0 need to be checked, ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

Dan Kenigsberg

24 Jan 24 Jan

4:57 p.m.

On Wed, Jan 20, 2010 at 03:14:57PM +0000, Daniel P. Berrange wrote:

...

This patch series does some work on te security drivers, and the QEMU code for managing DAC permissions on files.

The core goal is to turn the QEMU driver DAC file management code into a security driver. Instead of QEMU calling into the SELinux/AppArmour drivers directly, a stacked driver module is introduced. This delegates all operations to first the QEMU DAC driver, and then the main SELinux/AppArmour driver. The end result is that all the permissions management code is removed from the QEMU driver, and we're left with just simple security driver calls.

In the process of this a number of flaws in the current hotplug code were found, and code was generally tidied up with a view to making it easier to manage.

Finally, we add the ability to turn off the QEMU DAC file managment code, and also deal gracefully with failures to change ownership (eg on NFS with root squash, or readonly FS).

Thanks for this series. However, it seems that we still have a problem when trying to save domain to a root-squashing nfs export. When using qemu directly, as a user with write permissions to that export, there is no problem. When using libvirt, libvirt tries to write its own state to the target file. I would not want to pre-create the target file as world redable. How about performing open(path, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR)) with the euid of the qemu process? Dan.

Laine Stump

26 Jan 26 Jan

4:40 p.m.

On 01/24/2010 11:57 AM, Dan Kenigsberg wrote:

...

Thanks for this series. However, it seems that we still have a problem when trying to save domain to a root-squashing nfs export. When using qemu directly, as a user with write permissions to that export, there is no problem. When using libvirt, libvirt tries to write its own state to the target file. I would not want to pre-create the target file as world redable.

How about performing open(path, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR)) with the euid of the qemu process?

This sounds like what I did to fix storage volume creation on NFS: https://www.redhat.com/archives/libvir-list/2010-January/msg00612.html I talked to Dan Berrange about it this morning, and he agrees that something similar can/should be done for domain save. I'm starting work on it now, and should have something you can try out within a day or two.

Laine Stump

5 Feb 5 Feb

12:43 a.m.

New subject: [libvirt] root-squash NFS and qemu domain save

On 01/24/2010 11:57 AM, Dan Kenigsberg wrote:

...

On Wed, Jan 20, 2010 at 03:14:57PM +0000, Daniel P. Berrange wrote:

...
This patch series does some work on te security drivers, and the QEMU code for managing DAC permissions on files.

The core goal is to turn the QEMU driver DAC file management code into a security driver. Instead of QEMU calling into the SELinux/AppArmour drivers directly, a stacked driver module is introduced. This delegates all operations to first the QEMU DAC driver, and then the main SELinux/AppArmour driver. The end result is that all the permissions management code is removed from the QEMU driver, and we're left with just simple security driver calls.

In the process of this a number of flaws in the current hotplug code were found, and code was generally tidied up with a view to making it easier to manage.

Finally, we add the ability to turn off the QEMU DAC file managment code, and also deal gracefully with failures to change ownership (eg on NFS with root squash, or readonly FS).

Thanks for this series. However, it seems that we still have a problem when trying to save domain to a root-squashing nfs export. When using qemu directly, as a user with write permissions to that export, there is no problem. When using libvirt, libvirt tries to write its own state to the target file. I would not want to pre-create the target file as world redable.

How about performing open(path, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR)) with the euid of the qemu process?

I came up with a patch that creates the domain state file by first forking, followed by the child process doing setuid(qemu) and all the write-to-the-file stuff that creates the file (similar to the recent virFileCreate patch). This works all the way up through qemu's appending its state onto the file. The trouble comes when qemudDomainSave calls securityDriver->domainRestoreSavedStateLabel() (in this case, qemuSecurityStackedRestoreSavedStateLabel()). That function first calls the primary security driver's qemuSecurityDACRestoreSavedStateLabel(), which will fail because it attempts to chown the file to 0,0 (to prevent other qemu instances from messing with it). That can be disabled by setting "dynamic_ownership=0" in qemu.conf (needed in several other places when stuff is stored on root-squashing NFS anyway :-/). Then the stacked security driver calls the secondary security driver's SELinuxRestoreSavedStateLabel(), which also fails because of an SELinux function (matchpathcon) returning failure. We could ignore this failure, but that would create an unnecessary security hole for all those installations that *aren't* storing their domains on root-squashing NFS. So I *think* what I'm looking for a good way to avoid the SELinux stuff when the destination file is located on a root-squashing NFS share, but not ignore it otherwise (I believe the consensus is that we *don't* want to require another configuration knob if at all possible, since that would not only lead to extra complexity of config, but also to uninformed users turning it off when not necessary). Does anyone have any good ideas for doing this (or for a different approach)?

Dan Kenigsberg

26 Jan 26 Jan

10:06 a.m.

On Wed, Jan 20, 2010 at 03:14:57PM +0000, Daniel P. Berrange wrote:

...

This patch series does some work on te security drivers, and the QEMU code for managing DAC permissions on files.

The core goal is to turn the QEMU driver DAC file management code into a security driver. Instead of QEMU calling into the SELinux/AppArmour drivers directly, a stacked driver module is introduced. This delegates all operations to first the QEMU DAC driver, and then the main SELinux/AppArmour driver. The end result is that all the permissions management code is removed from the QEMU driver, and we're left with just simple security driver calls.

In the process of this a number of flaws in the current hotplug code were found, and code was generally tidied up with a view to making it easier to manage.

Finally, we add the ability to turn off the QEMU DAC file managment code, and also deal gracefully with failures to change ownership (eg on NFS with root squash, or readonly FS).

hmmm, there's another problem which this patch set does not address: error : virStorageFileGetMetadata:415 : cannot open file '/deep/into/my/root/squashing/export': Permission denied With dynamic_ownership = 0, libvirt down not mess with chown, but it now assumes that it can read disk images. Regrards, Dan.

Daniel P. Berrange

10:23 a.m.

On Tue, Jan 26, 2010 at 12:06:58PM +0200, Dan Kenigsberg wrote:

...

On Wed, Jan 20, 2010 at 03:14:57PM +0000, Daniel P. Berrange wrote:

...
This patch series does some work on te security drivers, and the QEMU code for managing DAC permissions on files.

The core goal is to turn the QEMU driver DAC file management code into a security driver. Instead of QEMU calling into the SELinux/AppArmour drivers directly, a stacked driver module is introduced. This delegates all operations to first the QEMU DAC driver, and then the main SELinux/AppArmour driver. The end result is that all the permissions management code is removed from the QEMU driver, and we're left with just simple security driver calls.

In the process of this a number of flaws in the current hotplug code were found, and code was generally tidied up with a view to making it easier to manage.

Finally, we add the ability to turn off the QEMU DAC file managment code, and also deal gracefully with failures to change ownership (eg on NFS with root squash, or readonly FS).

hmmm, there's another problem which this patch set does not address: error : virStorageFileGetMetadata:415 : cannot open file '/deep/into/my/root/squashing/export': Permission denied

With dynamic_ownership = 0, libvirt down not mess with chown, but it now assumes that it can read disk images.

That is a not unreasonable assumption. The model we want to support is one where - App uses virStoragePool APIs to setup the NFS mount point on the host - App uses virStorageVol APIs to create the new volume with correct ownership at time of creation - libvirt starts the guest, without needing chown() due to step 2 getting ownership correct at time of creation. If libvirt can't read the directories leading upto the image, then it would never have been able to create the volume in the first place. The patchs we have done have aimed at avoiding root squash problems which prevent chmod/chown of files between root & non-root. We still assume that we can read files / browse directories. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Dan Kenigsberg

12:28 p.m.

On Tue, Jan 26, 2010 at 10:23:01AM +0000, Daniel P. Berrange wrote:

...

On Tue, Jan 26, 2010 at 12:06:58PM +0200, Dan Kenigsberg wrote:

...
On Wed, Jan 20, 2010 at 03:14:57PM +0000, Daniel P. Berrange wrote:

...
This patch series does some work on te security drivers, and the QEMU code for managing DAC permissions on files.

The core goal is to turn the QEMU driver DAC file management code into a security driver. Instead of QEMU calling into the SELinux/AppArmour drivers directly, a stacked driver module is introduced. This delegates all operations to first the QEMU DAC driver, and then the main SELinux/AppArmour driver. The end result is that all the permissions management code is removed from the QEMU driver, and we're left with just simple security driver calls.

In the process of this a number of flaws in the current hotplug code were found, and code was generally tidied up with a view to making it easier to manage.

Finally, we add the ability to turn off the QEMU DAC file managment code, and also deal gracefully with failures to change ownership (eg on NFS with root squash, or readonly FS).

hmmm, there's another problem which this patch set does not address: error : virStorageFileGetMetadata:415 : cannot open file '/deep/into/my/root/squashing/export': Permission denied

With dynamic_ownership = 0, libvirt down not mess with chown, but it now assumes that it can read disk images.

That is a not unreasonable assumption. The model we want to support is one where

- App uses virStoragePool APIs to setup the NFS mount point on the host - App uses virStorageVol APIs to create the new volume with correct ownership at time of creation - libvirt starts the guest, without needing chown() due to step 2 getting ownership correct at time of creation.

If libvirt can't read the directories leading upto the image, then it would never have been able to create the volume in the first place. The patchs we have done have aimed at avoiding root squash problems which prevent chmod/chown of files between root & non-root. We still assume that we can read files / browse directories.

Understood. But if I am handling storage myself, I have a regression when I switch to libvirt from the sordid art of playing with qemu directly. I don't want to make my repository world-readable, and I don't want (yet) to use libvirt storage API. Do I have a third option? Note that before this patchset, I could apply a patch disabling chown's and I managed to start domains over root squashing nfs. Regards, Dan.

Daniel P. Berrange

12:56 p.m.

On Tue, Jan 26, 2010 at 02:28:45PM +0200, Dan Kenigsberg wrote:

...

On Tue, Jan 26, 2010 at 10:23:01AM +0000, Daniel P. Berrange wrote:

...
On Tue, Jan 26, 2010 at 12:06:58PM +0200, Dan Kenigsberg wrote:

...
On Wed, Jan 20, 2010 at 03:14:57PM +0000, Daniel P. Berrange wrote:

...
This patch series does some work on te security drivers, and the QEMU code for managing DAC permissions on files.

The core goal is to turn the QEMU driver DAC file management code into a security driver. Instead of QEMU calling into the SELinux/AppArmour drivers directly, a stacked driver module is introduced. This delegates all operations to first the QEMU DAC driver, and then the main SELinux/AppArmour driver. The end result is that all the permissions management code is removed from the QEMU driver, and we're left with just simple security driver calls.

In the process of this a number of flaws in the current hotplug code were found, and code was generally tidied up with a view to making it easier to manage.

Finally, we add the ability to turn off the QEMU DAC file managment code, and also deal gracefully with failures to change ownership (eg on NFS with root squash, or readonly FS).

hmmm, there's another problem which this patch set does not address: error : virStorageFileGetMetadata:415 : cannot open file '/deep/into/my/root/squashing/export': Permission denied

With dynamic_ownership = 0, libvirt down not mess with chown, but it now assumes that it can read disk images.

That is a not unreasonable assumption. The model we want to support is one where

- App uses virStoragePool APIs to setup the NFS mount point on the host - App uses virStorageVol APIs to create the new volume with correct ownership at time of creation - libvirt starts the guest, without needing chown() due to step 2 getting ownership correct at time of creation.

If libvirt can't read the directories leading upto the image, then it would never have been able to create the volume in the first place. The patchs we have done have aimed at avoiding root squash problems which prevent chmod/chown of files between root & non-root. We still assume that we can read files / browse directories.

Understood. But if I am handling storage myself, I have a regression when I switch to libvirt from the sordid art of playing with qemu directly. I don't want to make my repository world-readable, and I don't want (yet) to use libvirt storage API.

Do I have a third option?

virStorageFileGetMetadata() is used to determine if the disk configured for a guest has a backing file (eg qcow linked images). This is called from the DAC driver to setup ownership, and from the SELinux driver to setup labelling. You say you've already disabled the DAC driver ownership so it shouldn't be hitting it there. Thus I assume it must the SELinux driver that's causing the error.

...

Note that before this patchset, I could apply a patch disabling chown's and I managed to start domains over root squashing nfs.

I'm surprised if that's true, because I don't believe my patches changed this behaviour of the SELinux driver - it was always calling into the virStorageFileGetMetadata() method to determine backing store, so should always have hit the issue you describe Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Dan Kenigsberg

3 p.m.

On Tue, Jan 26, 2010 at 12:56:00PM +0000, Daniel P. Berrange wrote:

...

On Tue, Jan 26, 2010 at 02:28:45PM +0200, Dan Kenigsberg wrote:

...
On Tue, Jan 26, 2010 at 10:23:01AM +0000, Daniel P. Berrange wrote:

...
On Tue, Jan 26, 2010 at 12:06:58PM +0200, Dan Kenigsberg wrote:

...
On Wed, Jan 20, 2010 at 03:14:57PM +0000, Daniel P. Berrange wrote:

...
This patch series does some work on te security drivers, and the QEMU code for managing DAC permissions on files.

The core goal is to turn the QEMU driver DAC file management code into a security driver. Instead of QEMU calling into the SELinux/AppArmour drivers directly, a stacked driver module is introduced. This delegates all operations to first the QEMU DAC driver, and then the main SELinux/AppArmour driver. The end result is that all the permissions management code is removed from the QEMU driver, and we're left with just simple security driver calls.

In the process of this a number of flaws in the current hotplug code were found, and code was generally tidied up with a view to making it easier to manage.

Finally, we add the ability to turn off the QEMU DAC file managment code, and also deal gracefully with failures to change ownership (eg on NFS with root squash, or readonly FS).

hmmm, there's another problem which this patch set does not address: error : virStorageFileGetMetadata:415 : cannot open file '/deep/into/my/root/squashing/export': Permission denied

With dynamic_ownership = 0, libvirt down not mess with chown, but it now assumes that it can read disk images.

That is a not unreasonable assumption. The model we want to support is one where

- App uses virStoragePool APIs to setup the NFS mount point on the host - App uses virStorageVol APIs to create the new volume with correct ownership at time of creation - libvirt starts the guest, without needing chown() due to step 2 getting ownership correct at time of creation.

If libvirt can't read the directories leading upto the image, then it would never have been able to create the volume in the first place. The patchs we have done have aimed at avoiding root squash problems which prevent chmod/chown of files between root & non-root. We still assume that we can read files / browse directories.

Understood. But if I am handling storage myself, I have a regression when I switch to libvirt from the sordid art of playing with qemu directly. I don't want to make my repository world-readable, and I don't want (yet) to use libvirt storage API.

Do I have a third option?

virStorageFileGetMetadata() is used to determine if the disk configured for a guest has a backing file (eg qcow linked images). This is called from the DAC driver to setup ownership, and from the SELinux driver to setup labelling. You say you've already disabled the DAC driver ownership so it shouldn't be hitting it there. Thus I assume it must the SELinux driver that's causing the error.

...
Note that before this patchset, I could apply a patch disabling chown's and I managed to start domains over root squashing nfs.

I'm surprised if that's true, because I don't believe my patches changed this behaviour of the SELinux driver - it was always calling into the virStorageFileGetMetadata() method to determine backing store, so should always have hit the issue you describe

Ok I was probably too hasty to blame this patchset, after all, I tested it earlier on a different machine with a different os and a different storage export. However my question still stands: what can I do to make this work when the storage is not readable by root?

Daniel P. Berrange

3:06 p.m.

On Tue, Jan 26, 2010 at 05:00:53PM +0200, Dan Kenigsberg wrote:

...

On Tue, Jan 26, 2010 at 12:56:00PM +0000, Daniel P. Berrange wrote:

...
On Tue, Jan 26, 2010 at 02:28:45PM +0200, Dan Kenigsberg wrote:

...
On Tue, Jan 26, 2010 at 10:23:01AM +0000, Daniel P. Berrange wrote:

...
On Tue, Jan 26, 2010 at 12:06:58PM +0200, Dan Kenigsberg wrote:

...
On Wed, Jan 20, 2010 at 03:14:57PM +0000, Daniel P. Berrange wrote:

...
This patch series does some work on te security drivers, and the QEMU code for managing DAC permissions on files.

The core goal is to turn the QEMU driver DAC file management code into a security driver. Instead of QEMU calling into the SELinux/AppArmour drivers directly, a stacked driver module is introduced. This delegates all operations to first the QEMU DAC driver, and then the main SELinux/AppArmour driver. The end result is that all the permissions management code is removed from the QEMU driver, and we're left with just simple security driver calls.

In the process of this a number of flaws in the current hotplug code were found, and code was generally tidied up with a view to making it easier to manage.

Finally, we add the ability to turn off the QEMU DAC file managment code, and also deal gracefully with failures to change ownership (eg on NFS with root squash, or readonly FS).

hmmm, there's another problem which this patch set does not address: error : virStorageFileGetMetadata:415 : cannot open file '/deep/into/my/root/squashing/export': Permission denied

With dynamic_ownership = 0, libvirt down not mess with chown, but it now assumes that it can read disk images.

That is a not unreasonable assumption. The model we want to support is one where

- App uses virStoragePool APIs to setup the NFS mount point on the host - App uses virStorageVol APIs to create the new volume with correct ownership at time of creation - libvirt starts the guest, without needing chown() due to step 2 getting ownership correct at time of creation.

If libvirt can't read the directories leading upto the image, then it would never have been able to create the volume in the first place. The patchs we have done have aimed at avoiding root squash problems which prevent chmod/chown of files between root & non-root. We still assume that we can read files / browse directories.

Understood. But if I am handling storage myself, I have a regression when I switch to libvirt from the sordid art of playing with qemu directly. I don't want to make my repository world-readable, and I don't want (yet) to use libvirt storage API.

Do I have a third option?

virStorageFileGetMetadata() is used to determine if the disk configured for a guest has a backing file (eg qcow linked images). This is called from the DAC driver to setup ownership, and from the SELinux driver to setup labelling. You say you've already disabled the DAC driver ownership so it shouldn't be hitting it there. Thus I assume it must the SELinux driver that's causing the error.

...
Note that before this patchset, I could apply a patch disabling chown's and I managed to start domains over root squashing nfs.

I'm surprised if that's true, because I don't believe my patches changed this behaviour of the SELinux driver - it was always calling into the virStorageFileGetMetadata() method to determine backing store, so should always have hit the issue you describe

Ok I was probably too hasty to blame this patchset, after all, I tested it earlier on a different machine with a different os and a different storage export.

However my question still stands: what can I do to make this work when the storage is not readable by root?

Have you tried disabling the SELinux driver ? That ought to at least stop this code being called. We might also need to make the function virStorageFileGetMetadata() treat failure to open the file as non-fatal, though I'm kind of loathe todo that. I fear all the places where we're putting in hacks to ignore errors on NFS, are going to make it harder to see / diagnose real important errors on non-NFS filesystems Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

5684

Age (days ago)

5700

Last active (days ago)

List overview

Download

37 comments

5 participants

participants (5)

Dan Kenigsberg
Daniel P. Berrange
Daniel Veillard
Laine Stump
Stephen Smalley