Adding audit points showed that we were granting too much privilege to qemu; it should not need any mknod rights to recreate any devices other than ptys. On the other hand, lxc should have all device privileges. The solution is adding a flag parameter. * src/util/cgroup.h (virCgroup*Device*): Adjust prototypes. * src/util/cgroup.c (virCgroupAllowDevice) (virCgroupAllowDeviceMajor, virCgroupAllowDevicePath) (virCgroupDenyDevice, virCgroupDenyDeviceMajor) (virCgroupDenyDevicePath): Add parameter. * src/qemu/qemu_cgroup.c: Update clients. * src/qemu/qemu_driver.c (qemudDomainSaveFlag): Likewise. * src/lxc/lxc_controller.c (lxcSetContainerResources): Likewise. --- https://bugzilla.redhat.com/show_bug.cgi?id=683163 was created based on my audit patches. RFC for two reasons: This patch won't apply unless you have my other pending qemu audit patches in place (I still need to post those, but ran out of time for now...) I need to do a followup patch to also restrict write access to a disk explicitly marked read-only from qemu's point of view. But I wanted to at least get this email posted to start feedback. src/lxc/lxc_controller.c | 9 +++++-- src/qemu/qemu_cgroup.c | 23 +++++++++++------- src/qemu/qemu_driver.c | 9 +++++-- src/util/cgroup.c | 55 +++++++++++++++++++++++++++++++++------------ src/util/cgroup.h | 26 ++++++++++++++++----- 5 files changed, 86 insertions(+), 36 deletions(-) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index d2b113c..296b302 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -1,5 +1,6 @@ /* - * Copyright (C) 2010 Red Hat, Inc. Copyright IBM Corp. 2008 + * Copyright (C) 2010-2011 Red Hat, Inc. + * Copyright IBM Corp. 2008 * * lxc_controller.c: linux container process controller * @@ -168,7 +169,8 @@ static int lxcSetContainerResources(virDomainDefPtr def) rc = virCgroupAllowDevice(cgroup, dev->type, dev->major, - dev->minor); + dev->minor, + VIR_CGROUP_DEVICE_RWM); if (rc != 0) { virReportSystemError(-rc, _("Unable to allow device %c:%d:%d for domain %s"), @@ -177,7 +179,8 @@ static int lxcSetContainerResources(virDomainDefPtr def) } } - rc = virCgroupAllowDeviceMajor(cgroup, 'c', LXC_DEV_MAJ_PTY); + rc = virCgroupAllowDeviceMajor(cgroup, 'c', LXC_DEV_MAJ_PTY, + VIR_CGROUP_DEVICE_RWM); if (rc != 0) { virReportSystemError(-rc, _("Unable to allow PYT devices for domain %s"), diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index ebf9ad5..3c124fd 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -66,7 +66,8 @@ qemuSetupDiskPathAllow(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, VIR_DEBUG("Process path %s for disk", path); /* XXX RO vs RW */ - rc = virCgroupAllowDevicePath(data->cgroup, path); + rc = virCgroupAllowDevicePath(data->cgroup, path, + VIR_CGROUP_DEVICE_RW); qemuAuditCgroupPath(data->vm, data->cgroup, "allow", path, rc); if (rc < 0) { if (rc == -EACCES) { /* Get this for root squash NFS */ @@ -106,8 +107,8 @@ qemuTeardownDiskPathDeny(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, int rc; VIR_DEBUG("Process path %s for disk", path); - /* XXX RO vs RW */ - rc = virCgroupDenyDevicePath(data->cgroup, path); + rc = virCgroupDenyDevicePath(data->cgroup, path, + VIR_CGROUP_DEVICE_RWM); qemuAuditCgroupPath(data->vm, data->cgroup, "deny", path, rc); if (rc < 0) { if (rc == -EACCES) { /* Get this for root squash NFS */ @@ -150,7 +151,8 @@ qemuSetupChardevCgroup(virDomainDefPtr def, VIR_DEBUG("Process path '%s' for disk", dev->source.data.file.path); - rc = virCgroupAllowDevicePath(data->cgroup, dev->source.data.file.path); + rc = virCgroupAllowDevicePath(data->cgroup, dev->source.data.file.path, + VIR_CGROUP_DEVICE_RW); qemuAuditCgroupPath(data->vm, data->cgroup, "allow", dev->source.data.file.path, rc); if (rc < 0) { @@ -172,7 +174,8 @@ int qemuSetupHostUsbDeviceCgroup(usbDevice *dev ATTRIBUTE_UNUSED, int rc; VIR_DEBUG("Process path '%s' for USB device", path); - rc = virCgroupAllowDevicePath(data->cgroup, path); + rc = virCgroupAllowDevicePath(data->cgroup, path, + VIR_CGROUP_DEVICE_RW); qemuAuditCgroupPath(data->vm, data->cgroup, "allow", path, rc); if (rc < 0) { virReportSystemError(-rc, @@ -226,7 +229,8 @@ int qemuSetupCgroup(struct qemud_driver *driver, goto cleanup; } - rc = virCgroupAllowDeviceMajor(cgroup, 'c', DEVICE_PTY_MAJOR); + rc = virCgroupAllowDeviceMajor(cgroup, 'c', DEVICE_PTY_MAJOR, + VIR_CGROUP_DEVICE_RWM); qemuAuditCgroupMajor(vm, cgroup, "allow", DEVICE_PTY_MAJOR, "pty", rc == 0); if (rc != 0) { @@ -240,7 +244,8 @@ int qemuSetupCgroup(struct qemud_driver *driver, ((vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC && driver->vncAllowHostAudio) || (vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_SDL)))) { - rc = virCgroupAllowDeviceMajor(cgroup, 'c', DEVICE_SND_MAJOR); + rc = virCgroupAllowDeviceMajor(cgroup, 'c', DEVICE_SND_MAJOR, + VIR_CGROUP_DEVICE_RWM); qemuAuditCgroupMajor(vm, cgroup, "allow", DEVICE_SND_MAJOR, "sound", rc == 0); if (rc != 0) { @@ -251,8 +256,8 @@ int qemuSetupCgroup(struct qemud_driver *driver, } for (i = 0; deviceACL[i] != NULL ; i++) { - rc = virCgroupAllowDevicePath(cgroup, - deviceACL[i]); + rc = virCgroupAllowDevicePath(cgroup, deviceACL[i], + VIR_CGROUP_DEVICE_RW); qemuAuditCgroupPath(vm, cgroup, "allow", deviceACL[i], rc); if (rc < 0 && rc != -ENOENT) { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1981cdf..7b4edc5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1962,7 +1962,8 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, vm->def->name); goto endjob; } - rc = virCgroupAllowDevicePath(cgroup, path); + rc = virCgroupAllowDevicePath(cgroup, path, + VIR_CGROUP_DEVICE_RW); qemuAuditCgroupPath(vm, cgroup, "allow", path, rc); if (rc < 0) { virReportSystemError(-rc, @@ -2012,7 +2013,8 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, VIR_WARN("failed to restore save state label on %s", path); if (cgroup != NULL) { - rc = virCgroupDenyDevicePath(cgroup, path); + rc = virCgroupDenyDevicePath(cgroup, path, + VIR_CGROUP_DEVICE_RWM); qemuAuditCgroupPath(vm, cgroup, "deny", path, rc); if (rc < 0) VIR_WARN("Unable to deny device %s for %s %d", @@ -2044,7 +2046,8 @@ endjob: } if (cgroup != NULL) { - rc = virCgroupDenyDevicePath(cgroup, path); + rc = virCgroupDenyDevicePath(cgroup, path, + VIR_CGROUP_DEVICE_RWM); qemuAuditCgroupPath(vm, cgroup, "deny", path, rc); if (rc < 0) VIR_WARN("Unable to deny device %s for %s: %d", diff --git a/src/util/cgroup.c b/src/util/cgroup.c index a16b1ab..10e3143 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -1081,7 +1081,7 @@ int virCgroupGetSwapHardLimit(virCgroupPtr group, unsigned long long *kb) /** * virCgroupDenyAllDevices: * - * @group: The cgroup to deny devices for + * @group: The cgroup to deny all permissions, for all devices * * Returns: 0 on success */ @@ -1100,15 +1100,20 @@ int virCgroupDenyAllDevices(virCgroupPtr group) * @type: The device type (i.e., 'c' or 'b') * @major: The major number of the device * @minor: The minor number of the device + * @perms: Bitwise or of VIR_CGROUP_DEVICE permission bits to allow * * Returns: 0 on success */ -int virCgroupAllowDevice(virCgroupPtr group, char type, int major, int minor) +int virCgroupAllowDevice(virCgroupPtr group, char type, int major, int minor, + int perms) { int rc; char *devstr = NULL; - if (virAsprintf(&devstr, "%c %i:%i rwm", type, major, minor) == -1) { + if (virAsprintf(&devstr, "%c %i:%i %s%s%s", type, major, minor, + perms & VIR_CGROUP_DEVICE_READ ? "r" : "", + perms & VIR_CGROUP_DEVICE_WRITE ? "w" : "", + perms & VIR_CGROUP_DEVICE_MKNOD ? "m" : "") == -1) { rc = -ENOMEM; goto out; } @@ -1129,15 +1134,20 @@ out: * @group: The cgroup to allow an entire device major type for * @type: The device type (i.e., 'c' or 'b') * @major: The major number of the device type + * @perms: Bitwise or of VIR_CGROUP_DEVICE permission bits to allow * * Returns: 0 on success */ -int virCgroupAllowDeviceMajor(virCgroupPtr group, char type, int major) +int virCgroupAllowDeviceMajor(virCgroupPtr group, char type, int major, + int perms) { int rc; char *devstr = NULL; - if (virAsprintf(&devstr, "%c %i:* rwm", type, major) == -1) { + if (virAsprintf(&devstr, "%c %i:* %s%s%s", type, major, + perms & VIR_CGROUP_DEVICE_READ ? "r" : "", + perms & VIR_CGROUP_DEVICE_WRITE ? "w" : "", + perms & VIR_CGROUP_DEVICE_MKNOD ? "m" : "") == -1) { rc = -ENOMEM; goto out; } @@ -1157,6 +1167,7 @@ int virCgroupAllowDeviceMajor(virCgroupPtr group, char type, int major) * * @group: The cgroup to allow the device for * @path: the device to allow + * @perms: Bitwise or of VIR_CGROUP_DEVICE permission bits to allow * * Queries the type of device and its major/minor number, and * adds that to the cgroup ACL @@ -1165,7 +1176,7 @@ int virCgroupAllowDeviceMajor(virCgroupPtr group, char type, int major) * negative errno value on failure */ #if defined(major) && defined(minor) -int virCgroupAllowDevicePath(virCgroupPtr group, const char *path) +int virCgroupAllowDevicePath(virCgroupPtr group, const char *path, int perms) { struct stat sb; @@ -1178,11 +1189,13 @@ int virCgroupAllowDevicePath(virCgroupPtr group, const char *path) return virCgroupAllowDevice(group, S_ISCHR(sb.st_mode) ? 'c' : 'b', major(sb.st_rdev), - minor(sb.st_rdev)); + minor(sb.st_rdev), + perms); } #else int virCgroupAllowDevicePath(virCgroupPtr group ATTRIBUTE_UNUSED, - const char *path ATTRIBUTE_UNUSED) + const char *path ATTRIBUTE_UNUSED, + int perms ATTRIBUTE_UNUSED) { return -ENOSYS; } @@ -1196,15 +1209,20 @@ int virCgroupAllowDevicePath(virCgroupPtr group ATTRIBUTE_UNUSED, * @type: The device type (i.e., 'c' or 'b') * @major: The major number of the device * @minor: The minor number of the device + * @perms: Bitwise or of VIR_CGROUP_DEVICE permission bits to deny * * Returns: 0 on success */ -int virCgroupDenyDevice(virCgroupPtr group, char type, int major, int minor) +int virCgroupDenyDevice(virCgroupPtr group, char type, int major, int minor, + int perms) { int rc; char *devstr = NULL; - if (virAsprintf(&devstr, "%c %i:%i rwm", type, major, minor) == -1) { + if (virAsprintf(&devstr, "%c %i:%i %s%s%s", type, major, minor, + perms & VIR_CGROUP_DEVICE_READ ? "r" : "", + perms & VIR_CGROUP_DEVICE_WRITE ? "w" : "", + perms & VIR_CGROUP_DEVICE_MKNOD ? "m" : "") == -1) { rc = -ENOMEM; goto out; } @@ -1225,15 +1243,20 @@ out: * @group: The cgroup to deny an entire device major type for * @type: The device type (i.e., 'c' or 'b') * @major: The major number of the device type + * @perms: Bitwise or of VIR_CGROUP_DEVICE permission bits to deny * * Returns: 0 on success */ -int virCgroupDenyDeviceMajor(virCgroupPtr group, char type, int major) +int virCgroupDenyDeviceMajor(virCgroupPtr group, char type, int major, + int perms) { int rc; char *devstr = NULL; - if (virAsprintf(&devstr, "%c %i:* rwm", type, major) == -1) { + if (virAsprintf(&devstr, "%c %i:* %s%s%s", type, major, + perms & VIR_CGROUP_DEVICE_READ ? "r" : "", + perms & VIR_CGROUP_DEVICE_WRITE ? "w" : "", + perms & VIR_CGROUP_DEVICE_MKNOD ? "m" : "") == -1) { rc = -ENOMEM; goto out; } @@ -1249,7 +1272,7 @@ int virCgroupDenyDeviceMajor(virCgroupPtr group, char type, int major) } #if defined(major) && defined(minor) -int virCgroupDenyDevicePath(virCgroupPtr group, const char *path) +int virCgroupDenyDevicePath(virCgroupPtr group, const char *path, int perms) { struct stat sb; @@ -1262,11 +1285,13 @@ int virCgroupDenyDevicePath(virCgroupPtr group, const char *path) return virCgroupDenyDevice(group, S_ISCHR(sb.st_mode) ? 'c' : 'b', major(sb.st_rdev), - minor(sb.st_rdev)); + minor(sb.st_rdev), + perms); } #else int virCgroupDenyDevicePath(virCgroupPtr group ATTRIBUTE_UNUSED, - const char *path ATTRIBUTE_UNUSED) + const char *path ATTRIBUTE_UNUSED, + int perms ATTRIBUTE_UNUSED) { return -ENOSYS; } diff --git a/src/util/cgroup.h b/src/util/cgroup.h index b3c5f27..16ffb46 100644 --- a/src/util/cgroup.h +++ b/src/util/cgroup.h @@ -60,27 +60,41 @@ int virCgroupGetMemorySoftLimit(virCgroupPtr group, unsigned long long *kb); int virCgroupSetSwapHardLimit(virCgroupPtr group, unsigned long long kb); int virCgroupGetSwapHardLimit(virCgroupPtr group, unsigned long long *kb); +enum { + VIR_CGROUP_DEVICE_READ = 1, + VIR_CGROUP_DEVICE_WRITE = 2, + VIR_CGROUP_DEVICE_MKNOD = 4, + VIR_CGROUP_DEVICE_RW = VIR_CGROUP_DEVICE_READ | VIR_CGROUP_DEVICE_WRITE, + VIR_CGROUP_DEVICE_RWM = VIR_CGROUP_DEVICE_RW | VIR_CGROUP_DEVICE_MKNOD, +}; + int virCgroupDenyAllDevices(virCgroupPtr group); int virCgroupAllowDevice(virCgroupPtr group, char type, int major, - int minor); + int minor, + int perms); int virCgroupAllowDeviceMajor(virCgroupPtr group, char type, - int major); + int major, + int perms); int virCgroupAllowDevicePath(virCgroupPtr group, - const char *path); + const char *path, + int perms); int virCgroupDenyDevice(virCgroupPtr group, char type, int major, - int minor); + int minor, + int perms); int virCgroupDenyDeviceMajor(virCgroupPtr group, char type, - int major); + int major, + int perms); int virCgroupDenyDevicePath(virCgroupPtr group, - const char *path); + const char *path, + int perms); int virCgroupSetCpuShares(virCgroupPtr group, unsigned long long shares); int virCgroupGetCpuShares(virCgroupPtr group, unsigned long long *shares); -- 1.7.4