From 37131adada5b2829e31ead6dd40de4d5cfffd639 Mon Sep 17 00:00:00 2001 From: Michal Privoznik Date: Wed, 5 Sep 2018 14:00:20 +0200 Subject: [PATCH] qemu_security: Run transactions more frequently Now that committing transactions using pid == -1 means that we're not fork()-ing to run the transaction in a specific namespace, we can utilize the transaction processing semantics in order to start, run a or multiple commands, and then commit the transaction without being concerned with other interactions or transactions interrupting the processing. This will eventually allow us to have a single place where all the paths can be locked, followed by relabeling and unlocking again. Signed-off-by: Michal Privoznik Reviewed-by: John Ferlan --- src/qemu/qemu_security.c | 231 ++++++++++++++++++++++++--------------- 1 file changed, 144 insertions(+), 87 deletions(-) diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index c64fbdda38..34921b4046 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -39,9 +39,12 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver, { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; + pid_t pid = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerSetAllLabel(driver->securityManager, @@ -50,9 +53,7 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver, priv->chardevStdioLogd) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; @@ -68,17 +69,27 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver, bool migrated) { qemuDomainObjPrivatePtr priv = vm->privateData; + bool transactionStarted = false; + + /* In contrast to qemuSecuritySetAllLabel, do not use vm->pid + * here. This function is called from qemuProcessStop() which + * is meant to do cleanup after qemu process died. The + * domain's namespace is gone as qemu was the only process + * running there. We would not succeed in entering the + * namespace then. */ + if (virSecurityManagerTransactionStart(driver->securityManager) >= 0) + transactionStarted = true; - /* In contrast to qemuSecuritySetAllLabel, do not use - * secdriver transactions here. This function is called from - * qemuProcessStop() which is meant to do cleanup after qemu - * process died. If it did do, the namespace is gone as qemu - * was the only process running there. We would not succeed - * in entering the namespace then. */ virSecurityManagerRestoreAllLabel(driver->securityManager, vm->def, migrated, priv->chardevStdioLogd); + + if (transactionStarted && + virSecurityManagerTransactionCommit(driver->securityManager, -1) < 0) + VIR_WARN("Unable to run security manager transaction"); + + virSecurityManagerTransactionAbort(driver->securityManager); } @@ -87,10 +98,13 @@ qemuSecuritySetDiskLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk) { + pid_t pid = -1; int ret = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerSetDiskLabel(driver->securityManager, @@ -98,9 +112,7 @@ qemuSecuritySetDiskLabel(virQEMUDriverPtr driver, disk) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; @@ -115,10 +127,13 @@ qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk) { + pid_t pid = -1; int ret = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerRestoreDiskLabel(driver->securityManager, @@ -126,9 +141,7 @@ qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver, disk) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; @@ -143,10 +156,13 @@ qemuSecuritySetImageLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virStorageSourcePtr src) { + pid_t pid = -1; int ret = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerSetImageLabel(driver->securityManager, @@ -154,9 +170,7 @@ qemuSecuritySetImageLabel(virQEMUDriverPtr driver, src) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; @@ -171,10 +185,13 @@ qemuSecurityRestoreImageLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virStorageSourcePtr src) { + pid_t pid = -1; int ret = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerRestoreImageLabel(driver->securityManager, @@ -182,9 +199,7 @@ qemuSecurityRestoreImageLabel(virQEMUDriverPtr driver, src) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; @@ -199,10 +214,13 @@ qemuSecuritySetHostdevLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) { + pid_t pid = -1; int ret = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerSetHostdevLabel(driver->securityManager, @@ -211,9 +229,7 @@ qemuSecuritySetHostdevLabel(virQEMUDriverPtr driver, NULL) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; @@ -228,10 +244,13 @@ qemuSecurityRestoreHostdevLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) { + pid_t pid = -1; int ret = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerRestoreHostdevLabel(driver->securityManager, @@ -240,9 +259,7 @@ qemuSecurityRestoreHostdevLabel(virQEMUDriverPtr driver, NULL) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; @@ -257,10 +274,13 @@ qemuSecuritySetMemoryLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainMemoryDefPtr mem) { + pid_t pid = -1; int ret = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerSetMemoryLabel(driver->securityManager, @@ -268,9 +288,7 @@ qemuSecuritySetMemoryLabel(virQEMUDriverPtr driver, mem) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; @@ -285,10 +303,13 @@ qemuSecurityRestoreMemoryLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainMemoryDefPtr mem) { + pid_t pid = -1; int ret = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerRestoreMemoryLabel(driver->securityManager, @@ -296,9 +317,7 @@ qemuSecurityRestoreMemoryLabel(virQEMUDriverPtr driver, mem) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; @@ -314,10 +333,13 @@ qemuSecuritySetInputLabel(virDomainObjPtr vm, { qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverPtr driver = priv->driver; + pid_t pid = -1; int ret = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerSetInputLabel(driver->securityManager, @@ -325,9 +347,7 @@ qemuSecuritySetInputLabel(virDomainObjPtr vm, input) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; @@ -343,10 +363,13 @@ qemuSecurityRestoreInputLabel(virDomainObjPtr vm, { qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverPtr driver = priv->driver; + pid_t pid = -1; int ret = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerRestoreInputLabel(driver->securityManager, @@ -354,9 +377,7 @@ qemuSecurityRestoreInputLabel(virDomainObjPtr vm, input) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; @@ -373,9 +394,12 @@ qemuSecuritySetChardevLabel(virQEMUDriverPtr driver, { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; + pid_t pid = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerSetChardevLabel(driver->securityManager, @@ -384,9 +408,7 @@ qemuSecuritySetChardevLabel(virQEMUDriverPtr driver, priv->chardevStdioLogd) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; @@ -403,9 +425,12 @@ qemuSecurityRestoreChardevLabel(virQEMUDriverPtr driver, { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; + pid_t pid = -1; + + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerRestoreChardevLabel(driver->securityManager, @@ -414,9 +439,7 @@ qemuSecurityRestoreChardevLabel(virQEMUDriverPtr driver, priv->chardevStdioLogd) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; @@ -454,10 +477,21 @@ qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver, int *cmdret) { int ret = -1; + bool transactionStarted = false; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) + return -1; + transactionStarted = true; if (virSecurityManagerSetTPMLabels(driver->securityManager, - def) < 0) + def) < 0) { + virSecurityManagerTransactionAbort(driver->securityManager); + return -1; + } + + if (virSecurityManagerTransactionCommit(driver->securityManager, -1) < 0) goto cleanup; + transactionStarted = false; if (virSecurityManagerSetChildProcessLabel(driver->securityManager, def, cmd) < 0) @@ -481,8 +515,17 @@ qemuSecurityStartTPMEmulator(virQEMUDriverPtr driver, return 0; cleanup: + if (!transactionStarted && + virSecurityManagerTransactionStart(driver->securityManager) >= 0) + transactionStarted = true; + virSecurityManagerRestoreTPMLabels(driver->securityManager, def); + if (transactionStarted && + virSecurityManagerTransactionCommit(driver->securityManager, -1) < 0) + VIR_WARN("Unable to run security manager transaction"); + + virSecurityManagerTransactionAbort(driver->securityManager); return ret; } @@ -491,7 +534,18 @@ void qemuSecurityCleanupTPMEmulator(virQEMUDriverPtr driver, virDomainDefPtr def) { + bool transactionStarted = false; + + if (virSecurityManagerTransactionStart(driver->securityManager) >= 0) + transactionStarted = true; + virSecurityManagerRestoreTPMLabels(driver->securityManager, def); + + if (transactionStarted && + virSecurityManagerTransactionCommit(driver->securityManager, -1) < 0) + VIR_WARN("Unable to run security manager transaction"); + + virSecurityManagerTransactionAbort(driver->securityManager); } @@ -501,10 +555,13 @@ qemuSecurityDomainSetPathLabel(virQEMUDriverPtr driver, const char *path, bool allowSubtree) { + pid_t pid = -1; int ret = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerDomainSetPathLabel(driver->securityManager, @@ -513,9 +570,7 @@ qemuSecurityDomainSetPathLabel(virQEMUDriverPtr driver, allowSubtree) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; @@ -530,10 +585,13 @@ qemuSecuritySetSavedStateLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, const char *savefile) { + pid_t pid = -1; int ret = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerSetSavedStateLabel(driver->securityManager, @@ -541,9 +599,7 @@ qemuSecuritySetSavedStateLabel(virQEMUDriverPtr driver, savefile) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; @@ -558,10 +614,13 @@ qemuSecurityRestoreSavedStateLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, const char *savefile) { + pid_t pid = -1; int ret = -1; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionStart(driver->securityManager) < 0) + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) + pid = vm->pid; + + if (virSecurityManagerTransactionStart(driver->securityManager) < 0) goto cleanup; if (virSecurityManagerRestoreSavedStateLabel(driver->securityManager, @@ -569,9 +628,7 @@ qemuSecurityRestoreSavedStateLabel(virQEMUDriverPtr driver, savefile) < 0) goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && - virSecurityManagerTransactionCommit(driver->securityManager, - vm->pid) < 0) + if (virSecurityManagerTransactionCommit(driver->securityManager, pid) < 0) goto cleanup; ret = 0; -- GitLab