qemuDomainGetHostdevPath: Create /dev/vfio/vfio iff needed

So far, we are allowing /dev/vfio/vfio in the devices cgroup
unconditionally (and creating it in the namespace too). Even if
domain has no hostdev assignment configured. This is potential
security hole. Therefore, when starting the domain (or
hotplugging a hostdev) create & allow /dev/vfio/vfio too (if
needed).

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index 97d769d4234fdd1cd03e4f51fad476a13205333d..9f990c20dbce4d3ccdc14ef57531996f613f150c 100644 (file)
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -354,7 +354,7 @@
 #    "/dev/null", "/dev/full", "/dev/zero",
 #    "/dev/random", "/dev/urandom",
 #    "/dev/ptmx", "/dev/kvm", "/dev/kqemu",
-#    "/dev/rtc","/dev/hpet", "/dev/vfio/vfio"
+#    "/dev/rtc","/dev/hpet"
 #]
 #
 # RDMA migration requires the following extra files to be added to the list:

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 19832c2092f4ff7ec56cc2c86afe4c6d8ab4b2c2..944e8dc87813d929cc34ff1e57129a68eb35f6a4 100644 (file)
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -46,12 +46,13 @@ const char *const defaultDeviceACL[] = {
     "/dev/null", "/dev/full", "/dev/zero",
     "/dev/random", "/dev/urandom",
     "/dev/ptmx", "/dev/kvm", "/dev/kqemu",
-    "/dev/rtc", "/dev/hpet", "/dev/vfio/vfio",
+    "/dev/rtc", "/dev/hpet",
     NULL,
 };
 #define DEVICE_PTY_MAJOR 136
 #define DEVICE_SND_MAJOR 116
 
+#define DEV_VFIO "/dev/vfio/vfio"
 
 static int
 qemuSetupImagePathCgroup(virDomainObjPtr vm,
@@ -265,26 +266,31 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm,
                        virDomainHostdevDefPtr dev)
 {
     qemuDomainObjPrivatePtr priv = vm->privateData;
-    char *path = NULL;
-    int perms;
-    int ret = -1;
+    char **path = NULL;
+    int *perms = NULL;
+    size_t i, npaths = 0;
+    int rv, ret = -1;
 
-    if (qemuDomainGetHostdevPath(dev, &path, &perms) < 0)
+    if (qemuDomainGetHostdevPath(dev, &npaths, &path, &perms) < 0)
         goto cleanup;
 
-    if (!path) {
-        /* There's no path that we need to allow. Claim success. */
-        ret = 0;
-        goto cleanup;
+    for (i = 0; i < npaths; i++) {
+        VIR_DEBUG("Cgroup allow %s perms=%d", path[i], perms[i]);
+        rv = virCgroupAllowDevicePath(priv->cgroup, path[i], perms[i], false);
+        virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path[i],
+                                 virCgroupGetDevicePermsString(perms[i]),
+                                 ret == 0);
+        if (rv < 0)
+            goto cleanup;
     }
 
-    VIR_DEBUG("Cgroup allow %s perms=%d", path, perms);
-    ret = virCgroupAllowDevicePath(priv->cgroup, path, perms, false);
-    virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path,
-                             virCgroupGetDevicePermsString(perms), ret == 0);
+    ret = 0;
 
  cleanup:
+    for (i = 0; i < npaths; i++)
+        VIR_FREE(path[i]);
     VIR_FREE(path);
+    VIR_FREE(perms);
     return ret;
 }
 
@@ -312,6 +318,7 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm,
         case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
             if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
                 int rv;
+                size_t i, vfios = 0;
 
                 pci = virPCIDeviceNew(pcisrc->addr.domain,
                                       pcisrc->addr.bus,
@@ -330,6 +337,26 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm,
                                          "deny", path, "rwm", rv == 0);
                 if (rv < 0)
                     goto cleanup;
+
+                /* If this is the last hostdev with VFIO backend deny
+                 * /dev/vfio/vfio too. */
+                for (i = 0; i < vm->def->nhostdevs; i++) {
+                    virDomainHostdevDefPtr tmp = vm->def->hostdevs[i];
+                    if (tmp->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
+                        tmp->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI &&
+                        tmp->source.subsys.u.pci.backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO)
+                        vfios++;
+                }
+
+                if (vfios == 0) {
+                    VIR_DEBUG("Cgroup deny " DEV_VFIO " for PCI device assignment");
+                    rv = virCgroupDenyDevicePath(priv->cgroup, DEV_VFIO,
+                                                 VIR_CGROUP_DEVICE_RWM, false);
+                    virDomainAuditCgroupPath(vm, priv->cgroup,
+                                             "deny", DEV_VFIO, "rwm", rv == 0);
+                    if (rv < 0)
+                        goto cleanup;
+                }
             }
             break;
         case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 48c414ec14520b5a393bde805e51e8bfe745943d..68b954a73e3ccd8afeb78b6c0ccb570c1e7aca05 100644 (file)
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -107,6 +107,7 @@ VIR_ENUM_IMPL(qemuDomainNamespace, QEMU_DOMAIN_NS_LAST,
 
 #define PROC_MOUNTS "/proc/mounts"
 #define DEVPREFIX "/dev/"
+#define DEV_VFIO "/dev/vfio/vfio"
 
 
 struct _qemuDomainLogContext {
@@ -6846,18 +6847,24 @@ qemuDomainSupportsVideoVga(virDomainVideoDefPtr video,
 /**
  * qemuDomainGetHostdevPath:
  * @dev: host device definition
+ * @npaths: number of items in @path and @perms arrays
  * @path: resulting path to @dev
  * @perms: Optional pointer to VIR_CGROUP_DEVICE_* perms
  *
- * For given device @dev fetch its host path and store it at @path. Optionally,
- * caller can get @perms on the path (e.g. rw/ro).
+ * For given device @dev fetch its host path and store it at
+ * @path. If a device requires other paths to be present/allowed
+ * they are stored in the @path array after the actual path.
+ * Optionally, caller can get @perms on the path (e.g. rw/ro).
+ *
+ * The caller is responsible for freeing the memory.
  *
  * Returns 0 on success, -1 otherwise.
  */
 int
 qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
-                         char **path,
-                         int *perms)
+                         size_t *npaths,
+                         char ***path,
+                         int **perms)
 {
     int ret = -1;
     virDomainHostdevSubsysUSBPtr usbsrc = &dev->source.subsys.u.usb;
@@ -6870,8 +6877,13 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
     virSCSIVHostDevicePtr host = NULL;
     char *tmpPath = NULL;
     bool freeTmpPath = false;
+    bool includeVFIO = false;
+    char **tmpPaths = NULL;
+    int *tmpPerms = NULL;
+    size_t i, tmpNpaths = 0;
+    int perm = 0;
 
-    *path = NULL;
+    *npaths = 0;
 
     switch ((virDomainHostdevMode) dev->mode) {
     case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS:
@@ -6888,8 +6900,9 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
                 if (!(tmpPath = virPCIDeviceGetIOMMUGroupDev(pci)))
                     goto cleanup;
                 freeTmpPath = true;
-                if (perms)
-                    *perms = VIR_CGROUP_DEVICE_RW;
+
+                perm = VIR_CGROUP_DEVICE_RW;
+                includeVFIO = true;
             }
             break;
 
@@ -6904,8 +6917,7 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
 
             if (!(tmpPath = (char *) virUSBDeviceGetPath(usb)))
                 goto cleanup;
-            if (perms)
-                *perms = VIR_CGROUP_DEVICE_RW;
+            perm = VIR_CGROUP_DEVICE_RW;
             break;
 
         case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
@@ -6930,9 +6942,8 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
 
                 if (!(tmpPath = (char *) virSCSIDeviceGetPath(scsi)))
                     goto cleanup;
-                if (perms)
-                    *perms = virSCSIDeviceGetReadonly(scsi) ?
-                        VIR_CGROUP_DEVICE_READ : VIR_CGROUP_DEVICE_RW;
+                perm = virSCSIDeviceGetReadonly(scsi) ?
+                    VIR_CGROUP_DEVICE_READ : VIR_CGROUP_DEVICE_RW;
             }
             break;
 
@@ -6944,8 +6955,7 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
 
                 if (!(tmpPath = (char *) virSCSIVHostDeviceGetPath(host)))
                     goto cleanup;
-                if (perms)
-                    *perms = VIR_CGROUP_DEVICE_RW;
+                perm = VIR_CGROUP_DEVICE_RW;
             }
             break;
         }
@@ -6961,11 +6971,40 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
         break;
     }
 
-    if (VIR_STRDUP(*path, tmpPath) < 0)
-        goto cleanup;
+    if (tmpPath) {
+        size_t toAlloc = 1;
+
+        if (includeVFIO)
+            toAlloc = 2;
+
+        if (VIR_ALLOC_N(tmpPaths, toAlloc) < 0 ||
+            VIR_ALLOC_N(tmpPerms, toAlloc) < 0 ||
+            VIR_STRDUP(tmpPaths[0], tmpPath) < 0)
+            goto cleanup;
+        tmpNpaths = toAlloc;
+        tmpPerms[0] = perm;
+
+        if (includeVFIO) {
+            if (VIR_STRDUP(tmpPaths[1], DEV_VFIO) < 0)
+                goto cleanup;
+            tmpPerms[1] = VIR_CGROUP_DEVICE_RW;
+        }
+    }
 
+    *npaths = tmpNpaths;
+    tmpNpaths = 0;
+    *path = tmpPaths;
+    tmpPaths = NULL;
+    if (perms) {
+        *perms = tmpPerms;
+        tmpPerms = NULL;
+    }
     ret = 0;
  cleanup:
+    for (i = 0; i < tmpNpaths; i++)
+        VIR_FREE(tmpPaths[i]);
+    VIR_FREE(tmpPaths);
+    VIR_FREE(tmpPerms);
     virPCIDeviceFree(pci);
     virUSBDeviceFree(usb);
     virSCSIDeviceFree(scsi);
@@ -7369,22 +7408,21 @@ qemuDomainSetupHostdev(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
                        const char *devPath)
 {
     int ret = -1;
-    char *path = NULL;
+    char **path = NULL;
+    size_t i, npaths = 0;
 
-    if (qemuDomainGetHostdevPath(dev, &path, NULL) < 0)
+    if (qemuDomainGetHostdevPath(dev, &npaths, &path, NULL) < 0)
         goto cleanup;
 
-    if (!path) {
-        /* There's no /dev device that we need to create. Claim success. */
-        ret = 0;
-        goto cleanup;
+    for (i = 0; i < npaths; i++) {
+        if (qemuDomainCreateDevice(path[i], devPath, false) < 0)
+            goto cleanup;
     }
 
-    if (qemuDomainCreateDevice(path, devPath, false) < 0)
-        goto cleanup;
-
     ret = 0;
  cleanup:
+    for (i = 0; i < npaths; i++)
+        VIR_FREE(path[i]);
     VIR_FREE(path);
     return ret;
 }
@@ -8018,26 +8056,26 @@ qemuDomainNamespaceSetupHostdev(virQEMUDriverPtr driver,
                                 virDomainHostdevDefPtr hostdev)
 {
     int ret = -1;
-    char *path = NULL;
+    char **path = NULL;
+    size_t i, npaths = 0;
 
     if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
         return 0;
 
-    if (qemuDomainGetHostdevPath(hostdev, &path, NULL) < 0)
+    if (qemuDomainGetHostdevPath(hostdev, &npaths, &path, NULL) < 0)
         goto cleanup;
 
-    if (!path) {
-        /* There's no /dev device that we need to create. Claim success. */
-        ret = 0;
+    for (i = 0; i < npaths; i++) {
+        if (qemuDomainAttachDeviceMknod(driver,
+                                        vm,
+                                        path[i]) < 0)
         goto cleanup;
     }
 
-    if (qemuDomainAttachDeviceMknod(driver,
-                                    vm,
-                                    path) < 0)
-        goto cleanup;
     ret = 0;
  cleanup:
+    for (i = 0; i < npaths; i++)
+        VIR_FREE(path[i]);
     VIR_FREE(path);
     return ret;
 }
@@ -8049,25 +8087,25 @@ qemuDomainNamespaceTeardownHostdev(virQEMUDriverPtr driver,
                                    virDomainHostdevDefPtr hostdev)
 {
     int ret = -1;
-    char *path = NULL;
+    char **path = NULL;
+    size_t i, npaths = 0;
 
     if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
         return 0;
 
-    if (qemuDomainGetHostdevPath(hostdev, &path, NULL) < 0)
+    if (qemuDomainGetHostdevPath(hostdev, &npaths, &path, NULL) < 0)
         goto cleanup;
 
-    if (!path) {
-        /* There's no /dev device that we need to create. Claim success. */
-        ret = 0;
-        goto cleanup;
-    }
-
-    if (qemuDomainDetachDeviceUnlink(driver, vm, path) < 0)
+    /* Don't remove other paths than for the @hostdev itself.
+     * They might be still in use by other devices. */
+    if (npaths > 0 &&
+        qemuDomainDetachDeviceUnlink(driver, vm, path[0]) < 0)
         goto cleanup;
 
     ret = 0;
  cleanup:
+    for (i = 0; i < npaths; i++)
+        VIR_FREE(path[i]);
     VIR_FREE(path);
     return ret;
 }

diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 55868ce179c6efae3e46176e3b9f004f35e37af8..4216d10720ab959de7e7f273f0431efaa1a236a8 100644 (file)
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -803,8 +803,9 @@ bool qemuDomainSupportsVideoVga(virDomainVideoDefPtr video,
                                 virQEMUCapsPtr qemuCaps);
 
 int qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
-                             char **path,
-                             int *perms);
+                             size_t *npaths,
+                             char ***path,
+                             int **perms);
 
 int qemuDomainBuildNamespace(virQEMUDriverPtr driver,
                              virDomainObjPtr vm);

diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in
index bd25235d3a71327d9583892bd469964479cdfb22..6f03898c0cb43004a33571270fa5c460b0ce6727 100644 (file)
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/test_libvirtd_qemu.aug.in
@@ -55,7 +55,6 @@ module Test_libvirtd_qemu =
     { "8" = "/dev/kqemu" }
     { "9" = "/dev/rtc" }
     { "10" = "/dev/hpet" }
-    { "11" = "/dev/vfio/vfio" }
 }
 { "save_image_format" = "raw" }
 { "dump_image_format" = "raw" }