Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx> --- 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,22 +211,24 @@ 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,21 +222,23 @@ 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++) { virSecuritySELinuxContextItemPtr item = list->items[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; } -- 2.18.1 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list