[PATCH v1 0/2] DAC,SELinux: do not remember/recall labels for
Libvirt is trying to do multiple open() calls in /dev/vfio files, which results in errors inside the security drivers and QEMU returning error 125 when attempting to hotplug a hostdev which belongs to the same IOMMU group as an existing domain hostdev. See patch 2 commit msg for more details. I found this problem when testing a PCI multifunction hotplug implementation. Since this is a problem that can happen in other currently supported scenarios, I decided it was worth sending the fix right away. Michal, I'm CCing you because I mentioned commit 4e95cdcbb3 ("security: Don't remember labels for TPM") in patch 2, which seems to fix a very similar problem. Figured you might want to take a look. Daniel Henrique Barboza (2): security: Allow 'remember' to be set for HostdevLabelHelper security: do not remember/recall labels for VFIO src/security/security_dac.c | 20 ++++++++++++-------- src/security/security_selinux.c | 20 ++++++++++++-------- 2 files changed, 24 insertions(+), 16 deletions(-) -- 2.24.1
There is a case in which we do not want 'remember' to be set to true in SetOwnership() calls inside the HostdevLabelHelper() functions of both DAC and SELinux drivers. Next patch will explain and handle that scenario. For now, let's make virSecurityDACSetOwnership() and virSecuritySELinuxSetHostdevLabelHelper() accept a 'remember' flag, which will be used to set the 'remember' parameter of their respective SetOwnership() calls. No functional change is made. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/security/security_dac.c | 13 +++++++------ src/security/security_selinux.c | 14 ++++++++------ 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 2561ee440e..b456c59a02 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1144,6 +1144,7 @@ virSecurityDACMoveImageMetadata(virSecurityManagerPtr mgr, static int virSecurityDACSetHostdevLabelHelper(const char *file, + bool remember, void *opaque) { virSecurityDACCallbackDataPtr cbdata = opaque; @@ -1156,7 +1157,7 @@ virSecurityDACSetHostdevLabelHelper(const char *file, if (virSecurityDACGetIds(secdef, priv, &user, &group, NULL, NULL) < 0) return -1; - return virSecurityDACSetOwnership(mgr, NULL, file, user, group, true); + return virSecurityDACSetOwnership(mgr, NULL, file, user, group, remember); } @@ -1165,7 +1166,7 @@ virSecurityDACSetPCILabel(virPCIDevicePtr dev G_GNUC_UNUSED, const char *file, void *opaque) { - return virSecurityDACSetHostdevLabelHelper(file, opaque); + return virSecurityDACSetHostdevLabelHelper(file, true, opaque); } @@ -1174,7 +1175,7 @@ virSecurityDACSetUSBLabel(virUSBDevicePtr dev G_GNUC_UNUSED, const char *file, void *opaque) { - return virSecurityDACSetHostdevLabelHelper(file, opaque); + return virSecurityDACSetHostdevLabelHelper(file, true, opaque); } @@ -1183,7 +1184,7 @@ virSecurityDACSetSCSILabel(virSCSIDevicePtr dev G_GNUC_UNUSED, const char *file, void *opaque) { - return virSecurityDACSetHostdevLabelHelper(file, opaque); + return virSecurityDACSetHostdevLabelHelper(file, true, opaque); } @@ -1192,7 +1193,7 @@ virSecurityDACSetHostLabel(virSCSIVHostDevicePtr dev G_GNUC_UNUSED, const char *file, void *opaque) { - return virSecurityDACSetHostdevLabelHelper(file, opaque); + return virSecurityDACSetHostdevLabelHelper(file, true, opaque); } @@ -1312,7 +1313,7 @@ virSecurityDACSetHostdevLabel(virSecurityManagerPtr mgr, if (!(vfiodev = virMediatedDeviceGetIOMMUGroupDev(mdevsrc->uuidstr))) return -1; - ret = virSecurityDACSetHostdevLabelHelper(vfiodev, &cbdata); + ret = virSecurityDACSetHostdevLabelHelper(vfiodev, true, &cbdata); VIR_FREE(vfiodev); break; diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 21279e7622..86acc0a33f 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2001,7 +2001,9 @@ virSecuritySELinuxMoveImageMetadata(virSecurityManagerPtr mgr, static int -virSecuritySELinuxSetHostdevLabelHelper(const char *file, void *opaque) +virSecuritySELinuxSetHostdevLabelHelper(const char *file, + bool remember, + void *opaque) { virSecurityLabelDefPtr secdef; virSecuritySELinuxCallbackDataPtr data = opaque; @@ -2011,21 +2013,21 @@ virSecuritySELinuxSetHostdevLabelHelper(const char *file, void *opaque) secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); if (secdef == NULL) return 0; - return virSecuritySELinuxSetFilecon(mgr, file, secdef->imagelabel, true); + return virSecuritySELinuxSetFilecon(mgr, file, secdef->imagelabel, remember); } static int virSecuritySELinuxSetPCILabel(virPCIDevicePtr dev G_GNUC_UNUSED, const char *file, void *opaque) { - return virSecuritySELinuxSetHostdevLabelHelper(file, opaque); + return virSecuritySELinuxSetHostdevLabelHelper(file, true, opaque); } static int virSecuritySELinuxSetUSBLabel(virUSBDevicePtr dev G_GNUC_UNUSED, const char *file, void *opaque) { - return virSecuritySELinuxSetHostdevLabelHelper(file, opaque); + return virSecuritySELinuxSetHostdevLabelHelper(file, true, opaque); } static int @@ -2056,7 +2058,7 @@ static int virSecuritySELinuxSetHostLabel(virSCSIVHostDevicePtr dev G_GNUC_UNUSED, const char *file, void *opaque) { - return virSecuritySELinuxSetHostdevLabelHelper(file, opaque); + return virSecuritySELinuxSetHostdevLabelHelper(file, true, opaque); } @@ -2164,7 +2166,7 @@ virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManagerPtr mgr, if (!(vfiodev = virMediatedDeviceGetIOMMUGroupDev(mdevsrc->uuidstr))) return ret; - ret = virSecuritySELinuxSetHostdevLabelHelper(vfiodev, &data); + ret = virSecuritySELinuxSetHostdevLabelHelper(vfiodev, true, &data); VIR_FREE(vfiodev); break; -- 2.24.1
Files inside /dev/vfio/ can't be opened more than once, meaning that any subsequent open calls will fail. This behavior was introduced in kernel v3.11, commit 6d6768c61b39. When using the VFIO driver, we open a FD to /dev/vfio/N and pass it to QEMU. If any other call attempt for the same /dev/vfio/N happens while QEMU is still using the file, we are unable to open it and QEMU will report -EBUSY. This can happen if we hotplug a PCI hostdev that belongs to the same IOMMU group of an existing domain hostdev. The problem and solution is similar to what we already dealt with for TPM in commit 4e95cdcbb3. This patch changes both DAC and SELinux drivers to disable 'remember' for VFIO hostdevs in virSecurityDACSetHostdevLabelHelper() and virSecurityDACSetHostdevLabel(), and 'recall' in virSecurityDACRestoreHostdevLabel() and virSecuritySELinuxRestoreHostdevSubsysLabel(). Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/security/security_dac.c | 7 +++++-- src/security/security_selinux.c | 6 ++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index b456c59a02..216fe93a56 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1263,7 +1263,9 @@ virSecurityDACSetHostdevLabel(virSecurityManagerPtr mgr, virPCIDeviceFree(pci); return -1; } - ret = virSecurityDACSetPCILabel(pci, vfioGroupDev, &cbdata); + ret = virSecurityDACSetHostdevLabelHelper(vfioGroupDev, + false, + &cbdata); VIR_FREE(vfioGroupDev); } else { ret = virPCIDeviceFileIterate(pci, @@ -1430,7 +1432,8 @@ virSecurityDACRestoreHostdevLabel(virSecurityManagerPtr mgr, virPCIDeviceFree(pci); return -1; } - ret = virSecurityDACRestorePCILabel(pci, vfioGroupDev, mgr); + ret = virSecurityDACRestoreFileLabelInternal(mgr, NULL, + vfioGroupDev, false); VIR_FREE(vfioGroupDev); } else { ret = virPCIDeviceFileIterate(pci, virSecurityDACRestorePCILabel, mgr); diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 86acc0a33f..ce46df09da 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2118,7 +2118,9 @@ virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManagerPtr mgr, virPCIDeviceFree(pci); return -1; } - ret = virSecuritySELinuxSetPCILabel(pci, vfioGroupDev, &data); + ret = virSecuritySELinuxSetHostdevLabelHelper(vfioGroupDev, + false, + &data); VIR_FREE(vfioGroupDev); } else { ret = virPCIDeviceFileIterate(pci, virSecuritySELinuxSetPCILabel, &data); @@ -2356,7 +2358,7 @@ virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManagerPtr mgr, virPCIDeviceFree(pci); return -1; } - ret = virSecuritySELinuxRestorePCILabel(pci, vfioGroupDev, mgr); + ret = virSecuritySELinuxRestoreFileLabel(mgr, vfioGroupDev, false); VIR_FREE(vfioGroupDev); } else { ret = virPCIDeviceFileIterate(pci, virSecuritySELinuxRestorePCILabel, mgr); -- 2.24.1
On 1/27/20 7:23 PM, Daniel Henrique Barboza wrote:
Libvirt is trying to do multiple open() calls in /dev/vfio files, which results in errors inside the security drivers and QEMU returning error 125 when attempting to hotplug a hostdev which belongs to the same IOMMU group as an existing domain hostdev. See patch 2 commit msg for more details.
I found this problem when testing a PCI multifunction hotplug implementation. Since this is a problem that can happen in other currently supported scenarios, I decided it was worth sending the fix right away.
Michal, I'm CCing you because I mentioned commit 4e95cdcbb3 ("security: Don't remember labels for TPM") in patch 2, which seems to fix a very similar problem. Figured you might want to take a look.
Daniel Henrique Barboza (2): security: Allow 'remember' to be set for HostdevLabelHelper security: do not remember/recall labels for VFIO
src/security/security_dac.c | 20 ++++++++++++-------- src/security/security_selinux.c | 20 ++++++++++++-------- 2 files changed, 24 insertions(+), 16 deletions(-)
Huh, I had similar patches in my local branch, but never gotten around to send them (dunno why). Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Daniel Henrique Barboza -
Michal Privoznik