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? > 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. > 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. > + if (src->type != VIR_STORAGE_TYPE_NVME && > + (!src->path || !virStorageSourceIsLocalStorage(src))) > return 0; > Reviewed-by: Peter Krempa <pkrempa@xxxxxxxxxx>
