[libvirt] [PATCH 0/2] Fix security drivers for character devices

The security drivers were currently ignoring labelling for serial, parallel, console and channel devices. This has predictably bad results if you want to use file based backends with those devices.

The parallel, serial, console and channel devices are all just character devices. A lot of code needs todo the same thing to all these devices. This provides an convenient API for iterating over all of them. * src/conf/domain_conf.c, src/conf/domain_conf.c, src/libvirt_private.syms: Add virDomainChrDefForeach --- src/conf/domain_conf.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 9 ++++++++ src/libvirt_private.syms | 1 + 3 files changed, 62 insertions(+), 0 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 64b5cf3..b8bddda 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7168,4 +7168,56 @@ int virDomainSnapshotHasChildren(virDomainSnapshotObjPtr snap, } +int virDomainChrDefForeach(virDomainDefPtr def, + bool abortOnError, + virDomainChrDefIterator iter, + void *opaque) +{ + int i; + int rc = 0; + + for (i = 0 ; i < def->nserials ; i++) { + if ((iter)(def, + def->serials[i], + opaque) < 0) + rc = -1; + + if (abortOnError && rc != 0) + goto done; + } + + for (i = 0 ; i < def->nparallels ; i++) { + if ((iter)(def, + def->parallels[i], + opaque) < 0) + rc = -1; + + if (abortOnError && rc != 0) + goto done; + } + + for (i = 0 ; i < def->nchannels ; i++) { + if ((iter)(def, + def->channels[i], + opaque) < 0) + rc = -1; + + if (abortOnError && rc != 0) + goto done; + } + if (def->console) { + if ((iter)(def, + def->console, + opaque) < 0) + rc = -1; + + if (abortOnError && rc != 0) + goto done; + } + +done: + return rc; +} + + #endif /* ! PROXY */ diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f83de83..cc271dc 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1059,6 +1059,15 @@ int virDomainObjListGetInactiveNames(virDomainObjListPtr doms, char **const names, int maxnames); +typedef int (*virDomainChrDefIterator)(virDomainDefPtr def, + virDomainChrDefPtr dev, + void *opaque); + +int virDomainChrDefForeach(virDomainDefPtr def, + bool abortOnError, + virDomainChrDefIterator iter, + void *opaque); + VIR_ENUM_DECL(virDomainVirt) VIR_ENUM_DECL(virDomainBoot) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4e61e55..778ceb1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -224,6 +224,7 @@ virDomainSnapshotDefParseString; virDomainSnapshotDefFormat; virDomainSnapshotAssignDef; virDomainObjAssignDef; +virDomainChrDefForeach; # domain_event.h -- 1.6.6.1

2010/6/24 Daniel P. Berrange <berrange@redhat.com>:
The parallel, serial, console and channel devices are all just character devices. A lot of code needs todo the same thing to all these devices. This provides an convenient API for iterating over all of them.
* src/conf/domain_conf.c, src/conf/domain_conf.c, src/libvirt_private.syms: Add virDomainChrDefForeach --- src/conf/domain_conf.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 9 ++++++++ src/libvirt_private.syms | 1 + 3 files changed, 62 insertions(+), 0 deletions(-)
ACK. Matthias

When configuring serial, parallel, console or channel devices with a file, dev or pipe backend type, it is neccessary to label the file path in the security drivers. For char devices of type file, it is neccessary to pre-create (touch) the file if it does not already exist since QEMU won't be allowed todo so itself. dev/pipe configs already require the admin to pre-create before starting the guest. * src/qemu/qemu_security_dac.c: set file ownership for character devices * src/security/security_selinux.c: Set file labelling for character devices * src/qemu/qemu_driver.c: Add character devices to cgroup ACL --- src/qemu/qemu_driver.c | 59 +++++++++++++++++++ src/qemu/qemu_security_dac.c | 117 ++++++++++++++++++++++++++++++++++++++ src/security/security_selinux.c | 119 +++++++++++++++++++++++++++++++++++++++ src/util/cgroup.c | 2 +- 4 files changed, 296 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a7b3f25..6274d4c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2950,6 +2950,28 @@ qemuPrepareHostDevices(struct qemud_driver *driver, } +static int +qemuPrepareChardevDevice(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainChrDefPtr dev, + void *opaque ATTRIBUTE_UNUSED) +{ + int fd; + if (dev->type != VIR_DOMAIN_CHR_TYPE_FILE) + return 0; + + if ((fd = open(dev->data.file.path, O_CREAT | O_APPEND, S_IRUSR|S_IWUSR)) < 0) { + virReportSystemError(errno, + _("Unable to pre-create chardev file %s"), + dev->data.file.path); + return -1; + } + + close(fd); + + return 0; +} + + static void qemudReattachManagedDevice(pciDevice *dev) { @@ -3124,6 +3146,30 @@ cleanup: } +static int qemuSetupChardevCgroup(virDomainDefPtr def, + virDomainChrDefPtr dev, + void *opaque) +{ + virCgroupPtr cgroup = opaque; + int rc; + + if (dev->type != VIR_DOMAIN_CHR_TYPE_DEV) + return 0; + + + VIR_DEBUG("Process path %s for disk", dev->data.file.path); + rc = virCgroupAllowDevicePath(cgroup, dev->data.file.path); + if (rc != 0) { + virReportSystemError(-rc, + _("Unable to allow device %s for %s"), + dev->data.file.path, def->name); + return -1; + } + + return 0; +} + + static int qemuSetupCgroup(struct qemud_driver *driver, virDomainObjPtr vm) { @@ -3191,6 +3237,12 @@ static int qemuSetupCgroup(struct qemud_driver *driver, goto cleanup; } } + + if (virDomainChrDefForeach(vm->def, + true, + qemuSetupChardevCgroup, + cgroup) < 0) + goto cleanup; } done: @@ -3364,6 +3416,13 @@ static int qemudStartVMDaemon(virConnectPtr conn, if (qemuPrepareHostDevices(driver, vm->def) < 0) goto cleanup; + DEBUG0("Preparing chr devices"); + if (virDomainChrDefForeach(vm->def, + true, + qemuPrepareChardevDevice, + NULL) < 0) + goto cleanup; + /* If you are using a SecurityDriver with dynamic labelling, then generate a security label for isolation */ DEBUG0("Generating domain security label (if required)"); diff --git a/src/qemu/qemu_security_dac.c b/src/qemu/qemu_security_dac.c index ffc9b8d..95015b0 100644 --- a/src/qemu/qemu_security_dac.c +++ b/src/qemu/qemu_security_dac.c @@ -328,6 +328,100 @@ done: static int +qemuSecurityDACSetChardevLabel(virDomainObjPtr vm, + virDomainChrDefPtr dev) + +{ + const virSecurityLabelDefPtr secdef = &vm->def->seclabel; + char *in = NULL, *out = NULL; + int ret = -1; + + if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC) + return 0; + + switch (dev->type) { + case VIR_DOMAIN_CHR_TYPE_DEV: + case VIR_DOMAIN_CHR_TYPE_FILE: + ret = qemuSecurityDACSetOwnership(dev->data.file.path, driver->user, driver->group); + break; + + case VIR_DOMAIN_CHR_TYPE_PIPE: + if ((virAsprintf(&in, "%s.in", dev->data.file.path) < 0) || + (virAsprintf(&out, "%s.out", dev->data.file.path) < 0)) { + virReportOOMError(); + goto done; + } + if ((qemuSecurityDACSetOwnership(in, driver->user, driver->group) < 0) || + (qemuSecurityDACSetOwnership(out, driver->user, driver->group) < 0)) + goto done; + ret = 0; + break; + + default: + ret = 0; + break; + } + +done: + VIR_FREE(in); + VIR_FREE(out); + return ret; +} + +static int +qemuSecurityDACRestoreChardevLabel(virDomainObjPtr vm, + virDomainChrDefPtr dev) + +{ + const virSecurityLabelDefPtr secdef = &vm->def->seclabel; + char *in = NULL, *out = NULL; + int ret = -1; + + if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC) + return 0; + + switch (dev->type) { + case VIR_DOMAIN_CHR_TYPE_DEV: + case VIR_DOMAIN_CHR_TYPE_FILE: + ret = qemuSecurityDACRestoreSecurityFileLabel(dev->data.file.path); + break; + + case VIR_DOMAIN_CHR_TYPE_PIPE: + if ((virAsprintf(&out, "%s.out", dev->data.file.path) < 0) || + (virAsprintf(&in, "%s.in", dev->data.file.path) < 0)) { + virReportOOMError(); + goto done; + } + if ((qemuSecurityDACRestoreSecurityFileLabel(out) < 0) || + (qemuSecurityDACRestoreSecurityFileLabel(in) < 0)) + goto done; + ret = 0; + break; + + default: + ret = 0; + break; + } + +done: + VIR_FREE(in); + VIR_FREE(out); + return ret; +} + + +static int +qemuSecurityDACRestoreChardevCallback(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainChrDefPtr dev, + void *opaque) +{ + virDomainObjPtr vm = opaque; + + return qemuSecurityDACRestoreChardevLabel(vm, dev); +} + + +static int qemuSecurityDACRestoreSecurityAllLabel(virDomainObjPtr vm, int migrated) { @@ -352,6 +446,12 @@ qemuSecurityDACRestoreSecurityAllLabel(virDomainObjPtr vm, rc = -1; } + if (virDomainChrDefForeach(vm->def, + false, + qemuSecurityDACRestoreChardevCallback, + vm) < 0) + rc = -1; + if (vm->def->os.kernel && qemuSecurityDACRestoreSecurityFileLabel(vm->def->os.kernel) < 0) rc = -1; @@ -365,6 +465,17 @@ qemuSecurityDACRestoreSecurityAllLabel(virDomainObjPtr vm, static int +qemuSecurityDACSetChardevCallback(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainChrDefPtr dev, + void *opaque) +{ + virDomainObjPtr vm = opaque; + + return qemuSecurityDACSetChardevLabel(vm, dev); +} + + +static int qemuSecurityDACSetSecurityAllLabel(virDomainObjPtr vm, const char *stdin_path ATTRIBUTE_UNUSED) { int i; @@ -384,6 +495,12 @@ qemuSecurityDACSetSecurityAllLabel(virDomainObjPtr vm, const char *stdin_path AT return -1; } + if (virDomainChrDefForeach(vm->def, + true, + qemuSecurityDACSetChardevCallback, + vm) < 0) + return -1; + if (vm->def->os.kernel && qemuSecurityDACSetOwnership(vm->def->os.kernel, driver->user, diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 2b43f2d..d4e2edb 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -628,6 +628,101 @@ done: return ret; } + +static int +SELinuxSetSecurityChardevLabel(virDomainObjPtr vm, + virDomainChrDefPtr dev) + +{ + const virSecurityLabelDefPtr secdef = &vm->def->seclabel; + char *in = NULL, *out = NULL; + int ret = -1; + + if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC) + return 0; + + switch (dev->type) { + case VIR_DOMAIN_CHR_TYPE_DEV: + case VIR_DOMAIN_CHR_TYPE_FILE: + ret = SELinuxSetFilecon(dev->data.file.path, secdef->imagelabel); + break; + + case VIR_DOMAIN_CHR_TYPE_PIPE: + if ((virAsprintf(&in, "%s.in", dev->data.file.path) < 0) || + (virAsprintf(&out, "%s.out", dev->data.file.path) < 0)) { + virReportOOMError(); + goto done; + } + if ((SELinuxSetFilecon(in, secdef->imagelabel) < 0) || + (SELinuxSetFilecon(out, secdef->imagelabel) < 0)) + goto done; + ret = 0; + break; + + default: + ret = 0; + break; + } + +done: + VIR_FREE(in); + VIR_FREE(out); + return ret; +} + +static int +SELinuxRestoreSecurityChardevLabel(virDomainObjPtr vm, + virDomainChrDefPtr dev) + +{ + const virSecurityLabelDefPtr secdef = &vm->def->seclabel; + char *in = NULL, *out = NULL; + int ret = -1; + + if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC) + return 0; + + switch (dev->type) { + case VIR_DOMAIN_CHR_TYPE_DEV: + case VIR_DOMAIN_CHR_TYPE_FILE: + ret = SELinuxSetFilecon(dev->data.file.path, secdef->imagelabel); + break; + + case VIR_DOMAIN_CHR_TYPE_PIPE: + if ((virAsprintf(&out, "%s.out", dev->data.file.path) < 0) || + (virAsprintf(&in, "%s.in", dev->data.file.path) < 0)) { + virReportOOMError(); + goto done; + } + if ((SELinuxRestoreSecurityFileLabel(out) < 0) || + (SELinuxRestoreSecurityFileLabel(in) < 0)) + goto done; + ret = 0; + break; + + default: + ret = 0; + break; + } + +done: + VIR_FREE(in); + VIR_FREE(out); + return ret; +} + + +static int +SELinuxRestoreSecurityChardevCallback(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainChrDefPtr dev, + void *opaque) +{ + virDomainObjPtr vm = opaque; + + return SELinuxRestoreSecurityChardevLabel(vm, dev); +} + + static int SELinuxRestoreSecurityAllLabel(virDomainObjPtr vm, int migrated ATTRIBUTE_UNUSED) @@ -652,6 +747,12 @@ SELinuxRestoreSecurityAllLabel(virDomainObjPtr vm, rc = -1; } + if (virDomainChrDefForeach(vm->def, + false, + SELinuxRestoreSecurityChardevCallback, + vm) < 0) + rc = -1; + if (vm->def->os.kernel && SELinuxRestoreSecurityFileLabel(vm->def->os.kernel) < 0) rc = -1; @@ -858,6 +959,18 @@ SELinuxClearSecuritySocketLabel(virSecurityDriverPtr drv, return 0; } + +static int +SELinuxSetSecurityChardevCallback(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainChrDefPtr dev, + void *opaque) +{ + virDomainObjPtr vm = opaque; + + return SELinuxSetSecurityChardevLabel(vm, dev); +} + + static int SELinuxSetSecurityAllLabel(virDomainObjPtr vm, const char *stdin_path ATTRIBUTE_UNUSED) { @@ -882,6 +995,12 @@ SELinuxSetSecurityAllLabel(virDomainObjPtr vm, const char *stdin_path ATTRIBUTE_ return -1; } + if (virDomainChrDefForeach(vm->def, + true, + SELinuxSetSecurityChardevCallback, + vm) < 0) + return -1; + if (vm->def->os.kernel && SELinuxSetFilecon(vm->def->os.kernel, default_content_context) < 0) return -1; diff --git a/src/util/cgroup.c b/src/util/cgroup.c index 35e9691..33610aa 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -272,7 +272,7 @@ static int virCgroupSetValueStr(virCgroupPtr group, if (rc != 0) return rc; - VIR_DEBUG("Set value %s", keypath); + VIR_DEBUG("Set value '%s' to '%s'", keypath, value); rc = virFileWriteStr(keypath, value); if (rc < 0) { DEBUG("Failed to write value '%s': %m", value); -- 1.6.6.1

2010/6/24 Daniel P. Berrange <berrange@redhat.com>:
When configuring serial, parallel, console or channel devices with a file, dev or pipe backend type, it is neccessary to label the file path in the security drivers. For char devices of type file, it is neccessary to pre-create (touch) the file if it does not already exist since QEMU won't be allowed todo so itself. dev/pipe configs already require the admin to pre-create before starting the guest.
Two typos: s/neccessary/necessary
* src/qemu/qemu_security_dac.c: set file ownership for character devices * src/security/security_selinux.c: Set file labelling for character
Another typo: s/labelling/labeling
devices * src/qemu/qemu_driver.c: Add character devices to cgroup ACL --- src/qemu/qemu_driver.c | 59 +++++++++++++++++++ src/qemu/qemu_security_dac.c | 117 ++++++++++++++++++++++++++++++++++++++ src/security/security_selinux.c | 119 +++++++++++++++++++++++++++++++++++++++ src/util/cgroup.c | 2 +- 4 files changed, 296 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a7b3f25..6274d4c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2950,6 +2950,28 @@ qemuPrepareHostDevices(struct qemud_driver *driver, }
+static int +qemuPrepareChardevDevice(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainChrDefPtr dev, + void *opaque ATTRIBUTE_UNUSED) +{ + int fd; + if (dev->type != VIR_DOMAIN_CHR_TYPE_FILE) + return 0; + + if ((fd = open(dev->data.file.path, O_CREAT | O_APPEND, S_IRUSR|S_IWUSR)) < 0) { + virReportSystemError(errno, + _("Unable to pre-create chardev file %s"),
Maybe use '%s' here instead of plain %s.
+ dev->data.file.path); + return -1; + } + + close(fd); + + return 0; +} + + static void qemudReattachManagedDevice(pciDevice *dev) { @@ -3124,6 +3146,30 @@ cleanup: }
+static int qemuSetupChardevCgroup(virDomainDefPtr def, + virDomainChrDefPtr dev, + void *opaque) +{ + virCgroupPtr cgroup = opaque; + int rc; + + if (dev->type != VIR_DOMAIN_CHR_TYPE_DEV) + return 0; + + + VIR_DEBUG("Process path %s for disk", dev->data.file.path);
Again '%s' instead of plain %s.
+ rc = virCgroupAllowDevicePath(cgroup, dev->data.file.path); + if (rc != 0) { + virReportSystemError(-rc, + _("Unable to allow device %s for %s"),
Here too. ACK. Matthias

On Thu, Jun 24, 2010 at 06:36:36PM +0200, Matthias Bolte wrote:
2010/6/24 Daniel P. Berrange <berrange@redhat.com>:
When configuring serial, parallel, console or channel devices with a file, dev or pipe backend type, it is neccessary to label the file path in the security drivers. For char devices of type file, it is neccessary to pre-create (touch) the file if it does not already exist since QEMU won't be allowed todo so itself. dev/pipe configs already require the admin to pre-create before starting the guest.
Two typos: s/neccessary/necessary
* src/qemu/qemu_security_dac.c: set file ownership for character devices * src/security/security_selinux.c: Set file labelling for character
Another typo: s/labelling/labeling
devices * src/qemu/qemu_driver.c: Add character devices to cgroup ACL --- src/qemu/qemu_driver.c | 59 +++++++++++++++++++ src/qemu/qemu_security_dac.c | 117 ++++++++++++++++++++++++++++++++++++++ src/security/security_selinux.c | 119 +++++++++++++++++++++++++++++++++++++++ src/util/cgroup.c | 2 +- 4 files changed, 296 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a7b3f25..6274d4c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2950,6 +2950,28 @@ qemuPrepareHostDevices(struct qemud_driver *driver, }
+static int +qemuPrepareChardevDevice(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainChrDefPtr dev, + void *opaque ATTRIBUTE_UNUSED) +{ + int fd; + if (dev->type != VIR_DOMAIN_CHR_TYPE_FILE) + return 0; + + if ((fd = open(dev->data.file.path, O_CREAT | O_APPEND, S_IRUSR|S_IWUSR)) < 0) { + virReportSystemError(errno, + _("Unable to pre-create chardev file %s"),
Maybe use '%s' here instead of plain %s.
Yep, added this and the others Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (2)
-
Daniel P. Berrange
-
Matthias Bolte