From nobody Sun Feb 8 19:03:14 2026 Delivered-To: importer@patchew.org Received-SPF: pass (zoho.com: domain of redhat.com designates 209.132.183.37 as permitted sender) client-ip=209.132.183.37; envelope-from=libvir-list-bounces@redhat.com; helo=mx5-phx2.redhat.com; Authentication-Results: mx.zoho.com; spf=pass (zoho.com: domain of redhat.com designates 209.132.183.37 as permitted sender) smtp.mailfrom=libvir-list-bounces@redhat.com; Return-Path: Received: from mx5-phx2.redhat.com (mx5-phx2.redhat.com [209.132.183.37]) by mx.zohomail.com with SMTPS id 1486738853167611.1760830620786; Fri, 10 Feb 2017 07:00:53 -0800 (PST) Received: from lists01.pubmisc.prod.ext.phx2.redhat.com (lists01.pubmisc.prod.ext.phx2.redhat.com [10.5.19.33]) by mx5-phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v1AEvHaW059435; Fri, 10 Feb 2017 09:57:17 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by lists01.pubmisc.prod.ext.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id v1AEv2bu007027 for ; Fri, 10 Feb 2017 09:57:02 -0500 Received: from moe.brq.redhat.com (dhcp129-131.brq.redhat.com [10.34.129.131]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v1AEuvSI016574 for ; Fri, 10 Feb 2017 09:57:02 -0500 From: Michal Privoznik To: libvir-list@redhat.com Date: Fri, 10 Feb 2017 15:56:53 +0100 Message-Id: <4cc4699801882e7c3c3733e2f87609e4cb8d18bc.1486738487.git.mprivozn@redhat.com> In-Reply-To: References: In-Reply-To: References: X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-loop: libvir-list@redhat.com Subject: [libvirt] [PATCH 5/7] qemuDomainGetHostdevPath: Create /dev/vfio/vfio iff needed X-BeenThere: libvir-list@redhat.com X-Mailman-Version: 2.1.12 Precedence: junk List-Id: Development discussions about the libvirt library & tools List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Sender: libvir-list-bounces@redhat.com Errors-To: libvir-list-bounces@redhat.com X-ZohoMail: RSF_0 Z_629925259 SPT_0 Content-Type: text/plain; charset="utf-8" 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 --- 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 lis= t: 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[] =3D { "/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 =20 +#define DEV_VFIO "/dev/vfio/vfio" =20 static int qemuSetupImagePathCgroup(virDomainObjPtr vm, @@ -265,26 +266,31 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm, virDomainHostdevDefPtr dev) { qemuDomainObjPrivatePtr priv =3D vm->privateData; - char *path =3D NULL; - int perms; - int ret =3D -1; + char **path =3D NULL; + int *perms =3D NULL; + size_t i, npaths =3D 0; + int rv, ret =3D -1; =20 - if (qemuDomainGetHostdevPath(dev, &path, &perms) < 0) + if (qemuDomainGetHostdevPath(dev, &npaths, &path, &perms) < 0) goto cleanup; =20 - if (!path) { - /* There's no path that we need to allow. Claim success. */ - ret =3D 0; - goto cleanup; + for (i =3D 0; i < npaths; i++) { + VIR_DEBUG("Cgroup allow %s perms=3D%d", path[i], perms[i]); + rv =3D virCgroupAllowDevicePath(priv->cgroup, path[i], perms[i], f= alse); + virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path[i], + virCgroupGetDevicePermsString(perms[i]), + ret =3D=3D 0); + if (rv < 0) + goto cleanup; } =20 - VIR_DEBUG("Cgroup allow %s perms=3D%d", path, perms); - ret =3D virCgroupAllowDevicePath(priv->cgroup, path, perms, false); - virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path, - virCgroupGetDevicePermsString(perms), ret =3D= =3D 0); + ret =3D 0; =20 cleanup: + for (i =3D 0; i < npaths; i++) + VIR_FREE(path[i]); VIR_FREE(path); + VIR_FREE(perms); return ret; } =20 @@ -312,6 +318,7 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm, case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: if (pcisrc->backend =3D=3D VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO= ) { int rv; + size_t i, vfios =3D 0; =20 pci =3D virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus, @@ -330,6 +337,26 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm, "deny", path, "rwm", rv =3D=3D 0); if (rv < 0) goto cleanup; + + /* If this is the last hostdev with VFIO backend deny + * /dev/vfio/vfio too. */ + for (i =3D 0; i < vm->def->nhostdevs; i++) { + virDomainHostdevDefPtr tmp =3D vm->def->hostdevs[i]; + if (tmp->mode =3D=3D VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && + tmp->source.subsys.type =3D=3D VIR_DOMAIN_HOSTDEV_= SUBSYS_TYPE_PCI && + tmp->source.subsys.u.pci.backend =3D=3D VIR_DOMAIN= _HOSTDEV_PCI_BACKEND_VFIO) + vfios++; + } + + if (vfios =3D=3D 0) { + VIR_DEBUG("Cgroup deny " DEV_VFIO " for PCI device ass= ignment"); + rv =3D virCgroupDenyDevicePath(priv->cgroup, DEV_VFIO, + VIR_CGROUP_DEVICE_RWM, fa= lse); + virDomainAuditCgroupPath(vm, priv->cgroup, + "deny", DEV_VFIO, "rwm", rv = =3D=3D 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, =20 #define PROC_MOUNTS "/proc/mounts" #define DEVPREFIX "/dev/" +#define DEV_VFIO "/dev/vfio/vfio" =20 =20 struct _qemuDomainLogContext { @@ -6834,18 +6835,24 @@ qemuDomainSupportsVideoVga(virDomainVideoDefPtr vid= eo, /** * 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. Option= ally, - * 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 =3D -1; virDomainHostdevSubsysUSBPtr usbsrc =3D &dev->source.subsys.u.usb; @@ -6858,8 +6865,13 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev, virSCSIVHostDevicePtr host =3D NULL; char *tmpPath =3D NULL; bool freeTmpPath =3D false; + bool includeVFIO =3D false; + char **tmpPaths =3D NULL; + int *tmpPerms =3D NULL; + size_t i, tmpNpaths =3D 0; + int perm =3D 0; =20 - *path =3D NULL; + *npaths =3D 0; =20 switch ((virDomainHostdevMode) dev->mode) { case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS: @@ -6876,8 +6888,9 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev, if (!(tmpPath =3D virPCIDeviceGetIOMMUGroupDev(pci))) goto cleanup; freeTmpPath =3D true; - if (perms) - *perms =3D VIR_CGROUP_DEVICE_RW; + + perm =3D VIR_CGROUP_DEVICE_RW; + includeVFIO =3D true; } break; =20 @@ -6892,8 +6905,7 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev, =20 if (!(tmpPath =3D (char *) virUSBDeviceGetPath(usb))) goto cleanup; - if (perms) - *perms =3D VIR_CGROUP_DEVICE_RW; + perm =3D VIR_CGROUP_DEVICE_RW; break; =20 case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: @@ -6918,9 +6930,8 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev, =20 if (!(tmpPath =3D (char *) virSCSIDeviceGetPath(scsi))) goto cleanup; - if (perms) - *perms =3D virSCSIDeviceGetReadonly(scsi) ? - VIR_CGROUP_DEVICE_READ :VIR_CGROUP_DEVICE_RW; + perm =3D virSCSIDeviceGetReadonly(scsi) ? + VIR_CGROUP_DEVICE_READ :VIR_CGROUP_DEVICE_RW; } break; =20 @@ -6932,8 +6943,7 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev, =20 if (!(tmpPath =3D (char *) virSCSIVHostDeviceGetPath(host)= )) goto cleanup; - if (perms) - *perms =3D VIR_CGROUP_DEVICE_RW; + perm =3D VIR_CGROUP_DEVICE_RW; } break; } @@ -6949,11 +6959,40 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev, break; } =20 - if (VIR_STRDUP(*path, tmpPath) < 0) - goto cleanup; + if (tmpPath) { + size_t toAlloc =3D 1; =20 + if (includeVFIO) + toAlloc =3D 2; + + if (VIR_ALLOC_N(tmpPaths, toAlloc) < 0 || + VIR_ALLOC_N(tmpPerms, toAlloc) < 0 || + VIR_STRDUP(tmpPaths[0], tmpPath) < 0) + goto cleanup; + tmpNpaths =3D toAlloc; + tmpPerms[0] =3D perm; + + if (includeVFIO) { + if (VIR_STRDUP(tmpPaths[1], DEV_VFIO) < 0) + goto cleanup; + tmpPerms[1] =3D VIR_CGROUP_DEVICE_RW; + } + } + + *npaths =3D tmpNpaths; + tmpNpaths =3D 0; + *path =3D tmpPaths; + tmpPaths =3D NULL; + if (perms) { + *perms =3D tmpPerms; + tmpPerms =3D NULL; + } ret =3D 0; cleanup: + for (i =3D 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 ATTR= IBUTE_UNUSED, const char *devPath) { int ret =3D -1; - char *path =3D NULL; + char **path =3D NULL; + size_t i, npaths =3D 0; =20 - if (qemuDomainGetHostdevPath(dev, &path, NULL) < 0) + if (qemuDomainGetHostdevPath(dev, &npaths, &path, NULL) < 0) goto cleanup; =20 - if (!path) { - /* There's no /dev device that we need to create. Claim success. */ - ret =3D 0; - goto cleanup; + for (i =3D 0; i < npaths; i++) { + if (qemuDomainCreateDevice(path[i], devPath, false) < 0) + goto cleanup; } =20 - if (qemuDomainCreateDevice(path, devPath, false) < 0) - goto cleanup; - ret =3D 0; cleanup: + for (i =3D 0; i < npaths; i++) + VIR_FREE(path[i]); VIR_FREE(path); return ret; } @@ -7980,26 +8018,26 @@ qemuDomainNamespaceSetupHostdev(virQEMUDriverPtr dr= iver, virDomainHostdevDefPtr hostdev) { int ret =3D -1; - char *path =3D NULL; + char **path =3D NULL; + size_t i, npaths =3D 0; =20 if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) return 0; =20 - if (qemuDomainGetHostdevPath(hostdev, &path, NULL) < 0) + if (qemuDomainGetHostdevPath(hostdev, &npaths, &path, NULL) < 0) goto cleanup; =20 - if (!path) { - /* There's no /dev device that we need to create. Claim success. */ - ret =3D 0; + for (i =3D 0; i < npaths; i++) { + if (qemuDomainAttachDeviceMknod(driver, + vm, + path[i]) < 0) goto cleanup; } =20 - if (qemuDomainAttachDeviceMknod(driver, - vm, - path) < 0) - goto cleanup; ret =3D 0; cleanup: + for (i =3D 0; i < npaths; i++) + VIR_FREE(path[i]); VIR_FREE(path); return ret; } @@ -8011,25 +8049,25 @@ qemuDomainNamespaceTeardownHostdev(virQEMUDriverPtr= driver, virDomainHostdevDefPtr hostdev) { int ret =3D -1; - char *path =3D NULL; + char **path =3D NULL; + size_t i, npaths =3D 0; =20 if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) return 0; =20 - if (qemuDomainGetHostdevPath(hostdev, &path, NULL) < 0) + if (qemuDomainGetHostdevPath(hostdev, &npaths, &path, NULL) < 0) goto cleanup; =20 - if (!path) { - /* There's no /dev device that we need to create. Claim success. */ - ret =3D 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; =20 ret =3D 0; cleanup: + for (i =3D 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 vi= deo, virQEMUCapsPtr qemuCaps); =20 int qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev, - char **path, - int *perms); + size_t *npaths, + char ***path, + int **perms); =20 int qemuDomainBuildNamespace(virQEMUDriverPtr driver, virDomainObjPtr vm); diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qe= mu.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 =3D { "8" =3D "/dev/kqemu" } { "9" =3D "/dev/rtc" } { "10" =3D "/dev/hpet" } - { "11" =3D "/dev/vfio/vfio" } } { "save_image_format" =3D "raw" } { "dump_image_format" =3D "raw" } --=20 2.11.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list