From: Nathan Chen <nathanc@nvidia.com>
When launching a qemu VM with the iommufd feature enabled for VFIO
hostdevs:
- Do not allow cgroup, namespace, and seclabel access to VFIO
paths (/dev/vfio/vfio and /dev/vfio/<iommugroup>)
- Allow access to iommufd paths (/dev/iommu and
/dev/vfio/devices/vfio*) for AppArmor, SELinux, and DAC
Signed-off-by: Nathan Chen <nathanc@nvidia.com>
---
src/qemu/qemu_cgroup.c | 26 +++++++-------
src/qemu/qemu_namespace.c | 16 +++++----
src/security/security_apparmor.c | 32 +++++++++++++----
src/security/security_dac.c | 59 ++++++++++++++++++++++++++------
src/security/security_selinux.c | 57 ++++++++++++++++++++++++------
src/security/virt-aa-helper.c | 33 ++++++++++++++----
6 files changed, 170 insertions(+), 53 deletions(-)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 7dadef0739..7190a4f80f 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -479,21 +479,23 @@ qemuSetupHostdevCgroup(virDomainObj *vm,
g_autofree char *path = NULL;
int perms;
- if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES))
- return 0;
+ if (dev->source.subsys.u.pci.driver.iommufd != VIR_TRISTATE_BOOL_YES) {
+ if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES))
+ return 0;
- if (qemuDomainGetHostdevPath(dev, &path, &perms) < 0)
- return -1;
+ if (qemuDomainGetHostdevPath(dev, &path, &perms) < 0)
+ return -1;
- if (path &&
- qemuCgroupAllowDevicePath(vm, path, perms, false) < 0) {
- return -1;
- }
+ if (path &&
+ qemuCgroupAllowDevicePath(vm, path, perms, false) < 0) {
+ return -1;
+ }
- if (virHostdevNeedsVFIO(dev) &&
- qemuCgroupAllowDevicePath(vm, QEMU_DEV_VFIO,
- VIR_CGROUP_DEVICE_RW, false) < 0) {
- return -1;
+ if (virHostdevNeedsVFIO(dev) &&
+ qemuCgroupAllowDevicePath(vm, QEMU_DEV_VFIO,
+ VIR_CGROUP_DEVICE_RW, false) < 0) {
+ return -1;
+ }
}
return 0;
diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
index c689cc3e40..907b2773cf 100644
--- a/src/qemu/qemu_namespace.c
+++ b/src/qemu/qemu_namespace.c
@@ -345,15 +345,17 @@ qemuDomainSetupHostdev(virDomainObj *vm,
{
g_autofree char *path = NULL;
- if (qemuDomainGetHostdevPath(hostdev, &path, NULL) < 0)
- return -1;
+ if (hostdev->source.subsys.u.pci.driver.iommufd != VIR_TRISTATE_BOOL_YES) {
+ if (qemuDomainGetHostdevPath(hostdev, &path, NULL) < 0)
+ return -1;
- if (path)
- *paths = g_slist_prepend(*paths, g_steal_pointer(&path));
+ if (path)
+ *paths = g_slist_prepend(*paths, g_steal_pointer(&path));
- if (virHostdevNeedsVFIO(hostdev) &&
- (!hotplug || !qemuDomainNeedsVFIO(vm->def)))
- *paths = g_slist_prepend(*paths, g_strdup(QEMU_DEV_VFIO));
+ if (virHostdevNeedsVFIO(hostdev) &&
+ (!hotplug || !qemuDomainNeedsVFIO(vm->def)))
+ *paths = g_slist_prepend(*paths, g_strdup(QEMU_DEV_VFIO));
+ }
return 0;
}
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index 68ac39611f..362ca09562 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -848,14 +848,32 @@ AppArmorSetSecurityHostdevLabel(virSecurityManager *mgr,
goto done;
if (pcisrc->driver.name == VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_VFIO) {
- char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
-
- if (!vfioGroupDev) {
- virPCIDeviceFree(pci);
- goto done;
+ if (dev->source.subsys.u.pci.driver.iommufd != VIR_TRISTATE_BOOL_YES) {
+ char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
+
+ if (!vfioGroupDev) {
+ virPCIDeviceFree(pci);
+ goto done;
+ }
+ ret = AppArmorSetSecurityPCILabel(pci, vfioGroupDev, ptr);
+ VIR_FREE(vfioGroupDev);
+ } else {
+ g_autofree char *vfiofdDev = NULL;
+
+ if (virPCIDeviceGetVfioPath(&dev->source.subsys.u.pci.addr, &vfiofdDev) < 0)
+ return -1;
+
+ if (!virIOMMUFDSupported())
+ return -1;
+
+ ret = AppArmorSetSecurityPCILabel(pci, vfiofdDev, ptr);
+ if (ret)
+ return ret;
+
+ ret = AppArmorSetSecurityPCILabel(pci, VIR_IOMMU_DEV_PATH, ptr);
+ if (ret)
+ return ret;
}
- ret = AppArmorSetSecurityPCILabel(pci, vfioGroupDev, ptr);
- VIR_FREE(vfioGroupDev);
} else {
ret = virPCIDeviceFileIterate(pci, AppArmorSetSecurityPCILabel, ptr);
}
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 2f788b872a..fbe216637f 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -41,6 +41,7 @@
#include "virscsivhost.h"
#include "virstring.h"
#include "virutil.h"
+#include "viriommufd.h"
#define VIR_FROM_THIS VIR_FROM_SECURITY
@@ -1282,14 +1283,32 @@ virSecurityDACSetHostdevLabel(virSecurityManager *mgr,
return -1;
if (pcisrc->driver.name == VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_VFIO) {
- g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
+ if (dev->source.subsys.u.pci.driver.iommufd != VIR_TRISTATE_BOOL_YES) {
+ g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
- if (!vfioGroupDev)
- return -1;
+ if (!vfioGroupDev)
+ return -1;
+
+ ret = virSecurityDACSetHostdevLabelHelper(vfioGroupDev,
+ false,
+ &cbdata);
+ } else {
+ g_autofree char *vfiofdDev = NULL;
+
+ if (virPCIDeviceGetVfioPath(&dev->source.subsys.u.pci.addr, &vfiofdDev) < 0)
+ return -1;
- ret = virSecurityDACSetHostdevLabelHelper(vfioGroupDev,
- false,
- &cbdata);
+ if (!virIOMMUFDSupported())
+ return -1;
+
+ ret = virSecurityDACSetHostdevLabelHelper(vfiofdDev, false, &cbdata);
+ if (ret)
+ return ret;
+
+ ret = virSecurityDACSetHostdevLabelHelper(VIR_IOMMU_DEV_PATH, false, &cbdata);
+ if (ret)
+ return ret;
+ }
} else {
ret = virPCIDeviceFileIterate(pci,
virSecurityDACSetPCILabel,
@@ -1443,13 +1462,33 @@ virSecurityDACRestoreHostdevLabel(virSecurityManager *mgr,
return -1;
if (pcisrc->driver.name == VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_VFIO) {
- g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
+ if (dev->source.subsys.u.pci.driver.iommufd != VIR_TRISTATE_BOOL_YES) {
+ g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
- if (!vfioGroupDev)
- return -1;
+ if (!vfioGroupDev)
+ return -1;
- ret = virSecurityDACRestoreFileLabelInternal(mgr, NULL,
+ ret = virSecurityDACRestoreFileLabelInternal(mgr, NULL,
vfioGroupDev, false);
+ } else {
+ g_autofree char *vfiofdDev = NULL;
+
+ if (virPCIDeviceGetVfioPath(&dev->source.subsys.u.pci.addr, &vfiofdDev) < 0)
+ return -1;
+
+ if (!virIOMMUFDSupported())
+ return -1;
+
+ ret = virSecurityDACRestoreFileLabelInternal(mgr, NULL,
+ vfiofdDev, false);
+ if (ret)
+ return ret;
+
+ ret = virSecurityDACRestoreFileLabelInternal(mgr, NULL,
+ VIR_IOMMU_DEV_PATH, false);
+ if (ret)
+ return ret;
+ }
} else {
ret = virPCIDeviceFileIterate(pci, virSecurityDACRestorePCILabel, mgr);
}
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 2f3cc274a5..05086ad9e1 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -41,6 +41,7 @@
#include "virconf.h"
#include "virtpm.h"
#include "virstring.h"
+#include "viriommufd.h"
#define VIR_FROM_THIS VIR_FROM_SECURITY
@@ -2256,14 +2257,32 @@ virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManager *mgr,
return -1;
if (pcisrc->driver.name == VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_VFIO) {
- g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
+ if (dev->source.subsys.u.pci.driver.iommufd != VIR_TRISTATE_BOOL_YES) {
+ g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
- if (!vfioGroupDev)
- return -1;
+ if (!vfioGroupDev)
+ return -1;
+
+ ret = virSecuritySELinuxSetHostdevLabelHelper(vfioGroupDev,
+ false,
+ &data);
+ } else {
+ g_autofree char *vfiofdDev = NULL;
+
+ if (virPCIDeviceGetVfioPath(&dev->source.subsys.u.pci.addr, &vfiofdDev) < 0)
+ return -1;
- ret = virSecuritySELinuxSetHostdevLabelHelper(vfioGroupDev,
- false,
- &data);
+ if (!virIOMMUFDSupported())
+ return -1;
+
+ ret = virSecuritySELinuxSetHostdevLabelHelper(vfiofdDev, false, &data);
+ if (ret)
+ return ret;
+
+ ret = virSecuritySELinuxSetHostdevLabelHelper(VIR_IOMMU_DEV_PATH, false, &data);
+ if (ret)
+ return ret;
+ }
} else {
ret = virPCIDeviceFileIterate(pci, virSecuritySELinuxSetPCILabel, &data);
}
@@ -2491,12 +2510,30 @@ virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManager *mgr,
return -1;
if (pcisrc->driver.name == VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_VFIO) {
- g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
+ if (dev->source.subsys.u.pci.driver.iommufd != VIR_TRISTATE_BOOL_YES) {
+ g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
- if (!vfioGroupDev)
- return -1;
+ if (!vfioGroupDev)
+ return -1;
+
+ ret = virSecuritySELinuxRestoreFileLabel(mgr, vfioGroupDev, false, false);
+ } else {
+ g_autofree char *vfiofdDev = NULL;
+
+ if (virPCIDeviceGetVfioPath(&dev->source.subsys.u.pci.addr, &vfiofdDev) < 0)
+ return -1;
- ret = virSecuritySELinuxRestoreFileLabel(mgr, vfioGroupDev, false, false);
+ if (!virIOMMUFDSupported())
+ return -1;
+
+ ret = virSecuritySELinuxRestoreFileLabel(mgr, vfiofdDev, false, false);
+ if (ret)
+ return ret;
+
+ ret = virSecuritySELinuxRestoreFileLabel(mgr, VIR_IOMMU_DEV_PATH, false, false);
+ if (ret)
+ return ret;
+ }
} else {
ret = virPCIDeviceFileIterate(pci, virSecuritySELinuxRestorePCILabel, mgr);
}
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index de0a826063..43046ab831 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -50,6 +50,7 @@
#include "virstring.h"
#include "virgettext.h"
#include "virhostdev.h"
+#include "viriommufd.h"
#define VIR_FROM_THIS VIR_FROM_SECURITY
@@ -1114,8 +1115,9 @@ get_files(vahControl * ctl)
virDeviceHostdevPCIDriverName driverName = dev->source.subsys.u.pci.driver.name;
- if (driverName == VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_VFIO ||
- driverName == VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_DEFAULT) {
+ if ((driverName == VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_VFIO ||
+ driverName == VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_DEFAULT) &&
+ dev->source.subsys.u.pci.driver.iommufd != VIR_TRISTATE_BOOL_YES) {
needsVfio = true;
}
@@ -1348,6 +1350,7 @@ get_files(vahControl * ctl)
virBufferAddLit(&buf, " \"/dev/vfio/vfio\" rw,\n");
virBufferAddLit(&buf, " \"/dev/vfio/[0-9]*\" rw,\n");
}
+
if (needsgl) {
/* if using gl all sorts of further dri related paths will be needed */
virBufferAddLit(&buf, " # DRI/Mesa/(e)GL config and driver paths\n");
@@ -1385,9 +1388,18 @@ get_files(vahControl * ctl)
}
}
- if (ctl->newfile &&
- vah_add_file(&buf, ctl->newfile, "rwk") != 0) {
- return -1;
+ if (ctl->newfile) {
+ const char *perms = "rwk";
+
+ /* VFIO and iommufd devices need mmap permission */
+ if (STRPREFIX(ctl->newfile, "/dev/vfio/devices/vfio") ||
+ STREQ(ctl->newfile, VIR_IOMMU_DEV_PATH)) {
+ perms = "rwm";
+ }
+
+ if (vah_add_file(&buf, ctl->newfile, perms) != 0) {
+ return -1;
+ }
}
ctl->files = virBufferContentAndReset(&buf);
@@ -1561,8 +1573,15 @@ main(int argc, char **argv)
}
}
if (ctl->append && ctl->newfile) {
- if (vah_add_file(&buf, ctl->newfile, "rwk") != 0)
- goto cleanup;
+ const char *perms = "rwk";
+
+ if (STRPREFIX(ctl->newfile, "/dev/vfio/devices/vfio") ||
+ STREQ(ctl->newfile, VIR_IOMMU_DEV_PATH)) {
+ perms = "rwm";
+ }
+
+ if (vah_add_file(&buf, ctl->newfile, perms) != 0)
+ return -1;
} else {
if (ctl->def->virtType == VIR_DOMAIN_VIRT_QEMU ||
ctl->def->virtType == VIR_DOMAIN_VIRT_KQEMU ||
--
2.43.0On Tue, Jan 06, 2026 at 06:49:37PM -0800, Nathan Chen via Devel wrote:
> From: Nathan Chen <nathanc@nvidia.com>
>
> When launching a qemu VM with the iommufd feature enabled for VFIO
> hostdevs:
> - Do not allow cgroup, namespace, and seclabel access to VFIO
> paths (/dev/vfio/vfio and /dev/vfio/<iommugroup>)
> - Allow access to iommufd paths (/dev/iommu and
> /dev/vfio/devices/vfio*) for AppArmor, SELinux, and DAC
>
> Signed-off-by: Nathan Chen <nathanc@nvidia.com>
> ---
> src/qemu/qemu_cgroup.c | 26 +++++++-------
> src/qemu/qemu_namespace.c | 16 +++++----
> src/security/security_apparmor.c | 32 +++++++++++++----
> src/security/security_dac.c | 59 ++++++++++++++++++++++++++------
> src/security/security_selinux.c | 57 ++++++++++++++++++++++++------
> src/security/virt-aa-helper.c | 33 ++++++++++++++----
> 6 files changed, 170 insertions(+), 53 deletions(-)
>
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index 7dadef0739..7190a4f80f 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -479,21 +479,23 @@ qemuSetupHostdevCgroup(virDomainObj *vm,
> g_autofree char *path = NULL;
> int perms;
>
> - if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES))
> - return 0;
> + if (dev->source.subsys.u.pci.driver.iommufd != VIR_TRISTATE_BOOL_YES) {
> + if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES))
> + return 0;
I would change the code like this:
if (dev->source.subsys.u.pci.driver.iommufd == VIR_TRISTATE_BOOL_YES)
return 0;
...
That way you don't have to move the whole code into the if condition.
This applies to changes in qemu_namespace.c as well.
>
> - if (qemuDomainGetHostdevPath(dev, &path, &perms) < 0)
> - return -1;
> + if (qemuDomainGetHostdevPath(dev, &path, &perms) < 0)
> + return -1;
>
> - if (path &&
> - qemuCgroupAllowDevicePath(vm, path, perms, false) < 0) {
> - return -1;
> - }
> + if (path &&
> + qemuCgroupAllowDevicePath(vm, path, perms, false) < 0) {
> + return -1;
> + }
>
> - if (virHostdevNeedsVFIO(dev) &&
> - qemuCgroupAllowDevicePath(vm, QEMU_DEV_VFIO,
> - VIR_CGROUP_DEVICE_RW, false) < 0) {
> - return -1;
> + if (virHostdevNeedsVFIO(dev) &&
> + qemuCgroupAllowDevicePath(vm, QEMU_DEV_VFIO,
> + VIR_CGROUP_DEVICE_RW, false) < 0) {
> + return -1;
> + }
> }
>
> return 0;
> diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
> index c689cc3e40..907b2773cf 100644
> --- a/src/qemu/qemu_namespace.c
> +++ b/src/qemu/qemu_namespace.c
> @@ -345,15 +345,17 @@ qemuDomainSetupHostdev(virDomainObj *vm,
> {
> g_autofree char *path = NULL;
>
> - if (qemuDomainGetHostdevPath(hostdev, &path, NULL) < 0)
> - return -1;
> + if (hostdev->source.subsys.u.pci.driver.iommufd != VIR_TRISTATE_BOOL_YES) {
> + if (qemuDomainGetHostdevPath(hostdev, &path, NULL) < 0)
> + return -1;
>
> - if (path)
> - *paths = g_slist_prepend(*paths, g_steal_pointer(&path));
> + if (path)
> + *paths = g_slist_prepend(*paths, g_steal_pointer(&path));
>
> - if (virHostdevNeedsVFIO(hostdev) &&
> - (!hotplug || !qemuDomainNeedsVFIO(vm->def)))
> - *paths = g_slist_prepend(*paths, g_strdup(QEMU_DEV_VFIO));
> + if (virHostdevNeedsVFIO(hostdev) &&
> + (!hotplug || !qemuDomainNeedsVFIO(vm->def)))
> + *paths = g_slist_prepend(*paths, g_strdup(QEMU_DEV_VFIO));
> + }
>
> return 0;
> }
> diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
> index 68ac39611f..362ca09562 100644
> --- a/src/security/security_apparmor.c
> +++ b/src/security/security_apparmor.c
> @@ -848,14 +848,32 @@ AppArmorSetSecurityHostdevLabel(virSecurityManager *mgr,
> goto done;
>
> if (pcisrc->driver.name == VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_VFIO) {
> - char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
> -
> - if (!vfioGroupDev) {
> - virPCIDeviceFree(pci);
> - goto done;
> + if (dev->source.subsys.u.pci.driver.iommufd != VIR_TRISTATE_BOOL_YES) {
> + char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
> +
> + if (!vfioGroupDev) {
> + virPCIDeviceFree(pci);
> + goto done;
> + }
> + ret = AppArmorSetSecurityPCILabel(pci, vfioGroupDev, ptr);
> + VIR_FREE(vfioGroupDev);
> + } else {
> + g_autofree char *vfiofdDev = NULL;
> +
> + if (virPCIDeviceGetVfioPath(&dev->source.subsys.u.pci.addr, &vfiofdDev) < 0)
> + return -1;
> +
> + if (!virIOMMUFDSupported())
> + return -1;
Move this check before we try to get vfio path as there is no need to
construct the path if iommufd is not supported. We should also report
error here, if virIOMMUFDSupported() fails it only sets errno.
User goto done; instead of return -1; other we are going to leak ptr
and pci.
> +
> + ret = AppArmorSetSecurityPCILabel(pci, vfiofdDev, ptr);
> + if (ret)
> + return ret;
if (ret < 0)
goto end;
> +
> + ret = AppArmorSetSecurityPCILabel(pci, VIR_IOMMU_DEV_PATH, ptr);
> + if (ret)
> + return ret;
Don't call return or goto end; as it is not required here and we would
also leak pci.
I would suggest to change:
virPCIDevice *pci = virPCIDeviceNew(&pcisrc->addr);
into
g_autoptr(virPCIDevice) pci = virPCIDeviceNew(&pcisrc->addr);
remove the virPCIDeviceFree(pci); within this switch case and there is
no need to care about freeing pci.
> }
> - ret = AppArmorSetSecurityPCILabel(pci, vfioGroupDev, ptr);
> - VIR_FREE(vfioGroupDev);
> } else {
> ret = virPCIDeviceFileIterate(pci, AppArmorSetSecurityPCILabel, ptr);
> }
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 2f788b872a..fbe216637f 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -41,6 +41,7 @@
> #include "virscsivhost.h"
> #include "virstring.h"
> #include "virutil.h"
> +#include "viriommufd.h"
>
> #define VIR_FROM_THIS VIR_FROM_SECURITY
>
> @@ -1282,14 +1283,32 @@ virSecurityDACSetHostdevLabel(virSecurityManager *mgr,
> return -1;
>
> if (pcisrc->driver.name == VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_VFIO) {
> - g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
> + if (dev->source.subsys.u.pci.driver.iommufd != VIR_TRISTATE_BOOL_YES) {
> + g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
>
> - if (!vfioGroupDev)
> - return -1;
> + if (!vfioGroupDev)
> + return -1;
> +
> + ret = virSecurityDACSetHostdevLabelHelper(vfioGroupDev,
> + false,
> + &cbdata);
> + } else {
> + g_autofree char *vfiofdDev = NULL;
> +
> + if (virPCIDeviceGetVfioPath(&dev->source.subsys.u.pci.addr, &vfiofdDev) < 0)
> + return -1;
>
> - ret = virSecurityDACSetHostdevLabelHelper(vfioGroupDev,
> - false,
> - &cbdata);
> + if (!virIOMMUFDSupported())
> + return -1;
Same as in apparmor, move this check before virPCIDeviceGetVfioPath()
and report error.
> +
> + ret = virSecurityDACSetHostdevLabelHelper(vfiofdDev, false, &cbdata);
> + if (ret)
> + return ret;
> +
> + ret = virSecurityDACSetHostdevLabelHelper(VIR_IOMMU_DEV_PATH, false, &cbdata);
> + if (ret)
> + return ret;
> + }
> } else {
> ret = virPCIDeviceFileIterate(pci,
> virSecurityDACSetPCILabel,
> @@ -1443,13 +1462,33 @@ virSecurityDACRestoreHostdevLabel(virSecurityManager *mgr,
> return -1;
>
> if (pcisrc->driver.name == VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_VFIO) {
> - g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
> + if (dev->source.subsys.u.pci.driver.iommufd != VIR_TRISTATE_BOOL_YES) {
> + g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
>
> - if (!vfioGroupDev)
> - return -1;
> + if (!vfioGroupDev)
> + return -1;
>
> - ret = virSecurityDACRestoreFileLabelInternal(mgr, NULL,
> + ret = virSecurityDACRestoreFileLabelInternal(mgr, NULL,
> vfioGroupDev, false);
> + } else {
> + g_autofree char *vfiofdDev = NULL;
> +
> + if (virPCIDeviceGetVfioPath(&dev->source.subsys.u.pci.addr, &vfiofdDev) < 0)
> + return -1;
> +
> + if (!virIOMMUFDSupported())
> + return -1;
Same here.
> +
> + ret = virSecurityDACRestoreFileLabelInternal(mgr, NULL,
> + vfiofdDev, false);
> + if (ret)
> + return ret;
> +
> + ret = virSecurityDACRestoreFileLabelInternal(mgr, NULL,
> + VIR_IOMMU_DEV_PATH, false);
> + if (ret)
> + return ret;
> + }
> } else {
> ret = virPCIDeviceFileIterate(pci, virSecurityDACRestorePCILabel, mgr);
> }
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index 2f3cc274a5..05086ad9e1 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -41,6 +41,7 @@
> #include "virconf.h"
> #include "virtpm.h"
> #include "virstring.h"
> +#include "viriommufd.h"
>
> #define VIR_FROM_THIS VIR_FROM_SECURITY
>
> @@ -2256,14 +2257,32 @@ virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManager *mgr,
> return -1;
>
> if (pcisrc->driver.name == VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_VFIO) {
> - g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
> + if (dev->source.subsys.u.pci.driver.iommufd != VIR_TRISTATE_BOOL_YES) {
> + g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
>
> - if (!vfioGroupDev)
> - return -1;
> + if (!vfioGroupDev)
> + return -1;
> +
> + ret = virSecuritySELinuxSetHostdevLabelHelper(vfioGroupDev,
> + false,
> + &data);
> + } else {
> + g_autofree char *vfiofdDev = NULL;
> +
> + if (virPCIDeviceGetVfioPath(&dev->source.subsys.u.pci.addr, &vfiofdDev) < 0)
> + return -1;
>
> - ret = virSecuritySELinuxSetHostdevLabelHelper(vfioGroupDev,
> - false,
> - &data);
> + if (!virIOMMUFDSupported())
> + return -1;
Same here.
> +
> + ret = virSecuritySELinuxSetHostdevLabelHelper(vfiofdDev, false, &data);
> + if (ret)
> + return ret;
> +
> + ret = virSecuritySELinuxSetHostdevLabelHelper(VIR_IOMMU_DEV_PATH, false, &data);
> + if (ret)
> + return ret;
> + }
> } else {
> ret = virPCIDeviceFileIterate(pci, virSecuritySELinuxSetPCILabel, &data);
> }
> @@ -2491,12 +2510,30 @@ virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManager *mgr,
> return -1;
>
> if (pcisrc->driver.name == VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_VFIO) {
> - g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
> + if (dev->source.subsys.u.pci.driver.iommufd != VIR_TRISTATE_BOOL_YES) {
> + g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
>
> - if (!vfioGroupDev)
> - return -1;
> + if (!vfioGroupDev)
> + return -1;
> +
> + ret = virSecuritySELinuxRestoreFileLabel(mgr, vfioGroupDev, false, false);
> + } else {
> + g_autofree char *vfiofdDev = NULL;
> +
> + if (virPCIDeviceGetVfioPath(&dev->source.subsys.u.pci.addr, &vfiofdDev) < 0)
> + return -1;
>
> - ret = virSecuritySELinuxRestoreFileLabel(mgr, vfioGroupDev, false, false);
> + if (!virIOMMUFDSupported())
> + return -1;
Same here.
> +
> + ret = virSecuritySELinuxRestoreFileLabel(mgr, vfiofdDev, false, false);
> + if (ret)
> + return ret;
> +
> + ret = virSecuritySELinuxRestoreFileLabel(mgr, VIR_IOMMU_DEV_PATH, false, false);
> + if (ret)
> + return ret;
> + }
> } else {
> ret = virPCIDeviceFileIterate(pci, virSecuritySELinuxRestorePCILabel, mgr);
> }
Pavel
On 1/15/2026 10:09 AM, Pavel Hrdina wrote:
> On Tue, Jan 06, 2026 at 06:49:37PM -0800, Nathan Chen via Devel wrote:
>> From: Nathan Chen<nathanc@nvidia.com>
>>
>> When launching a qemu VM with the iommufd feature enabled for VFIO
>> hostdevs:
>> - Do not allow cgroup, namespace, and seclabel access to VFIO
>> paths (/dev/vfio/vfio and /dev/vfio/<iommugroup>)
>> - Allow access to iommufd paths (/dev/iommu and
>> /dev/vfio/devices/vfio*) for AppArmor, SELinux, and DAC
>>
>> Signed-off-by: Nathan Chen<nathanc@nvidia.com>
>> ---
>> src/qemu/qemu_cgroup.c | 26 +++++++-------
>> src/qemu/qemu_namespace.c | 16 +++++----
>> src/security/security_apparmor.c | 32 +++++++++++++----
>> src/security/security_dac.c | 59 ++++++++++++++++++++++++++------
>> src/security/security_selinux.c | 57 ++++++++++++++++++++++++------
>> src/security/virt-aa-helper.c | 33 ++++++++++++++----
>> 6 files changed, 170 insertions(+), 53 deletions(-)
>>
>> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
>> index 7dadef0739..7190a4f80f 100644
>> --- a/src/qemu/qemu_cgroup.c
>> +++ b/src/qemu/qemu_cgroup.c
>> @@ -479,21 +479,23 @@ qemuSetupHostdevCgroup(virDomainObj *vm,
>> g_autofree char *path = NULL;
>> int perms;
>>
>> - if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES))
>> - return 0;
>> + if (dev->source.subsys.u.pci.driver.iommufd != VIR_TRISTATE_BOOL_YES) {
>> + if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES))
>> + return 0;
> I would change the code like this:
>
> if (dev->source.subsys.u.pci.driver.iommufd == VIR_TRISTATE_BOOL_YES)
> return 0;
>
> ...
>
>
> That way you don't have to move the whole code into the if condition.
>
> This applies to changes in qemu_namespace.c as well.
>
Got it, I will do this in the next revision.
>>
>> - if (qemuDomainGetHostdevPath(dev, &path, &perms) < 0)
>> - return -1;
>> + if (qemuDomainGetHostdevPath(dev, &path, &perms) < 0)
>> + return -1;
>>
>> - if (path &&
>> - qemuCgroupAllowDevicePath(vm, path, perms, false) < 0) {
>> - return -1;
>> - }
>> + if (path &&
>> + qemuCgroupAllowDevicePath(vm, path, perms, false) < 0) {
>> + return -1;
>> + }
>>
>> - if (virHostdevNeedsVFIO(dev) &&
>> - qemuCgroupAllowDevicePath(vm, QEMU_DEV_VFIO,
>> - VIR_CGROUP_DEVICE_RW, false) < 0) {
>> - return -1;
>> + if (virHostdevNeedsVFIO(dev) &&
>> + qemuCgroupAllowDevicePath(vm, QEMU_DEV_VFIO,
>> + VIR_CGROUP_DEVICE_RW, false) < 0) {
>> + return -1;
>> + }
>> }
>>
>> return 0;
>> diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
>> index c689cc3e40..907b2773cf 100644
>> --- a/src/qemu/qemu_namespace.c
>> +++ b/src/qemu/qemu_namespace.c
>> @@ -345,15 +345,17 @@ qemuDomainSetupHostdev(virDomainObj *vm,
>> {
>> g_autofree char *path = NULL;
>>
>> - if (qemuDomainGetHostdevPath(hostdev, &path, NULL) < 0)
>> - return -1;
>> + if (hostdev->source.subsys.u.pci.driver.iommufd != VIR_TRISTATE_BOOL_YES) {
>> + if (qemuDomainGetHostdevPath(hostdev, &path, NULL) < 0)
>> + return -1;
>>
>> - if (path)
>> - *paths = g_slist_prepend(*paths, g_steal_pointer(&path));
>> + if (path)
>> + *paths = g_slist_prepend(*paths, g_steal_pointer(&path));
>>
>> - if (virHostdevNeedsVFIO(hostdev) &&
>> - (!hotplug || !qemuDomainNeedsVFIO(vm->def)))
>> - *paths = g_slist_prepend(*paths, g_strdup(QEMU_DEV_VFIO));
>> + if (virHostdevNeedsVFIO(hostdev) &&
>> + (!hotplug || !qemuDomainNeedsVFIO(vm->def)))
>> + *paths = g_slist_prepend(*paths, g_strdup(QEMU_DEV_VFIO));
>> + }
>>
>> return 0;
>> }
>> diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
>> index 68ac39611f..362ca09562 100644
>> --- a/src/security/security_apparmor.c
>> +++ b/src/security/security_apparmor.c
>> @@ -848,14 +848,32 @@ AppArmorSetSecurityHostdevLabel(virSecurityManager *mgr,
>> goto done;
>>
>> if (pcisrc->driver.name == VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_VFIO) {
>> - char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
>> -
>> - if (!vfioGroupDev) {
>> - virPCIDeviceFree(pci);
>> - goto done;
>> + if (dev->source.subsys.u.pci.driver.iommufd != VIR_TRISTATE_BOOL_YES) {
>> + char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
>> +
>> + if (!vfioGroupDev) {
>> + virPCIDeviceFree(pci);
>> + goto done;
>> + }
>> + ret = AppArmorSetSecurityPCILabel(pci, vfioGroupDev, ptr);
>> + VIR_FREE(vfioGroupDev);
>> + } else {
>> + g_autofree char *vfiofdDev = NULL;
>> +
>> + if (virPCIDeviceGetVfioPath(&dev->source.subsys.u.pci.addr, &vfiofdDev) < 0)
>> + return -1;
>> +
>> + if (!virIOMMUFDSupported())
>> + return -1;
> Move this check before we try to get vfio path as there is no need to
> construct the path if iommufd is not supported. We should also report
> error here, if virIOMMUFDSupported() fails it only sets errno.
>
> User goto done; instead of return -1; other we are going to leak ptr
> and pci.
>
I plan to remove virIOMMUFDSupported() entirely as discussed in our
thread with Jano. For the conditional with virPCIDeviceGetVfioPath(), I
will change it to goto done; instead of return -1;
>> +
>> + ret = AppArmorSetSecurityPCILabel(pci, vfiofdDev, ptr);
>> + if (ret)
>> + return ret;
> if (ret < 0)
> goto end;
>
I will change this to goto done;
>> +
>> + ret = AppArmorSetSecurityPCILabel(pci, VIR_IOMMU_DEV_PATH, ptr);
>> + if (ret)
>> + return ret;
> Don't call return or goto end; as it is not required here and we would
> also leak pci.
>
Ok, I will omit this check.
> I would suggest to change:
>
> virPCIDevice *pci = virPCIDeviceNew(&pcisrc->addr);
>
> into
>
> g_autoptr(virPCIDevice) pci = virPCIDeviceNew(&pcisrc->addr);
>
> remove the virPCIDeviceFree(pci); within this switch case and there is
> no need to care about freeing pci.
>
Ok, I will implement this to match security_dac.c and security_selinux.c.
>> }
>> - ret = AppArmorSetSecurityPCILabel(pci, vfioGroupDev, ptr);
>> - VIR_FREE(vfioGroupDev);
>> } else {
>> ret = virPCIDeviceFileIterate(pci, AppArmorSetSecurityPCILabel, ptr);
>> }
>> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
>> index 2f788b872a..fbe216637f 100644
>> --- a/src/security/security_dac.c
>> +++ b/src/security/security_dac.c
>> @@ -41,6 +41,7 @@
>> #include "virscsivhost.h"
>> #include "virstring.h"
>> #include "virutil.h"
>> +#include "viriommufd.h"
>>
>> #define VIR_FROM_THIS VIR_FROM_SECURITY
>>
>> @@ -1282,14 +1283,32 @@ virSecurityDACSetHostdevLabel(virSecurityManager *mgr,
>> return -1;
>>
>> if (pcisrc->driver.name == VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_VFIO) {
>> - g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
>> + if (dev->source.subsys.u.pci.driver.iommufd != VIR_TRISTATE_BOOL_YES) {
>> + g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
>>
>> - if (!vfioGroupDev)
>> - return -1;
>> + if (!vfioGroupDev)
>> + return -1;
>> +
>> + ret = virSecurityDACSetHostdevLabelHelper(vfioGroupDev,
>> + false,
>> + &cbdata);
>> + } else {
>> + g_autofree char *vfiofdDev = NULL;
>> +
>> + if (virPCIDeviceGetVfioPath(&dev->source.subsys.u.pci.addr, &vfiofdDev) < 0)
>> + return -1;
>>
>> - ret = virSecurityDACSetHostdevLabelHelper(vfioGroupDev,
>> - false,
>> - &cbdata);
>> + if (!virIOMMUFDSupported())
>> + return -1;
> Same as in apparmor, move this check before virPCIDeviceGetVfioPath()
> and report error.
>
>> +
>> + ret = virSecurityDACSetHostdevLabelHelper(vfiofdDev, false, &cbdata);
>> + if (ret)
>> + return ret;
>> +
>> + ret = virSecurityDACSetHostdevLabelHelper(VIR_IOMMU_DEV_PATH, false, &cbdata);
>> + if (ret)
>> + return ret;
>> + }
>> } else {
>> ret = virPCIDeviceFileIterate(pci,
>> virSecurityDACSetPCILabel,
>> @@ -1443,13 +1462,33 @@ virSecurityDACRestoreHostdevLabel(virSecurityManager *mgr,
>> return -1;
>>
>> if (pcisrc->driver.name == VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_VFIO) {
>> - g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
>> + if (dev->source.subsys.u.pci.driver.iommufd != VIR_TRISTATE_BOOL_YES) {
>> + g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
>>
>> - if (!vfioGroupDev)
>> - return -1;
>> + if (!vfioGroupDev)
>> + return -1;
>>
>> - ret = virSecurityDACRestoreFileLabelInternal(mgr, NULL,
>> + ret = virSecurityDACRestoreFileLabelInternal(mgr, NULL,
>> vfioGroupDev, false);
>> + } else {
>> + g_autofree char *vfiofdDev = NULL;
>> +
>> + if (virPCIDeviceGetVfioPath(&dev->source.subsys.u.pci.addr, &vfiofdDev) < 0)
>> + return -1;
>> +
>> + if (!virIOMMUFDSupported())
>> + return -1;
> Same here.
>
>> +
>> + ret = virSecurityDACRestoreFileLabelInternal(mgr, NULL,
>> + vfiofdDev, false);
>> + if (ret)
>> + return ret;
>> +
>> + ret = virSecurityDACRestoreFileLabelInternal(mgr, NULL,
>> + VIR_IOMMU_DEV_PATH, false);
>> + if (ret)
>> + return ret;
>> + }
>> } else {
>> ret = virPCIDeviceFileIterate(pci, virSecurityDACRestorePCILabel, mgr);
>> }
>> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
>> index 2f3cc274a5..05086ad9e1 100644
>> --- a/src/security/security_selinux.c
>> +++ b/src/security/security_selinux.c
>> @@ -41,6 +41,7 @@
>> #include "virconf.h"
>> #include "virtpm.h"
>> #include "virstring.h"
>> +#include "viriommufd.h"
>>
>> #define VIR_FROM_THIS VIR_FROM_SECURITY
>>
>> @@ -2256,14 +2257,32 @@ virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManager *mgr,
>> return -1;
>>
>> if (pcisrc->driver.name == VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_VFIO) {
>> - g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
>> + if (dev->source.subsys.u.pci.driver.iommufd != VIR_TRISTATE_BOOL_YES) {
>> + g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
>>
>> - if (!vfioGroupDev)
>> - return -1;
>> + if (!vfioGroupDev)
>> + return -1;
>> +
>> + ret = virSecuritySELinuxSetHostdevLabelHelper(vfioGroupDev,
>> + false,
>> + &data);
>> + } else {
>> + g_autofree char *vfiofdDev = NULL;
>> +
>> + if (virPCIDeviceGetVfioPath(&dev->source.subsys.u.pci.addr, &vfiofdDev) < 0)
>> + return -1;
>>
>> - ret = virSecuritySELinuxSetHostdevLabelHelper(vfioGroupDev,
>> - false,
>> - &data);
>> + if (!virIOMMUFDSupported())
>> + return -1;
> Same here.
>
>> +
>> + ret = virSecuritySELinuxSetHostdevLabelHelper(vfiofdDev, false, &data);
>> + if (ret)
>> + return ret;
>> +
>> + ret = virSecuritySELinuxSetHostdevLabelHelper(VIR_IOMMU_DEV_PATH, false, &data);
>> + if (ret)
>> + return ret;
>> + }
>> } else {
>> ret = virPCIDeviceFileIterate(pci, virSecuritySELinuxSetPCILabel, &data);
>> }
>> @@ -2491,12 +2510,30 @@ virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManager *mgr,
>> return -1;
>>
>> if (pcisrc->driver.name == VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_VFIO) {
>> - g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
>> + if (dev->source.subsys.u.pci.driver.iommufd != VIR_TRISTATE_BOOL_YES) {
>> + g_autofree char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
>>
>> - if (!vfioGroupDev)
>> - return -1;
>> + if (!vfioGroupDev)
>> + return -1;
>> +
>> + ret = virSecuritySELinuxRestoreFileLabel(mgr, vfioGroupDev, false, false);
>> + } else {
>> + g_autofree char *vfiofdDev = NULL;
>> +
>> + if (virPCIDeviceGetVfioPath(&dev->source.subsys.u.pci.addr, &vfiofdDev) < 0)
>> + return -1;
>>
>> - ret = virSecuritySELinuxRestoreFileLabel(mgr, vfioGroupDev, false, false);
>> + if (!virIOMMUFDSupported())
>> + return -1;
> Same here.
>
>> +
>> + ret = virSecuritySELinuxRestoreFileLabel(mgr, vfiofdDev, false, false);
>> + if (ret)
>> + return ret;
>> +
>> + ret = virSecuritySELinuxRestoreFileLabel(mgr, VIR_IOMMU_DEV_PATH, false, false);
>> + if (ret)
>> + return ret;
>> + }
>> } else {
>> ret = virPCIDeviceFileIterate(pci, virSecuritySELinuxRestorePCILabel, mgr);
>> }
> Pavel
Thanks,
Nathan
On a Thursday in 2026, Pavel Hrdina via Devel wrote:
>On Tue, Jan 06, 2026 at 06:49:37PM -0800, Nathan Chen via Devel wrote:
>> From: Nathan Chen <nathanc@nvidia.com>
>>
>> When launching a qemu VM with the iommufd feature enabled for VFIO
>> hostdevs:
>> - Do not allow cgroup, namespace, and seclabel access to VFIO
>> paths (/dev/vfio/vfio and /dev/vfio/<iommugroup>)
>> - Allow access to iommufd paths (/dev/iommu and
>> /dev/vfio/devices/vfio*) for AppArmor, SELinux, and DAC
>>
>> Signed-off-by: Nathan Chen <nathanc@nvidia.com>
>> ---
>> src/qemu/qemu_cgroup.c | 26 +++++++-------
>> src/qemu/qemu_namespace.c | 16 +++++----
>> src/security/security_apparmor.c | 32 +++++++++++++----
>> src/security/security_dac.c | 59 ++++++++++++++++++++++++++------
>> src/security/security_selinux.c | 57 ++++++++++++++++++++++++------
>> src/security/virt-aa-helper.c | 33 ++++++++++++++----
>> 6 files changed, 170 insertions(+), 53 deletions(-)
>>
>> diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
>> index 68ac39611f..362ca09562 100644
>> --- a/src/security/security_apparmor.c
>> +++ b/src/security/security_apparmor.c
>> @@ -848,14 +848,32 @@ AppArmorSetSecurityHostdevLabel(virSecurityManager *mgr,
>> goto done;
>>
>> if (pcisrc->driver.name == VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_VFIO) {
>> - char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
>> -
>> - if (!vfioGroupDev) {
>> - virPCIDeviceFree(pci);
>> - goto done;
>> + if (dev->source.subsys.u.pci.driver.iommufd != VIR_TRISTATE_BOOL_YES) {
>> + char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
>> +
>> + if (!vfioGroupDev) {
>> + virPCIDeviceFree(pci);
>> + goto done;
>> + }
>> + ret = AppArmorSetSecurityPCILabel(pci, vfioGroupDev, ptr);
>> + VIR_FREE(vfioGroupDev);
>> + } else {
>> + g_autofree char *vfiofdDev = NULL;
>> +
>> + if (virPCIDeviceGetVfioPath(&dev->source.subsys.u.pci.addr, &vfiofdDev) < 0)
>> + return -1;
>> +
>> + if (!virIOMMUFDSupported())
>> + return -1;
>
>Move this check before we try to get vfio path as there is no need to
>construct the path if iommufd is not supported. We should also report
>error here, if virIOMMUFDSupported() fails it only sets errno.
>
I don't think we should get here at all without IOMMUFD being supported.
Per Pavel's suggestion to patch 4/7, moving the qemuProcessOpenVfioFds call
to qemuProcessPrepareHost would mean we hit that function before attempting
to set security labels.
Jano
>User goto done; instead of return -1; other we are going to leak ptr
>and pci.
>
On 1/16/2026 8:32 AM, Ján Tomko wrote:
> On a Thursday in 2026, Pavel Hrdina via Devel wrote:
>> On Tue, Jan 06, 2026 at 06:49:37PM -0800, Nathan Chen via Devel wrote:
>>> From: Nathan Chen <nathanc@nvidia.com>
>>>
>>> When launching a qemu VM with the iommufd feature enabled for VFIO
>>> hostdevs:
>>> - Do not allow cgroup, namespace, and seclabel access to VFIO
>>> paths (/dev/vfio/vfio and /dev/vfio/<iommugroup>)
>>> - Allow access to iommufd paths (/dev/iommu and
>>> /dev/vfio/devices/vfio*) for AppArmor, SELinux, and DAC
>>>
>>> Signed-off-by: Nathan Chen <nathanc@nvidia.com>
>>> ---
>>> src/qemu/qemu_cgroup.c | 26 +++++++-------
>>> src/qemu/qemu_namespace.c | 16 +++++----
>>> src/security/security_apparmor.c | 32 +++++++++++++----
>>> src/security/security_dac.c | 59 ++++++++++++++++++++++++++------
>>> src/security/security_selinux.c | 57 ++++++++++++++++++++++++------
>>> src/security/virt-aa-helper.c | 33 ++++++++++++++----
>>> 6 files changed, 170 insertions(+), 53 deletions(-)
>>>
>>> diff --git a/src/security/security_apparmor.c b/src/security/
>>> security_apparmor.c
>>> index 68ac39611f..362ca09562 100644
>>> --- a/src/security/security_apparmor.c
>>> +++ b/src/security/security_apparmor.c
>>> @@ -848,14 +848,32 @@
>>> AppArmorSetSecurityHostdevLabel(virSecurityManager *mgr,
>>> goto done;
>>>
>>> if (pcisrc->driver.name ==
>>> VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_VFIO) {
>>> - char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
>>> -
>>> - if (!vfioGroupDev) {
>>> - virPCIDeviceFree(pci);
>>> - goto done;
>>> + if (dev->source.subsys.u.pci.driver.iommufd !=
>>> VIR_TRISTATE_BOOL_YES) {
>>> + char *vfioGroupDev = virPCIDeviceGetIOMMUGroupDev(pci);
>>> +
>>> + if (!vfioGroupDev) {
>>> + virPCIDeviceFree(pci);
>>> + goto done;
>>> + }
>>> + ret = AppArmorSetSecurityPCILabel(pci, vfioGroupDev,
>>> ptr);
>>> + VIR_FREE(vfioGroupDev);
>>> + } else {
>>> + g_autofree char *vfiofdDev = NULL;
>>> +
>>> + if (virPCIDeviceGetVfioPath(&dev-
>>> >source.subsys.u.pci.addr, &vfiofdDev) < 0)
>>> + return -1;
>>> +
>>> + if (!virIOMMUFDSupported())
>>> + return -1;
>>
>> Move this check before we try to get vfio path as there is no need to
>> construct the path if iommufd is not supported. We should also report
>> error here, if virIOMMUFDSupported() fails it only sets errno.
>>
>
> I don't think we should get here at all without IOMMUFD being supported.
>
> Per Pavel's suggestion to patch 4/7, moving the qemuProcessOpenVfioFds call
> to qemuProcessPrepareHost would mean we hit that function before attempting
> to set security labels.
I think we can remove virIOMMUFDSupported() altogether as it is only
used in the security labeling changes. As you mention, if
qemuProcessOpenVfioFds() gets called in qemuProcessPrepareHost(), it
will call qemuProcessOpenIommuFd() which returns -1 if the open() call
to /dev/iommu fails. Then we wouldn't reach the seclabel lines here.
Nathan
© 2016 - 2026 Red Hat, Inc.