[PATCH v4 0/8] qemu: Introduce shared_filesystems configuration option
For justification see v3: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/PISB... This version includes patches that deal with seclabel remembering without instructing users to disable it. Patch 2/8 was modified to change the docs for the new option. Patches 1-5 will get an R-b by me as I've adopted them. Patches 6-8 are new. Andrea Bolognani (5): security: Fix alignment qemu: Introduce shared_filesystems configuration option qemu: Propagate shared_filesystems utils: Use overrides in virFileIsSharedFS() qemu: Always set labels for TPM state Peter Krempa (3): virFileIsSharedFSOverride: Export storage_source: Add field for skipping seclabel remembering qemu: migration: Don't remember seclabel for images shared from current host src/conf/storage_source_conf.c | 3 ++ src/conf/storage_source_conf.h | 9 ++++ src/libvirt_private.syms | 1 + src/lxc/lxc_controller.c | 3 +- src/lxc/lxc_driver.c | 2 +- src/lxc/lxc_process.c | 4 +- src/qemu/libvirtd_qemu.aug | 3 ++ src/qemu/qemu.conf.in | 26 +++++++++ src/qemu/qemu_conf.c | 31 +++++++++++ src/qemu/qemu_conf.h | 2 + src/qemu/qemu_domain.c | 7 ++- src/qemu/qemu_extdevice.c | 2 +- src/qemu/qemu_migration.c | 72 +++++++++++++++++++++---- src/qemu/qemu_security.c | 85 +++++++++++++++++++++++------- src/qemu/qemu_tpm.c | 38 +++++++------ src/qemu/qemu_tpm.h | 10 ++-- src/qemu/test_libvirtd_qemu.aug.in | 5 ++ src/security/security_apparmor.c | 8 ++- src/security/security_dac.c | 50 ++++++++++++++---- src/security/security_driver.h | 8 ++- src/security/security_manager.c | 33 +++++++++--- src/security/security_manager.h | 9 +++- src/security/security_nop.c | 5 ++ src/security/security_selinux.c | 59 ++++++++++++++++----- src/security/security_stack.c | 32 ++++++++--- src/util/virfile.c | 63 ++++++++++++++++++++-- src/util/virfile.h | 5 +- tests/securityselinuxlabeltest.c | 2 +- tests/virfiletest.c | 2 +- 29 files changed, 472 insertions(+), 107 deletions(-) -- 2.45.2
From: Andrea Bolognani <abologna@redhat.com> Signed-off-by: Andrea Bolognani <abologna@redhat.com> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> --- src/security/security_selinux.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index e29f627bc2..173f47316b 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1983,9 +1983,9 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManager *mgr, static int virSecuritySELinuxSetImageLabel(virSecurityManager *mgr, - virDomainDef *def, - virStorageSource *src, - virSecurityDomainImageLabelFlags flags) + virDomainDef *def, + virStorageSource *src, + virSecurityDomainImageLabelFlags flags) { virStorageSource *parent = src; virStorageSource *n; -- 2.45.2
From: Andrea Bolognani <abologna@redhat.com> As explained in the comment, this can help in scenarios where a shared filesystem can't be detected as such by libvirt, by giving the admin the opportunity to provide this information manually. Signed-off-by: Andrea Bolognani <abologna@redhat.com> Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/libvirtd_qemu.aug | 3 +++ src/qemu/qemu.conf.in | 26 +++++++++++++++++++++++++ src/qemu/qemu_conf.c | 31 ++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 2 ++ src/qemu/test_libvirtd_qemu.aug.in | 5 +++++ 5 files changed, 67 insertions(+) diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index 2b6526538f..1377fd89cc 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -143,6 +143,8 @@ module Libvirtd_qemu = let storage_entry = bool_entry "storage_use_nbdkit" + let filesystem_entry = str_array_entry "shared_filesystems" + (* Entries that used to exist in the config which are now * deleted. We keep on parsing them so we don't break * ability to parse old configs after upgrade @@ -173,6 +175,7 @@ module Libvirtd_qemu = | swtpm_entry | capability_filters_entry | storage_entry + | filesystem_entry | obsolete_entry let comment = [ label "#comment" . del /#[ \t]*/ "# " . store /([^ \t\n][^\n]*)?/ . del /\n/ "\n" ] diff --git a/src/qemu/qemu.conf.in b/src/qemu/qemu.conf.in index 6bc2140dcb..b93024a489 100644 --- a/src/qemu/qemu.conf.in +++ b/src/qemu/qemu.conf.in @@ -985,3 +985,29 @@ # note that the default might change in future releases. # #storage_use_nbdkit = @USE_NBDKIT_DEFAULT@ + +# libvirt will normally prevent migration if the storage backing the VM is not +# on a shared filesystems. Sometimes, however, the storage *is* shared despite +# not being detected as such: for example, this is the case when one of the +# hosts involved in the migration is exporting its local storage to the other +# one via NFS. +# +# Any directory listed here will be assumed to live on a shared filesystem, +# making migration possible in scenarios such as the one described above. It's +# the system's administrator responsibility to ensure that other hosts can +# access this directory. +# +# This option is not symmetrical and should only be used on hosts where the +# storage is being exported from. It must not be used on hosts accessing the +# storage via a remote protocol. +# +# NOTE: this option is intended to help in very specific scenarios that are +# rarely encountered. If you find yourself reaching for this option, consider +# reworking your environment so that it follows a more common architecture +# rather than using it. +# +#shared_filesystems = [ +# "/path/to/images", +# "/path/to/nvram", +# "/path/to/swtpm" +#] diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index b36bede6c3..9c51da6cca 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -374,6 +374,8 @@ static void virQEMUDriverConfigDispose(void *obj) g_strfreev(cfg->capabilityfilters); + g_strfreev(cfg->sharedFilesystems); + g_free(cfg->deprecationBehavior); } @@ -1084,6 +1086,32 @@ virQEMUDriverConfigLoadStorageEntry(virQEMUDriverConfig *cfg, } +static int +virQEMUDriverConfigLoadFilesystemEntry(virQEMUDriverConfig *cfg, + virConf *conf) +{ + char **iter; + + if (virConfGetValueStringList(conf, "shared_filesystems", false, + &cfg->sharedFilesystems) < 0) + return -1; + + if (!cfg->sharedFilesystems) + return 0; + + /* The paths provided by the user might contain trailing slashes + * and other fun diversions, which would break the naive string + * comparisons that we're later going to use them for */ + for (iter = cfg->sharedFilesystems; *iter; iter++) { + char *canon = virFileCanonicalizePath(*iter); + g_free(*iter); + *iter = canon; + } + + return 0; +} + + int virQEMUDriverConfigLoadFile(virQEMUDriverConfig *cfg, const char *filename, bool privileged) @@ -1158,6 +1186,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfig *cfg, if (virQEMUDriverConfigLoadStorageEntry(cfg, conf) < 0) return -1; + if (virQEMUDriverConfigLoadFilesystemEntry(cfg, conf) < 0) + return -1; + return 0; } diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index aa1e1a626c..b9cdc75c6b 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -233,6 +233,8 @@ struct _virQEMUDriverConfig { bool storageUseNbdkit; virQEMUSchedCore schedCore; + + char **sharedFilesystems; }; G_DEFINE_AUTOPTR_CLEANUP_FUNC(virQEMUDriverConfig, virObjectUnref); diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index b97e6de11e..69fdae215a 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -119,3 +119,8 @@ module Test_libvirtd_qemu = { "deprecation_behavior" = "none" } { "sched_core" = "none" } { "storage_use_nbdkit" = "@USE_NBDKIT_DEFAULT@" } +{ "shared_filesystems" + { "1" = "/path/to/images" } + { "2" = "/path/to/nvram" } + { "3" = "/path/to/swtpm" } +} -- 2.45.2
From: Andrea Bolognani <abologna@redhat.com> virFileIsSharedFS() is the function that ultimately decides whether a filesystem should be considered shared, but the list of manually configured shared filesystems is part of the QEMU driver's configuration, so we need to pass the information through several layers in order to make use of it. Note that with this change the list is propagated all the way through, but its contents are still ignored, so the behavior remains the same for now. Signed-off-by: Andrea Bolognani <abologna@redhat.com> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> --- src/lxc/lxc_controller.c | 3 +- src/lxc/lxc_driver.c | 2 +- src/lxc/lxc_process.c | 4 +- src/qemu/qemu_domain.c | 7 ++- src/qemu/qemu_extdevice.c | 2 +- src/qemu/qemu_migration.c | 23 ++++----- src/qemu/qemu_security.c | 85 ++++++++++++++++++++++++-------- src/qemu/qemu_tpm.c | 29 +++++++---- src/qemu/qemu_tpm.h | 10 ++-- src/security/security_apparmor.c | 8 ++- src/security/security_dac.c | 47 ++++++++++++++---- src/security/security_driver.h | 8 ++- src/security/security_manager.c | 33 ++++++++++--- src/security/security_manager.h | 9 +++- src/security/security_nop.c | 5 ++ src/security/security_selinux.c | 50 ++++++++++++++----- src/security/security_stack.c | 32 +++++++++--- src/util/virfile.c | 13 +++-- src/util/virfile.h | 3 +- tests/securityselinuxlabeltest.c | 2 +- tests/virfiletest.c | 2 +- 21 files changed, 281 insertions(+), 96 deletions(-) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 505b71d05e..7b432a1160 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -1919,7 +1919,8 @@ static int virLXCControllerSetupDisk(virLXCController *ctrl, /* Labelling normally operates on src, but we need * to actually label the dst here, so hack the config */ def->src->path = dst; - if (virSecurityManagerSetImageLabel(securityDriver, ctrl->def, def->src, + if (virSecurityManagerSetImageLabel(securityDriver, + NULL, ctrl->def, def->src, VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN) < 0) goto cleanup; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index f76d09e8a9..bf79be477e 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -3262,7 +3262,7 @@ lxcDomainAttachDeviceMknodHelper(pid_t pid G_GNUC_UNUSED, char *tmpsrc = def->src->path; def->src->path = data->file; if (virSecurityManagerSetImageLabel(data->driver->securityManager, - data->vm->def, def->src, + NULL, data->vm->def, def->src, VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN) < 0) { def->src->path = tmpsrc; goto cleanup; diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index f5eb5383ec..205ab96ebb 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -170,7 +170,7 @@ static void virLXCProcessCleanup(virLXCDriver *driver, } if (flags & VIR_LXC_PROCESS_CLEANUP_RESTORE_SECLABEL) { - virSecurityManagerRestoreAllLabel(driver->securityManager, + virSecurityManagerRestoreAllLabel(driver->securityManager, NULL, vm->def, false, false); } @@ -1320,7 +1320,7 @@ int virLXCProcessStart(virLXCDriver * driver, stopFlags |= VIR_LXC_PROCESS_CLEANUP_RELEASE_SECLABEL; VIR_DEBUG("Setting domain security labels"); - if (virSecurityManagerSetAllLabel(driver->securityManager, + if (virSecurityManagerSetAllLabel(driver->securityManager, NULL, vm->def, NULL, false, false) < 0) goto cleanup; stopFlags |= VIR_LXC_PROCESS_CLEANUP_RESTORE_SECLABEL; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2134b11038..39ee35b8f5 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11935,7 +11935,12 @@ virQEMUFileOpenAs(uid_t fallback_uid, bool need_unlink = false; unsigned int vfoflags = 0; int fd = -1; - int path_shared = virFileIsSharedFS(path); + /* Note that it would be pointless to pass + * virQEMUDriverConfig.sharedFilesystems here, since those + * listed there are by definition paths that can be accessed + * as local from the current host. Thus, a second attempt at + * opening the file would not make a difference */ + int path_shared = virFileIsSharedFS(path, NULL); uid_t uid = geteuid(); gid_t gid = getegid(); diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c index ed5976d1f7..dc1bb56237 100644 --- a/src/qemu/qemu_extdevice.c +++ b/src/qemu/qemu_extdevice.c @@ -165,7 +165,7 @@ qemuExtDevicesCleanupHost(virQEMUDriver *driver, virDomainTPMDef *tpm = def->tpms[i]; if (tpm->type == VIR_DOMAIN_TPM_TYPE_EMULATOR) - qemuExtTPMCleanupHost(tpm, flags, outgoingMigration); + qemuExtTPMCleanupHost(driver, tpm, flags, outgoingMigration); } } diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 4fd7a0aafb..2c462fd5f5 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1430,6 +1430,7 @@ qemuMigrationSrcIsAllowed(virDomainObj *vm, unsigned int flags) { qemuDomainObjPrivate *priv = vm->privateData; + virQEMUDriver *driver = priv->driver; int nsnapshots; int pauseReason; size_t i; @@ -1604,7 +1605,7 @@ qemuMigrationSrcIsAllowed(virDomainObj *vm, } } - if (qemuTPMHasSharedStorage(vm->def)&& + if (qemuTPMHasSharedStorage(driver, vm->def) && !qemuTPMCanMigrateSharedStorage(vm->def)) { virReportError(VIR_ERR_NO_SUPPORT, "%s", _("the running swtpm does not support migration with shared storage")); @@ -1616,20 +1617,23 @@ qemuMigrationSrcIsAllowed(virDomainObj *vm, } static bool -qemuMigrationSrcIsSafe(virDomainDef *def, - virQEMUCaps *qemuCaps, +qemuMigrationSrcIsSafe(virDomainObj *vm, size_t nmigrate_disks, const char **migrate_disks, unsigned int flags) { + qemuDomainObjPrivate *priv = vm->privateData; + virQEMUCaps *qemuCaps = priv->qemuCaps; + virQEMUDriver *driver = priv->driver; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); bool storagemigration = flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC); size_t i; int rc; - for (i = 0; i < def->ndisks; i++) { - virDomainDiskDef *disk = def->disks[i]; + for (i = 0; i < vm->def->ndisks; i++) { + virDomainDiskDef *disk = vm->def->disks[i]; const char *src = virDomainDiskGetSource(disk); virStorageType actualType = virStorageSourceGetActualType(disk->src); bool unsafe = false; @@ -1648,7 +1652,7 @@ qemuMigrationSrcIsSafe(virDomainDef *def, /* However, disks on local FS (e.g. ext4) are not safe. */ switch (actualType) { case VIR_STORAGE_TYPE_FILE: - if ((rc = virFileIsSharedFS(src)) < 0) { + if ((rc = virFileIsSharedFS(src, cfg->sharedFilesystems)) < 0) { return false; } else if (rc == 0) { unsafe = true; @@ -2608,8 +2612,7 @@ qemuMigrationSrcBeginPhase(virQEMUDriver *driver, return NULL; if (!(flags & (VIR_MIGRATE_UNSAFE | VIR_MIGRATE_OFFLINE)) && - !qemuMigrationSrcIsSafe(vm->def, priv->qemuCaps, - nmigrate_disks, migrate_disks, flags)) + !qemuMigrationSrcIsSafe(vm, nmigrate_disks, migrate_disks, flags)) return NULL; if (flags & VIR_MIGRATE_POSTCOPY && @@ -6075,7 +6078,6 @@ qemuMigrationSrcPerformJob(virQEMUDriver *driver, int ret = -1; virErrorPtr orig_err = NULL; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); - qemuDomainObjPrivate *priv = vm->privateData; qemuDomainJobPrivate *jobPriv = vm->job->privateData; if (flags & VIR_MIGRATE_POSTCOPY_RESUME) { @@ -6100,8 +6102,7 @@ qemuMigrationSrcPerformJob(virQEMUDriver *driver, goto endjob; if (!(flags & (VIR_MIGRATE_UNSAFE | VIR_MIGRATE_OFFLINE)) && - !qemuMigrationSrcIsSafe(vm->def, priv->qemuCaps, - nmigrate_disks, migrate_disks, flags)) + !qemuMigrationSrcIsSafe(vm, nmigrate_disks, migrate_disks, flags)) goto endjob; qemuMigrationSrcStoreDomainState(vm); diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index 4aaa863ae9..996c95acc0 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -38,15 +38,18 @@ qemuSecuritySetAllLabel(virQEMUDriver *driver, { int ret = -1; qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); pid_t pid = -1; if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) pid = vm->pid; - if (virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (virSecurityManagerTransactionStart(driver->securityManager, + cfg->sharedFilesystems) < 0) goto cleanup; if (virSecurityManagerSetAllLabel(driver->securityManager, + cfg->sharedFilesystems, vm->def, incomingPath, priv->chardevStdioLogd, @@ -70,6 +73,7 @@ qemuSecurityRestoreAllLabel(virQEMUDriver *driver, bool migrated) { qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); bool transactionStarted = false; /* In contrast to qemuSecuritySetAllLabel, do not use vm->pid @@ -78,10 +82,12 @@ qemuSecurityRestoreAllLabel(virQEMUDriver *driver, * domain's namespace is gone as qemu was the only process * running there. We would not succeed in entering the * namespace then. */ - if (virSecurityManagerTransactionStart(driver->securityManager) >= 0) + if (virSecurityManagerTransactionStart(driver->securityManager, + cfg->sharedFilesystems) >= 0) transactionStarted = true; virSecurityManagerRestoreAllLabel(driver->securityManager, + cfg->sharedFilesystems, vm->def, migrated, priv->chardevStdioLogd); @@ -103,6 +109,7 @@ qemuSecuritySetImageLabel(virQEMUDriver *driver, bool chainTop) { qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); pid_t pid = -1; int ret = -1; virSecurityDomainImageLabelFlags labelFlags = 0; @@ -116,10 +123,12 @@ qemuSecuritySetImageLabel(virQEMUDriver *driver, if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) pid = vm->pid; - if (virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (virSecurityManagerTransactionStart(driver->securityManager, + cfg->sharedFilesystems) < 0) goto cleanup; if (virSecurityManagerSetImageLabel(driver->securityManager, + cfg->sharedFilesystems, vm->def, src, labelFlags) < 0) goto cleanup; @@ -141,6 +150,7 @@ qemuSecurityRestoreImageLabel(virQEMUDriver *driver, bool backingChain) { qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); pid_t pid = -1; int ret = -1; virSecurityDomainImageLabelFlags labelFlags = 0; @@ -151,10 +161,12 @@ qemuSecurityRestoreImageLabel(virQEMUDriver *driver, if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) pid = vm->pid; - if (virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (virSecurityManagerTransactionStart(driver->securityManager, + cfg->sharedFilesystems) < 0) goto cleanup; if (virSecurityManagerRestoreImageLabel(driver->securityManager, + cfg->sharedFilesystems, vm->def, src, labelFlags) < 0) goto cleanup; @@ -176,6 +188,7 @@ qemuSecurityMoveImageMetadata(virQEMUDriver *driver, virStorageSource *dst) { qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); pid_t pid = -1; if (!priv->rememberOwner) @@ -184,7 +197,9 @@ qemuSecurityMoveImageMetadata(virQEMUDriver *driver, if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) pid = vm->pid; - return virSecurityManagerMoveImageMetadata(driver->securityManager, pid, src, dst); + return virSecurityManagerMoveImageMetadata(driver->securityManager, + cfg->sharedFilesystems, + pid, src, dst); } @@ -194,13 +209,15 @@ qemuSecuritySetHostdevLabel(virQEMUDriver *driver, virDomainHostdevDef *hostdev) { qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); pid_t pid = -1; int ret = -1; if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) pid = vm->pid; - if (virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (virSecurityManagerTransactionStart(driver->securityManager, + cfg->sharedFilesystems) < 0) goto cleanup; if (virSecurityManagerSetHostdevLabel(driver->securityManager, @@ -226,13 +243,15 @@ qemuSecurityRestoreHostdevLabel(virQEMUDriver *driver, virDomainHostdevDef *hostdev) { qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); pid_t pid = -1; int ret = -1; if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) pid = vm->pid; - if (virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (virSecurityManagerTransactionStart(driver->securityManager, + cfg->sharedFilesystems) < 0) goto cleanup; if (virSecurityManagerRestoreHostdevLabel(driver->securityManager, @@ -258,13 +277,15 @@ qemuSecuritySetMemoryLabel(virQEMUDriver *driver, virDomainMemoryDef *mem) { qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); pid_t pid = -1; int ret = -1; if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) pid = vm->pid; - if (virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (virSecurityManagerTransactionStart(driver->securityManager, + cfg->sharedFilesystems) < 0) goto cleanup; if (virSecurityManagerSetMemoryLabel(driver->securityManager, @@ -289,13 +310,15 @@ qemuSecurityRestoreMemoryLabel(virQEMUDriver *driver, virDomainMemoryDef *mem) { qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); pid_t pid = -1; int ret = -1; if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) pid = vm->pid; - if (virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (virSecurityManagerTransactionStart(driver->securityManager, + cfg->sharedFilesystems) < 0) goto cleanup; if (virSecurityManagerRestoreMemoryLabel(driver->securityManager, @@ -320,13 +343,15 @@ qemuSecuritySetInputLabel(virDomainObj *vm, { qemuDomainObjPrivate *priv = vm->privateData; virQEMUDriver *driver = priv->driver; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); pid_t pid = -1; int ret = -1; if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) pid = vm->pid; - if (virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (virSecurityManagerTransactionStart(driver->securityManager, + cfg->sharedFilesystems) < 0) goto cleanup; if (virSecurityManagerSetInputLabel(driver->securityManager, @@ -351,13 +376,15 @@ qemuSecurityRestoreInputLabel(virDomainObj *vm, { qemuDomainObjPrivate *priv = vm->privateData; virQEMUDriver *driver = priv->driver; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); pid_t pid = -1; int ret = -1; if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) pid = vm->pid; - if (virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (virSecurityManagerTransactionStart(driver->securityManager, + cfg->sharedFilesystems) < 0) goto cleanup; if (virSecurityManagerRestoreInputLabel(driver->securityManager, @@ -383,12 +410,14 @@ qemuSecuritySetChardevLabel(virQEMUDriver *driver, { int ret = -1; qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); pid_t pid = -1; if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) pid = vm->pid; - if (virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (virSecurityManagerTransactionStart(driver->securityManager, + cfg->sharedFilesystems) < 0) goto cleanup; if (virSecurityManagerSetChardevLabel(driver->securityManager, @@ -415,12 +444,14 @@ qemuSecurityRestoreChardevLabel(virQEMUDriver *driver, { int ret = -1; qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); pid_t pid = -1; if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) pid = vm->pid; - if (virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (virSecurityManagerTransactionStart(driver->securityManager, + cfg->sharedFilesystems) < 0) goto cleanup; if (virSecurityManagerRestoreChardevLabel(driver->securityManager, @@ -446,12 +477,14 @@ qemuSecuritySetNetdevLabel(virQEMUDriver *driver, { int ret = -1; qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); pid_t pid = -1; if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) pid = vm->pid; - if (virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (virSecurityManagerTransactionStart(driver->securityManager, + cfg->sharedFilesystems) < 0) goto cleanup; if (virSecurityManagerSetNetdevLabel(driver->securityManager, @@ -476,12 +509,14 @@ qemuSecurityRestoreNetdevLabel(virQEMUDriver *driver, { int ret = -1; qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); pid_t pid = -1; if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) pid = vm->pid; - if (virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (virSecurityManagerTransactionStart(driver->securityManager, + cfg->sharedFilesystems) < 0) goto cleanup; if (virSecurityManagerRestoreNetdevLabel(driver->securityManager, @@ -505,9 +540,11 @@ qemuSecuritySetTPMLabels(virQEMUDriver *driver, bool setTPMStateLabel) { qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); int ret = -1; - if (virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (virSecurityManagerTransactionStart(driver->securityManager, + cfg->sharedFilesystems) < 0) goto cleanup; if (virSecurityManagerSetTPMLabels(driver->securityManager, @@ -531,9 +568,11 @@ qemuSecurityRestoreTPMLabels(virQEMUDriver *driver, bool restoreTPMStateLabel) { qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); int ret = -1; - if (virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (virSecurityManagerTransactionStart(driver->securityManager, + cfg->sharedFilesystems) < 0) goto cleanup; if (virSecurityManagerRestoreTPMLabels(driver->securityManager, @@ -558,13 +597,15 @@ qemuSecurityDomainSetPathLabel(virQEMUDriver *driver, bool allowSubtree) { qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); pid_t pid = -1; int ret = -1; if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) pid = vm->pid; - if (virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (virSecurityManagerTransactionStart(driver->securityManager, + cfg->sharedFilesystems) < 0) goto cleanup; if (virSecurityManagerDomainSetPathLabel(driver->securityManager, @@ -590,13 +631,15 @@ qemuSecurityDomainRestorePathLabel(virQEMUDriver *driver, const char *path) { qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); pid_t pid = -1; int ret = -1; if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) pid = vm->pid; - if (virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (virSecurityManagerTransactionStart(driver->securityManager, + cfg->sharedFilesystems) < 0) goto cleanup; if (virSecurityManagerDomainRestorePathLabel(driver->securityManager, @@ -634,6 +677,7 @@ qemuSecurityDomainSetMountNSPathLabel(virQEMUDriver *driver, const char *path) { int ret = -1; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) { VIR_DEBUG("Not labeling '%s': mount namespace disabled for domain '%s'", @@ -641,7 +685,8 @@ qemuSecurityDomainSetMountNSPathLabel(virQEMUDriver *driver, return 1; } - if (virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (virSecurityManagerTransactionStart(driver->securityManager, + cfg->sharedFilesystems) < 0) goto cleanup; if (virSecurityManagerDomainSetPathLabel(driver->securityManager, diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 2f17918cbb..08af5aad2e 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -538,6 +538,7 @@ qemuTPMEmulatorReconfigure(const char *storagepath, * @privileged: whether we are running in privileged mode * @swtpm_user: The uid for the swtpm to run as (drop privileges to from root) * @swtpm_group: The gid for the swtpm to run as + * @sharedFilesystems: list of filesystem to consider shared * @incomingMigration: whether we have an incoming migration * * Create the virCommand use for starting the emulator @@ -551,6 +552,7 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm, bool privileged, uid_t swtpm_user, gid_t swtpm_group, + char *const *sharedFilesystems, bool incomingMigration) { g_autoptr(virCommand) cmd = NULL; @@ -568,7 +570,7 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm, /* Do not create storage and run swtpm_setup on incoming migration over * shared storage */ - on_shared_storage = virFileIsSharedFS(tpm->data.emulator.storagepath) == 1; + on_shared_storage = virFileIsSharedFS(tpm->data.emulator.storagepath, sharedFilesystems) == 1; if (incomingMigration && on_shared_storage) create_storage = false; @@ -738,6 +740,7 @@ qemuTPMEmulatorInitPaths(virDomainTPMDef *tpm, /** * qemuTPMEmulatorCleanupHost: + * @driver: QEMU driver * @tpm: TPM definition * @flags: flags indicating whether to keep or remove TPM persistent state * @outgoingMigration: whether cleanup is due to an outgoing migration @@ -745,15 +748,18 @@ qemuTPMEmulatorInitPaths(virDomainTPMDef *tpm, * Clean up persistent storage for the swtpm. */ static void -qemuTPMEmulatorCleanupHost(virDomainTPMDef *tpm, +qemuTPMEmulatorCleanupHost(virQEMUDriver *driver, + virDomainTPMDef *tpm, virDomainUndefineFlagsValues flags, bool outgoingMigration) { + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + /* Never remove the state in case of outgoing migration with shared * storage. */ if (outgoingMigration && - virFileIsSharedFS(tpm->data.emulator.storagepath) == 1) + virFileIsSharedFS(tpm->data.emulator.storagepath, cfg->sharedFilesystems) == 1) return; /* @@ -939,6 +945,7 @@ qemuTPMEmulatorStart(virQEMUDriver *driver, driver->privileged, cfg->swtpm_user, cfg->swtpm_group, + cfg->sharedFilesystems, incomingMigration))) return -1; @@ -954,7 +961,7 @@ qemuTPMEmulatorStart(virQEMUDriver *driver, virCommandSetErrorFD(cmd, &errfd); if (incomingMigration && - virFileIsSharedFS(tpm->data.emulator.storagepath) == 1) { + virFileIsSharedFS(tpm->data.emulator.storagepath, cfg->sharedFilesystems) == 1) { /* security labels must have been set up on source already */ setTPMStateLabel = false; } @@ -1014,8 +1021,10 @@ qemuTPMEmulatorStart(virQEMUDriver *driver, bool -qemuTPMHasSharedStorage(virDomainDef *def) +qemuTPMHasSharedStorage(virQEMUDriver *driver, + virDomainDef *def) { + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); size_t i; for (i = 0; i < def->ntpms; i++) { @@ -1023,7 +1032,8 @@ qemuTPMHasSharedStorage(virDomainDef *def) switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_EMULATOR: - return virFileIsSharedFS(tpm->data.emulator.storagepath) == 1; + return virFileIsSharedFS(tpm->data.emulator.storagepath, + cfg->sharedFilesystems) == 1; case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: case VIR_DOMAIN_TPM_TYPE_EXTERNAL: case VIR_DOMAIN_TPM_TYPE_LAST: @@ -1101,11 +1111,12 @@ qemuExtTPMPrepareHost(virQEMUDriver *driver, void -qemuExtTPMCleanupHost(virDomainTPMDef *tpm, +qemuExtTPMCleanupHost(virQEMUDriver *driver, + virDomainTPMDef *tpm, virDomainUndefineFlagsValues flags, bool outgoingMigration) { - qemuTPMEmulatorCleanupHost(tpm, flags, outgoingMigration); + qemuTPMEmulatorCleanupHost(driver, tpm, flags, outgoingMigration); } @@ -1137,7 +1148,7 @@ qemuExtTPMStop(virQEMUDriver *driver, return; qemuTPMEmulatorStop(cfg->swtpmStateDir, shortName); - if (outgoingMigration && qemuTPMHasSharedStorage(vm->def)) + if (outgoingMigration && qemuTPMHasSharedStorage(driver, vm->def)) restoreTPMStateLabel = false; if (qemuSecurityRestoreTPMLabels(driver, vm, restoreTPMStateLabel) < 0) diff --git a/src/qemu/qemu_tpm.h b/src/qemu/qemu_tpm.h index 33ba5d2268..3071dc3f71 100644 --- a/src/qemu/qemu_tpm.h +++ b/src/qemu/qemu_tpm.h @@ -35,10 +35,11 @@ int qemuExtTPMPrepareHost(virQEMUDriver *driver, ATTRIBUTE_NONNULL(3) G_GNUC_WARN_UNUSED_RESULT; -void qemuExtTPMCleanupHost(virDomainTPMDef *tpm, +void qemuExtTPMCleanupHost(virQEMUDriver *driver, + virDomainTPMDef *tpm, virDomainUndefineFlagsValues flags, bool outgoingMigration) - ATTRIBUTE_NONNULL(1); + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int qemuExtTPMStart(virQEMUDriver *driver, virDomainObj *vm, @@ -59,8 +60,9 @@ int qemuExtTPMSetupCgroup(virQEMUDriver *driver, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) G_GNUC_WARN_UNUSED_RESULT; -bool qemuTPMHasSharedStorage(virDomainDef *def) - ATTRIBUTE_NONNULL(1) +bool qemuTPMHasSharedStorage(virQEMUDriver *driver, + virDomainDef *def) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; bool qemuTPMCanMigrateSharedStorage(virDomainDef *def) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 27184aef7f..38d4817fae 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -433,6 +433,7 @@ AppArmorGenSecurityLabel(virSecurityManager *mgr G_GNUC_UNUSED, static int AppArmorSetSecurityAllLabel(virSecurityManager *mgr, + char *const *sharedFilesystems G_GNUC_UNUSED, virDomainDef *def, const char *incomingPath, bool chardevStdioLogd G_GNUC_UNUSED, @@ -507,6 +508,7 @@ AppArmorReleaseSecurityLabel(virSecurityManager *mgr G_GNUC_UNUSED, static int AppArmorRestoreSecurityAllLabel(virSecurityManager *mgr G_GNUC_UNUSED, + char *const *sharedFilesystems G_GNUC_UNUSED, virDomainDef *def, bool migrated G_GNUC_UNUSED, bool chardevStdioLogd G_GNUC_UNUSED) @@ -625,6 +627,7 @@ AppArmorClearSecuritySocketLabel(virSecurityManager *mgr G_GNUC_UNUSED, /* Called when hotplugging */ static int AppArmorRestoreSecurityImageLabel(virSecurityManager *mgr, + char *const *sharedFilesystems G_GNUC_UNUSED, virDomainDef *def, virStorageSource *src, virSecurityDomainImageLabelFlags flags G_GNUC_UNUSED) @@ -729,6 +732,7 @@ AppArmorRestoreInputLabel(virSecurityManager *mgr, /* Called when hotplugging */ static int AppArmorSetSecurityImageLabelInternal(virSecurityManager *mgr, + char *const *sharedFilesystems G_GNUC_UNUSED, virDomainDef *def, virStorageSource *src) { @@ -762,6 +766,7 @@ AppArmorSetSecurityImageLabelInternal(virSecurityManager *mgr, static int AppArmorSetSecurityImageLabel(virSecurityManager *mgr, + char *const *sharedFilesystems, virDomainDef *def, virStorageSource *src, virSecurityDomainImageLabelFlags flags G_GNUC_UNUSED) @@ -777,7 +782,8 @@ AppArmorSetSecurityImageLabel(virSecurityManager *mgr, return 0; for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) { - if (AppArmorSetSecurityImageLabelInternal(mgr, def, n) < 0) + if (AppArmorSetSecurityImageLabelInternal(mgr, sharedFilesystems, + def, n) < 0) return -1; } diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 1a3b51a298..f2f784b4fa 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -79,6 +79,7 @@ struct _virSecurityDACChownItem { typedef struct _virSecurityDACChownList virSecurityDACChownList; struct _virSecurityDACChownList { virSecurityManager *manager; + char **sharedFilesystems; virSecurityDACChownItem **items; size_t nItems; bool lock; @@ -137,6 +138,7 @@ virSecurityDACChownListFree(void *opaque) virSecurityDACChownItemFree(list->items[i]); g_free(list->items); virObjectUnref(list->manager); + g_strfreev(list->sharedFilesystems); g_free(list); } @@ -228,7 +230,9 @@ virSecurityDACTransactionRun(pid_t pid G_GNUC_UNUSED, VIR_APPEND_ELEMENT_COPY_INPLACE(paths, npaths, p); } - if (!(state = virSecurityManagerMetadataLock(list->manager, paths, npaths))) + if (!(state = virSecurityManagerMetadataLock(list->manager, + list->sharedFilesystems, + paths, npaths))) return -1; for (i = 0; i < list->nItems; i++) { @@ -533,6 +537,7 @@ virSecurityDACPreFork(virSecurityManager *mgr) /** * virSecurityDACTransactionStart: * @mgr: security manager + * @sharedFilesystems: list of filesystem to consider shared * * Starts a new transaction. In transaction nothing is chown()-ed until * TransactionCommit() is called. This is implemented as a list that is @@ -544,7 +549,8 @@ virSecurityDACPreFork(virSecurityManager *mgr) * -1 otherwise. */ static int -virSecurityDACTransactionStart(virSecurityManager *mgr) +virSecurityDACTransactionStart(virSecurityManager *mgr, + char *const *sharedFilesystems) { g_autoptr(virSecurityDACChownList) list = NULL; @@ -557,6 +563,7 @@ virSecurityDACTransactionStart(virSecurityManager *mgr) list = g_new0(virSecurityDACChownList, 1); list->manager = virObjectRef(mgr); + list->sharedFilesystems = g_strdupv((char **) sharedFilesystems); if (virThreadLocalSet(&chownList, list) < 0) { virReportSystemError(errno, "%s", @@ -859,6 +866,7 @@ virSecurityDACRestoreFileLabel(virSecurityManager *mgr, static int virSecurityDACSetImageLabelInternal(virSecurityManager *mgr, + char *const *sharedFilesystems G_GNUC_UNUSED, virDomainDef *def, virStorageSource *src, virStorageSource *parent, @@ -938,6 +946,7 @@ virSecurityDACSetImageLabelInternal(virSecurityManager *mgr, static int virSecurityDACSetImageLabel(virSecurityManager *mgr, + char *const *sharedFilesystems, virDomainDef *def, virStorageSource *src, virSecurityDomainImageLabelFlags flags) @@ -948,7 +957,8 @@ virSecurityDACSetImageLabel(virSecurityManager *mgr, for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) { const bool isChainTop = flags & VIR_SECURITY_DOMAIN_IMAGE_PARENT_CHAIN_TOP; - if (virSecurityDACSetImageLabelInternal(mgr, def, n, parent, isChainTop) < 0) + if (virSecurityDACSetImageLabelInternal(mgr, sharedFilesystems, + def, n, parent, isChainTop) < 0) return -1; if (!(flags & VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN)) @@ -962,6 +972,7 @@ virSecurityDACSetImageLabel(virSecurityManager *mgr, static int virSecurityDACRestoreImageLabelInt(virSecurityManager *mgr, + char *const *sharedFilesystems, virDomainDef *def, virStorageSource *src, bool migrated) @@ -1004,7 +1015,7 @@ virSecurityDACRestoreImageLabelInt(virSecurityManager *mgr, if (!src->path) return 0; - if ((rc = virFileIsSharedFS(src->path)) < 0) + if ((rc = virFileIsSharedFS(src->path, sharedFilesystems)) < 0) return -1; } @@ -1038,16 +1049,19 @@ virSecurityDACRestoreImageLabelInt(virSecurityManager *mgr, static int virSecurityDACRestoreImageLabel(virSecurityManager *mgr, + char *const *sharedFilesystems, virDomainDef *def, virStorageSource *src, virSecurityDomainImageLabelFlags flags G_GNUC_UNUSED) { - return virSecurityDACRestoreImageLabelInt(mgr, def, src, false); + return virSecurityDACRestoreImageLabelInt(mgr, sharedFilesystems, + def, src, false); } struct virSecurityDACMoveImageMetadataData { virSecurityManager *mgr; + char **sharedFilesystems; const char *src; const char *dst; }; @@ -1062,7 +1076,9 @@ virSecurityDACMoveImageMetadataHelper(pid_t pid G_GNUC_UNUSED, virSecurityManagerMetadataLockState *state; int ret; - if (!(state = virSecurityManagerMetadataLock(data->mgr, paths, G_N_ELEMENTS(paths)))) + if (!(state = virSecurityManagerMetadataLock(data->mgr, + data->sharedFilesystems, + paths, G_N_ELEMENTS(paths)))) return -1; ret = virSecurityMoveRememberedLabel(SECURITY_DAC_NAME, data->src, data->dst); @@ -1079,12 +1095,17 @@ virSecurityDACMoveImageMetadataHelper(pid_t pid G_GNUC_UNUSED, static int virSecurityDACMoveImageMetadata(virSecurityManager *mgr, + char *const *sharedFilesystems, pid_t pid, virStorageSource *src, virStorageSource *dst) { virSecurityDACData *priv = virSecurityManagerGetPrivateData(mgr); - struct virSecurityDACMoveImageMetadataData data = { .mgr = mgr, 0 }; + struct virSecurityDACMoveImageMetadataData data = { + .mgr = mgr, + .sharedFilesystems = (char **) sharedFilesystems, + 0 + }; int rc; /* If dynamicOwnership is turned off, or owner remembering is @@ -1883,6 +1904,7 @@ virSecurityDACRestoreSysinfoLabel(virSecurityManager *mgr, static int virSecurityDACRestoreAllLabel(virSecurityManager *mgr, + char *const *sharedFilesystems, virDomainDef *def, bool migrated, bool chardevStdioLogd) @@ -1907,6 +1929,7 @@ virSecurityDACRestoreAllLabel(virSecurityManager *mgr, for (i = 0; i < def->ndisks; i++) { if (virSecurityDACRestoreImageLabelInt(mgr, + sharedFilesystems, def, def->disks[i]->src, migrated) < 0) @@ -1974,7 +1997,8 @@ virSecurityDACRestoreAllLabel(virSecurityManager *mgr, } if (def->os.loader && def->os.loader->nvram) { - if (virSecurityDACRestoreImageLabelInt(mgr, def, def->os.loader->nvram, + if (virSecurityDACRestoreImageLabelInt(mgr, sharedFilesystems, + def, def->os.loader->nvram, migrated) < 0) rc = -1; } @@ -2116,6 +2140,7 @@ virSecurityDACSetSysinfoLabel(virSecurityManager *mgr, static int virSecurityDACSetAllLabel(virSecurityManager *mgr, + char *const *sharedFilesystems, virDomainDef *def, const char *incomingPath G_GNUC_UNUSED, bool chardevStdioLogd, @@ -2141,7 +2166,8 @@ virSecurityDACSetAllLabel(virSecurityManager *mgr, /* XXX fixme - we need to recursively label the entire tree :-( */ if (virDomainDiskGetType(def->disks[i]) == VIR_STORAGE_TYPE_DIR) continue; - if (virSecurityDACSetImageLabel(mgr, def, def->disks[i]->src, + if (virSecurityDACSetImageLabel(mgr, sharedFilesystems, + def, def->disks[i]->src, VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN | VIR_SECURITY_DOMAIN_IMAGE_PARENT_CHAIN_TOP) < 0) return -1; @@ -2210,7 +2236,8 @@ virSecurityDACSetAllLabel(virSecurityManager *mgr, } if (def->os.loader && def->os.loader->nvram) { - if (virSecurityDACSetImageLabel(mgr, def, def->os.loader->nvram, + if (virSecurityDACSetImageLabel(mgr, sharedFilesystems, + def, def->os.loader->nvram, VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN | VIR_SECURITY_DOMAIN_IMAGE_PARENT_CHAIN_TOP) < 0) return -1; diff --git a/src/security/security_driver.h b/src/security/security_driver.h index aa1fb2125d..2956e002ff 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -46,7 +46,8 @@ typedef const char *(*virSecurityDriverGetBaseLabel) (virSecurityManager *mgr, typedef int (*virSecurityDriverPreFork) (virSecurityManager *mgr); -typedef int (*virSecurityDriverTransactionStart) (virSecurityManager *mgr); +typedef int (*virSecurityDriverTransactionStart) (virSecurityManager *mgr, + char *const *sharedFilesystems); typedef int (*virSecurityDriverTransactionCommit) (virSecurityManager *mgr, pid_t pid, bool lock); @@ -80,11 +81,13 @@ typedef int (*virSecurityDomainReserveLabel) (virSecurityManager *mgr, typedef int (*virSecurityDomainReleaseLabel) (virSecurityManager *mgr, virDomainDef *sec); typedef int (*virSecurityDomainSetAllLabel) (virSecurityManager *mgr, + char *const *sharedFilesystems, virDomainDef *sec, const char *incomingPath, bool chardevStdioLogd, bool migrated); typedef int (*virSecurityDomainRestoreAllLabel) (virSecurityManager *mgr, + char *const *sharedFilesystems, virDomainDef *def, bool migrated, bool chardevStdioLogd); @@ -113,14 +116,17 @@ typedef int (*virSecurityDomainSetHugepages) (virSecurityManager *mgr, const char *path); typedef int (*virSecurityDomainSetImageLabel) (virSecurityManager *mgr, + char *const *sharedFilesystems, virDomainDef *def, virStorageSource *src, virSecurityDomainImageLabelFlags flags); typedef int (*virSecurityDomainRestoreImageLabel) (virSecurityManager *mgr, + char *const *sharedFilesystems, virDomainDef *def, virStorageSource *src, virSecurityDomainImageLabelFlags flags); typedef int (*virSecurityDomainMoveImageMetadata) (virSecurityManager *mgr, + char *const *sharedFilesystems, pid_t pid, virStorageSource *src, virStorageSource *dst); diff --git a/src/security/security_manager.c b/src/security/security_manager.c index c49c4f708f..65b173e670 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -244,6 +244,7 @@ virSecurityManagerPostFork(virSecurityManager *mgr) /** * virSecurityManagerTransactionStart: * @mgr: security manager + * @sharedFilesystems: list of filesystem to consider shared * * Starts a new transaction. In transaction nothing is changed security * label until virSecurityManagerTransactionCommit() is called. @@ -252,14 +253,15 @@ virSecurityManagerPostFork(virSecurityManager *mgr) * -1 otherwise. */ int -virSecurityManagerTransactionStart(virSecurityManager *mgr) +virSecurityManagerTransactionStart(virSecurityManager *mgr, + char *const *sharedFilesystems) { VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); if (!mgr->drv->transactionStart) return 0; - return mgr->drv->transactionStart(mgr); + return mgr->drv->transactionStart(mgr, sharedFilesystems); } @@ -402,6 +404,7 @@ virSecurityManagerGetPrivileged(virSecurityManager *mgr) /** * virSecurityManagerRestoreImageLabel: * @mgr: security manager object + * @sharedFilesystems: list of filesystem to consider shared * @vm: domain definition object * @src: disk source definition to operate on * @flags: bitwise or of 'virSecurityDomainImageLabelFlags' @@ -412,6 +415,7 @@ virSecurityManagerGetPrivileged(virSecurityManager *mgr) */ int virSecurityManagerRestoreImageLabel(virSecurityManager *mgr, + char *const *sharedFilesystems, virDomainDef *vm, virStorageSource *src, virSecurityDomainImageLabelFlags flags) @@ -423,13 +427,15 @@ virSecurityManagerRestoreImageLabel(virSecurityManager *mgr, return -1; } - return mgr->drv->domainRestoreSecurityImageLabel(mgr, vm, src, flags); + return mgr->drv->domainRestoreSecurityImageLabel(mgr, sharedFilesystems, + vm, src, flags); } /** * virSecurityManagerMoveImageMetadata: * @mgr: security manager + * @sharedFilesystems: list of filesystem to consider shared * @pid: domain's PID * @src: source of metadata * @dst: destination to move metadata to @@ -449,6 +455,7 @@ virSecurityManagerRestoreImageLabel(virSecurityManager *mgr, */ int virSecurityManagerMoveImageMetadata(virSecurityManager *mgr, + char *const *sharedFilesystems, pid_t pid, virStorageSource *src, virStorageSource *dst) @@ -458,7 +465,8 @@ virSecurityManagerMoveImageMetadata(virSecurityManager *mgr, if (!mgr->drv->domainMoveImageMetadata) return 0; - return mgr->drv->domainMoveImageMetadata(mgr, pid, src, dst); + return mgr->drv->domainMoveImageMetadata(mgr, sharedFilesystems, + pid, src, dst); } @@ -510,6 +518,7 @@ virSecurityManagerClearSocketLabel(virSecurityManager *mgr, /** * virSecurityManagerSetImageLabel: * @mgr: security manager object + * @sharedFilesystems: list of filesystem to consider shared * @vm: domain definition object * @src: disk source definition to operate on * @flags: bitwise or of 'virSecurityDomainImageLabelFlags' @@ -520,6 +529,7 @@ virSecurityManagerClearSocketLabel(virSecurityManager *mgr, */ int virSecurityManagerSetImageLabel(virSecurityManager *mgr, + char *const *sharedFilesystems, virDomainDef *vm, virStorageSource *src, virSecurityDomainImageLabelFlags flags) @@ -531,7 +541,8 @@ virSecurityManagerSetImageLabel(virSecurityManager *mgr, return -1; } - return mgr->drv->domainSetSecurityImageLabel(mgr, vm, src, flags); + return mgr->drv->domainSetSecurityImageLabel(mgr, sharedFilesystems, + vm, src, flags); } @@ -816,6 +827,7 @@ int virSecurityManagerCheckAllLabel(virSecurityManager *mgr, int virSecurityManagerSetAllLabel(virSecurityManager *mgr, + char *const *sharedFilesystems, virDomainDef *vm, const char *incomingPath, bool chardevStdioLogd, @@ -828,13 +840,15 @@ virSecurityManagerSetAllLabel(virSecurityManager *mgr, return -1; } - return mgr->drv->domainSetSecurityAllLabel(mgr, vm, incomingPath, + return mgr->drv->domainSetSecurityAllLabel(mgr, sharedFilesystems, + vm, incomingPath, chardevStdioLogd, migrated); } int virSecurityManagerRestoreAllLabel(virSecurityManager *mgr, + char *const *sharedFilesystems, virDomainDef *vm, bool migrated, bool chardevStdioLogd) @@ -846,7 +860,8 @@ virSecurityManagerRestoreAllLabel(virSecurityManager *mgr, return -1; } - return mgr->drv->domainRestoreSecurityAllLabel(mgr, vm, migrated, + return mgr->drv->domainRestoreSecurityAllLabel(mgr, sharedFilesystems, + vm, migrated, chardevStdioLogd); } @@ -1292,6 +1307,7 @@ cmpstringp(const void *p1, /** * virSecurityManagerMetadataLock: * @mgr: security manager object + * @sharedFilesystems: list of filesystem to consider shared * @paths: paths to lock * @npaths: number of items in @paths array * @@ -1307,6 +1323,7 @@ cmpstringp(const void *p1, */ virSecurityManagerMetadataLockState * virSecurityManagerMetadataLock(virSecurityManager *mgr G_GNUC_UNUSED, + char *const *sharedFilesystems, const char **paths, size_t npaths) { @@ -1377,7 +1394,7 @@ virSecurityManagerMetadataLock(virSecurityManager *mgr G_GNUC_UNUSED, } #endif /* !WIN32 */ - if (virFileIsSharedFS(p)) { + if (virFileIsSharedFS(p, sharedFilesystems)) { /* Probably a root squashed NFS. */ continue; } diff --git a/src/security/security_manager.h b/src/security/security_manager.h index bb6d22bc31..bf0059b2e0 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -81,7 +81,8 @@ virSecurityManager *virSecurityManagerNewDAC(const char *virtDriver, int virSecurityManagerPreFork(virSecurityManager *mgr); void virSecurityManagerPostFork(virSecurityManager *mgr); -int virSecurityManagerTransactionStart(virSecurityManager *mgr); +int virSecurityManagerTransactionStart(virSecurityManager *mgr, + char *const *sharedFilesystems); int virSecurityManagerTransactionCommit(virSecurityManager *mgr, pid_t pid, bool lock); @@ -129,11 +130,13 @@ int virSecurityManagerReleaseLabel(virSecurityManager *mgr, int virSecurityManagerCheckAllLabel(virSecurityManager *mgr, virDomainDef *sec); int virSecurityManagerSetAllLabel(virSecurityManager *mgr, + char *const *sharedFilesystems, virDomainDef *sec, const char *incomingPath, bool chardevStdioLogd, bool migrated); int virSecurityManagerRestoreAllLabel(virSecurityManager *mgr, + char *const *sharedFilesystems, virDomainDef *def, bool migrated, bool chardevStdioLogd); @@ -170,14 +173,17 @@ typedef enum { } virSecurityDomainImageLabelFlags; int virSecurityManagerSetImageLabel(virSecurityManager *mgr, + char *const *sharedFilesystems, virDomainDef *vm, virStorageSource *src, virSecurityDomainImageLabelFlags flags); int virSecurityManagerRestoreImageLabel(virSecurityManager *mgr, + char *const *sharedFilesystems, virDomainDef *vm, virStorageSource *src, virSecurityDomainImageLabelFlags flags); int virSecurityManagerMoveImageMetadata(virSecurityManager *mgr, + char *const *sharedFilesystems, pid_t pid, virStorageSource *src, virStorageSource *dst); @@ -246,6 +252,7 @@ struct _virSecurityManagerMetadataLockState { virSecurityManagerMetadataLockState * virSecurityManagerMetadataLock(virSecurityManager *mgr, + char *const *sharedFilesystems, const char **paths, size_t npaths); diff --git a/src/security/security_nop.c b/src/security/security_nop.c index 1413f43d57..e6e337a49d 100644 --- a/src/security/security_nop.c +++ b/src/security/security_nop.c @@ -116,6 +116,7 @@ virSecurityDomainReleaseLabelNop(virSecurityManager *mgr G_GNUC_UNUSED, static int virSecurityDomainSetAllLabelNop(virSecurityManager *mgr G_GNUC_UNUSED, + char *const *sharedFilesystems G_GNUC_UNUSED, virDomainDef *sec G_GNUC_UNUSED, const char *incomingPath G_GNUC_UNUSED, bool chardevStdioLogd G_GNUC_UNUSED, @@ -126,6 +127,7 @@ virSecurityDomainSetAllLabelNop(virSecurityManager *mgr G_GNUC_UNUSED, static int virSecurityDomainRestoreAllLabelNop(virSecurityManager *mgr G_GNUC_UNUSED, + char *const *sharedFilesystems G_GNUC_UNUSED, virDomainDef *vm G_GNUC_UNUSED, bool migrated G_GNUC_UNUSED, bool chardevStdioLogd G_GNUC_UNUSED) @@ -189,6 +191,7 @@ virSecurityGetBaseLabel(virSecurityManager *mgr G_GNUC_UNUSED, static int virSecurityDomainRestoreImageLabelNop(virSecurityManager *mgr G_GNUC_UNUSED, + char *const *sharedFilesystems G_GNUC_UNUSED, virDomainDef *def G_GNUC_UNUSED, virStorageSource *src G_GNUC_UNUSED, virSecurityDomainImageLabelFlags flags G_GNUC_UNUSED) @@ -198,6 +201,7 @@ virSecurityDomainRestoreImageLabelNop(virSecurityManager *mgr G_GNUC_UNUSED, static int virSecurityDomainSetImageLabelNop(virSecurityManager *mgr G_GNUC_UNUSED, + char *const *sharedFilesystems G_GNUC_UNUSED, virDomainDef *def G_GNUC_UNUSED, virStorageSource *src G_GNUC_UNUSED, virSecurityDomainImageLabelFlags flags G_GNUC_UNUSED) @@ -207,6 +211,7 @@ virSecurityDomainSetImageLabelNop(virSecurityManager *mgr G_GNUC_UNUSED, static int virSecurityDomainMoveImageMetadataNop(virSecurityManager *mgr G_GNUC_UNUSED, + char *const *sharedFilesystems G_GNUC_UNUSED, pid_t pid G_GNUC_UNUSED, virStorageSource *src G_GNUC_UNUSED, virStorageSource *dst G_GNUC_UNUSED) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 173f47316b..994f6cd3d0 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -77,6 +77,7 @@ struct _virSecuritySELinuxContextItem { typedef struct _virSecuritySELinuxContextList virSecuritySELinuxContextList; struct _virSecuritySELinuxContextList { virSecurityManager *manager; + char **sharedFilesystems; virSecuritySELinuxContextItem **items; size_t nItems; bool lock; @@ -141,6 +142,7 @@ virSecuritySELinuxContextListFree(void *opaque) g_free(list->items); virObjectUnref(list->manager); + g_strfreev(list->sharedFilesystems); g_free(list); } @@ -254,7 +256,9 @@ virSecuritySELinuxTransactionRun(pid_t pid G_GNUC_UNUSED, VIR_APPEND_ELEMENT_COPY_INPLACE(paths, npaths, p); } - if (!(state = virSecurityManagerMetadataLock(list->manager, paths, npaths))) + if (!(state = virSecurityManagerMetadataLock(list->manager, + list->sharedFilesystems, + paths, npaths))) goto cleanup; for (i = 0; i < list->nItems; i++) { @@ -1102,6 +1106,7 @@ virSecuritySELinuxGetDOI(virSecurityManager *mgr G_GNUC_UNUSED) /** * virSecuritySELinuxTransactionStart: * @mgr: security manager + * @sharedFilesystems: list of filesystem to consider shared * * Starts a new transaction. In transaction nothing is changed context * until TransactionCommit() is called. This is implemented as a list @@ -1114,7 +1119,8 @@ virSecuritySELinuxGetDOI(virSecurityManager *mgr G_GNUC_UNUSED) * -1 otherwise. */ static int -virSecuritySELinuxTransactionStart(virSecurityManager *mgr) +virSecuritySELinuxTransactionStart(virSecurityManager *mgr, + char *const *sharedFilesystems) { virSecuritySELinuxContextList *list; @@ -1128,6 +1134,7 @@ virSecuritySELinuxTransactionStart(virSecurityManager *mgr) list = g_new0(virSecuritySELinuxContextList, 1); list->manager = virObjectRef(mgr); + list->sharedFilesystems = g_strdupv((char **) sharedFilesystems); if (virThreadLocalSet(&contextList, list) < 0) { virReportSystemError(errno, "%s", @@ -1777,6 +1784,7 @@ virSecuritySELinuxRestoreTPMFileLabelInt(virSecurityManager *mgr, static int virSecuritySELinuxRestoreImageLabelInt(virSecurityManager *mgr, + char *const *sharedFilesystems, virDomainDef *def, virStorageSource *src, bool migrated) @@ -1835,7 +1843,7 @@ virSecuritySELinuxRestoreImageLabelInt(virSecurityManager *mgr, if (!src->path) return 0; - if ((rc = virFileIsSharedFS(src->path)) < 0) + if ((rc = virFileIsSharedFS(src->path, sharedFilesystems)) < 0) return -1; } @@ -1867,16 +1875,19 @@ virSecuritySELinuxRestoreImageLabelInt(virSecurityManager *mgr, static int virSecuritySELinuxRestoreImageLabel(virSecurityManager *mgr, + char *const *sharedFilesystems, virDomainDef *def, virStorageSource *src, virSecurityDomainImageLabelFlags flags G_GNUC_UNUSED) { - return virSecuritySELinuxRestoreImageLabelInt(mgr, def, src, false); + return virSecuritySELinuxRestoreImageLabelInt(mgr, sharedFilesystems, + def, src, false); } static int virSecuritySELinuxSetImageLabelInternal(virSecurityManager *mgr, + char *const *sharedFilesystems G_GNUC_UNUSED, virDomainDef *def, virStorageSource *src, virStorageSource *parent, @@ -1983,6 +1994,7 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManager *mgr, static int virSecuritySELinuxSetImageLabel(virSecurityManager *mgr, + char *const *sharedFilesystems, virDomainDef *def, virStorageSource *src, virSecurityDomainImageLabelFlags flags) @@ -1993,7 +2005,9 @@ virSecuritySELinuxSetImageLabel(virSecurityManager *mgr, for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) { const bool isChainTop = flags & VIR_SECURITY_DOMAIN_IMAGE_PARENT_CHAIN_TOP; - if (virSecuritySELinuxSetImageLabelInternal(mgr, def, n, parent, isChainTop) < 0) + if (virSecuritySELinuxSetImageLabelInternal(mgr, sharedFilesystems, + def, n, parent, + isChainTop) < 0) return -1; if (!(flags & VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN)) @@ -2008,6 +2022,7 @@ virSecuritySELinuxSetImageLabel(virSecurityManager *mgr, struct virSecuritySELinuxMoveImageMetadataData { virSecurityManager *mgr; + char **sharedFilesystems; const char *src; const char *dst; }; @@ -2022,7 +2037,9 @@ virSecuritySELinuxMoveImageMetadataHelper(pid_t pid G_GNUC_UNUSED, virSecurityManagerMetadataLockState *state; int ret; - if (!(state = virSecurityManagerMetadataLock(data->mgr, paths, G_N_ELEMENTS(paths)))) + if (!(state = virSecurityManagerMetadataLock(data->mgr, + data->sharedFilesystems, + paths, G_N_ELEMENTS(paths)))) return -1; ret = virSecurityMoveRememberedLabel(SECURITY_SELINUX_NAME, data->src, data->dst); @@ -2039,11 +2056,16 @@ virSecuritySELinuxMoveImageMetadataHelper(pid_t pid G_GNUC_UNUSED, static int virSecuritySELinuxMoveImageMetadata(virSecurityManager *mgr, + char *const *sharedFilesystems, pid_t pid, virStorageSource *src, virStorageSource *dst) { - struct virSecuritySELinuxMoveImageMetadataData data = { .mgr = mgr, 0 }; + struct virSecuritySELinuxMoveImageMetadataData data = { + .mgr = mgr, + .sharedFilesystems = (char **) sharedFilesystems, + 0 + }; int rc; if (src && virStorageSourceIsLocalStorage(src)) @@ -2820,6 +2842,7 @@ virSecuritySELinuxRestoreSysinfoLabel(virSecurityManager *mgr, static int virSecuritySELinuxRestoreAllLabel(virSecurityManager *mgr, + char *const *sharedFilesystems, virDomainDef *def, bool migrated, bool chardevStdioLogd) @@ -2844,7 +2867,8 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManager *mgr, for (i = 0; i < def->ndisks; i++) { virDomainDiskDef *disk = def->disks[i]; - if (virSecuritySELinuxRestoreImageLabelInt(mgr, def, disk->src, + if (virSecuritySELinuxRestoreImageLabelInt(mgr, sharedFilesystems, + def, disk->src, migrated) < 0) rc = -1; } @@ -2890,7 +2914,8 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManager *mgr, } if (def->os.loader && def->os.loader->nvram) { - if (virSecuritySELinuxRestoreImageLabelInt(mgr, def, def->os.loader->nvram, + if (virSecuritySELinuxRestoreImageLabelInt(mgr, sharedFilesystems, + def, def->os.loader->nvram, migrated) < 0) rc = -1; } @@ -3232,6 +3257,7 @@ virSecuritySELinuxSetSysinfoLabel(virSecurityManager *mgr, static int virSecuritySELinuxSetAllLabel(virSecurityManager *mgr, + char *const *sharedFilesystems, virDomainDef *def, const char *incomingPath G_GNUC_UNUSED, bool chardevStdioLogd, @@ -3259,7 +3285,8 @@ virSecuritySELinuxSetAllLabel(virSecurityManager *mgr, def->disks[i]->dst); continue; } - if (virSecuritySELinuxSetImageLabel(mgr, def, def->disks[i]->src, + if (virSecuritySELinuxSetImageLabel(mgr, sharedFilesystems, + def, def->disks[i]->src, VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN | VIR_SECURITY_DOMAIN_IMAGE_PARENT_CHAIN_TOP) < 0) return -1; @@ -3309,7 +3336,8 @@ virSecuritySELinuxSetAllLabel(virSecurityManager *mgr, } if (def->os.loader && def->os.loader->nvram) { - if (virSecuritySELinuxSetImageLabel(mgr, def, def->os.loader->nvram, + if (virSecuritySELinuxSetImageLabel(mgr, sharedFilesystems, + def, def->os.loader->nvram, VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN | VIR_SECURITY_DOMAIN_IMAGE_PARENT_CHAIN_TOP) < 0) return -1; diff --git a/src/security/security_stack.c b/src/security/security_stack.c index 369b5dd3a6..11800535b9 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -140,13 +140,15 @@ virSecurityStackPreFork(virSecurityManager *mgr) static int -virSecurityStackTransactionStart(virSecurityManager *mgr) +virSecurityStackTransactionStart(virSecurityManager *mgr, + char *const *sharedFilesystems) { virSecurityStackData *priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItem *item = priv->itemsHead; for (; item; item = item->next) { - if (virSecurityManagerTransactionStart(item->securityManager) < 0) + if (virSecurityManagerTransactionStart(item->securityManager, + sharedFilesystems) < 0) goto rollback; } @@ -337,6 +339,7 @@ virSecurityStackRestoreHostdevLabel(virSecurityManager *mgr, static int virSecurityStackSetAllLabel(virSecurityManager *mgr, + char *const *sharedFilesystems, virDomainDef *vm, const char *incomingPath, bool chardevStdioLogd, @@ -346,8 +349,9 @@ virSecurityStackSetAllLabel(virSecurityManager *mgr, virSecurityStackItem *item = priv->itemsHead; for (; item; item = item->next) { - if (virSecurityManagerSetAllLabel(item->securityManager, vm, - incomingPath, chardevStdioLogd, + if (virSecurityManagerSetAllLabel(item->securityManager, + sharedFilesystems, + vm, incomingPath, chardevStdioLogd, migrated) < 0) goto rollback; } @@ -357,6 +361,7 @@ virSecurityStackSetAllLabel(virSecurityManager *mgr, rollback: for (item = item->prev; item; item = item->prev) { if (virSecurityManagerRestoreAllLabel(item->securityManager, + sharedFilesystems, vm, migrated, chardevStdioLogd) < 0) { @@ -373,6 +378,7 @@ virSecurityStackSetAllLabel(virSecurityManager *mgr, static int virSecurityStackRestoreAllLabel(virSecurityManager *mgr, + char *const *sharedFilesystems, virDomainDef *vm, bool migrated, bool chardevStdioLogd) @@ -382,8 +388,11 @@ virSecurityStackRestoreAllLabel(virSecurityManager *mgr, int rc = 0; for (; item; item = item->next) { - if (virSecurityManagerRestoreAllLabel(item->securityManager, vm, - migrated, chardevStdioLogd) < 0) + if (virSecurityManagerRestoreAllLabel(item->securityManager, + sharedFilesystems, + vm, + migrated, + chardevStdioLogd) < 0) rc = -1; } @@ -638,6 +647,7 @@ virSecurityStackGetBaseLabel(virSecurityManager *mgr, int virtType) static int virSecurityStackSetImageLabel(virSecurityManager *mgr, + char *const *sharedFilesystems, virDomainDef *vm, virStorageSource *src, virSecurityDomainImageLabelFlags flags) @@ -646,8 +656,9 @@ virSecurityStackSetImageLabel(virSecurityManager *mgr, virSecurityStackItem *item = priv->itemsHead; for (; item; item = item->next) { - if (virSecurityManagerSetImageLabel(item->securityManager, vm, src, - flags) < 0) + if (virSecurityManagerSetImageLabel(item->securityManager, + sharedFilesystems, + vm, src, flags) < 0) goto rollback; } @@ -656,6 +667,7 @@ virSecurityStackSetImageLabel(virSecurityManager *mgr, rollback: for (item = item->prev; item; item = item->prev) { if (virSecurityManagerRestoreImageLabel(item->securityManager, + sharedFilesystems, vm, src, flags) < 0) { @@ -672,6 +684,7 @@ virSecurityStackSetImageLabel(virSecurityManager *mgr, static int virSecurityStackRestoreImageLabel(virSecurityManager *mgr, + char *const *sharedFilesystems, virDomainDef *vm, virStorageSource *src, virSecurityDomainImageLabelFlags flags) @@ -682,6 +695,7 @@ virSecurityStackRestoreImageLabel(virSecurityManager *mgr, for (; item; item = item->next) { if (virSecurityManagerRestoreImageLabel(item->securityManager, + sharedFilesystems, vm, src, flags) < 0) rc = -1; } @@ -691,6 +705,7 @@ virSecurityStackRestoreImageLabel(virSecurityManager *mgr, static int virSecurityStackMoveImageMetadata(virSecurityManager *mgr, + char *const *sharedFilesystems, pid_t pid, virStorageSource *src, virStorageSource *dst) @@ -701,6 +716,7 @@ virSecurityStackMoveImageMetadata(virSecurityManager *mgr, for (; item; item = item->next) { if (virSecurityManagerMoveImageMetadata(item->securityManager, + sharedFilesystems, pid, src, dst) < 0) rc = -1; } diff --git a/src/util/virfile.c b/src/util/virfile.c index d820172405..e02ad0ef65 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2604,8 +2604,14 @@ virFileOpenAs(const char *path, goto error; /* On Linux we can also verify the FS-type of the - * directory. (this is a NOP on other platforms). */ - if (virFileIsSharedFS(path) <= 0) + * directory. (this is a NOP on other platforms). + * + * Note that it would be pointless to pass + * virQEMUDriverConfig.sharedFilesystems here, since those + * listed there are by definition paths that can be accessed + * as local from the current host. Thus, a second attempt at + * opening the file would not make a difference */ + if (virFileIsSharedFS(path, NULL) <= 0) goto error; } @@ -3798,7 +3804,8 @@ virFileGetDefaultHugepage(virHugeTLBFS *fs, return NULL; } -int virFileIsSharedFS(const char *path) +int virFileIsSharedFS(const char *path, + char *const *overrides G_GNUC_UNUSED) { return virFileIsSharedFSType(path, VIR_FILE_SHFS_NFS | diff --git a/src/util/virfile.h b/src/util/virfile.h index 7df3fcb840..4f9d2bd5da 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -235,7 +235,8 @@ enum { }; int virFileIsSharedFSType(const char *path, unsigned int fstypes) ATTRIBUTE_NONNULL(1); -int virFileIsSharedFS(const char *path) ATTRIBUTE_NONNULL(1); +int virFileIsSharedFS(const char *path, + char *const *overrides) ATTRIBUTE_NONNULL(1); int virFileIsClusterFS(const char *path) ATTRIBUTE_NONNULL(1); int virFileIsMountPoint(const char *file) ATTRIBUTE_NONNULL(1); int virFileIsCDROM(const char *path) diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c index 7b7cf53569..43db128b3a 100644 --- a/tests/securityselinuxlabeltest.c +++ b/tests/securityselinuxlabeltest.c @@ -270,7 +270,7 @@ testSELinuxLabeling(const void *opaque) if (!(def = testSELinuxLoadDef(testname))) goto cleanup; - if (virSecurityManagerSetAllLabel(mgr, def, NULL, false, false) < 0) + if (virSecurityManagerSetAllLabel(mgr, NULL, def, NULL, false, false) < 0) goto cleanup; if (testSELinuxCheckLabels(files, nfiles) < 0) diff --git a/tests/virfiletest.c b/tests/virfiletest.c index 9fbfc37e56..e05925a321 100644 --- a/tests/virfiletest.c +++ b/tests/virfiletest.c @@ -313,7 +313,7 @@ testFileIsSharedFSType(const void *opaque G_GNUC_UNUSED) goto cleanup; } - actual = virFileIsSharedFS(data->filename); + actual = virFileIsSharedFS(data->filename, NULL); if (actual != data->expected) { fprintf(stderr, "Unexpected FS type. Expected %d got %d\n", -- 2.45.2
From: Andrea Bolognani <abologna@redhat.com> If the local admin has explicitly declared that a certain filesystem is to be considered shared, we should treat it as such. Signed-off-by: Andrea Bolognani <abologna@redhat.com> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> --- src/util/virfile.c | 42 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index e02ad0ef65..a8abd7d913 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -3804,9 +3804,49 @@ virFileGetDefaultHugepage(virHugeTLBFS *fs, return NULL; } +static bool +virFileIsSharedFSOverride(const char *path, + char *const *overrides) +{ + g_autofree char *dirpath = NULL; + char *p = NULL; + + if (!path || path[0] != '/' || !overrides) + return false; + + if (g_strv_contains((const char *const *) overrides, path)) + return true; + + dirpath = g_strdup(path); + + /* Continue until we've scanned the entire path */ + while (p != dirpath) { + + /* Find the last slash */ + if ((p = strrchr(dirpath, '/')) == NULL) + break; + + /* Truncate the path by overwriting the slash that we've just + * found with a null byte. If it is the very first slash in + * the path, we need to handle things slightly differently */ + if (p == dirpath) + *(p+1) = '\0'; + else + *p = '\0'; + + if (g_strv_contains((const char *const *) overrides, dirpath)) + return true; + } + + return false; +} + int virFileIsSharedFS(const char *path, - char *const *overrides G_GNUC_UNUSED) + char *const *overrides) { + if (virFileIsSharedFSOverride(path, overrides)) + return 1; + return virFileIsSharedFSType(path, VIR_FILE_SHFS_NFS | VIR_FILE_SHFS_GFS2 | -- 2.45.2
From: Andrea Bolognani <abologna@redhat.com> Up until this point, we have avoided setting labels for incoming migration when the TPM state is stored on a shared filesystem. This seems to make sense, because since the underlying storage is shared surely the labels will be as well. There's one problem, though: when a guest is migrated, the SELinux context for the destination process is different from the one of the source process. We haven't hit any issues with the current approach so far because NFS doesn't support SELinux, so effectively it doesn't matter whether relabeling happens or not: even if the SELinux contexts of the source and target processes are different, both will be able to access the storage. Now that it's possible for the local admin to manually mark exported directories as shared filesystems, however, things can get problematic. Consider the case in which one host (mig-one) exports its local filesystem /srv/nfs/libvirt/swtpm via NFS, and at the same time bind-mounts it to /var/lib/libvirt/swtpm; another host (mig-two) mounts the same filesystem to the same location, this time via NFS. Additionally, in order to allow migration in both directions, on mig-one the /var/lib/libvirt/swtpm directory is listed in the shared_filesystems qemu.conf option. When migrating from mig-one to mig-two, things work just fine; going in the opposite direction, however, results in an error: # virsh migrate cirros qemu+ssh://mig-one/system error: internal error: QEMU unexpectedly closed the monitor (vm='cirros'): qemu-system-x86_64: tpm-emulator: Setting the stateblob (type 1) failed with a TPM error 0x1f qemu-system-x86_64: error while loading state for instance 0x0 of device 'tpm-emulator' qemu-system-x86_64: load of migration failed: Input/output error This is because the directory on mig-one is considered a shared filesystem and thus labeling is skipped, resulting in a SELinux denial. The solution is quite simple: remove the check and always relabel. We know that it's okay to do so not just because it makes the error seen above go away, but also because no such check currently exists for disks and other types of persistent storage such as NVRAM files, which always get relabeled. Signed-off-by: Andrea Bolognani <abologna@redhat.com> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> --- src/qemu/qemu_tpm.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 08af5aad2e..55927b4582 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -933,7 +933,6 @@ qemuTPMEmulatorStart(virQEMUDriver *driver, g_autofree char *pidfile = NULL; virTimeBackOffVar timebackoff; const unsigned long long timeout = 1000; /* ms */ - bool setTPMStateLabel = true; pid_t pid = -1; cfg = virQEMUDriverGetConfig(driver); @@ -960,13 +959,7 @@ qemuTPMEmulatorStart(virQEMUDriver *driver, virCommandSetPidFile(cmd, pidfile); virCommandSetErrorFD(cmd, &errfd); - if (incomingMigration && - virFileIsSharedFS(tpm->data.emulator.storagepath, cfg->sharedFilesystems) == 1) { - /* security labels must have been set up on source already */ - setTPMStateLabel = false; - } - - if (qemuSecuritySetTPMLabels(driver, vm, setTPMStateLabel) < 0) + if (qemuSecuritySetTPMLabels(driver, vm, true) < 0) return -1; if (qemuSecurityCommandRun(driver, vm, cmd, cfg->swtpm_user, @@ -1015,7 +1008,7 @@ qemuTPMEmulatorStart(virQEMUDriver *driver, virProcessKillPainfully(pid, true); if (pidfile) unlink(pidfile); - qemuSecurityRestoreTPMLabels(driver, vm, setTPMStateLabel); + qemuSecurityRestoreTPMLabels(driver, vm, true); return -1; } -- 2.45.2
Document the function and export it for use outside of the 'virfile' utils module. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virfile.c | 12 +++++++++++- src/util/virfile.h | 2 ++ 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c35366c9e1..5f2f8f7031 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2353,6 +2353,7 @@ virFileIsLink; virFileIsMountPoint; virFileIsRegular; virFileIsSharedFS; +virFileIsSharedFSOverride; virFileIsSharedFSType; virFileLength; virFileLinkPointsTo; diff --git a/src/util/virfile.c b/src/util/virfile.c index a8abd7d913..6ac0f4efb3 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -3804,7 +3804,16 @@ virFileGetDefaultHugepage(virHugeTLBFS *fs, return NULL; } -static bool + +/** + * virFileIsSharedFSOverride: + * @path: Path to check + * @overrides: string list of path overrides + * + * Checks whether @path is inside any of the shared filesystem override + * directories passed as @overrides. + */ +bool virFileIsSharedFSOverride(const char *path, char *const *overrides) { @@ -3841,6 +3850,7 @@ virFileIsSharedFSOverride(const char *path, return false; } + int virFileIsSharedFS(const char *path, char *const *overrides) { diff --git a/src/util/virfile.h b/src/util/virfile.h index 4f9d2bd5da..e760724037 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -234,6 +234,8 @@ enum { VIR_FILE_SHFS_BEEGFS = (1 << 11), /* BeeGFS/fhGFS */ }; +bool virFileIsSharedFSOverride(const char *path, + char *const *overrides); int virFileIsSharedFSType(const char *path, unsigned int fstypes) ATTRIBUTE_NONNULL(1); int virFileIsSharedFS(const char *path, char *const *overrides) ATTRIBUTE_NONNULL(1); -- 2.45.2
In case of incoming migration where a local directory is shared to other hosts we'll need to avoid seclabel remembering as the code would remember the seclabel already allowing access to the image. As the decision requires a lot of information not available in the security driver it would either require plumbing in unpleasant callbacks able to pass in the data or alternatively we can mark this in the 'virStorageSource' struct. This patch chose to do the latter approach by adding a field called 'seclabelSkipRemeber' which will be filled before starting the process in cases when it will be required. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/storage_source_conf.c | 3 +++ src/conf/storage_source_conf.h | 9 +++++++++ src/security/security_dac.c | 3 +++ src/security/security_selinux.c | 3 +++ 4 files changed, 18 insertions(+) diff --git a/src/conf/storage_source_conf.c b/src/conf/storage_source_conf.c index 908bc5fab2..3cbed8188f 100644 --- a/src/conf/storage_source_conf.c +++ b/src/conf/storage_source_conf.c @@ -820,6 +820,9 @@ virStorageSourceCopy(const virStorageSource *src, /* storage driver metadata are not copied */ def->drv = NULL; + /* flag to avoid seclabel rember is not copied */ + def->seclabelSkipRemeber = false; + def->path = g_strdup(src->path); def->fdgroup = g_strdup(src->fdgroup); def->volume = g_strdup(src->volume); diff --git a/src/conf/storage_source_conf.h b/src/conf/storage_source_conf.h index 05b4bda16c..96dedc63b5 100644 --- a/src/conf/storage_source_conf.h +++ b/src/conf/storage_source_conf.h @@ -431,6 +431,15 @@ struct _virStorageSource { bool thresholdEventWithIndex; virStorageSourceFDTuple *fdtuple; + + /* Setting 'seclabelSkipRemeber' to true will cause the security driver to + * not remembe the security label even if it otherwise were to be + * remembered. This is needed in cases such as incoming migration for + * shared images where the existing security label may no longer be the + * correct. The security driver otherwise doesn't have enough information + * to do this decision. + */ + bool seclabelSkipRemeber; }; G_DEFINE_AUTOPTR_CLEANUP_FUNC(virStorageSource, virObjectUnref); diff --git a/src/security/security_dac.c b/src/security/security_dac.c index f2f784b4fa..f5eac5d3e1 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -940,6 +940,9 @@ virSecurityDACSetImageLabelInternal(virSecurityManager *mgr, */ remember = isChainTop && !src->readonly && !src->shared; + if (src->seclabelSkipRemeber) + remember = false; + return virSecurityDACSetOwnership(mgr, src, NULL, user, group, remember); } diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 994f6cd3d0..a981173230 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1985,6 +1985,9 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManager *mgr, ret = virSecuritySELinuxFSetFilecon(src->fdtuple->fds[0], use_label); } else { + if (src->seclabelSkipRemeber) + remember = false; + ret = virSecuritySELinuxSetFilecon(mgr, path, use_label, remember); } -- 2.45.2
In case when the user exports images from current host and there is an incoming migration from a remote host, security label remembering would be possible but would attempt to remember the label allowing access to the image as the image is already used by a VM on remote host. To prevent remembering the wrong label, we'll skip the remembering of the label for any shared resource, so that the code behaves identically regardless of how the image is accessed. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 49 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 2c462fd5f5..ec8b77bab9 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -533,6 +533,53 @@ qemuMigrationDstPrepareStorage(virDomainObj *vm, } +static void +qemuMigrationDstPrepareDiskSeclabels(virDomainObj *vm, + size_t nmigrate_disks, + const char **migrate_disks, + unsigned int flags) +{ + qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver); + size_t i; + + /* In case when storage is exported from this host, security label + * remembering would behave differently compared to the host which mounts + * the exported filesystem. Specifically for incoming migration remembering + * a seclabel would remember a seclabel already allowing access to the image, + * which is not desired. Thus we skip remembering of seclabels for images + * which are local to this host but accessed in a shared way from another + * host. + */ + if (!cfg->sharedFilesystems || + cfg->sharedFilesystems[0] == NULL) + return; + + for (i = 0; i < vm->def->ndisks; i++) { + virDomainDiskDef *disk = vm->def->disks[i]; + + /* We care only about existing local storage */ + if (virStorageSourceIsEmpty(disk->src) || + !virStorageSourceIsLocalStorage(disk->src)) + continue; + + /* Only paths which are on local filesystem but shared elsewhere are + * relevant */ + if (!virFileIsSharedFSOverride(disk->src->path, cfg->sharedFilesystems)) + continue; + + /* Any storage that was migrated via NBD is technically fully local so + * we want seclabels remembered */ + if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC)) { + if (qemuMigrationAnyCopyDisk(disk, nmigrate_disks, migrate_disks)) + continue; + } + + disk->src->seclabelSkipRemeber = true; + } +} + + /** * qemuMigrationDstStartNBDServer: * @driver: qemu driver @@ -3154,6 +3201,8 @@ qemuMigrationDstPrepareActive(virQEMUDriver *driver, dataFD[0]))) goto error; + qemuMigrationDstPrepareDiskSeclabels(vm, nmigrate_disks, migrate_disks, flags); + if (qemuProcessPrepareDomain(driver, vm, startFlags) < 0) goto error; -- 2.45.2
On Wed, Jul 24, 2024 at 05:34:32PM GMT, Peter Krempa wrote:
+static void +qemuMigrationDstPrepareDiskSeclabels(virDomainObj *vm, + size_t nmigrate_disks, + const char **migrate_disks, + unsigned int flags) +{ + qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver); + size_t i; + + /* In case when storage is exported from this host, security label + * remembering would behave differently compared to the host which mounts + * the exported filesystem. Specifically for incoming migration remembering + * a seclabel would remember a seclabel already allowing access to the image, + * which is not desired. Thus we skip remembering of seclabels for images + * which are local to this host but accessed in a shared way from another + * host. + */ + if (!cfg->sharedFilesystems || + cfg->sharedFilesystems[0] == NULL) + return; + + for (i = 0; i < vm->def->ndisks; i++) { + virDomainDiskDef *disk = vm->def->disks[i];
This only iterates over disks, so it's probably not surprising that it fails when other types of storage are involved. For example, if the domain uses UEFI, local -> remote migration works but when attempting to migrate back I get error: Requested operation is not valid: Setting different SELinux label on /var/lib/libvirt/qemu/nvram/cirros-efi_VARS.fd which is already in use If I add TPM into the mix, local -> remote migration fails with error: unable to lock /var/lib/libvirt/swtpm/50601047-47f7-4f45-9b41-9112dfaa3539/tpm2/.lock for metadata change: Resource temporarily unavailable Setting remember_owner=0 in qemu.conf makes all these errors go away, but of course that's a very big hammer and the idea here is to use a much smaller, more targeted one. I think it might be enough to extend this logic beyond disks so that it covers UEFI and TPM too, but I'm not entirely sure those use a virStorageSource behind the scenes. Hopefully that's the case and the changes needed on top are minimal, but I figure you're in a better position than me to look into it.
+ /* We care only about existing local storage */ + if (virStorageSourceIsEmpty(disk->src) || + !virStorageSourceIsLocalStorage(disk->src)) + continue; + + /* Only paths which are on local filesystem but shared elsewhere are + * relevant */ + if (!virFileIsSharedFSOverride(disk->src->path, cfg->sharedFilesystems)) + continue; + + /* Any storage that was migrated via NBD is technically fully local so + * we want seclabels remembered */ + if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC)) { + if (qemuMigrationAnyCopyDisk(disk, nmigrate_disks, migrate_disks)) + continue; + } + + disk->src->seclabelSkipRemeber = true;
You misspelled "remember" here.
@@ -3154,6 +3201,8 @@ qemuMigrationDstPrepareActive(virQEMUDriver *driver, dataFD[0]))) goto error;
+ qemuMigrationDstPrepareDiskSeclabels(vm, nmigrate_disks, migrate_disks, flags); + if (qemuProcessPrepareDomain(driver, vm, startFlags) < 0) goto error;
One thing that concerns me is that the changes appear to only be on the destination side of the migration. So only the remote -> local path is affected. But what about the local -> remote path? If we assume remember_owner=1, then obviously the various XATTRs related to remembering will be set on domain startup. What will take care of collecting that garbage once local -> remote migration has been performed? I think things might be fine based on the fact that the XATTRs seems to be dropped for disks even in the current state, but I thought I'd mention this specifically just in case. Thanks a lot for looking into this! -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Jul 26, 2024 at 07:47:57 -0700, Andrea Bolognani wrote:
On Wed, Jul 24, 2024 at 05:34:32PM GMT, Peter Krempa wrote:
+static void +qemuMigrationDstPrepareDiskSeclabels(virDomainObj *vm, + size_t nmigrate_disks, + const char **migrate_disks, + unsigned int flags) +{ + qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver); + size_t i; + + /* In case when storage is exported from this host, security label + * remembering would behave differently compared to the host which mounts + * the exported filesystem. Specifically for incoming migration remembering + * a seclabel would remember a seclabel already allowing access to the image, + * which is not desired. Thus we skip remembering of seclabels for images + * which are local to this host but accessed in a shared way from another + * host. + */ + if (!cfg->sharedFilesystems || + cfg->sharedFilesystems[0] == NULL) + return; + + for (i = 0; i < vm->def->ndisks; i++) { + virDomainDiskDef *disk = vm->def->disks[i];
This only iterates over disks, so it's probably not surprising that it fails when other types of storage are involved.
Hmm, yeah.
For example, if the domain uses UEFI, local -> remote migration works but when attempting to migrate back I get
error: Requested operation is not valid: Setting different SELinux label on /var/lib/libvirt/qemu/nvram/cirros-efi_VARS.fd which is already in use
So I've kind of forgot about this one. That's mainly as sharing this path is not actually needed for migration to work, as the contents are shared inside the migration stream automatically. Given though that users might want to share this one, so that they can start the VM withouth migration on a different host and also we allow setting the path to a more reasonable directory for sharing (users should not share /var/lib/libvirt/qemu/nvram/, or anything belonging to libvirt in the first place) we should treat this one the same as disk sources. Good thing that the nvram path is now a virStorage source. Now this also shows that we've forgotten to give it the same treatment that disk images get inside qemuProcessStop() (see below for explanation.
If I add TPM into the mix, local -> remote migration fails with
error: unable to lock /var/lib/libvirt/swtpm/50601047-47f7-4f45-9b41-9112dfaa3539/tpm2/.lock for metadata change: Resource temporarily unavailable
This makes it look like the file is still locked, that might be indication of a different problem as libvirt will not keep the lock after updating the XATTRs.
Setting remember_owner=0 in qemu.conf makes all these errors go away, but of course that's a very big hammer and the idea here is to use a much smaller, more targeted one.
Yup, as I've stated we must never suggest that users disable this feature.
I think it might be enough to extend this logic beyond disks so that it covers UEFI and TPM too, but I'm not entirely sure those use a virStorageSource behind the scenes. Hopefully that's the case and the
At least the R/W pflash device is handled by virStorageSource so that should be easy.
changes needed on top are minimal, but I figure you're in a better position than me to look into it.
+ /* We care only about existing local storage */ + if (virStorageSourceIsEmpty(disk->src) || + !virStorageSourceIsLocalStorage(disk->src)) + continue; + + /* Only paths which are on local filesystem but shared elsewhere are + * relevant */ + if (!virFileIsSharedFSOverride(disk->src->path, cfg->sharedFilesystems)) + continue; + + /* Any storage that was migrated via NBD is technically fully local so + * we want seclabels remembered */ + if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC)) { + if (qemuMigrationAnyCopyDisk(disk, nmigrate_disks, migrate_disks)) + continue; + } + + disk->src->seclabelSkipRemeber = true;
You misspelled "remember" here.
Eh ... and copied it all over place :D
@@ -3154,6 +3201,8 @@ qemuMigrationDstPrepareActive(virQEMUDriver *driver, dataFD[0]))) goto error;
+ qemuMigrationDstPrepareDiskSeclabels(vm, nmigrate_disks, migrate_disks, flags); + if (qemuProcessPrepareDomain(driver, vm, startFlags) < 0) goto error;
One thing that concerns me is that the changes appear to only be on the destination side of the migration. So only the remote -> local path is affected. But what about the local -> remote path?
On the source side, at least for disks we simply nuke the metadata(xattrs). In fact we do that in most cases when stopping the VM, not only for migration. See qemuProcessStop(), in the loop which calls qemuBlockRemoveImageMetadata(). That call removes the metadata from all disk top level sources. While the original intention might have been different (block jobs) it also ensures that this behaves correctly in this corner case. Now it seems that at least the varstore pflash virStorageSource requires the same treatment there.
If we assume remember_owner=1, then obviously the various XATTRs related to remembering will be set on domain startup. What will take care of collecting that garbage once local -> remote migration has been performed? I think things might be fine based on the fact that the XATTRs seems to be dropped for disks even in the current state, but I thought I'd mention this specifically just in case.
So apart from the code I've mentioned above which explicitly drops the xattrs, they are also not shared via NFS. Thus the remote side itself would not get them. If we'd leak them though it would mean that a re-start or incoming migration on the source of the migration would no longer work, so we need the same treatment for the other resources as well. I'll post another version adding the missing treatment for uefi, but I don't think I care enough about TPMs to go digging how that stuff is plumbed in or why it complains about locks being held which don't seem to be held by libvirt.
On Mon, Jul 29, 2024 at 03:09:55PM GMT, Peter Krempa wrote:
On Fri, Jul 26, 2024 at 07:47:57 -0700, Andrea Bolognani wrote:
For example, if the domain uses UEFI, local -> remote migration works but when attempting to migrate back I get
error: Requested operation is not valid: Setting different SELinux label on /var/lib/libvirt/qemu/nvram/cirros-efi_VARS.fd which is already in use
So I've kind of forgot about this one. That's mainly as sharing this path is not actually needed for migration to work, as the contents are shared inside the migration stream automatically.
Given though that users might want to share this one, so that they can start the VM withouth migration on a different host and also we allow setting the path to a more reasonable directory for sharing (users should not share /var/lib/libvirt/qemu/nvram/, or anything belonging to libvirt in the first place)
What's wrong with sharing this? It's just another type of storage after all. And it's true that in some cases you might be able to start the domain even with the NVRAM file being missing, but that's not true in the general case. Or are you saying that it's okay to share the directory where NVRAM files are stored, as long as it's not under /var/lib/libvirt?
If I add TPM into the mix, local -> remote migration fails with
error: unable to lock /var/lib/libvirt/swtpm/50601047-47f7-4f45-9b41-9112dfaa3539/tpm2/.lock for metadata change: Resource temporarily unavailable
This makes it look like the file is still locked, that might be indication of a different problem as libvirt will not keep the lock after updating the XATTRs.
I think there were some interesting timing issues with swtpm attempting to lock the file while libvirt was trying to perform its own metadata bookkeeping. I need to revisit that because honestly I've forgotten almost everything about it.
One thing that concerns me is that the changes appear to only be on the destination side of the migration. So only the remote -> local path is affected. But what about the local -> remote path?
On the source side, at least for disks we simply nuke the metadata(xattrs). In fact we do that in most cases when stopping the VM, not only for migration.
See qemuProcessStop(), in the loop which calls qemuBlockRemoveImageMetadata(). That call removes the metadata from all disk top level sources.
I see, thanks for the pointer.
If we assume remember_owner=1, then obviously the various XATTRs related to remembering will be set on domain startup. What will take care of collecting that garbage once local -> remote migration has been performed? I think things might be fine based on the fact that the XATTRs seems to be dropped for disks even in the current state, but I thought I'd mention this specifically just in case.
So apart from the code I've mentioned above which explicitly drops the xattrs, they are also not shared via NFS. Thus the remote side itself would not get them.
If we'd leak them though it would mean that a re-start or incoming migration on the source of the migration would no longer work, so we need the same treatment for the other resources as well.
Right, that's exactly my concern. I have encountered at least one case in which XATTRs not being cleaned up correctly on (failed?) migration resulted in having to perform manual fixup tasks before the domain would start again.
I'll post another version adding the missing treatment for uefi, but I don't think I care enough about TPMs to go digging how that stuff is plumbed in or why it complains about locks being held which don't seem to be held by libvirt.
I will try to look into it myself on top of your updated patches. I'm pretty sure TPM is a requirement for KubeVirt, so we definitely need it to be handled correctly or the only recourse from their side will be to set remember_owner=0 - and we've already established that we don't want that to happen :) -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Jul 29, 2024 at 08:29:42 -0700, Andrea Bolognani wrote:
On Mon, Jul 29, 2024 at 03:09:55PM GMT, Peter Krempa wrote:
On Fri, Jul 26, 2024 at 07:47:57 -0700, Andrea Bolognani wrote:
For example, if the domain uses UEFI, local -> remote migration works but when attempting to migrate back I get
error: Requested operation is not valid: Setting different SELinux label on /var/lib/libvirt/qemu/nvram/cirros-efi_VARS.fd which is already in use
So I've kind of forgot about this one. That's mainly as sharing this path is not actually needed for migration to work, as the contents are shared inside the migration stream automatically.
Given though that users might want to share this one, so that they can start the VM withouth migration on a different host and also we allow setting the path to a more reasonable directory for sharing (users should not share /var/lib/libvirt/qemu/nvram/, or anything belonging to libvirt in the first place)
What's wrong with sharing this? It's just another type of storage after all.
Well, in case of this specific directory it'd be okay. But we had users wanting to share the XML directory too. In general I'd suggest user not share anything that belongs to libvirt. In worst case they'd share /var/lib/libvirt, which also contains stuff that must not be shared, such as the master keys for qemu, dnsmasq data files etc. Similarly, sharing /var/lib/libvirt/qemu as whole is a bad idea.
And it's true that in some cases you might be able to start the domain even with the NVRAM file being missing, but that's not true in the general case.
Sure, this specific case will work. Arguably though you'd want to store the nvrams in a location where you also have the VM images shared from, so configuring specifically /var/li/libvirt/qemu/nvram to be shared extra seems a bit weird.
Or are you saying that it's okay to share the directory where NVRAM files are stored, as long as it's not under /var/lib/libvirt?
As said, the nvram directory itself is okay, but it's under a directory where other libvirt stuff is shared so users might misinterpret that. Either way, we need to be able to handle the NVRAM image labels properly regardless of where they are stored.
If I add TPM into the mix, local -> remote migration fails with
error: unable to lock /var/lib/libvirt/swtpm/50601047-47f7-4f45-9b41-9112dfaa3539/tpm2/.lock for metadata change: Resource temporarily unavailable
This makes it look like the file is still locked, that might be indication of a different problem as libvirt will not keep the lock after updating the XATTRs.
I think there were some interesting timing issues with swtpm attempting to lock the file while libvirt was trying to perform its own metadata bookkeeping. I need to revisit that because honestly I've forgotten almost everything about it.
IIRC libvirt uses a specific block to apply the lock on so that it's locking it only for internal purposes. If 'swtpm' is locking the same block it might cause problems.
One thing that concerns me is that the changes appear to only be on the destination side of the migration. So only the remote -> local path is affected. But what about the local -> remote path?
On the source side, at least for disks we simply nuke the metadata(xattrs). In fact we do that in most cases when stopping the VM, not only for migration.
See qemuProcessStop(), in the loop which calls qemuBlockRemoveImageMetadata(). That call removes the metadata from all disk top level sources.
I see, thanks for the pointer.
If we assume remember_owner=1, then obviously the various XATTRs related to remembering will be set on domain startup. What will take care of collecting that garbage once local -> remote migration has been performed? I think things might be fine based on the fact that the XATTRs seems to be dropped for disks even in the current state, but I thought I'd mention this specifically just in case.
So apart from the code I've mentioned above which explicitly drops the xattrs, they are also not shared via NFS. Thus the remote side itself would not get them.
If we'd leak them though it would mean that a re-start or incoming migration on the source of the migration would no longer work, so we need the same treatment for the other resources as well.
Right, that's exactly my concern. I have encountered at least one case in which XATTRs not being cleaned up correctly on (failed?) migration resulted in having to perform manual fixup tasks before the domain would start again.
It'd be great to know when that happened.
I'll post another version adding the missing treatment for uefi, but I don't think I care enough about TPMs to go digging how that stuff is plumbed in or why it complains about locks being held which don't seem to be held by libvirt.
I will try to look into it myself on top of your updated patches. I'm pretty sure TPM is a requirement for KubeVirt, so we definitely need it to be handled correctly or the only recourse from their side will be to set remember_owner=0 - and we've already established that we don't want that to happen :)
On Tue, Jul 30, 2024 at 12:21:56PM GMT, Peter Krempa wrote:
On Mon, Jul 29, 2024 at 08:29:42 -0700, Andrea Bolognani wrote:
On Mon, Jul 29, 2024 at 03:09:55PM GMT, Peter Krempa wrote:
Given though that users might want to share this one, so that they can start the VM withouth migration on a different host and also we allow setting the path to a more reasonable directory for sharing (users should not share /var/lib/libvirt/qemu/nvram/, or anything belonging to libvirt in the first place)
What's wrong with sharing this? It's just another type of storage after all.
Well, in case of this specific directory it'd be okay. But we had users wanting to share the XML directory too. In general I'd suggest user not share anything that belongs to libvirt.
In worst case they'd share /var/lib/libvirt, which also contains stuff that must not be shared, such as the master keys for qemu, dnsmasq data files etc.
Similarly, sharing /var/lib/libvirt/qemu as whole is a bad idea.
Right, I agree with you that indiscriminately sharing libvirt-private directories would be a terrible idea. I was talking about the NVRAM location and that one only.
I think there were some interesting timing issues with swtpm attempting to lock the file while libvirt was trying to perform its own metadata bookkeeping. I need to revisit that because honestly I've forgotten almost everything about it.
IIRC libvirt uses a specific block to apply the lock on so that it's locking it only for internal purposes. If 'swtpm' is locking the same block it might cause problems.
IIRC libvirt wanted to acquire the lock opportunistically and specifically for the scope of the relabel operation, while swtpm owns the storage and so it wants to have a long-term exclusive lock on it.
I have encountered at least one case in which XATTRs not being cleaned up correctly on (failed?) migration resulted in having to perform manual fixup tasks before the domain would start again.
It'd be great to know when that happened.
Once I start looking into this again, on top of your updated patches, I'll probably manage to hit the issue whether I like it or not. When that happens, I'll make sure to write down some details this time around. -- Andrea Bolognani / Red Hat / Virtualization
participants (2)
-
Andrea Bolognani -
Peter Krempa