In upcoming commits, virSecurityManagerSetAllLabel() will perform rollback in case of failure by calling virSecurityManagerRestoreAllLabel(). But in order to do that, the former needs to have @migrated argument so that it can be passed to the latter. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/lxc/lxc_process.c | 2 +- src/qemu/qemu_process.c | 3 ++- src/qemu/qemu_security.c | 6 ++++-- src/qemu/qemu_security.h | 3 ++- src/security/security_apparmor.c | 3 ++- src/security/security_dac.c | 3 ++- src/security/security_driver.h | 3 ++- src/security/security_manager.c | 6 ++++-- src/security/security_manager.h | 3 ++- src/security/security_nop.c | 3 ++- src/security/security_selinux.c | 3 ++- src/security/security_stack.c | 6 ++++-- tests/qemusecuritytest.c | 2 +- tests/securityselinuxlabeltest.c | 2 +- 14 files changed, 31 insertions(+), 17 deletions(-) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index cbdc7b1268..65775424cb 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1346,7 +1346,7 @@ int virLXCProcessStart(virConnectPtr conn, VIR_DEBUG("Setting domain security labels"); if (virSecurityManagerSetAllLabel(driver->securityManager, - vm->def, NULL, false) < 0) + vm->def, NULL, false, false) < 0) goto cleanup; VIR_DEBUG("Setting up consoles"); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 955ba4de4c..4348a6dd36 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6937,7 +6937,8 @@ qemuProcessLaunch(virConnectPtr conn, VIR_DEBUG("Setting domain security labels"); if (qemuSecuritySetAllLabel(driver, vm, - incoming ? incoming->path : NULL) < 0) + incoming ? incoming->path : NULL, + incoming != NULL) < 0) goto cleanup; /* Security manager labeled all devices, therefore diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index 91dd34f0e7..f4e815e966 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -32,7 +32,8 @@ VIR_LOG_INIT("qemu.qemu_process"); int qemuSecuritySetAllLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, - const char *stdin_path) + const char *stdin_path, + bool migrated) { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; @@ -47,7 +48,8 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver, if (virSecurityManagerSetAllLabel(driver->securityManager, vm->def, stdin_path, - priv->chardevStdioLogd) < 0) + priv->chardevStdioLogd, + migrated) < 0) goto cleanup; if (virSecurityManagerTransactionCommit(driver->securityManager, diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h index 224a4d61c9..29908141ba 100644 --- a/src/qemu/qemu_security.h +++ b/src/qemu/qemu_security.h @@ -26,7 +26,8 @@ int qemuSecuritySetAllLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, - const char *stdin_path); + const char *stdin_path, + bool migrated); void qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 77eee9410c..699590ee00 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -488,7 +488,8 @@ static int AppArmorSetSecurityAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, const char *stdin_path, - bool chardevStdioLogd ATTRIBUTE_UNUSED) + bool chardevStdioLogd ATTRIBUTE_UNUSED, + bool migrated ATTRIBUTE_UNUSED) { virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME); diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 4b4afef18a..9e71513f14 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1983,7 +1983,8 @@ static int virSecurityDACSetAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, const char *stdin_path ATTRIBUTE_UNUSED, - bool chardevStdioLogd) + bool chardevStdioLogd, + bool migrated ATTRIBUTE_UNUSED) { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityLabelDefPtr secdef; diff --git a/src/security/security_driver.h b/src/security/security_driver.h index b4ffed29ec..3353955813 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -83,7 +83,8 @@ typedef int (*virSecurityDomainReleaseLabel) (virSecurityManagerPtr mgr, typedef int (*virSecurityDomainSetAllLabel) (virSecurityManagerPtr mgr, virDomainDefPtr sec, const char *stdin_path, - bool chardevStdioLogd); + bool chardevStdioLogd, + bool migrated); typedef int (*virSecurityDomainRestoreAllLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, bool migrated, diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 7c905f0785..a04d2d848d 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -852,13 +852,15 @@ int virSecurityManagerSetAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, const char *stdin_path, - bool chardevStdioLogd) + bool chardevStdioLogd, + bool migrated) { if (mgr->drv->domainSetSecurityAllLabel) { int ret; virObjectLock(mgr); ret = mgr->drv->domainSetSecurityAllLabel(mgr, vm, stdin_path, - chardevStdioLogd); + chardevStdioLogd, + migrated); virObjectUnlock(mgr); return ret; } diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 0d2375b263..1d4928fae3 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -121,7 +121,8 @@ int virSecurityManagerCheckAllLabel(virSecurityManagerPtr mgr, int virSecurityManagerSetAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr sec, const char *stdin_path, - bool chardevStdioLogd); + bool chardevStdioLogd, + bool migrated); int virSecurityManagerRestoreAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, bool migrated, diff --git a/src/security/security_nop.c b/src/security/security_nop.c index 966b9d41a1..96cdac03d8 100644 --- a/src/security/security_nop.c +++ b/src/security/security_nop.c @@ -136,7 +136,8 @@ static int virSecurityDomainSetAllLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr sec ATTRIBUTE_UNUSED, const char *stdin_path ATTRIBUTE_UNUSED, - bool chardevStdioLogd ATTRIBUTE_UNUSED) + bool chardevStdioLogd ATTRIBUTE_UNUSED, + bool migrated ATTRIBUTE_UNUSED) { return 0; } diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index e879fa39ab..df0523abeb 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -3104,7 +3104,8 @@ static int virSecuritySELinuxSetAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, const char *stdin_path, - bool chardevStdioLogd) + bool chardevStdioLogd, + bool migrated ATTRIBUTE_UNUSED) { size_t i; virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr); diff --git a/src/security/security_stack.c b/src/security/security_stack.c index d445c0773e..dd055075cb 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -316,7 +316,8 @@ static int virSecurityStackSetAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, const char *stdin_path, - bool chardevStdioLogd) + bool chardevStdioLogd, + bool migrated) { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; @@ -324,7 +325,8 @@ virSecurityStackSetAllLabel(virSecurityManagerPtr mgr, for (; item; item = item->next) { if (virSecurityManagerSetAllLabel(item->securityManager, vm, - stdin_path, chardevStdioLogd) < 0) + stdin_path, chardevStdioLogd, + migrated) < 0) rc = -1; } diff --git a/tests/qemusecuritytest.c b/tests/qemusecuritytest.c index 2d88979168..9efc15c105 100644 --- a/tests/qemusecuritytest.c +++ b/tests/qemusecuritytest.c @@ -116,7 +116,7 @@ testDomain(const void *opaque) if (setenv(ENVVAR, "1", 0) < 0) return -1; - if (qemuSecuritySetAllLabel(data->driver, vm, NULL) < 0) + if (qemuSecuritySetAllLabel(data->driver, vm, NULL, false) < 0) goto cleanup; qemuSecurityRestoreAllLabel(data->driver, vm, false); diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c index 8c3cb29c41..6f9b5c0e70 100644 --- a/tests/securityselinuxlabeltest.c +++ b/tests/securityselinuxlabeltest.c @@ -310,7 +310,7 @@ testSELinuxLabeling(const void *opaque) if (!(def = testSELinuxLoadDef(testname))) goto cleanup; - if (virSecurityManagerSetAllLabel(mgr, def, NULL, false) < 0) + if (virSecurityManagerSetAllLabel(mgr, def, NULL, false, false) < 0) goto cleanup; if (testSELinuxCheckLabels(files, nfiles) < 0) -- 2.21.0

This function returns the name of the secdriver. Since the name is invariant we don't really need to lock the manager - it won't change. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/security_manager.c | 7 +++++++ src/security/security_manager.h | 1 + 2 files changed, 8 insertions(+) diff --git a/src/security/security_manager.c b/src/security/security_manager.c index c513740a13..99281821d9 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -332,6 +332,13 @@ virSecurityManagerGetVirtDriver(virSecurityManagerPtr mgr) } +const char * +virSecurityManagerGetDriver(virSecurityManagerPtr mgr) +{ + return mgr->drv->name; +} + + const char * virSecurityManagerGetDOI(virSecurityManagerPtr mgr) { diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 306de92978..f835356b7e 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -80,6 +80,7 @@ void virSecurityManagerTransactionAbort(virSecurityManagerPtr mgr); void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr); +const char *virSecurityManagerGetDriver(virSecurityManagerPtr mgr); const char *virSecurityManagerGetVirtDriver(virSecurityManagerPtr mgr); const char *virSecurityManagerGetDOI(virSecurityManagerPtr mgr); const char *virSecurityManagerGetModel(virSecurityManagerPtr mgr); -- 2.21.0

In order to have multiple security drivers hidden under one virSecurity* call, we have virSecurityStack driver which holds a list of registered security drivers and for every virSecurity* call it iterates over the list and calls corresponding callback in real security drivers. For instance, for virSecurityManagerSetAllLabel() it calls domainSetSecurityAllLabel callback sequentially in NOP, DAC and (possibly) SELinux or AppArmor drivers. This works just fine if the callback from every driver returns success. Problem arises when one of the drivers fails. For instance, aforementioned SetAllLabel() succeeds for DAC but fails in SELinux in which case all files that DAC relabelled are now owned by qemu:qemu (or whomever runs qemu) and thus permissions are leaked. This is even more visible with XATTRs which remain set for DAC. The solution is to perform a rollback on failure, i.e. call opposite action on drivers that succeeded. I'm providing rollback only for set calls and intentionally omitting restore calls for two reasons: 1) restore calls are less likely to fail (they merely remove XATTRs and chown()/setfilecon() file - all of these operations succeeded in set call), 2) we are not really interested in restore failures - in a very few places we check for retval of a restore function we do so only to print a warning. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/security_stack.c | 212 ++++++++++++++++++++++++++++------ 1 file changed, 176 insertions(+), 36 deletions(-) diff --git a/src/security/security_stack.c b/src/security/security_stack.c index 51a8b76748..f27b09265c 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -24,8 +24,10 @@ #include "virerror.h" #include "viralloc.h" +#include "virlog.h" #define VIR_FROM_THIS VIR_FROM_SECURITY +VIR_LOG_INIT("security.security_stack"); typedef struct _virSecurityStackData virSecurityStackData; typedef virSecurityStackData *virSecurityStackDataPtr; @@ -145,14 +147,18 @@ virSecurityStackTransactionStart(virSecurityManagerPtr mgr) { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; - int rc = 0; for (; item; item = item->next) { if (virSecurityManagerTransactionStart(item->securityManager) < 0) - rc = -1; + goto rollback; } - return rc; + return 0; + + rollback: + for (item = item->prev; item; item = item->prev) + virSecurityManagerTransactionAbort(item->securityManager); + return -1; } @@ -163,14 +169,18 @@ virSecurityStackTransactionCommit(virSecurityManagerPtr mgr, { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; - int rc = 0; for (; item; item = item->next) { if (virSecurityManagerTransactionCommit(item->securityManager, pid, lock) < 0) - rc = -1; + goto rollback; } - return rc; + return 0; + + rollback: + for (item = item->prev; item; item = item->prev) + virSecurityManagerTransactionAbort(item->securityManager); + return -1; } @@ -278,17 +288,31 @@ virSecurityStackSetHostdevLabel(virSecurityManagerPtr mgr, { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; - int rc = 0; for (; item; item = item->next) { if (virSecurityManagerSetHostdevLabel(item->securityManager, vm, dev, vroot) < 0) - rc = -1; + goto rollback; } - return rc; + return 0; + + rollback: + for (item = item->prev; item; item = item->prev) { + if (virSecurityManagerRestoreHostdevLabel(item->securityManager, + vm, + dev, + vroot) < 0) { + VIR_WARN("Unable to restore hostdev label after failed set label " + "call virDriver=%s driver=%s domain=%s hostdev=%p", + virSecurityManagerGetVirtDriver(mgr), + virSecurityManagerGetDriver(item->securityManager), + vm->name, dev); + } + } + return -1; } @@ -323,16 +347,30 @@ virSecurityStackSetAllLabel(virSecurityManagerPtr mgr, { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; - int rc = 0; for (; item; item = item->next) { if (virSecurityManagerSetAllLabel(item->securityManager, vm, stdin_path, chardevStdioLogd, migrated) < 0) - rc = -1; + goto rollback; } - return rc; + return 0; + + rollback: + for (item = item->prev; item; item = item->prev) { + if (virSecurityManagerRestoreAllLabel(item->securityManager, + vm, + migrated, + chardevStdioLogd) < 0) { + VIR_WARN("Unable to restore all labels after failed set label call " + "virDriver=%s driver=%s domain=%s migrated=%d", + virSecurityManagerGetVirtDriver(mgr), + virSecurityManagerGetDriver(item->securityManager), + vm->name, migrated); + } + } + return -1; } @@ -363,14 +401,27 @@ virSecurityStackSetSavedStateLabel(virSecurityManagerPtr mgr, { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; - int rc = 0; for (; item; item = item->next) { if (virSecurityManagerSetSavedStateLabel(item->securityManager, vm, savefile) < 0) - rc = -1; + goto rollback; } - return rc; + 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; } @@ -451,14 +502,25 @@ virSecurityStackSetDaemonSocketLabel(virSecurityManagerPtr mgr, { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; - int rc = 0; for (; item; item = item->next) { if (virSecurityManagerSetDaemonSocketLabel(item->securityManager, vm) < 0) - rc = -1; + goto rollback; } - return rc; + return 0; + rollback: + for (item = item->prev; item; item = item->prev) { + if (virSecurityManagerClearSocketLabel(item->securityManager, + vm) < 0) { + VIR_WARN("Unable to clear new daemon socket label after failed " + "set label call virDriver=%s driver=%s domain=%s", + virSecurityManagerGetVirtDriver(mgr), + virSecurityManagerGetDriver(item->securityManager), + vm->name); + } + } + return -1; } @@ -468,14 +530,25 @@ virSecurityStackSetSocketLabel(virSecurityManagerPtr mgr, { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; - int rc = 0; for (; item; item = item->next) { if (virSecurityManagerSetSocketLabel(item->securityManager, vm) < 0) - rc = -1; + goto rollback; } - return rc; + return 0; + rollback: + for (item = item->prev; item; item = item->prev) { + if (virSecurityManagerClearSocketLabel(item->securityManager, + vm) < 0) { + VIR_WARN("Unable to clear new socket label after failed " + "set label call virDriver=%s driver=%s domain=%s", + virSecurityManagerGetVirtDriver(mgr), + virSecurityManagerGetDriver(item->securityManager), + vm->name); + } + } + return -1; } @@ -573,15 +646,30 @@ virSecurityStackSetImageLabel(virSecurityManagerPtr mgr, { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; - int rc = 0; for (; item; item = item->next) { if (virSecurityManagerSetImageLabel(item->securityManager, vm, src, flags) < 0) - rc = -1; + goto rollback; } - return rc; + return 0; + + rollback: + for (item = item->prev; item; item = item->prev) { + if (virSecurityManagerRestoreImageLabel(item->securityManager, + vm, + src, + flags) < 0) { + VIR_WARN("Unable to restore image label after failed set label " + "call virDriver=%s driver=%s domain=%s src=%p (path=%s) " + "flags=0x%x", + virSecurityManagerGetVirtDriver(mgr), + virSecurityManagerGetDriver(item->securityManager), + vm->name, src, NULLSTR(src->path), flags); + } + } + return -1; } static int @@ -629,14 +717,27 @@ virSecurityStackSetMemoryLabel(virSecurityManagerPtr mgr, { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; - int rc = 0; for (; item; item = item->next) { if (virSecurityManagerSetMemoryLabel(item->securityManager, vm, mem) < 0) - rc = -1; + goto rollback; } - return rc; + return 0; + + rollback: + for (item = item->prev; item; item = item->prev) { + if (virSecurityManagerRestoreMemoryLabel(item->securityManager, + vm, + mem) < 0) { + VIR_WARN("Unable to restore memory label after failed set label " + "call virDriver=%s driver=%s domain=%s mem=%p", + virSecurityManagerGetVirtDriver(mgr), + virSecurityManagerGetDriver(item->securityManager), + vm->name, mem); + } + } + return -1; } static int @@ -664,14 +765,27 @@ virSecurityStackSetInputLabel(virSecurityManagerPtr mgr, { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; - int rc = 0; for (; item; item = item->next) { if (virSecurityManagerSetInputLabel(item->securityManager, vm, input) < 0) - rc = -1; + goto rollback; } - return rc; + return 0; + + rollback: + for (item = item->prev; item; item = item->prev) { + if (virSecurityManagerRestoreInputLabel(item->securityManager, + vm, + input) < 0) { + VIR_WARN("Unable to restore input label after failed set label " + "call virDriver=%s driver=%s domain=%s input=%p", + virSecurityManagerGetVirtDriver(mgr), + virSecurityManagerGetDriver(item->securityManager), + vm->name, input); + } + } + return -1; } static int @@ -719,16 +833,30 @@ virSecurityStackDomainSetChardevLabel(virSecurityManagerPtr mgr, { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; - int rc = 0; for (; item; item = item->next) { if (virSecurityManagerSetChardevLabel(item->securityManager, def, dev_source, chardevStdioLogd) < 0) - rc = -1; + goto rollback; } - return rc; + return 0; + + rollback: + for (item = item->prev; item; item = item->prev) { + if (virSecurityManagerRestoreChardevLabel(item->securityManager, + def, + dev_source, + chardevStdioLogd) < 0) { + VIR_WARN("Unable to restore chardev label after failed set label " + "call virDriver=%s driver=%s domain=%s dev_source=%p", + virSecurityManagerGetVirtDriver(mgr), + virSecurityManagerGetDriver(item->securityManager), + def->name, dev_source); + } + } + return -1; } static int @@ -758,15 +886,27 @@ virSecurityStackSetTPMLabels(virSecurityManagerPtr mgr, { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; - int rc = 0; for (; item; item = item->next) { if (virSecurityManagerSetTPMLabels(item->securityManager, vm) < 0) - rc = -1; + goto rollback; } - return rc; + return 0; + + rollback: + for (item = item->prev; item; item = item->prev) { + if (virSecurityManagerRestoreTPMLabels(item->securityManager, + vm) < 0) { + VIR_WARN("Unable to restore TPM label after failed set label " + "call virDriver=%s driver=%s domain=%s", + virSecurityManagerGetVirtDriver(mgr), + virSecurityManagerGetDriver(item->securityManager), + vm->name); + } + } + return -1; } -- 2.21.0