From a2f0b97ab797460c2661e72000556442a038a474 Mon Sep 17 00:00:00 2001 From: Michal Privoznik Date: Tue, 13 Nov 2018 10:57:25 +0100 Subject: [PATCH] virSecurityManagerTransactionCommit: Do metadata locking iff enabled in config When metadata locking is enabled that means the security commit processing will be run in a fork similar to how namespaces use fork()'s for processing. This is done to ensure libvirt can properly and synchronously modify the metadata to store the original owner data. Since fork()'s (e.g. virFork) have been seen as a performance bottleneck being able to disable them allows the admin to choose whether the performance 'hit' is worth the extra 'security' of being able to remember the original owner of a lock. Signed-off-by: Michal Privoznik Reviewed-by: John Ferlan --- src/qemu/qemu_security.c | 73 ++++++++++++++++++++++++--------- src/security/security_dac.c | 56 ++++++++++++++++--------- src/security/security_driver.h | 3 +- src/security/security_manager.c | 9 +++- src/security/security_manager.h | 3 +- src/security/security_selinux.c | 54 ++++++++++++++++-------- src/security/security_stack.c | 5 ++- 7 files changed, 140 insertions(+), 63 deletions(-) diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index bf45abf93a..372bc53396 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -53,7 +53,8 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver, priv->chardevStdioLogd) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; @@ -86,7 +87,8 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver, priv->chardevStdioLogd); if (transactionStarted && - virSecurityManagerTransactionCommit(driver->securityManager, -1) < 0) + virSecurityManagerTransactionCommit(driver->securityManager, + -1, priv->rememberOwner) < 0) VIR_WARN("Unable to run security manager transaction"); virSecurityManagerTransactionAbort(driver->securityManager); @@ -98,6 +100,7 @@ qemuSecuritySetDiskLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk) { + qemuDomainObjPrivatePtr priv = vm->privateData; pid_t pid = -1; int ret = -1; @@ -112,7 +115,8 @@ qemuSecuritySetDiskLabel(virQEMUDriverPtr driver, disk) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; @@ -127,6 +131,7 @@ qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk) { + qemuDomainObjPrivatePtr priv = vm->privateData; pid_t pid = -1; int ret = -1; @@ -141,7 +146,8 @@ qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver, disk) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; @@ -156,6 +162,7 @@ qemuSecuritySetImageLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virStorageSourcePtr src) { + qemuDomainObjPrivatePtr priv = vm->privateData; pid_t pid = -1; int ret = -1; @@ -170,7 +177,8 @@ qemuSecuritySetImageLabel(virQEMUDriverPtr driver, src) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; @@ -185,6 +193,7 @@ qemuSecurityRestoreImageLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virStorageSourcePtr src) { + qemuDomainObjPrivatePtr priv = vm->privateData; pid_t pid = -1; int ret = -1; @@ -199,7 +208,8 @@ qemuSecurityRestoreImageLabel(virQEMUDriverPtr driver, src) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; @@ -214,6 +224,7 @@ qemuSecuritySetHostdevLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) { + qemuDomainObjPrivatePtr priv = vm->privateData; pid_t pid = -1; int ret = -1; @@ -229,7 +240,8 @@ qemuSecuritySetHostdevLabel(virQEMUDriverPtr driver, NULL) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; @@ -244,6 +256,7 @@ qemuSecurityRestoreHostdevLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) { + qemuDomainObjPrivatePtr priv = vm->privateData; pid_t pid = -1; int ret = -1; @@ -259,7 +272,8 @@ qemuSecurityRestoreHostdevLabel(virQEMUDriverPtr driver, NULL) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; @@ -274,6 +288,7 @@ qemuSecuritySetMemoryLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainMemoryDefPtr mem) { + qemuDomainObjPrivatePtr priv = vm->privateData; pid_t pid = -1; int ret = -1; @@ -288,7 +303,8 @@ qemuSecuritySetMemoryLabel(virQEMUDriverPtr driver, mem) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; @@ -303,6 +319,7 @@ qemuSecurityRestoreMemoryLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainMemoryDefPtr mem) { + qemuDomainObjPrivatePtr priv = vm->privateData; pid_t pid = -1; int ret = -1; @@ -317,7 +334,8 @@ qemuSecurityRestoreMemoryLabel(virQEMUDriverPtr driver, mem) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; @@ -347,7 +365,8 @@ qemuSecuritySetInputLabel(virDomainObjPtr vm, input) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; @@ -377,7 +396,8 @@ qemuSecurityRestoreInputLabel(virDomainObjPtr vm, input) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; @@ -408,7 +428,8 @@ qemuSecuritySetChardevLabel(virQEMUDriverPtr driver, priv->chardevStdioLogd) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; @@ -439,7 +460,8 @@ qemuSecurityRestoreChardevLabel(virQEMUDriverPtr driver, priv->chardevStdioLogd) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; @@ -476,6 +498,7 @@ qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver, int *exitstatus, int *cmdret) { + qemuDomainObjPrivatePtr priv = vm->privateData; int ret = -1; bool transactionStarted = false; @@ -489,7 +512,8 @@ qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver, return -1; } - if (virSecurityManagerTransactionCommit(driver->securityManager, -1) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + -1, priv->rememberOwner) < 0) goto cleanup; transactionStarted = false; @@ -522,7 +546,8 @@ qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver, virSecurityManagerRestoreTPMLabels(driver->securityManager, vm->def); if (transactionStarted && - virSecurityManagerTransactionCommit(driver->securityManager, -1) < 0) + virSecurityManagerTransactionCommit(driver->securityManager, + -1, priv->rememberOwner) < 0) VIR_WARN("Unable to run security manager transaction"); virSecurityManagerTransactionAbort(driver->securityManager); @@ -534,6 +559,7 @@ void qemuSecurityCleanupTPMEmulator(virQEMUDriverPtr driver, virDomainObjPtr vm) { + qemuDomainObjPrivatePtr priv = vm->privateData; bool transactionStarted = false; if (virSecurityManagerTransactionStart(driver->securityManager) >= 0) @@ -542,7 +568,8 @@ qemuSecurityCleanupTPMEmulator(virQEMUDriverPtr driver, virSecurityManagerRestoreTPMLabels(driver->securityManager, vm->def); if (transactionStarted && - virSecurityManagerTransactionCommit(driver->securityManager, -1) < 0) + virSecurityManagerTransactionCommit(driver->securityManager, + -1, priv->rememberOwner) < 0) VIR_WARN("Unable to run security manager transaction"); virSecurityManagerTransactionAbort(driver->securityManager); @@ -555,6 +582,7 @@ qemuSecurityDomainSetPathLabel(virQEMUDriverPtr driver, const char *path, bool allowSubtree) { + qemuDomainObjPrivatePtr priv = vm->privateData; pid_t pid = -1; int ret = -1; @@ -570,7 +598,8 @@ qemuSecurityDomainSetPathLabel(virQEMUDriverPtr driver, allowSubtree) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; @@ -585,6 +614,7 @@ qemuSecuritySetSavedStateLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, const char *savefile) { + qemuDomainObjPrivatePtr priv = vm->privateData; pid_t pid = -1; int ret = -1; @@ -599,7 +629,8 @@ qemuSecuritySetSavedStateLabel(virQEMUDriverPtr driver, savefile) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; @@ -614,6 +645,7 @@ qemuSecurityRestoreSavedStateLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, const char *savefile) { + qemuDomainObjPrivatePtr priv = vm->privateData; pid_t pid = -1; int ret = -1; @@ -628,7 +660,8 @@ qemuSecurityRestoreSavedStateLabel(virQEMUDriverPtr driver, savefile) < 0) goto cleanup; - if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, + pid, priv->rememberOwner) < 0) goto cleanup; ret = 0; diff --git a/src/security/security_dac.c b/src/security/security_dac.c index da4a6c72fe..0e100f7895 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -86,6 +86,7 @@ struct _virSecurityDACChownList { virSecurityManagerPtr manager; virSecurityDACChownItemPtr *items; size_t nItems; + bool lock; }; @@ -210,21 +211,23 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED, int rv = 0; int ret = -1; - if (VIR_ALLOC_N(paths, list->nItems) < 0) - return -1; + if (list->lock) { + if (VIR_ALLOC_N(paths, list->nItems) < 0) + return -1; - for (i = 0; i < list->nItems; i++) { - const char *p = list->items[i]->path; + for (i = 0; i < list->nItems; i++) { + const char *p = list->items[i]->path; - if (!p || - virFileIsDir(p)) - continue; + if (!p || + virFileIsDir(p)) + continue; - VIR_APPEND_ELEMENT_COPY_INPLACE(paths, npaths, p); - } + VIR_APPEND_ELEMENT_COPY_INPLACE(paths, npaths, p); + } - if (virSecurityManagerMetadataLock(list->manager, paths, npaths) < 0) - goto cleanup; + if (virSecurityManagerMetadataLock(list->manager, paths, npaths) < 0) + goto cleanup; + } for (i = 0; i < list->nItems; i++) { virSecurityDACChownItemPtr item = list->items[i]; @@ -246,7 +249,8 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED, break; } - if (virSecurityManagerMetadataUnlock(list->manager, paths, npaths) < 0) + if (list->lock && + virSecurityManagerMetadataUnlock(list->manager, paths, npaths) < 0) goto cleanup; if (rv < 0) @@ -529,11 +533,15 @@ virSecurityDACTransactionStart(virSecurityManagerPtr mgr) * virSecurityDACTransactionCommit: * @mgr: security manager * @pid: domain's PID + * @lock: lock and unlock paths that are relabeled * * If @pid is not -1 then enter the @pid namespace (usually @pid refers * to a domain) and perform all the chown()-s on the list. If @pid is -1 * then the transaction is performed in the namespace of the caller. * + * If @lock is true then all the paths that transaction would + * touch are locked before and unlocked after it is done so. + * * Note that the transaction is also freed, therefore new one has to be * started after successful return from this function. Also it is * considered as error if there's no transaction set and this function @@ -544,9 +552,11 @@ virSecurityDACTransactionStart(virSecurityManagerPtr mgr) */ static int virSecurityDACTransactionCommit(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - pid_t pid) + pid_t pid, + bool lock) { virSecurityDACChownListPtr list; + int rc; int ret = -1; list = virThreadLocalGet(&chownList); @@ -562,12 +572,20 @@ virSecurityDACTransactionCommit(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, goto cleanup; } - if ((pid == -1 && - virSecurityDACTransactionRun(pid, list) < 0) || - (pid != -1 && - virProcessRunInMountNamespace(pid, - virSecurityDACTransactionRun, - list) < 0)) + list->lock = lock; + + if (pid == -1) { + if (lock) + rc = virProcessRunInFork(virSecurityDACTransactionRun, list); + else + rc = virSecurityDACTransactionRun(pid, list); + } else { + rc = virProcessRunInMountNamespace(pid, + virSecurityDACTransactionRun, + list); + } + + if (rc < 0) goto cleanup; ret = 0; diff --git a/src/security/security_driver.h b/src/security/security_driver.h index cbf0ecff6e..cd221f1c78 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -53,7 +53,8 @@ typedef int (*virSecurityDriverPreFork) (virSecurityManagerPtr mgr); typedef int (*virSecurityDriverTransactionStart) (virSecurityManagerPtr mgr); typedef int (*virSecurityDriverTransactionCommit) (virSecurityManagerPtr mgr, - pid_t pid); + pid_t pid, + bool lock); typedef void (*virSecurityDriverTransactionAbort) (virSecurityManagerPtr mgr); typedef int (*virSecurityDomainRestoreDiskLabel) (virSecurityManagerPtr mgr, diff --git a/src/security/security_manager.c b/src/security/security_manager.c index c6c80e6165..712b785ae9 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -299,12 +299,16 @@ virSecurityManagerTransactionStart(virSecurityManagerPtr mgr) * virSecurityManagerTransactionCommit: * @mgr: security manager * @pid: domain's PID + * @lock: lock and unlock paths that are relabeled * * If @pid is not -1 then enter the @pid namespace (usually @pid refers * to a domain) and perform all the operations on the transaction list. * If @pid is -1 then the transaction is performed in the namespace of * the caller. * + * If @lock is true then all the paths that transaction would + * touch are locked before and unlocked after it is done so. + * * Note that the transaction is also freed, therefore new one has to be * started after successful return from this function. Also it is * considered as error if there's no transaction set and this function @@ -315,13 +319,14 @@ virSecurityManagerTransactionStart(virSecurityManagerPtr mgr) */ int virSecurityManagerTransactionCommit(virSecurityManagerPtr mgr, - pid_t pid) + pid_t pid, + bool lock) { int ret = 0; virObjectLock(mgr); if (mgr->drv->transactionCommit) - ret = mgr->drv->transactionCommit(mgr, pid); + ret = mgr->drv->transactionCommit(mgr, pid, lock); virObjectUnlock(mgr); return ret; } diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 10ebe5cc29..04bb54f61e 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -79,7 +79,8 @@ void virSecurityManagerPostFork(virSecurityManagerPtr mgr); int virSecurityManagerTransactionStart(virSecurityManagerPtr mgr); int virSecurityManagerTransactionCommit(virSecurityManagerPtr mgr, - pid_t pid); + pid_t pid, + bool lock); void virSecurityManagerTransactionAbort(virSecurityManagerPtr mgr); void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr); diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 780d650c69..5e72a3589a 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -93,6 +93,7 @@ struct _virSecuritySELinuxContextList { virSecurityManagerPtr manager; virSecuritySELinuxContextItemPtr *items; size_t nItems; + bool lock; }; #define SECURITY_SELINUX_VOID_DOI "0" @@ -221,20 +222,22 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED, int rv; int ret = -1; - if (VIR_ALLOC_N(paths, list->nItems) < 0) - return -1; + if (list->lock) { + if (VIR_ALLOC_N(paths, list->nItems) < 0) + return -1; - for (i = 0; i < list->nItems; i++) { - const char *p = list->items[i]->path; + for (i = 0; i < list->nItems; i++) { + const char *p = list->items[i]->path; - if (virFileIsDir(p)) - continue; + if (virFileIsDir(p)) + continue; - VIR_APPEND_ELEMENT_COPY_INPLACE(paths, npaths, p); - } + VIR_APPEND_ELEMENT_COPY_INPLACE(paths, npaths, p); + } - if (virSecurityManagerMetadataLock(list->manager, paths, npaths) < 0) - goto cleanup; + if (virSecurityManagerMetadataLock(list->manager, paths, npaths) < 0) + goto cleanup; + } rv = 0; for (i = 0; i < list->nItems; i++) { @@ -250,7 +253,8 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED, } } - if (virSecurityManagerMetadataUnlock(list->manager, paths, npaths) < 0) + if (list->lock && + virSecurityManagerMetadataUnlock(list->manager, paths, npaths) < 0) goto cleanup; if (rv < 0) @@ -1072,12 +1076,16 @@ virSecuritySELinuxTransactionStart(virSecurityManagerPtr mgr) * virSecuritySELinuxTransactionCommit: * @mgr: security manager * @pid: domain's PID + * @lock: lock and unlock paths that are relabeled * * If @pis is not -1 then enter the @pid namespace (usually @pid refers * to a domain) and perform all the sefilecon()-s on the list. If @pid * is -1 then the transaction is performed in the namespace of the * caller. * + * If @lock is true then all the paths that transaction would + * touch are locked before and unlocked after it is done so. + * * Note that the transaction is also freed, therefore new one has to be * started after successful return from this function. Also it is * considered as error if there's no transaction set and this function @@ -1088,9 +1096,11 @@ virSecuritySELinuxTransactionStart(virSecurityManagerPtr mgr) */ static int virSecuritySELinuxTransactionCommit(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - pid_t pid) + pid_t pid, + bool lock) { virSecuritySELinuxContextListPtr list; + int rc; int ret = -1; list = virThreadLocalGet(&contextList); @@ -1106,12 +1116,20 @@ virSecuritySELinuxTransactionCommit(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, goto cleanup; } - if ((pid == -1 && - virSecuritySELinuxTransactionRun(pid, list) < 0) || - (pid != -1 && - virProcessRunInMountNamespace(pid, - virSecuritySELinuxTransactionRun, - list) < 0)) + list->lock = lock; + + if (pid == -1) { + if (lock) + rc = virProcessRunInFork(virSecuritySELinuxTransactionRun, list); + else + rc = virSecuritySELinuxTransactionRun(pid, list); + } else { + rc = virProcessRunInMountNamespace(pid, + virSecuritySELinuxTransactionRun, + list); + } + + if (rc < 0) goto cleanup; ret = 0; diff --git a/src/security/security_stack.c b/src/security/security_stack.c index e37a681293..3e60d5d2b7 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -156,14 +156,15 @@ virSecurityStackTransactionStart(virSecurityManagerPtr mgr) static int virSecurityStackTransactionCommit(virSecurityManagerPtr mgr, - pid_t pid) + pid_t pid, + bool lock) { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; int rc = 0; for (; item; item = item->next) { - if (virSecurityManagerTransactionCommit(item->securityManager, pid) < 0) + if (virSecurityManagerTransactionCommit(item->securityManager, pid, lock) < 0) rc = -1; } -- GitLab