[PATCH 0/4] selinux: skip fallback label restore for shared media
https://issues.redhat.com/browse/RHEL-126945 selinux label set/restore has race conditions on qemu:///session, where the xattr label remembering path is not used. See the above issue for a more detailed description of the problem. This series dodges the issue by skipping the fallback label restore for readonly resources like kernel and initrd, basically anything that would get the virt_content_t label. Note, disks already skip _all_ attempts to remember or restore selinux labels if the disk is marked readonly or shareable, and has done so for a long time. Maybe we should extend that out for anything that is inherently readonly or shareable, like kernel + initrd. But for now I stuck with the more conservative approach. And finally, this doesn't actually fix the race condition. If label remembering is working, the refcounting covers us. But if you disable label remembering at the qemu.conf level, there's likely similar issues in the DAC driver even for qemu:///system. I did not look into fixing the race but I suspect it involves keeping the security driver locked until the VM fully boots up. First two patches are not strictly related, but I noticed them while I was in the area Patch 3 adds the plumbing but no behavior change Patch 4 changes behavior Cole Robinson (4): selinux: Match remember/recall arguments for SavedStateLabel selinux: Don't remember labels for shareable SCSI devices selinux: Add is_shared plumbing to RestoreFileLabel selinux: Mark anything using content_context as shared src/security/security_selinux.c | 107 +++++++++++++++++++------------- 1 file changed, 63 insertions(+), 44 deletions(-) -- 2.51.1
virSecuritySELinuxSetSavedStateLabel uses remember=false, but virSecuritySELinuxRestoreSavedStateLabel uses recall=true. This doesn't cause problems in practice, just some redundant xattr calls. But Set and Restore calls should be matched here. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/security/security_selinux.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index fa5d1568eb..19e550460c 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2631,7 +2631,7 @@ virSecuritySELinuxRestoreSavedStateLabel(virSecurityManager *mgr, if (!secdef || !secdef->relabel) return 0; - return virSecuritySELinuxRestoreFileLabel(mgr, savefile, true); + return virSecuritySELinuxRestoreFileLabel(mgr, savefile, false); } -- 2.51.1
For shareable/readonly devices, label restore is skipped entirely in virSecuritySELinuxRestoreSCSILabel. So requesting remember=true here doesn't accomplish anything Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/security/security_selinux.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 19e550460c..3a91ea46d3 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2171,10 +2171,10 @@ virSecuritySELinuxSetSCSILabel(virSCSIDevice *dev, if (virSCSIDeviceGetShareable(dev)) return virSecuritySELinuxSetFilecon(mgr, file, - data->file_context, true); + data->file_context, false); else if (virSCSIDeviceGetReadonly(dev)) return virSecuritySELinuxSetFilecon(mgr, file, - data->content_context, true); + data->content_context, false); else return virSecuritySELinuxSetFilecon(mgr, file, secdef->imagelabel, true); -- 2.51.1
If set, we will skip fallback label restore attempts, if label remembering fails or isn't supported. This is a no-op, as every caller passes in `false` which matches existing behavior. Next patch will make use of it Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/security/security_selinux.c | 103 +++++++++++++++++++------------- 1 file changed, 61 insertions(+), 42 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 3a91ea46d3..898f253256 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -75,6 +75,7 @@ struct _virSecuritySELinuxContextItem { char *tcon; bool remember; /* Whether owner remembering should be done for @path/@src */ bool restore; /* Whether current operation is 'set' or 'restore' */ + bool is_shared; /* @path is shared, so don't use fallback restore path */ }; typedef struct _virSecuritySELinuxContextList virSecuritySELinuxContextList; @@ -115,7 +116,8 @@ virSecuritySELinuxContextListAppend(virSecuritySELinuxContextList *list, const char *path, const char *tcon, bool remember, - bool restore) + bool restore, + bool is_shared) { virSecuritySELinuxContextItem *item = NULL; @@ -126,6 +128,7 @@ virSecuritySELinuxContextListAppend(virSecuritySELinuxContextList *list, item->remember = remember; item->restore = restore; + item->is_shared = is_shared; VIR_APPEND_ELEMENT(list->items, list->nItems, item); @@ -172,7 +175,8 @@ static int virSecuritySELinuxTransactionAppend(const char *path, const char *tcon, bool remember, - bool restore) + bool restore, + bool is_shared) { virSecuritySELinuxContextList *list; @@ -181,7 +185,7 @@ virSecuritySELinuxTransactionAppend(const char *path, return 0; if (virSecuritySELinuxContextListAppend(list, path, tcon, - remember, restore) < 0) + remember, restore, is_shared) < 0) return -1; return 1; @@ -264,7 +268,8 @@ static int virSecuritySELinuxSetFilecon(virSecurityManager *mgr, static int virSecuritySELinuxRestoreFileLabel(virSecurityManager *mgr, const char *path, - bool recall); + bool recall, + bool is_shared); /** @@ -335,7 +340,8 @@ virSecuritySELinuxTransactionRun(pid_t pid G_GNUC_UNUSED, } else { rv = virSecuritySELinuxRestoreFileLabel(list->manager, item->path, - remember); + remember, + item->is_shared); } if (rv < 0) @@ -349,7 +355,8 @@ virSecuritySELinuxTransactionRun(pid_t pid G_GNUC_UNUSED, if (!item->restore) { virSecuritySELinuxRestoreFileLabel(list->manager, item->path, - remember); + remember, + item->is_shared); } else { VIR_WARN("Ignoring failed restore attempt on %s", item->path); } @@ -1387,7 +1394,7 @@ virSecuritySELinuxSetFilecon(virSecurityManager *mgr, int ret = -1; if ((rc = virSecuritySELinuxTransactionAppend(path, tcon, - remember, false)) < 0) + remember, false, false)) < 0) return -1; else if (rc > 0) return 0; @@ -1446,7 +1453,7 @@ virSecuritySELinuxSetFilecon(virSecurityManager *mgr, * this function. However, if our attempt fails, there's * not much we can do. XATTRs refcounting is fubar'ed and * the only option we have is warn users. */ - if (virSecuritySELinuxRestoreFileLabel(mgr, path, remember) < 0) + if (virSecuritySELinuxRestoreFileLabel(mgr, path, remember, false) < 0) VIR_WARN("Unable to restore label on '%s'. " "XATTRs might have been left in inconsistent state.", path); @@ -1502,7 +1509,8 @@ getContext(virSecurityManager *mgr G_GNUC_UNUSED, static int virSecuritySELinuxRestoreFileLabel(virSecurityManager *mgr, const char *path, - bool recall) + bool recall, + bool is_shared) { bool privileged = virSecurityManagerGetPrivileged(mgr); struct stat buf; @@ -1527,7 +1535,8 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManager *mgr, } if ((rc = virSecuritySELinuxTransactionAppend(path, NULL, - recall, true)) < 0) { + recall, true, + is_shared)) < 0) { return -1; } else if (rc > 0) { return 0; @@ -1545,6 +1554,13 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManager *mgr, } if (!recall || rc == -2) { + /* if path is marked as shared (eg. using label virt_content_t), + * skip fallback labelling, which has race conditions with multiple + * VM startup: https://issues.redhat.com/browse/RHEL-126945 + */ + if (is_shared) + return 0; + if (stat(newpath, &buf) != 0) { VIR_WARN("cannot stat %s: %s", newpath, g_strerror(errno)); @@ -1611,7 +1627,7 @@ virSecuritySELinuxRestoreInputLabel(virSecurityManager *mgr, switch ((virDomainInputType)input->type) { case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH: case VIR_DOMAIN_INPUT_TYPE_EVDEV: - return virSecuritySELinuxRestoreFileLabel(mgr, input->source.evdev, true); + return virSecuritySELinuxRestoreFileLabel(mgr, input->source.evdev, true, false); case VIR_DOMAIN_INPUT_TYPE_MOUSE: case VIR_DOMAIN_INPUT_TYPE_TABLET: @@ -1689,8 +1705,8 @@ virSecuritySELinuxRestoreMemoryLabel(virSecurityManager *mgr, path = mem->source.virtio_pmem.path; break; case VIR_DOMAIN_MEMORY_MODEL_SGX_EPC: - ret = virSecuritySELinuxRestoreFileLabel(mgr, DEV_SGX_VEPC, true); - if (virSecuritySELinuxRestoreFileLabel(mgr, DEV_SGX_PROVISION, true) < 0) + ret = virSecuritySELinuxRestoreFileLabel(mgr, DEV_SGX_VEPC, true, false); + if (virSecuritySELinuxRestoreFileLabel(mgr, DEV_SGX_PROVISION, true, false) < 0) ret = -1; return ret; @@ -1704,7 +1720,7 @@ virSecuritySELinuxRestoreMemoryLabel(virSecurityManager *mgr, if (!path) return 0; - return virSecuritySELinuxRestoreFileLabel(mgr, path, true); + return virSecuritySELinuxRestoreFileLabel(mgr, path, true, false); } @@ -1773,10 +1789,10 @@ virSecuritySELinuxRestoreTPMFileLabelInt(virSecurityManager *mgr, switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: tpmdev = tpm->data.passthrough.source->data.file.path; - rc = virSecuritySELinuxRestoreFileLabel(mgr, tpmdev, false); + rc = virSecuritySELinuxRestoreFileLabel(mgr, tpmdev, false, false); if ((cancel_path = virTPMCreateCancelPath(tpmdev)) != NULL) { - if (virSecuritySELinuxRestoreFileLabel(mgr, cancel_path, false) < 0) + if (virSecuritySELinuxRestoreFileLabel(mgr, cancel_path, false, false) < 0) rc = -1; } break; @@ -1885,7 +1901,7 @@ virSecuritySELinuxRestoreImageLabelInt(virSecurityManager *mgr, path = vfioGroupDev; } - return virSecuritySELinuxRestoreFileLabel(mgr, path, true); + return virSecuritySELinuxRestoreFileLabel(mgr, path, true, false); } @@ -2385,7 +2401,7 @@ virSecuritySELinuxRestorePCILabel(virPCIDevice *dev G_GNUC_UNUSED, { virSecurityManager *mgr = opaque; - return virSecuritySELinuxRestoreFileLabel(mgr, file, true); + return virSecuritySELinuxRestoreFileLabel(mgr, file, true, false); } static int @@ -2395,7 +2411,7 @@ virSecuritySELinuxRestoreUSBLabel(virUSBDevice *dev G_GNUC_UNUSED, { virSecurityManager *mgr = opaque; - return virSecuritySELinuxRestoreFileLabel(mgr, file, true); + return virSecuritySELinuxRestoreFileLabel(mgr, file, true, false); } @@ -2412,7 +2428,7 @@ virSecuritySELinuxRestoreSCSILabel(virSCSIDevice *dev, if (virSCSIDeviceGetShareable(dev) || virSCSIDeviceGetReadonly(dev)) return 0; - return virSecuritySELinuxRestoreFileLabel(mgr, file, true); + return virSecuritySELinuxRestoreFileLabel(mgr, file, true, false); } static int @@ -2422,7 +2438,7 @@ virSecuritySELinuxRestoreHostLabel(virSCSIVHostDevice *dev G_GNUC_UNUSED, { virSecurityManager *mgr = opaque; - return virSecuritySELinuxRestoreFileLabel(mgr, file, true); + return virSecuritySELinuxRestoreFileLabel(mgr, file, true, false); } @@ -2480,7 +2496,7 @@ virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManager *mgr, if (!vfioGroupDev) return -1; - ret = virSecuritySELinuxRestoreFileLabel(mgr, vfioGroupDev, false); + ret = virSecuritySELinuxRestoreFileLabel(mgr, vfioGroupDev, false, false); } else { ret = virPCIDeviceFileIterate(pci, virSecuritySELinuxRestorePCILabel, mgr); } @@ -2520,7 +2536,7 @@ virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManager *mgr, if (!(vfiodev = virMediatedDeviceGetIOMMUGroupDev(mdevsrc->uuidstr))) return -1; - ret = virSecuritySELinuxRestoreFileLabel(mgr, vfiodev, false); + ret = virSecuritySELinuxRestoreFileLabel(mgr, vfiodev, false, false); break; } @@ -2549,7 +2565,7 @@ virSecuritySELinuxRestoreHostdevCapsLabel(virSecurityManager *mgr, } else { path = g_strdup(dev->source.caps.u.storage.block); } - ret = virSecuritySELinuxRestoreFileLabel(mgr, path, true); + ret = virSecuritySELinuxRestoreFileLabel(mgr, path, true, false); break; } @@ -2561,7 +2577,7 @@ virSecuritySELinuxRestoreHostdevCapsLabel(virSecurityManager *mgr, } else { path = g_strdup(dev->source.caps.u.misc.chardev); } - ret = virSecuritySELinuxRestoreFileLabel(mgr, path, true); + ret = virSecuritySELinuxRestoreFileLabel(mgr, path, true, false); break; } @@ -2631,7 +2647,7 @@ virSecuritySELinuxRestoreSavedStateLabel(virSecurityManager *mgr, if (!secdef || !secdef->relabel) return 0; - return virSecuritySELinuxRestoreFileLabel(mgr, savefile, false); + return virSecuritySELinuxRestoreFileLabel(mgr, savefile, false, false); } @@ -2748,7 +2764,8 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManager *mgr, case VIR_DOMAIN_CHR_TYPE_FILE: if (virSecuritySELinuxRestoreFileLabel(mgr, dev_source->data.file.path, - true) < 0) + true, + false) < 0) return -1; break; @@ -2757,7 +2774,8 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManager *mgr, if (!dev_source->data.nix.listen) { if (virSecuritySELinuxRestoreFileLabel(mgr, dev_source->data.nix.path, - true) < 0) + true, + false) < 0) return -1; } @@ -2767,14 +2785,15 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManager *mgr, g_autofree char *out = g_strdup_printf("%s.out", dev_source->data.file.path); g_autofree char *in = g_strdup_printf("%s.in", dev_source->data.file.path); if (virFileExists(in) && virFileExists(out)) { - if ((virSecuritySELinuxRestoreFileLabel(mgr, out, true) < 0) || - (virSecuritySELinuxRestoreFileLabel(mgr, in, true) < 0)) + if ((virSecuritySELinuxRestoreFileLabel(mgr, out, true, false) < 0) || + (virSecuritySELinuxRestoreFileLabel(mgr, in, true, false) < 0)) return -1; } else { if (virSecuritySELinuxRestoreFileLabel(mgr, dev_source->data.file.path, - true) < 0) + true, + false) < 0) return -1; } } @@ -2822,7 +2841,7 @@ virSecuritySELinuxRestoreSecuritySmartcardCallback(virDomainDef *def, database = dev->data.cert.database; if (!database) database = VIR_DOMAIN_SMARTCARD_DEFAULT_DATABASE; - return virSecuritySELinuxRestoreFileLabel(mgr, database, true); + return virSecuritySELinuxRestoreFileLabel(mgr, database, true, false); case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH: return virSecuritySELinuxRestoreChardevLabel(mgr, def, @@ -2859,7 +2878,7 @@ virSecuritySELinuxRestoreSysinfoLabel(virSecurityManager *mgr, virSysinfoFWCfgDef *f = &def->fw_cfgs[i]; if (f->file && - virSecuritySELinuxRestoreFileLabel(mgr, f->file, true) < 0) + virSecuritySELinuxRestoreFileLabel(mgr, f->file, true, false) < 0) return -1; } @@ -2955,28 +2974,28 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManager *mgr, } if (def->os.kernel && - virSecuritySELinuxRestoreFileLabel(mgr, def->os.kernel, true) < 0) + virSecuritySELinuxRestoreFileLabel(mgr, def->os.kernel, true, false) < 0) rc = -1; if (def->os.initrd && - virSecuritySELinuxRestoreFileLabel(mgr, def->os.initrd, true) < 0) + virSecuritySELinuxRestoreFileLabel(mgr, def->os.initrd, true, false) < 0) rc = -1; if (def->os.shim && - virSecuritySELinuxRestoreFileLabel(mgr, def->os.shim, true) < 0) + virSecuritySELinuxRestoreFileLabel(mgr, def->os.shim, true, false) < 0) rc = -1; if (def->os.dtb && - virSecuritySELinuxRestoreFileLabel(mgr, def->os.dtb, true) < 0) + virSecuritySELinuxRestoreFileLabel(mgr, def->os.dtb, true, false) < 0) rc = -1; for (i = 0; i < def->os.nacpiTables; i++) { - if (virSecuritySELinuxRestoreFileLabel(mgr, def->os.acpiTables[i]->path, true) < 0) + if (virSecuritySELinuxRestoreFileLabel(mgr, def->os.acpiTables[i]->path, true, false) < 0) rc = -1; } if (def->pstore && - virSecuritySELinuxRestoreFileLabel(mgr, def->pstore->path, true) < 0) + virSecuritySELinuxRestoreFileLabel(mgr, def->pstore->path, true, false) < 0) rc = -1; return rc; @@ -3589,7 +3608,7 @@ virSecuritySELinuxDomainRestorePathLabel(virSecurityManager *mgr, if (!secdef || !secdef->relabel) return 0; - return virSecuritySELinuxRestoreFileLabel(mgr, path, true); + return virSecuritySELinuxRestoreFileLabel(mgr, path, true, false); } @@ -3657,7 +3676,7 @@ virSecuritySELinuxRestoreFileLabels(virSecurityManager *mgr, struct dirent *ent; g_autoptr(DIR) dir = NULL; - if ((ret = virSecuritySELinuxRestoreFileLabel(mgr, path, true))) + if ((ret = virSecuritySELinuxRestoreFileLabel(mgr, path, true, false))) return ret; if (!virFileIsDir(path)) @@ -3668,7 +3687,7 @@ virSecuritySELinuxRestoreFileLabels(virSecurityManager *mgr, while ((ret = virDirRead(dir, &ent, path)) > 0) { g_autofree char *filename = g_strdup_printf("%s/%s", path, ent->d_name); - ret = virSecuritySELinuxRestoreFileLabel(mgr, filename, true); + ret = virSecuritySELinuxRestoreFileLabel(mgr, filename, true, false); if (ret < 0) break; } -- 2.51.1
This marks kernel, initrd, dtb, and similar elements with is_shared, meaning we skip label restore if xattr label remembering is not enabled or supported (like on qemu:///session). non-xattr based label restore is subject to race conditions if multiple VMs are starting and stopping using shared media: https://issues.redhat.com/browse/RHEL-126945 This convers every case that is using content_context (virt_content_t) as SetFileLabel time, which is how we are marking content as readonly. All the shareable cases (marked with file_context) are already skipping remembering/label restore entirely. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/security/security_selinux.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 898f253256..2f3cc274a5 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2841,7 +2841,7 @@ virSecuritySELinuxRestoreSecuritySmartcardCallback(virDomainDef *def, database = dev->data.cert.database; if (!database) database = VIR_DOMAIN_SMARTCARD_DEFAULT_DATABASE; - return virSecuritySELinuxRestoreFileLabel(mgr, database, true, false); + return virSecuritySELinuxRestoreFileLabel(mgr, database, true, true); case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH: return virSecuritySELinuxRestoreChardevLabel(mgr, def, @@ -2878,7 +2878,7 @@ virSecuritySELinuxRestoreSysinfoLabel(virSecurityManager *mgr, virSysinfoFWCfgDef *f = &def->fw_cfgs[i]; if (f->file && - virSecuritySELinuxRestoreFileLabel(mgr, f->file, true, false) < 0) + virSecuritySELinuxRestoreFileLabel(mgr, f->file, true, true) < 0) return -1; } @@ -2974,23 +2974,23 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManager *mgr, } if (def->os.kernel && - virSecuritySELinuxRestoreFileLabel(mgr, def->os.kernel, true, false) < 0) + virSecuritySELinuxRestoreFileLabel(mgr, def->os.kernel, true, true) < 0) rc = -1; if (def->os.initrd && - virSecuritySELinuxRestoreFileLabel(mgr, def->os.initrd, true, false) < 0) + virSecuritySELinuxRestoreFileLabel(mgr, def->os.initrd, true, true) < 0) rc = -1; if (def->os.shim && - virSecuritySELinuxRestoreFileLabel(mgr, def->os.shim, true, false) < 0) + virSecuritySELinuxRestoreFileLabel(mgr, def->os.shim, true, true) < 0) rc = -1; if (def->os.dtb && - virSecuritySELinuxRestoreFileLabel(mgr, def->os.dtb, true, false) < 0) + virSecuritySELinuxRestoreFileLabel(mgr, def->os.dtb, true, true) < 0) rc = -1; for (i = 0; i < def->os.nacpiTables; i++) { - if (virSecuritySELinuxRestoreFileLabel(mgr, def->os.acpiTables[i]->path, true, false) < 0) + if (virSecuritySELinuxRestoreFileLabel(mgr, def->os.acpiTables[i]->path, true, true) < 0) rc = -1; } -- 2.51.1
On 11/11/25 18:39, Cole Robinson via Devel wrote:
https://issues.redhat.com/browse/RHEL-126945
selinux label set/restore has race conditions on qemu:///session, where the xattr label remembering path is not used. See the above issue for a more detailed description of the problem.
This series dodges the issue by skipping the fallback label restore for readonly resources like kernel and initrd, basically anything that would get the virt_content_t label.
Note, disks already skip _all_ attempts to remember or restore selinux labels if the disk is marked readonly or shareable, and has done so for a long time. Maybe we should extend that out for anything that is inherently readonly or shareable, like kernel + initrd. But for now I stuck with the more conservative approach.
And finally, this doesn't actually fix the race condition. If label remembering is working, the refcounting covers us. But if you disable label remembering at the qemu.conf level, there's likely similar issues in the DAC driver even for qemu:///system. I did not look into fixing the race but I suspect it involves keeping the security driver locked until the VM fully boots up.
First two patches are not strictly related, but I noticed them while I was in the area Patch 3 adds the plumbing but no behavior change Patch 4 changes behavior
Cole Robinson (4): selinux: Match remember/recall arguments for SavedStateLabel selinux: Don't remember labels for shareable SCSI devices selinux: Add is_shared plumbing to RestoreFileLabel selinux: Mark anything using content_context as shared
src/security/security_selinux.c | 107 +++++++++++++++++++------------- 1 file changed, 63 insertions(+), 44 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Cole Robinson -
Michal Prívozník