From 95576b4df0334953c2731152a36a8e1360c301c3 Mon Sep 17 00:00:00 2001 From: Michal Privoznik Date: Wed, 14 Dec 2016 10:14:49 +0100 Subject: [PATCH] security driver: Introduce transaction APIs With our new qemu namespace code in place, the relabelling of devices is done not as good is it could: a child process is spawned, it enters the mount namespace of the qemu process and then runs desired API of the security driver. Problem with this approach is that internal state transition of the security driver done in the child process is not reflected in the parent process. While currently it wouldn't matter that much, it is fairly easy to forget about that. We should take the extra step now while this limitation is still fresh in our minds. Three new APIs are introduced here: virSecurityManagerTransactionStart() virSecurityManagerTransactionCommit() virSecurityManagerTransactionAbort() The Start() is going to be used to let security driver know that we are starting a new transaction. During a transaction no security labels are actually touched, but rather recorded and only at Commit() phase they are actually updated. Should something go wrong Abort() aborts the transaction freeing up all memory allocated by transaction. Signed-off-by: Michal Privoznik --- src/libvirt_private.syms | 3 ++ src/security/security_driver.h | 9 +++++ src/security/security_manager.c | 68 +++++++++++++++++++++++++++++++++ src/security/security_manager.h | 5 +++ src/security/security_stack.c | 49 ++++++++++++++++++++++++ 5 files changed, 134 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 82adf7ecb7..b69e8bcc25 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1183,6 +1183,9 @@ virSecurityManagerSetSavedStateLabel; virSecurityManagerSetSocketLabel; virSecurityManagerSetTapFDLabel; virSecurityManagerStackAddNested; +virSecurityManagerTransactionAbort; +virSecurityManagerTransactionCommit; +virSecurityManagerTransactionStart; virSecurityManagerVerify; diff --git a/src/security/security_driver.h b/src/security/security_driver.h index b48f2aaed8..fa65eb3595 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -51,6 +51,11 @@ typedef const char *(*virSecurityDriverGetBaseLabel) (virSecurityManagerPtr mgr, typedef int (*virSecurityDriverPreFork) (virSecurityManagerPtr mgr); +typedef int (*virSecurityDriverTransactionStart) (virSecurityManagerPtr mgr); +typedef int (*virSecurityDriverTransactionCommit) (virSecurityManagerPtr mgr, + pid_t pid); +typedef void (*virSecurityDriverTransactionAbort) (virSecurityManagerPtr mgr); + typedef int (*virSecurityDomainRestoreDiskLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainDiskDefPtr disk); @@ -135,6 +140,10 @@ struct _virSecurityDriver { virSecurityDriverPreFork preFork; + virSecurityDriverTransactionStart transactionStart; + virSecurityDriverTransactionCommit transactionCommit; + virSecurityDriverTransactionAbort transactionAbort; + virSecurityDomainSecurityVerify domainSecurityVerify; virSecurityDomainSetDiskLabel domainSetSecurityDiskLabel; diff --git a/src/security/security_manager.c b/src/security/security_manager.c index f98c7d2d1c..d8c6facc81 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -235,6 +235,74 @@ virSecurityManagerPostFork(virSecurityManagerPtr mgr) virObjectUnlock(mgr); } + +/** + * virSecurityManagerTransactionStart: + * @mgr: security manager + * + * Starts a new transaction. In transaction nothing is changed security + * label until virSecurityManagerTransactionCommit() is called. + * + * Returns 0 on success, + * -1 otherwise. + */ +int +virSecurityManagerTransactionStart(virSecurityManagerPtr mgr) +{ + int ret = 0; + + virObjectLock(mgr); + if (mgr->drv->transactionStart) + ret = mgr->drv->transactionStart(mgr); + virObjectUnlock(mgr); + return ret; +} + + +/** + * virSecurityManagerTransactionCommit: + * @mgr: security manager + * @pid: domain's PID + * + * Enters the @pid namespace (usually @pid refers to a domain) and + * performs all the operations on the transaction list. 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 is called. + * + * Returns: 0 on success, + * -1 otherwise. + */ +int +virSecurityManagerTransactionCommit(virSecurityManagerPtr mgr, + pid_t pid) +{ + int ret = 0; + + virObjectLock(mgr); + if (mgr->drv->transactionCommit) + ret = mgr->drv->transactionCommit(mgr, pid); + virObjectUnlock(mgr); + return ret; +} + + +/** + * virSecurityManagerTransactionAbort: + * @mgr: security manager + * + * Cancels and frees any out standing transaction. + */ +void +virSecurityManagerTransactionAbort(virSecurityManagerPtr mgr) +{ + virObjectLock(mgr); + if (mgr->drv->transactionAbort) + mgr->drv->transactionAbort(mgr); + virObjectUnlock(mgr); +} + + void * virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr) { diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 80fceddc84..bae8493eea 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -76,6 +76,11 @@ virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver, int virSecurityManagerPreFork(virSecurityManagerPtr mgr); void virSecurityManagerPostFork(virSecurityManagerPtr mgr); +int virSecurityManagerTransactionStart(virSecurityManagerPtr mgr); +int virSecurityManagerTransactionCommit(virSecurityManagerPtr mgr, + pid_t pid); +void virSecurityManagerTransactionAbort(virSecurityManagerPtr mgr); + void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr); const char *virSecurityManagerGetDriver(virSecurityManagerPtr mgr); diff --git a/src/security/security_stack.c b/src/security/security_stack.c index c123931a0b..6056ae321d 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -137,6 +137,51 @@ virSecurityStackPreFork(virSecurityManagerPtr mgr) return rc; } + +static int +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; + } + + return rc; +} + + +static int +virSecurityStackTransactionCommit(virSecurityManagerPtr mgr, + pid_t pid) +{ + virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityStackItemPtr item = priv->itemsHead; + int rc = 0; + + for (; item; item = item->next) { + if (virSecurityManagerTransactionCommit(item->securityManager, pid) < 0) + rc = -1; + } + + return rc; +} + + +static void +virSecurityStackTransactionAbort(virSecurityManagerPtr mgr) +{ + virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityStackItemPtr item = priv->itemsHead; + + for (; item; item = item->next) + virSecurityManagerTransactionAbort(item->securityManager); +} + + static int virSecurityStackVerify(virSecurityManagerPtr mgr, virDomainDefPtr def) @@ -612,6 +657,10 @@ virSecurityDriver virSecurityDriverStack = { .preFork = virSecurityStackPreFork, + .transactionStart = virSecurityStackTransactionStart, + .transactionCommit = virSecurityStackTransactionCommit, + .transactionAbort = virSecurityStackTransactionAbort, + .domainSecurityVerify = virSecurityStackVerify, .domainSetSecurityDiskLabel = virSecurityStackSetDiskLabel,