[libvirt] [PATCH 5/7] qemuDomainGetHostdevPath: Create /dev/vfio/vfio iff needed
Marc-André Lureau
marcandre.lureau at gmail.com
Thu Feb 16 12:23:46 UTC 2017
Hi
On Fri, Feb 10, 2017 at 6:57 PM Michal Privoznik <mprivozn at redhat.com>
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 <mprivozn at redhat.com>
>
looks good to me,
Reviewed-by: Marc-André Lureau <marcandre.lureau at redhat.com>
> ---
> 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
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
Marc-André Lureau
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170216/67ef289f/attachment-0001.htm>
More information about the libvir-list
mailing list