12:23 p.m.
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
12:23 p.m.
New subject: [PATCH v1 1/2] security: Allow 'remember' to be set for HostdevLabelHelper
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(a)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
12:23 p.m.
New subject: [PATCH v1 2/2] security: do not remember/recall labels for VFIO
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(a)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
6:53 a.m.
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(a)redhat.com>
Michal
1794
days inactive
1796
days old
3 comments
2 participants
participants (2)
-
Daniel Henrique Barboza -
Michal Privoznik