Hi On Fri, Feb 10, 2017 at 6:57 PM Michal Privoznik <[email protected]> wrote:
> 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 <[email protected]> > looks good to me, Reviewed-by: Marc-André Lureau <[email protected]> > --- > src/qemu/qemu.conf | 2 +- > src/qemu/qemu_cgroup.c | 53 ++++++++++++---- > src/qemu/qemu_domain.c | 124 > ++++++++++++++++++++++++------------- > src/qemu/qemu_domain.h | 5 +- > src/qemu/test_libvirtd_qemu.aug.in | 1 - > 5 files changed, 125 insertions(+), 60 deletions(-) > > diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf > index 97d769d42..9f990c20d 100644 > --- 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 19832c209..944e8dc87 100644 > --- 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 c6d32525f..530eced33 100644 > --- 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 { > @@ -6834,18 +6835,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; > @@ -6858,8 +6865,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: > @@ -6876,8 +6888,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; > > @@ -6892,8 +6905,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: > @@ -6918,9 +6930,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; > > @@ -6932,8 +6943,7 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev, > > if (!(tmpPath = (char *) virSCSIVHostDeviceGetPath(host))) > goto cleanup; > - if (perms) > - *perms = VIR_CGROUP_DEVICE_RW; > + perm = VIR_CGROUP_DEVICE_RW; > } > break; > } > @@ -6949,11 +6959,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); > @@ -7347,22 +7386,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; > } > @@ -7980,26 +8018,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; > } > @@ -8011,25 +8049,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 f81550e2f..e64aa25ba 100644 > --- 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 bd25235d3..6f03898c0 100644 > --- 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" } > -- > 2.11.0 > > -- > libvir-list mailing list > [email protected] > https://www.redhat.com/mailman/listinfo/libvir-list > -- Marc-André Lureau
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
