cgroup: allow fine-tuning of device ACL permissions

Adding audit points showed that we were granting too much privilege
to qemu; it should not need any mknod rights to recreate any
devices.  On the other hand, lxc should have all device privileges.
The solution is adding a flag parameter.

This also lets us restrict write access to read-only disks.

* src/util/cgroup.h (virCgroup*Device*): Adjust prototypes.
* src/util/cgroup.c (virCgroupAllowDevice)
(virCgroupAllowDeviceMajor, virCgroupAllowDevicePath)
(virCgroupDenyDevice, virCgroupDenyDeviceMajor)
(virCgroupDenyDevicePath): Add parameter.
* src/qemu/qemu_driver.c (qemudDomainSaveFlag): Update clients.
* src/lxc/lxc_controller.c (lxcSetContainerResources): Likewise.
* src/qemu/qemu_cgroup.c: Likewise.
(qemuSetupDiskPathAllow): Also, honor read-only disks.

src/lxc/lxc_controller.c

 /*
- * 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"),

src/qemu/qemu_cgroup.c

@@ -56,7 +56,7 @@ int qemuCgroupControllerActive(struct qemud_driver *driver,
 }
 
 static int
-qemuSetupDiskPathAllow(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED,
+qemuSetupDiskPathAllow(virDomainDiskDefPtr disk,
                        const char *path,
                        size_t depth ATTRIBUTE_UNUSED,
                        void *opaque)
@@ -65,8 +65,9 @@ qemuSetupDiskPathAllow(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED,
     int rc;
 
     VIR_DEBUG("Process path %s for disk", path);
-    /* XXX RO vs RW */
-    rc = virCgroupAllowDevicePath(data->cgroup, path);
+    rc = virCgroupAllowDevicePath(data->cgroup, path,
+                                  (disk->readonly ? VIR_CGROUP_DEVICE_READ
+                                   : 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_RW);
         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_RW);
             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) {

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",

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;
 }

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);