On 9/22/22 13:57, Peter Krempa wrote: > On Thu, Sep 22, 2022 at 13:40:43 +0200, Michal Privoznik wrote: >> For NVMe disks we skip setting SELinux label on corresponding >> VFIO group (/dev/vfio/X). This bug is the best visible with > > best visible? or only visible? Yeah, I think the former because if SELinux policy wakes up AFTER we instruct QEMU you'd hit the same issue. Although that is almost impossible because plenty of stuff happens in libvirt after we've detached NVMe from the host and before we instruct QEMU, so it's rather theoretical. But okay, I'll change it to the latter to avoid confusion. > >> namespaces and goes as follows: >> >> 1) libvirt assigns NVMe disk to vfio-pci driver, >> 2) kernel creates /dev/vfio/X node with generic device_t SELinux >> label, >> 3) our namespace code creates the exact copy of the node in >> domain's private /dev, >> 4) SELinux policy kicks in an changes the label on the node to >> vfio_device_t (in the top most namespace), > > Seems like this would prevent that bug without namespaces. Yeah, almost. I'm still puzzled how this worked in RHEL-8 era, because I'm almost certain that NVMe hotplug was tested there too (I mean this code I'm fixing was released in libvirt-6.0.0). Perhaps kernel creates the vfio node with the correct context from the beginning? Or maybe, the policy wasn't that strict. > >> 5) libvirt tells QEMU to attach the NVMe disk, which is denied by >> SELinux policy. >> >> While one can argue that kernel should have created the >> /dev/vfio/X node with the correct SELinux label from the >> beginning (step 2), libvirt can't rely on that and needs to set >> label on its own. >> >> Surprisingly, I already wrote the code that aims on this specific >> case (v6.0.0-rc1~241), but because of a shortcut we take earlier >> it is never ran. The reason is that >> virStorageSourceIsLocalStorage() considers NVMe disks as >> non-local because their source is not accessible via src->path >> (or even if it is, it's not a local path). >> >> Therefore, do not exit early for NVMe disks and let the function >> continue. >> >> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2121441 >> Fixes: 284a12bae0e4cf93ea72797965d6c12e3a103f40 >> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx> >> --- >> src/security/security_selinux.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c >> index 9f2872decc..a296cb7613 100644 >> --- a/src/security/security_selinux.c >> +++ b/src/security/security_selinux.c >> @@ -1818,7 +1818,11 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManager *mgr, >> const char *path = src->path; >> int ret; >> >> - if (!src->path || !virStorageSourceIsLocalStorage(src)) >> + /* Special case NVMe. Per virStorageSourceIsLocalStorage() it's >> + * considered not local, but we still want the code below to set >> + * label on VFIO group. */ > > This comment gets deleted right in the next patch. Consider just > omitting it completely. Per your review, it's not going to be merged, so I'll keep it. > >> + if (src->type != VIR_STORAGE_TYPE_NVME && >> + (!src->path || !virStorageSourceIsLocalStorage(src))) >> return 0; >> > > Reviewed-by: Peter Krempa <pkrempa@xxxxxxxxxx> > Pushed, thanks! Michal
