[PATCH 0/6] Restore seclabels on the correct path on 'virsh restore'

When restoring a domain from a block device a harmless error is reported. See 6/6 for explanation. But anyway, still worth fixing. Michal Prívozník (6): qemu: Use qemuSecurityDomainSetPathLabel() to set seclabes on not saved state files qemu: Drop unused qemuSecuritySetSavedStateLabel() security: Drop unused virSecurityManagerSetSavedStateLabel() security: Rename virSecurityManagerRestoreSavedStateLabel() qemu: Rename qemuSecurityRestoreSavedStateLabel() qemuSecurityDomainRestorePathLabel: Introduce @ignoreNS argument src/libvirt_private.syms | 3 +- src/qemu/qemu_driver.c | 6 +-- src/qemu/qemu_security.c | 43 ++++--------------- src/qemu/qemu_security.h | 9 ++-- src/security/security_apparmor.c | 18 ++------ src/security/security_dac.c | 46 +++++--------------- src/security/security_driver.h | 13 ++---- src/security/security_manager.c | 63 +++++++++++++--------------- src/security/security_manager.h | 11 +++-- src/security/security_nop.c | 19 --------- src/security/security_selinux.c | 49 +++++++--------------- src/security/security_stack.c | 72 +++++++++----------------------- 12 files changed, 101 insertions(+), 251 deletions(-) -- 2.26.2

There are two places within qemu driver that misuse qemuSecuritySetSavedStateLabel() to set seclabels on tempfiles that are not state files: qemuDomainScreenshot() and qemuDomainMemoryPeek(). They are doing so because of lack of qemuSecurityDomainSetPathLabel() at the time of their introduction. In all three secdrivers (well, four if you count NOP driver) the implementation of .domainSetSavedStateLabel and .domainSetPathLabel callbacks is the same anyway. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e482d08f3a..3fad440272 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4064,7 +4064,7 @@ qemuDomainScreenshot(virDomainPtr dom, } unlink_tmp = true; - qemuSecuritySetSavedStateLabel(driver, vm, tmp); + qemuSecurityDomainSetPathLabel(driver, vm, tmp, false); qemuDomainObjEnterMonitor(driver, vm); if (qemuMonitorScreendump(priv->mon, videoAlias, screen, tmp) < 0) { @@ -11666,7 +11666,7 @@ qemuDomainMemoryPeek(virDomainPtr dom, goto endjob; } - qemuSecuritySetSavedStateLabel(driver, vm, tmp); + qemuSecurityDomainSetPathLabel(driver, vm, tmp, false); priv = vm->privateData; qemuDomainObjEnterMonitor(driver, vm); -- 2.26.2

On Wed, Jun 17, 2020 at 01:35:35PM +0200, Michal Privoznik wrote:
There are two places within qemu driver that misuse qemuSecuritySetSavedStateLabel() to set seclabels on tempfiles that are not state files: qemuDomainScreenshot() and qemuDomainMemoryPeek(). They are doing so because of lack of qemuSecurityDomainSetPathLabel() at the time of their introduction.
In all three secdrivers (well, four if you count NOP driver) the implementation of .domainSetSavedStateLabel and .domainSetPathLabel callbacks is the same anyway.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>

After previous commit this function is used no more. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_security.c | 31 ------------------------------- src/qemu/qemu_security.h | 4 ---- 2 files changed, 35 deletions(-) diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index 61b9e4f0e3..d47f4cc3c0 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -614,37 +614,6 @@ qemuSecurityDomainSetPathLabel(virQEMUDriverPtr driver, } -int -qemuSecuritySetSavedStateLabel(virQEMUDriverPtr driver, - virDomainObjPtr vm, - const char *savefile) -{ - qemuDomainObjPrivatePtr priv = vm->privateData; - pid_t pid = -1; - int ret = -1; - - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) - pid = vm->pid; - - if (virSecurityManagerTransactionStart(driver->securityManager) < 0) - goto cleanup; - - if (virSecurityManagerSetSavedStateLabel(driver->securityManager, - vm->def, - savefile) < 0) - goto cleanup; - - if (virSecurityManagerTransactionCommit(driver->securityManager, - pid, priv->rememberOwner) < 0) - goto cleanup; - - ret = 0; - cleanup: - virSecurityManagerTransactionAbort(driver->securityManager); - return ret; -} - - int qemuSecurityRestoreSavedStateLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h index c8516005ac..4e701221cd 100644 --- a/src/qemu/qemu_security.h +++ b/src/qemu/qemu_security.h @@ -101,10 +101,6 @@ int qemuSecurityDomainSetPathLabel(virQEMUDriverPtr driver, const char *path, bool allowSubtree); -int qemuSecuritySetSavedStateLabel(virQEMUDriverPtr driver, - virDomainObjPtr vm, - const char *savefile); - int qemuSecurityRestoreSavedStateLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, const char *savefile); -- 2.26.2

On Wed, Jun 17, 2020 at 01:35:36PM +0200, Michal Privoznik wrote:
After previous commit this function is used no more.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>

After previous commit this function is used no more. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 - src/security/security_apparmor.c | 9 --------- src/security/security_dac.c | 20 -------------------- src/security/security_driver.h | 4 ---- src/security/security_manager.c | 17 ----------------- src/security/security_manager.h | 3 --- src/security/security_nop.c | 9 --------- src/security/security_selinux.c | 16 ---------------- src/security/security_stack.c | 32 -------------------------------- 9 files changed, 111 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fc7406f2b7..b93e05b43c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1569,7 +1569,6 @@ virSecurityManagerSetImageLabel; virSecurityManagerSetInputLabel; virSecurityManagerSetMemoryLabel; virSecurityManagerSetProcessLabel; -virSecurityManagerSetSavedStateLabel; virSecurityManagerSetSocketLabel; virSecurityManagerSetTapFDLabel; virSecurityManagerSetTPMLabels; diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 7c8fd39584..30f7701975 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -1048,14 +1048,6 @@ AppArmorRestoreChardevLabel(virSecurityManagerPtr mgr, return reload_profile(mgr, def, NULL, false); } -static int -AppArmorSetSavedStateLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def, - const char *savefile) -{ - return reload_profile(mgr, def, savefile, true); -} - static int AppArmorSetPathLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, @@ -1165,7 +1157,6 @@ virSecurityDriver virAppArmorSecurityDriver = { .domainSetSecurityHostdevLabel = AppArmorSetSecurityHostdevLabel, .domainRestoreSecurityHostdevLabel = AppArmorRestoreSecurityHostdevLabel, - .domainSetSavedStateLabel = AppArmorSetSavedStateLabel, .domainRestoreSavedStateLabel = AppArmorRestoreSavedStateLabel, .domainSetPathLabel = AppArmorSetPathLabel, diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 7e65b78fbe..2f531cb86b 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -2257,25 +2257,6 @@ virSecurityDACSetAllLabel(virSecurityManagerPtr mgr, } -static int -virSecurityDACSetSavedStateLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def, - const char *savefile) -{ - virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); - virSecurityLabelDefPtr secdef; - uid_t user; - gid_t group; - - secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); - - if (virSecurityDACGetImageIds(secdef, priv, &user, &group) < 0) - return -1; - - return virSecurityDACSetOwnership(mgr, NULL, savefile, user, group, true); -} - - static int virSecurityDACRestoreSavedStateLabel(virSecurityManagerPtr mgr, virDomainDefPtr def G_GNUC_UNUSED, @@ -2635,7 +2616,6 @@ virSecurityDriver virSecurityDriverDAC = { .domainSetSecurityHostdevLabel = virSecurityDACSetHostdevLabel, .domainRestoreSecurityHostdevLabel = virSecurityDACRestoreHostdevLabel, - .domainSetSavedStateLabel = virSecurityDACSetSavedStateLabel, .domainRestoreSavedStateLabel = virSecurityDACRestoreSavedStateLabel, .domainSetSecurityImageFDLabel = virSecurityDACSetImageFDLabel, diff --git a/src/security/security_driver.h b/src/security/security_driver.h index d23b64668d..33887f4c16 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -67,9 +67,6 @@ typedef int (*virSecurityDomainSetHostdevLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainHostdevDefPtr dev, const char *vroot); -typedef int (*virSecurityDomainSetSavedStateLabel) (virSecurityManagerPtr mgr, - virDomainDefPtr def, - const char *savefile); typedef int (*virSecurityDomainRestoreSavedStateLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, const char *savefile); @@ -203,7 +200,6 @@ struct _virSecurityDriver { virSecurityDomainSetHostdevLabel domainSetSecurityHostdevLabel; virSecurityDomainRestoreHostdevLabel domainRestoreSecurityHostdevLabel; - virSecurityDomainSetSavedStateLabel domainSetSavedStateLabel; virSecurityDomainRestoreSavedStateLabel domainRestoreSavedStateLabel; virSecurityDomainSetImageFDLabel domainSetSecurityImageFDLabel; diff --git a/src/security/security_manager.c b/src/security/security_manager.c index b1237d63b6..b2f3f1a6bb 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -596,23 +596,6 @@ virSecurityManagerSetHostdevLabel(virSecurityManagerPtr mgr, } -int -virSecurityManagerSetSavedStateLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm, - const char *savefile) -{ - if (mgr->drv->domainSetSavedStateLabel) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainSetSavedStateLabel(mgr, vm, savefile); - virObjectUnlock(mgr); - return ret; - } - - virReportUnsupportedError(); - return -1; -} - int virSecurityManagerRestoreSavedStateLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 2c5fa3ee15..ac50100f0f 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -104,9 +104,6 @@ int virSecurityManagerSetHostdevLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainHostdevDefPtr dev, const char *vroot); -int virSecurityManagerSetSavedStateLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def, - const char *savefile); int virSecurityManagerRestoreSavedStateLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, const char *savefile); diff --git a/src/security/security_nop.c b/src/security/security_nop.c index c1856eb421..d5720ee495 100644 --- a/src/security/security_nop.c +++ b/src/security/security_nop.c @@ -94,14 +94,6 @@ virSecurityDomainSetHostdevLabelNop(virSecurityManagerPtr mgr G_GNUC_UNUSED, return 0; } -static int -virSecurityDomainSetSavedStateLabelNop(virSecurityManagerPtr mgr G_GNUC_UNUSED, - virDomainDefPtr vm G_GNUC_UNUSED, - const char *savefile G_GNUC_UNUSED) -{ - return 0; -} - static int virSecurityDomainRestoreSavedStateLabelNop(virSecurityManagerPtr mgr G_GNUC_UNUSED, virDomainDefPtr vm G_GNUC_UNUSED, @@ -316,7 +308,6 @@ virSecurityDriver virSecurityDriverNop = { .domainSetSecurityHostdevLabel = virSecurityDomainSetHostdevLabelNop, .domainRestoreSecurityHostdevLabel = virSecurityDomainRestoreHostdevLabelNop, - .domainSetSavedStateLabel = virSecurityDomainSetSavedStateLabelNop, .domainRestoreSavedStateLabel = virSecurityDomainRestoreSavedStateLabelNop, .domainSetSecurityImageFDLabel = virSecurityDomainSetFDLabelNop, diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 7359a45a96..02b1100420 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2858,21 +2858,6 @@ virSecuritySELinuxReleaseLabel(virSecurityManagerPtr mgr, } -static int -virSecuritySELinuxSetSavedStateLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def, - const char *savefile) -{ - virSecurityLabelDefPtr secdef; - - secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (!secdef || !secdef->relabel) - return 0; - - return virSecuritySELinuxSetFilecon(mgr, savefile, secdef->imagelabel, true); -} - - static int virSecuritySELinuxRestoreSavedStateLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, @@ -3635,7 +3620,6 @@ virSecurityDriver virSecurityDriverSELinux = { .domainSetSecurityHostdevLabel = virSecuritySELinuxSetHostdevLabel, .domainRestoreSecurityHostdevLabel = virSecuritySELinuxRestoreHostdevLabel, - .domainSetSavedStateLabel = virSecuritySELinuxSetSavedStateLabel, .domainRestoreSavedStateLabel = virSecuritySELinuxRestoreSavedStateLabel, .domainSetSecurityImageFDLabel = virSecuritySELinuxSetImageFDLabel, diff --git a/src/security/security_stack.c b/src/security/security_stack.c index 165303a1f8..8e04b4fcfe 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -394,37 +394,6 @@ virSecurityStackRestoreAllLabel(virSecurityManagerPtr mgr, } -static int -virSecurityStackSetSavedStateLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm, - const char *savefile) -{ - virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); - virSecurityStackItemPtr item = priv->itemsHead; - - for (; item; item = item->next) { - if (virSecurityManagerSetSavedStateLabel(item->securityManager, vm, savefile) < 0) - goto rollback; - } - - return 0; - - rollback: - for (item = item->prev; item; item = item->prev) { - if (virSecurityManagerRestoreSavedStateLabel(item->securityManager, - vm, - savefile) < 0) { - VIR_WARN("Unable to restore saved state label after failed set " - "label call virDriver=%s driver=%s savefile=%s", - virSecurityManagerGetVirtDriver(mgr), - virSecurityManagerGetDriver(item->securityManager), - savefile); - } - } - return -1; -} - - static int virSecurityStackRestoreSavedStateLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, @@ -994,7 +963,6 @@ virSecurityDriver virSecurityDriverStack = { .domainSetSecurityHostdevLabel = virSecurityStackSetHostdevLabel, .domainRestoreSecurityHostdevLabel = virSecurityStackRestoreHostdevLabel, - .domainSetSavedStateLabel = virSecurityStackSetSavedStateLabel, .domainRestoreSavedStateLabel = virSecurityStackRestoreSavedStateLabel, .domainSetSecurityImageFDLabel = virSecurityStackSetImageFDLabel, -- 2.26.2

On Wed, Jun 17, 2020 at 01:35:37PM +0200, Michal Privoznik wrote:
After previous commit this function is used no more.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>

The new name is virSecurityManagerDomainRestorePathLabel(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 2 +- src/qemu/qemu_security.c | 2 +- src/security/security_apparmor.c | 9 +++---- src/security/security_dac.c | 26 +++++++----------- src/security/security_driver.h | 9 +++---- src/security/security_manager.c | 46 +++++++++++++++++++------------- src/security/security_manager.h | 8 +++--- src/security/security_nop.c | 10 ------- src/security/security_selinux.c | 33 +++++++++++------------ src/security/security_stack.c | 40 +++++++++++++-------------- 10 files changed, 89 insertions(+), 96 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b93e05b43c..30f8a7421e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1534,6 +1534,7 @@ virSecurityDriverLookup; # security/security_manager.h virSecurityManagerCheckAllLabel; virSecurityManagerClearSocketLabel; +virSecurityManagerDomainRestorePathLabel; virSecurityManagerDomainSetPathLabel; virSecurityManagerDomainSetPathLabelRO; virSecurityManagerGenLabel; @@ -1557,7 +1558,6 @@ virSecurityManagerRestoreHostdevLabel; virSecurityManagerRestoreImageLabel; virSecurityManagerRestoreInputLabel; virSecurityManagerRestoreMemoryLabel; -virSecurityManagerRestoreSavedStateLabel; virSecurityManagerRestoreTPMLabels; virSecurityManagerSetAllLabel; virSecurityManagerSetChardevLabel; diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index d47f4cc3c0..de4df23847 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -629,7 +629,7 @@ qemuSecurityRestoreSavedStateLabel(virQEMUDriverPtr driver, if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; - if (virSecurityManagerRestoreSavedStateLabel(driver->securityManager, + if (virSecurityManagerDomainRestorePathLabel(driver->securityManager, vm->def, savefile) < 0) goto cleanup; diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 30f7701975..583e872614 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -1069,9 +1069,9 @@ AppArmorSetPathLabel(virSecurityManagerPtr mgr, } static int -AppArmorRestoreSavedStateLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def, - const char *savefile G_GNUC_UNUSED) +AppArmorRestorePathLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + const char *path G_GNUC_UNUSED) { return reload_profile(mgr, def, NULL, false); } @@ -1157,9 +1157,8 @@ virSecurityDriver virAppArmorSecurityDriver = { .domainSetSecurityHostdevLabel = AppArmorSetSecurityHostdevLabel, .domainRestoreSecurityHostdevLabel = AppArmorRestoreSecurityHostdevLabel, - .domainRestoreSavedStateLabel = AppArmorRestoreSavedStateLabel, - .domainSetPathLabel = AppArmorSetPathLabel, + .domainRestorePathLabel = AppArmorRestorePathLabel, .domainSetSecurityChardevLabel = AppArmorSetChardevLabel, .domainRestoreSecurityChardevLabel = AppArmorRestoreChardevLabel, diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 2f531cb86b..afc0a9fcb9 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -2257,20 +2257,6 @@ virSecurityDACSetAllLabel(virSecurityManagerPtr mgr, } -static int -virSecurityDACRestoreSavedStateLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def G_GNUC_UNUSED, - const char *savefile) -{ - virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); - - if (!priv->dynamicOwnership) - return 0; - - return virSecurityDACRestoreFileLabel(mgr, savefile); -} - - static int virSecurityDACSetProcessLabel(virSecurityManagerPtr mgr, virDomainDefPtr def) @@ -2570,6 +2556,15 @@ virSecurityDACDomainSetPathLabel(virSecurityManagerPtr mgr, return virSecurityDACSetOwnership(mgr, NULL, path, user, group, true); } +static int +virSecurityDACDomainRestorePathLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def G_GNUC_UNUSED, + const char *path) +{ + return virSecurityDACRestoreFileLabel(mgr, path); +} + + virSecurityDriver virSecurityDriverDAC = { .privateDataLen = sizeof(virSecurityDACData), .name = SECURITY_DAC_NAME, @@ -2616,8 +2611,6 @@ virSecurityDriver virSecurityDriverDAC = { .domainSetSecurityHostdevLabel = virSecurityDACSetHostdevLabel, .domainRestoreSecurityHostdevLabel = virSecurityDACRestoreHostdevLabel, - .domainRestoreSavedStateLabel = virSecurityDACRestoreSavedStateLabel, - .domainSetSecurityImageFDLabel = virSecurityDACSetImageFDLabel, .domainSetSecurityTapFDLabel = virSecurityDACSetTapFDLabel, @@ -2626,6 +2619,7 @@ virSecurityDriver virSecurityDriverDAC = { .getBaseLabel = virSecurityDACGetBaseLabel, .domainSetPathLabel = virSecurityDACDomainSetPathLabel, + .domainRestorePathLabel = virSecurityDACDomainRestorePathLabel, .domainSetSecurityChardevLabel = virSecurityDACSetChardevLabel, .domainRestoreSecurityChardevLabel = virSecurityDACRestoreChardevLabel, diff --git a/src/security/security_driver.h b/src/security/security_driver.h index 33887f4c16..bfff789552 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -67,9 +67,6 @@ typedef int (*virSecurityDomainSetHostdevLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainHostdevDefPtr dev, const char *vroot); -typedef int (*virSecurityDomainRestoreSavedStateLabel) (virSecurityManagerPtr mgr, - virDomainDefPtr def, - const char *savefile); typedef int (*virSecurityDomainGenLabel) (virSecurityManagerPtr mgr, virDomainDefPtr sec); typedef int (*virSecurityDomainReserveLabel) (virSecurityManagerPtr mgr, @@ -140,6 +137,9 @@ typedef int (*virSecurityDomainSetPathLabel) (virSecurityManagerPtr mgr, typedef int (*virSecurityDomainSetPathLabelRO) (virSecurityManagerPtr mgr, virDomainDefPtr def, const char *path); +typedef int (*virSecurityDomainRestorePathLabel) (virSecurityManagerPtr mgr, + virDomainDefPtr def, + const char *path); typedef int (*virSecurityDomainSetChardevLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainChrSourceDefPtr dev_source, @@ -200,8 +200,6 @@ struct _virSecurityDriver { virSecurityDomainSetHostdevLabel domainSetSecurityHostdevLabel; virSecurityDomainRestoreHostdevLabel domainRestoreSecurityHostdevLabel; - virSecurityDomainRestoreSavedStateLabel domainRestoreSavedStateLabel; - virSecurityDomainSetImageFDLabel domainSetSecurityImageFDLabel; virSecurityDomainSetTapFDLabel domainSetSecurityTapFDLabel; @@ -211,6 +209,7 @@ struct _virSecurityDriver { virSecurityDomainSetPathLabel domainSetPathLabel; virSecurityDomainSetPathLabelRO domainSetPathLabelRO; + virSecurityDomainRestorePathLabel domainRestorePathLabel; virSecurityDomainSetChardevLabel domainSetSecurityChardevLabel; virSecurityDomainRestoreChardevLabel domainRestoreSecurityChardevLabel; diff --git a/src/security/security_manager.c b/src/security/security_manager.c index b2f3f1a6bb..ad1938caeb 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -596,24 +596,6 @@ virSecurityManagerSetHostdevLabel(virSecurityManagerPtr mgr, } -int -virSecurityManagerRestoreSavedStateLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm, - const char *savefile) -{ - if (mgr->drv->domainRestoreSavedStateLabel) { - int ret; - virObjectLock(mgr); - ret = mgr->drv->domainRestoreSavedStateLabel(mgr, vm, savefile); - virObjectUnlock(mgr); - return ret; - } - - virReportUnsupportedError(); - return -1; -} - - int virSecurityManagerGenLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm) @@ -1087,6 +1069,34 @@ virSecurityManagerDomainSetPathLabelRO(virSecurityManagerPtr mgr, return 0; } +/** + * virSecurityManagerDomainRestorePathLabel: + * @mgr: security manager object + * @vm: domain definition object + * @path: path to restore labels one + * + * This function is a counterpart to virSecurityManagerDomainSetPathLabel() and + * virSecurityManagerDomainSetPathLabelRO() as it restores any labels set by them. + * + * Returns: 0 on success, -1 on error. + */ +int +virSecurityManagerDomainRestorePathLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + const char *path) +{ + if (mgr->drv->domainRestorePathLabel) { + int ret; + virObjectLock(mgr); + ret = mgr->drv->domainRestorePathLabel(mgr, vm, path); + virObjectUnlock(mgr); + return ret; + } + + return 0; +} + + /** * virSecurityManagerSetMemoryLabel: diff --git a/src/security/security_manager.h b/src/security/security_manager.h index ac50100f0f..999752ce09 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -104,9 +104,6 @@ int virSecurityManagerSetHostdevLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainHostdevDefPtr dev, const char *vroot); -int virSecurityManagerRestoreSavedStateLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def, - const char *savefile); int virSecurityManagerGenLabel(virSecurityManagerPtr mgr, virDomainDefPtr sec); int virSecurityManagerReserveLabel(virSecurityManagerPtr mgr, @@ -190,6 +187,11 @@ int virSecurityManagerDomainSetPathLabelRO(virSecurityManagerPtr mgr, virDomainDefPtr vm, const char *path); +int virSecurityManagerDomainRestorePathLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + const char *path); + + int virSecurityManagerSetChardevLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainChrSourceDefPtr dev_source, diff --git a/src/security/security_nop.c b/src/security/security_nop.c index d5720ee495..de5da1ee1c 100644 --- a/src/security/security_nop.c +++ b/src/security/security_nop.c @@ -94,14 +94,6 @@ virSecurityDomainSetHostdevLabelNop(virSecurityManagerPtr mgr G_GNUC_UNUSED, return 0; } -static int -virSecurityDomainRestoreSavedStateLabelNop(virSecurityManagerPtr mgr G_GNUC_UNUSED, - virDomainDefPtr vm G_GNUC_UNUSED, - const char *savefile G_GNUC_UNUSED) -{ - return 0; -} - static int virSecurityDomainGenLabelNop(virSecurityManagerPtr mgr G_GNUC_UNUSED, virDomainDefPtr sec G_GNUC_UNUSED) @@ -308,8 +300,6 @@ virSecurityDriver virSecurityDriverNop = { .domainSetSecurityHostdevLabel = virSecurityDomainSetHostdevLabelNop, .domainRestoreSecurityHostdevLabel = virSecurityDomainRestoreHostdevLabelNop, - .domainRestoreSavedStateLabel = virSecurityDomainRestoreSavedStateLabelNop, - .domainSetSecurityImageFDLabel = virSecurityDomainSetFDLabelNop, .domainSetSecurityTapFDLabel = virSecurityDomainSetFDLabelNop, diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 02b1100420..4cc2707c3b 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2858,21 +2858,6 @@ virSecuritySELinuxReleaseLabel(virSecurityManagerPtr mgr, } -static int -virSecuritySELinuxRestoreSavedStateLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def, - const char *savefile) -{ - virSecurityLabelDefPtr secdef; - - secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (!secdef || !secdef->relabel) - return 0; - - return virSecuritySELinuxRestoreFileLabel(mgr, savefile, true); -} - - static int virSecuritySELinuxVerify(virSecurityManagerPtr mgr G_GNUC_UNUSED, virDomainDefPtr def) @@ -3428,6 +3413,21 @@ virSecuritySELinuxDomainSetPathLabelRO(virSecurityManagerPtr mgr, return virSecuritySELinuxSetFilecon(mgr, path, data->content_context, false); } +static int +virSecuritySELinuxDomainRestorePathLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + const char *path) +{ + virSecurityLabelDefPtr secdef; + + secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); + if (!secdef || !secdef->relabel) + return 0; + + return virSecuritySELinuxRestoreFileLabel(mgr, path, true); +} + + /* * virSecuritySELinuxSetFileLabels: * @@ -3620,8 +3620,6 @@ virSecurityDriver virSecurityDriverSELinux = { .domainSetSecurityHostdevLabel = virSecuritySELinuxSetHostdevLabel, .domainRestoreSecurityHostdevLabel = virSecuritySELinuxRestoreHostdevLabel, - .domainRestoreSavedStateLabel = virSecuritySELinuxRestoreSavedStateLabel, - .domainSetSecurityImageFDLabel = virSecuritySELinuxSetImageFDLabel, .domainSetSecurityTapFDLabel = virSecuritySELinuxSetTapFDLabel, @@ -3630,6 +3628,7 @@ virSecurityDriver virSecurityDriverSELinux = { .domainSetPathLabel = virSecuritySELinuxDomainSetPathLabel, .domainSetPathLabelRO = virSecuritySELinuxDomainSetPathLabelRO, + .domainRestorePathLabel = virSecuritySELinuxDomainRestorePathLabel, .domainSetSecurityChardevLabel = virSecuritySELinuxSetChardevLabel, .domainRestoreSecurityChardevLabel = virSecuritySELinuxRestoreChardevLabel, diff --git a/src/security/security_stack.c b/src/security/security_stack.c index 8e04b4fcfe..379c9302bc 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -394,24 +394,6 @@ virSecurityStackRestoreAllLabel(virSecurityManagerPtr mgr, } -static int -virSecurityStackRestoreSavedStateLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm, - const char *savefile) -{ - virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); - virSecurityStackItemPtr item = priv->itemsHead; - int rc = 0; - - for (; item; item = item->next) { - if (virSecurityManagerRestoreSavedStateLabel(item->securityManager, vm, savefile) < 0) - rc = -1; - } - - return rc; -} - - static int virSecurityStackSetProcessLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm) @@ -814,6 +796,25 @@ virSecurityStackDomainSetPathLabelRO(virSecurityManagerPtr mgr, } +static int +virSecurityStackDomainRestorePathLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + const char *path) +{ + virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityStackItemPtr item = priv->itemsHead; + int rc = 0; + + for (; item; item = item->next) { + if (virSecurityManagerDomainRestorePathLabel(item->securityManager, + vm, path) < 0) + rc = -1; + } + + return rc; +} + + static int virSecurityStackDomainSetChardevLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, @@ -963,8 +964,6 @@ virSecurityDriver virSecurityDriverStack = { .domainSetSecurityHostdevLabel = virSecurityStackSetHostdevLabel, .domainRestoreSecurityHostdevLabel = virSecurityStackRestoreHostdevLabel, - .domainRestoreSavedStateLabel = virSecurityStackRestoreSavedStateLabel, - .domainSetSecurityImageFDLabel = virSecurityStackSetImageFDLabel, .domainSetSecurityTapFDLabel = virSecurityStackSetTapFDLabel, @@ -974,6 +973,7 @@ virSecurityDriver virSecurityDriverStack = { .domainSetPathLabel = virSecurityStackDomainSetPathLabel, .domainSetPathLabelRO = virSecurityStackDomainSetPathLabelRO, + .domainRestorePathLabel = virSecurityStackDomainRestorePathLabel, .domainSetSecurityChardevLabel = virSecurityStackDomainSetChardevLabel, .domainRestoreSecurityChardevLabel = virSecurityStackDomainRestoreChardevLabel, -- 2.26.2

On Wed, Jun 17, 2020 at 01:35:38PM +0200, Michal Privoznik wrote:
The new name is virSecurityManagerDomainRestorePathLabel().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>

The function calls virSecurityManagerDomainRestorePathLabel() after all. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_security.c | 8 ++++---- src/qemu/qemu_security.h | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3fad440272..80648f1d32 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6953,7 +6953,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, asyncJob, VIR_QEMU_PROCESS_STOP_MIGRATED); } - if (qemuSecurityRestoreSavedStateLabel(driver, vm, path) < 0) + if (qemuSecurityDomainRestorePathLabel(driver, vm, path) < 0) VIR_WARN("failed to restore save state label on %s", path); return ret; } diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index de4df23847..98f973ab12 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -615,9 +615,9 @@ qemuSecurityDomainSetPathLabel(virQEMUDriverPtr driver, int -qemuSecurityRestoreSavedStateLabel(virQEMUDriverPtr driver, - virDomainObjPtr vm, - const char *savefile) +qemuSecurityDomainRestorePathLabel(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *path) { qemuDomainObjPrivatePtr priv = vm->privateData; pid_t pid = -1; @@ -631,7 +631,7 @@ qemuSecurityRestoreSavedStateLabel(virQEMUDriverPtr driver, if (virSecurityManagerDomainRestorePathLabel(driver->securityManager, vm->def, - savefile) < 0) + path) < 0) goto cleanup; if (virSecurityManagerTransactionCommit(driver->securityManager, diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h index 4e701221cd..ed6b762662 100644 --- a/src/qemu/qemu_security.h +++ b/src/qemu/qemu_security.h @@ -101,9 +101,9 @@ int qemuSecurityDomainSetPathLabel(virQEMUDriverPtr driver, const char *path, bool allowSubtree); -int qemuSecurityRestoreSavedStateLabel(virQEMUDriverPtr driver, +int qemuSecurityDomainRestorePathLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, - const char *savefile); + const char *path); int qemuSecurityCommandRun(virQEMUDriverPtr driver, virDomainObjPtr vm, -- 2.26.2

On Wed, Jun 17, 2020 at 01:35:39PM +0200, Michal Privoznik wrote:
The function calls virSecurityManagerDomainRestorePathLabel() after all.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>

In a few cases we might set seclabels on a path outside of namespaces. For instance, when restoring a domain from a file, the file is opened, relabelled and only then the namespace is created and the FD is passed to QEMU (see v6.3.0-rc1~108 for more info). Therefore, when restoring the label on the restore file, we must ignore domain namespaces and restore the label directly in the host. This bug demonstrates itself when restoring a domain from a block device. We don't create the block device inside the domain namespace and thus the following error is reported at the end of (otherwise successful) restore: error : virProcessRunInFork:1236 : internal error: child reported (status=125): unable to stat: /dev/sda: No such file or directory error : virProcessRunInFork:1240 : unable to stat: /dev/sda: No such file or directory Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_security.c | 6 ++++-- src/qemu/qemu_security.h | 3 ++- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 80648f1d32..4f62b5c838 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6953,7 +6953,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, asyncJob, VIR_QEMU_PROCESS_STOP_MIGRATED); } - if (qemuSecurityDomainRestorePathLabel(driver, vm, path) < 0) + if (qemuSecurityDomainRestorePathLabel(driver, vm, path, true) < 0) VIR_WARN("failed to restore save state label on %s", path); return ret; } diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index 98f973ab12..f49c0890f2 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -617,13 +617,15 @@ qemuSecurityDomainSetPathLabel(virQEMUDriverPtr driver, int qemuSecurityDomainRestorePathLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, - const char *path) + const char *path, + bool ignoreNS) { qemuDomainObjPrivatePtr priv = vm->privateData; pid_t pid = -1; int ret = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + if (!ignoreNS && + qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) pid = vm->pid; if (virSecurityManagerTransactionStart(driver->securityManager) < 0) diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h index ed6b762662..df34820af8 100644 --- a/src/qemu/qemu_security.h +++ b/src/qemu/qemu_security.h @@ -103,7 +103,8 @@ int qemuSecurityDomainSetPathLabel(virQEMUDriverPtr driver, int qemuSecurityDomainRestorePathLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, - const char *path); + const char *path, + bool ignoreNS); int qemuSecurityCommandRun(virQEMUDriverPtr driver, virDomainObjPtr vm, -- 2.26.2

On Wed, Jun 17, 2020 at 01:35:40PM +0200, Michal Privoznik wrote:
In a few cases we might set seclabels on a path outside of namespaces. For instance, when restoring a domain from a file, the file is opened, relabelled and only then the namespace is created and the FD is passed to QEMU (see v6.3.0-rc1~108 for more info). Therefore, when restoring the label on the restore file, we must ignore domain namespaces and restore the label directly in the host.
This bug demonstrates itself when restoring a domain from a block device. We don't create the block device inside the domain namespace and thus the following error is reported at the end of (otherwise successful) restore:
error : virProcessRunInFork:1236 : internal error: child reported (status=125): unable to stat: /dev/sda: No such file or directory error : virProcessRunInFork:1240 : unable to stat: /dev/sda: No such file or directory
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Erik Skultety <eskultet@redhat.com>
participants (2)
-
Erik Skultety
-
Michal Privoznik