It doesn't make much sense to have two different prefix for functions within the same driver. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/security_selinux.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 1776a630e..fec19b98c 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -780,7 +780,7 @@ virSecuritySELinuxReserveLabel(virSecurityManagerPtr mgr, static int -virSecuritySELinuxSecurityDriverProbe(const char *virtDriver) +virSecuritySELinuxDriverProbe(const char *virtDriver) { if (is_selinux_enabled() <= 0) return SECURITY_DRIVER_DISABLE; @@ -797,14 +797,14 @@ virSecuritySELinuxSecurityDriverProbe(const char *virtDriver) static int -virSecuritySELinuxSecurityDriverOpen(virSecurityManagerPtr mgr) +virSecuritySELinuxDriverOpen(virSecurityManagerPtr mgr) { return virSecuritySELinuxInitialize(mgr); } static int -virSecuritySELinuxSecurityDriverClose(virSecurityManagerPtr mgr) +virSecuritySELinuxDriverClose(virSecurityManagerPtr mgr) { virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr); @@ -828,13 +828,13 @@ virSecuritySELinuxSecurityDriverClose(virSecurityManagerPtr mgr) static const char * -virSecuritySELinuxSecurityGetModel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) +virSecuritySELinuxGetModel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) { return SECURITY_SELINUX_NAME; } static const char * -virSecuritySELinuxSecurityGetDOI(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) +virSecuritySELinuxGetDOI(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) { /* * Where will the DOI come from? SELinux configuration, or qemu @@ -2149,8 +2149,8 @@ virSecuritySELinuxRestoreSavedStateLabel(virSecurityManagerPtr mgr, static int -virSecuritySELinuxSecurityVerify(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainDefPtr def) +virSecuritySELinuxVerify(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr def) { virSecurityLabelDefPtr secdef; @@ -2660,14 +2660,14 @@ virSecuritySELinuxDomainSetPathLabel(virSecurityManagerPtr mgr, virSecurityDriver virSecurityDriverSELinux = { .privateDataLen = sizeof(virSecuritySELinuxData), .name = SECURITY_SELINUX_NAME, - .probe = virSecuritySELinuxSecurityDriverProbe, - .open = virSecuritySELinuxSecurityDriverOpen, - .close = virSecuritySELinuxSecurityDriverClose, + .probe = virSecuritySELinuxDriverProbe, + .open = virSecuritySELinuxDriverOpen, + .close = virSecuritySELinuxDriverClose, - .getModel = virSecuritySELinuxSecurityGetModel, - .getDOI = virSecuritySELinuxSecurityGetDOI, + .getModel = virSecuritySELinuxGetModel, + .getDOI = virSecuritySELinuxGetDOI, - .domainSecurityVerify = virSecuritySELinuxSecurityVerify, + .domainSecurityVerify = virSecuritySELinuxVerify, .domainSetSecurityDiskLabel = virSecuritySELinuxSetDiskLabel, .domainRestoreSecurityDiskLabel = virSecuritySELinuxRestoreDiskLabel, -- 2.11.0

The code at the very bottom of the DAC secdriver that calls chown() should be fine with read-only data. If something needs to be prepared it should have been done beforehand. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 28 ++++++++++++++++++++-------- src/security/security_dac.c | 4 ++-- src/security/security_manager.h | 2 +- src/storage/storage_backend.h | 2 +- src/storage/storage_backend_fs.c | 2 +- src/storage/storage_backend_gluster.c | 2 +- src/storage/storage_driver.c | 6 +++--- src/storage/storage_driver.h | 4 ++-- src/util/virstoragefile.c | 2 +- src/util/virstoragefile.h | 2 +- 10 files changed, 33 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1a464337e..f68b6ff30 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -329,10 +329,11 @@ qemuAutostartDomains(virQEMUDriverPtr driver) static int -qemuSecurityChownCallback(virStorageSourcePtr src, +qemuSecurityChownCallback(const virStorageSource *storage, uid_t uid, gid_t gid) { + virStorageSourcePtr src = NULL; struct stat sb; int save_errno = 0; int ret = -1; @@ -340,26 +341,35 @@ qemuSecurityChownCallback(virStorageSourcePtr src, if (!virStorageFileSupportsSecurityDriver(src)) return 0; + if (!(src = virStorageSourceCopy(storage, false))) + goto cleanup; + if (virStorageSourceIsLocalStorage(src)) { /* use direct chmod for local files so that the file doesn't * need to be initialized */ - if (!src->path) - return 0; + if (!src->path) { + ret = 0; + goto cleanup; + } if (stat(src->path, &sb) >= 0) { if (sb.st_uid == uid && sb.st_gid == gid) { /* It's alright, there's nothing to change anyway. */ - return 0; + ret = 0; + goto cleanup; } } - return chown(src->path, uid, gid); + ret = chown(src->path, uid, gid); + goto cleanup; } /* storage file init reports errors, return -2 on failure */ - if (virStorageFileInit(src) < 0) - return -2; + if (virStorageFileInit(src) < 0) { + ret = -2; + goto cleanup; + } if (virStorageFileChown(src, uid, gid) < 0) { save_errno = errno; @@ -370,7 +380,9 @@ qemuSecurityChownCallback(virStorageSourcePtr src, cleanup: virStorageFileDeinit(src); - errno = save_errno; + virStorageSourceFree(src); + if (save_errno) + errno = save_errno; return ret; } diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 649219e52..b6f75fe1d 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -279,8 +279,8 @@ virSecurityDACPreFork(virSecurityManagerPtr mgr) } static int -virSecurityDACSetOwnershipInternal(virSecurityDACDataPtr priv, - virStorageSourcePtr src, +virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, + const virStorageSource *src, const char *path, uid_t uid, gid_t gid) diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 0d22cb15f..80fceddc8 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -62,7 +62,7 @@ int virSecurityManagerStackAddNested(virSecurityManagerPtr stack, * @src. The callback shall return 0 on success, -1 on error and errno set (no * libvirt error reported) OR -2 and a libvirt error reported. */ typedef int -(*virSecurityManagerDACChownCallback)(virStorageSourcePtr src, +(*virSecurityManagerDACChownCallback)(const virStorageSource *src, uid_t uid, gid_t gid); diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 28e1a6517..82fbbbf6d 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -285,7 +285,7 @@ typedef int int mode); typedef int -(*virStorageFileBackendChown)(virStorageSourcePtr src, +(*virStorageFileBackendChown)(const virStorageSource *src, uid_t uid, gid_t gid); diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index de0e8d57d..9de0c17c0 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1600,7 +1600,7 @@ virStorageFileBackendFileAccess(virStorageSourcePtr src, static int -virStorageFileBackendFileChown(virStorageSourcePtr src, +virStorageFileBackendFileChown(const virStorageSource *src, uid_t uid, gid_t gid) { diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 8e867043e..ae0611543 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -809,7 +809,7 @@ virStorageFileBackendGlusterGetUniqueIdentifier(virStorageSourcePtr src) static int -virStorageFileBackendGlusterChown(virStorageSourcePtr src, +virStorageFileBackendGlusterChown(const virStorageSource *src, uid_t uid, gid_t gid) { diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index a79acc6f0..1ac07dbf2 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2832,7 +2832,7 @@ int storageRegister(void) /* ----------- file handlers cooperating with storage driver --------------- */ static bool -virStorageFileIsInitialized(virStorageSourcePtr src) +virStorageFileIsInitialized(const virStorageSource *src) { return src && src->drv; } @@ -2872,7 +2872,7 @@ virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src) * driver to perform labelling */ bool -virStorageFileSupportsSecurityDriver(virStorageSourcePtr src) +virStorageFileSupportsSecurityDriver(const virStorageSource *src) { int actualType; virStorageFileBackendPtr backend; @@ -3163,7 +3163,7 @@ virStorageFileAccess(virStorageSourcePtr src, * by libvirt storage backend. */ int -virStorageFileChown(virStorageSourcePtr src, +virStorageFileChown(const virStorageSource *src, uid_t uid, gid_t gid) { diff --git a/src/storage/storage_driver.h b/src/storage/storage_driver.h index 3f2549da5..682c9ff82 100644 --- a/src/storage/storage_driver.h +++ b/src/storage/storage_driver.h @@ -44,9 +44,9 @@ ssize_t virStorageFileReadHeader(virStorageSourcePtr src, char **buf); const char *virStorageFileGetUniqueIdentifier(virStorageSourcePtr src); int virStorageFileAccess(virStorageSourcePtr src, int mode); -int virStorageFileChown(virStorageSourcePtr src, uid_t uid, gid_t gid); +int virStorageFileChown(const virStorageSource *src, uid_t uid, gid_t gid); -bool virStorageFileSupportsSecurityDriver(virStorageSourcePtr src); +bool virStorageFileSupportsSecurityDriver(const virStorageSource *src); int virStorageFileGetMetadata(virStorageSourcePtr src, uid_t uid, gid_t gid, diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index ce6d21388..3d4bda700 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2082,7 +2082,7 @@ virStorageSourceGetActualType(const virStorageSource *def) bool -virStorageSourceIsLocalStorage(virStorageSourcePtr src) +virStorageSourceIsLocalStorage(const virStorageSource *src) { virStorageType type = virStorageSourceGetActualType(src); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 1f62244db..bea1c35a2 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -349,7 +349,7 @@ int virStorageSourceInitChainElement(virStorageSourcePtr newelem, void virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr def); void virStorageSourceClear(virStorageSourcePtr def); int virStorageSourceGetActualType(const virStorageSource *def); -bool virStorageSourceIsLocalStorage(virStorageSourcePtr src); +bool virStorageSourceIsLocalStorage(const virStorageSource *src); bool virStorageSourceIsEmpty(virStorageSourcePtr src); bool virStorageSourceIsBlockLocal(const virStorageSource *src); void virStorageSourceFree(virStorageSourcePtr def); -- 2.11.0

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 forge about that. We should take the extra step now while this limitation is still freshly 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 <mprivozn(a)redhat.com&gt; --- src/libvirt_private.syms | 3 +++ src/security/security_driver.h | 9 ++++++++ src/security/security_manager.c | 38 ++++++++++++++++++++++++++++++++ src/security/security_manager.h | 5 +++++ src/security/security_stack.c | 49 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 104 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4c0170c03..400295b7b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1172,6 +1172,9 @@ virSecurityManagerSetSavedStateLabel; virSecurityManagerSetSocketLabel; virSecurityManagerSetTapFDLabel; virSecurityManagerStackAddNested; +virSecurityManagerTransactionAbort; +virSecurityManagerTransactionCommit; +virSecurityManagerTransactionStart; virSecurityManagerVerify; diff --git a/src/security/security_driver.h b/src/security/security_driver.h index b48f2aaed..fa65eb359 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 f98c7d2d1..4b6b4a926 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -235,6 +235,44 @@ virSecurityManagerPostFork(virSecurityManagerPtr mgr) virObjectUnlock(mgr); } + +int +virSecurityManagerTransactionStart(virSecurityManagerPtr mgr) +{ + int ret = 0; + + virObjectLock(mgr); + if (mgr->drv->transactionStart) + ret = mgr->drv->transactionStart(mgr); + virObjectUnlock(mgr); + return ret; +} + + +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; +} + + +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 80fceddc8..bae8493ee 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 c123931a0..6056ae321 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, -- 2.11.0

On 12/19/2016 10:57 AM, Michal Privoznik wrote: Might have been nice to add some basic/sparse documentation over return values... BTW: I'm stopping here since I have some functionality concerns that would probably be replicated in _selinux and the qemu implementation. This is one example where documenting return values would help... Since whether 'list' is NULL or not is important from the callers viewpoint. I also note the caller will do something different when ret > 0, but I see no way for that to happen... Should this be ret = 1 so the caller "knows" something was put on list? and thus returns 0 rather than running the rest of the code. Some comments here would be nice. Not that it should fail, but part of me wonders if a VIR_DEBUG should issued... This point in the process is the key step in the whole process - it's from this point on that we do the "real" work. Still after a bit of reading on and thinking - should this clearing be done in Commit instead prior to calling Run If we have a failure, then should we rollback everything done to this point and of course how would we accomplish that? The problem being list is available only here and in Commit so how do we really know how much has been *really* committed and of course does that matter if our caller can "rollback" for us. If I read things right, the caller would generate the same "list" in rollback fashion and rollback everything rather than perhaps being smarter and rolling back only what was changed. Although I suppose rolling back everything would/could cause failure at the same point as Commit. Still we have no real way of restoring/rolling back from the point of failure unless commit and/or this function returns that mechanism. It would seem the rollback of RestoreAll labels would/could be overkill. Wouldn't !list only happen if there was an error and we're calling Commit more or less unexpectedly (either after some error that caused Abort() to be called or a second time)? IOW: Should this be an error? Not that it should fail, but part of me wonders if a VIR_DEBUG should issued... Secondary to that - should setting to NULL be done before RunInMountNamespace and not in TransactionRun? Also for a rollback/error type handling, rather than passing list - should we pass a structure that includes list and a count of successful commits. That could be compared against list.nItems and on error returned to the caller to have some chance at successful rollback. Not that it should fail, but part of me wonders if a VIR_DEBUG should issued... If we call virSecurityDACChownListAppend and ret = 0, then we still fall into the code below which from my read of what this code is going to do may not be expected... John

Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/security_selinux.c | 193 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 190 insertions(+), 3 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index fec19b98c..af226bff6 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -78,6 +78,22 @@ struct _virSecuritySELinuxCallbackData { virDomainDefPtr def; }; +typedef struct _virSecuritySELinuxContextItem virSecuritySELinuxContextItem; +typedef virSecuritySELinuxContextItem *virSecuritySELinuxContextItemPtr; +struct _virSecuritySELinuxContextItem { + const char *path; + const char *tcon; + bool optional; +}; + +typedef struct _virSecuritySELinuxContextList virSecuritySELinuxContextList; +typedef virSecuritySELinuxContextList *virSecuritySELinuxContextListPtr; +struct _virSecuritySELinuxContextList { + bool privileged; + virSecuritySELinuxContextItemPtr *items; + size_t nItems; +}; + #define SECURITY_SELINUX_VOID_DOI "0" #define SECURITY_SELINUX_NAME "selinux" @@ -87,6 +103,94 @@ virSecuritySELinuxRestoreTPMFileLabelInt(virSecurityManagerPtr mgr, virDomainTPMDefPtr tpm); +virThreadLocal contextList; + +static int +virSecuritySELinuxContextListAppend(virSecuritySELinuxContextListPtr list, + const char *path, + const char *tcon, + bool optional) +{ + virSecuritySELinuxContextItemPtr item; + + if (VIR_ALLOC(item) < 0) + return -1; + + item->path = path; + item->tcon = tcon; + item->optional = optional; + + if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0) { + VIR_FREE(item); + return -1; + } + + return 0; +} + +static void +virSecuritySELinuxContextListFree(void *opaque) +{ + virSecuritySELinuxContextListPtr list = opaque; + size_t i; + + if (!list) + return; + + for (i = 0; i < list->nItems; i++) + VIR_FREE(list->items[i]); + VIR_FREE(list); +} + + +static int +virSecuritySELinuxTransactionAppend(const char *path, + const char *tcon, + bool optional) +{ + virSecuritySELinuxContextListPtr list; + int ret = -1; + + list = virThreadLocalGet(&contextList); + if (!list) + return 0; + + if (virSecuritySELinuxContextListAppend(list, path, tcon, optional) < 0) + goto cleanup; + + ret = 0; + cleanup: + return ret; +} + + +static int virSecuritySELinuxSetFileconHelper(const char *path, + const char *tcon, + bool optional, + bool privileged); + +static int +virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED, + void *opaque) +{ + virSecuritySELinuxContextListPtr list = opaque; + size_t i; + + ignore_value(virThreadLocalSet(&contextList, NULL)); + for (i = 0; i < list->nItems; i++) { + virSecuritySELinuxContextItemPtr item = list->items[i]; + + if (virSecuritySELinuxSetFileconHelper(item->path, + item->tcon, + item->optional, + list->privileged) < 0) + return -1; + } + + return 0; +} + + /* * Returns 0 on success, 1 if already reserved, or -1 on fatal error */ @@ -560,6 +664,14 @@ static int virSecuritySELinuxInitialize(virSecurityManagerPtr mgr) { VIR_DEBUG("SELinuxInitialize %s", virSecurityManagerGetDriver(mgr)); + + if (virThreadLocalInit(&contextList, + virSecuritySELinuxContextListFree) < 0) { + virReportSystemError(errno, "%s", + _("Unable to initialize thread local variable")); + return -1; + } + if (STREQ(virSecurityManagerGetDriver(mgr), "LXC")) { return virSecuritySELinuxLXCInitialize(mgr); } else { @@ -843,6 +955,68 @@ virSecuritySELinuxGetDOI(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) return SECURITY_SELINUX_VOID_DOI; } +static int +virSecuritySELinuxTransactionStart(virSecurityManagerPtr mgr) +{ + bool privileged = virSecurityManagerGetPrivileged(mgr); + virSecuritySELinuxContextListPtr list; + + list = virThreadLocalGet(&contextList); + if (list) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Another relabel transaction is already started")); + return -1; + } + + if (VIR_ALLOC(list) < 0) + return -1; + + list->privileged = privileged; + + if (virThreadLocalSet(&contextList, list) < 0) { + virReportSystemError(errno, "%s", + _("Unable to set thread local variable")); + VIR_FREE(list); + return -1; + } + + return 0; +} + +static int +virSecuritySELinuxTransactionCommit(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + pid_t pid) +{ + virSecuritySELinuxContextListPtr list; + int ret = 0; + + list = virThreadLocalGet(&contextList); + if (!list) + return 0; + + if (virProcessRunInMountNamespace(pid, + virSecuritySELinuxTransactionRun, + list) < 0) + ret = -1; + + ignore_value(virThreadLocalSet(&contextList, NULL)); + virSecuritySELinuxContextListFree(list); + return ret; +} + +static void +virSecuritySELinuxTransactionAbort(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) +{ + virSecuritySELinuxContextListPtr list; + + list = virThreadLocalGet(&contextList); + if (!list) + return; + + ignore_value(virThreadLocalSet(&contextList, NULL)); + virSecuritySELinuxContextListFree(list); +} + static int virSecuritySELinuxGetProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr def ATTRIBUTE_UNUSED, @@ -885,10 +1059,19 @@ virSecuritySELinuxGetProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, * return 1 if labelling was not possible. Otherwise, require a label * change, and return 0 for success, -1 for failure. */ static int -virSecuritySELinuxSetFileconHelper(const char *path, char *tcon, +virSecuritySELinuxSetFileconHelper(const char *path, const char *tcon, bool optional, bool privileged) { security_context_t econ; + int rc; + + /* Be aware that this function might run in a separate process. + * Therefore, any driver state changes would be thrown away. */ + + if ((rc = virSecuritySELinuxTransactionAppend(path, tcon, optional)) < 0) + return -1; + else if (rc > 0) + return 0; VIR_INFO("Setting SELinux context on '%s' to '%s'", path, tcon); @@ -945,7 +1128,7 @@ virSecuritySELinuxSetFileconHelper(const char *path, char *tcon, static int virSecuritySELinuxSetFileconOptional(virSecurityManagerPtr mgr, - const char *path, char *tcon) + const char *path, const char *tcon) { bool privileged = virSecurityManagerGetPrivileged(mgr); return virSecuritySELinuxSetFileconHelper(path, tcon, true, privileged); @@ -953,7 +1136,7 @@ virSecuritySELinuxSetFileconOptional(virSecurityManagerPtr mgr, static int virSecuritySELinuxSetFilecon(virSecurityManagerPtr mgr, - const char *path, char *tcon) + const char *path, const char *tcon) { bool privileged = virSecurityManagerGetPrivileged(mgr); return virSecuritySELinuxSetFileconHelper(path, tcon, false, privileged); @@ -2667,6 +2850,10 @@ virSecurityDriver virSecurityDriverSELinux = { .getModel = virSecuritySELinuxGetModel, .getDOI = virSecuritySELinuxGetDOI, + .transactionStart = virSecuritySELinuxTransactionStart, + .transactionCommit = virSecuritySELinuxTransactionCommit, + .transactionAbort = virSecuritySELinuxTransactionAbort, + .domainSecurityVerify = virSecuritySELinuxVerify, .domainSetSecurityDiskLabel = virSecuritySELinuxSetDiskLabel, -- 2.11.0