[libvirt] [PATCH v2 0/5] Don't run whole sec driver in namespace

v2 of: https://www.redhat.com/archives/libvir-list/2016-December/msg00907.html diff to v1: - Pushed 1/6 from the original series as it was ACKed - Added more comments - Fixed return value of virSecurityDACTransactionAppend and virSecuritySELinuxTransactionAppend - Dropped ignore_value() around virThreadLocalSet() - Unset thread local just in transactionCommit APIs I have not implemented any rollback yet, but I've added comments where the implementation should go ;-) Michal Privoznik (5): security_dac: Resolve virSecurityDACSetOwnershipInternal const correctness security driver: Introduce transaction APIs security_dac: Implement transaction APIs security_selinux: Implement transaction APIs qemu: Use transactions from security driver src/libvirt_private.syms | 3 + src/qemu/qemu_driver.c | 25 ++-- src/qemu/qemu_security.c | 100 ++++--------- src/security/security_dac.c | 261 +++++++++++++++++++++++++++++++++- src/security/security_driver.h | 9 ++ src/security/security_manager.c | 68 +++++++++ src/security/security_manager.h | 7 +- src/security/security_selinux.c | 256 ++++++++++++++++++++++++++++++++- src/security/security_stack.c | 49 +++++++ 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 +- 16 files changed, 703 insertions(+), 95 deletions(-) -- 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@redhat.com> --- src/qemu/qemu_driver.c | 25 ++++++++++++++++--------- 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, 29 insertions(+), 22 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 675a4d0e7..6a45ccd9b 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 *src, uid_t uid, gid_t gid) { + virStorageSourcePtr cpy = NULL; struct stat sb; int save_errno = 0; int ret = -1; @@ -355,22 +356,28 @@ qemuSecurityChownCallback(virStorageSourcePtr src, } return chown(src->path, uid, gid); - } + } else { + if (!(cpy = virStorageSourceCopy(src, false))) + goto cleanup; - /* storage file init reports errors, return -2 on failure */ - if (virStorageFileInit(src) < 0) - return -2; + /* src file init reports errors, return -2 on failure */ + if (virStorageFileInit(cpy) < 0) { + ret = -2; + goto cleanup; + } - if (virStorageFileChown(src, uid, gid) < 0) { - save_errno = errno; - goto cleanup; + if (virStorageFileChown(cpy, uid, gid) < 0) { + save_errno = errno; + goto cleanup; + } } ret = 0; cleanup: - virStorageFileDeinit(src); errno = save_errno; + virStorageFileDeinit(cpy); + virStorageSourceFree(cpy); 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 5fc706634..8f1d3f04c 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2848,7 +2848,7 @@ int storageRegister(void) /* ----------- file handlers cooperating with storage driver --------------- */ static bool -virStorageFileIsInitialized(virStorageSourcePtr src) +virStorageFileIsInitialized(const virStorageSource *src) { return src && src->drv; } @@ -2888,7 +2888,7 @@ virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src) * driver to perform labelling */ bool -virStorageFileSupportsSecurityDriver(virStorageSourcePtr src) +virStorageFileSupportsSecurityDriver(const virStorageSource *src) { int actualType; virStorageFileBackendPtr backend; @@ -3179,7 +3179,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

On 01/09/2017 07:58 AM, Michal Privoznik wrote:
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@redhat.com> --- src/qemu/qemu_driver.c | 25 ++++++++++++++++--------- 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, 29 insertions(+), 22 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 675a4d0e7..6a45ccd9b 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 *src, uid_t uid, gid_t gid) { + virStorageSourcePtr cpy = NULL; struct stat sb; int save_errno = 0; int ret = -1; @@ -355,22 +356,28 @@ qemuSecurityChownCallback(virStorageSourcePtr src, }
return chown(src->path, uid, gid);
^^ v1 had a "ret = chown()"; goto cleanup; hence why I suggested else logic. Of course if you just return then the else isn't necessary. I just envision someone coming along shortly and writing a patch to remove the else because it's decidedly unnecessary now. ACK - with either a ret = chown or just undoing the else now.... John
- } + } else { + if (!(cpy = virStorageSourceCopy(src, false))) + goto cleanup;
- /* storage file init reports errors, return -2 on failure */ - if (virStorageFileInit(src) < 0) - return -2; + /* src file init reports errors, return -2 on failure */ + if (virStorageFileInit(cpy) < 0) { + ret = -2; + goto cleanup; + }
- if (virStorageFileChown(src, uid, gid) < 0) { - save_errno = errno; - goto cleanup; + if (virStorageFileChown(cpy, uid, gid) < 0) { + save_errno = errno; + goto cleanup; + } }
ret = 0;
cleanup: - virStorageFileDeinit(src); errno = save_errno; + virStorageFileDeinit(cpy); + virStorageSourceFree(cpy);
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 5fc706634..8f1d3f04c 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2848,7 +2848,7 @@ int storageRegister(void)
/* ----------- file handlers cooperating with storage driver --------------- */ static bool -virStorageFileIsInitialized(virStorageSourcePtr src) +virStorageFileIsInitialized(const virStorageSource *src) { return src && src->drv; } @@ -2888,7 +2888,7 @@ virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src) * driver to perform labelling */ bool -virStorageFileSupportsSecurityDriver(virStorageSourcePtr src) +virStorageFileSupportsSecurityDriver(const virStorageSource *src) { int actualType; virStorageFileBackendPtr backend; @@ -3179,7 +3179,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);

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 <mprivozn@redhat.com> --- 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 c5b9cee45..97c1123b3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1176,6 +1176,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..d8c6facc8 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 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

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 257 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 257 insertions(+) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index b6f75fe1d..d457e6aa2 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -68,6 +68,137 @@ struct _virSecurityDACCallbackData { virSecurityLabelDefPtr secdef; }; +typedef struct _virSecurityDACChownItem virSecurityDACChownItem; +typedef virSecurityDACChownItem *virSecurityDACChownItemPtr; +struct _virSecurityDACChownItem { + const char *path; + const virStorageSource *src; + uid_t uid; + gid_t gid; +}; + +typedef struct _virSecurityDACChownList virSecurityDACChownList; +typedef virSecurityDACChownList *virSecurityDACChownListPtr; +struct _virSecurityDACChownList { + virSecurityDACDataPtr priv; + virSecurityDACChownItemPtr *items; + size_t nItems; +}; + + +virThreadLocal chownList; + +static int +virSecurityDACChownListAppend(virSecurityDACChownListPtr list, + const char *path, + const virStorageSource *src, + uid_t uid, + gid_t gid) +{ + virSecurityDACChownItemPtr item; + + if (VIR_ALLOC(item) < 0) + return -1; + + item->path = path; + item->src = src; + item->uid = uid; + item->gid = gid; + + if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0) { + VIR_FREE(item); + return -1; + } + + return 0; +} + +static void +virSecurityDACChownListFree(void *opaque) +{ + virSecurityDACChownListPtr list = opaque; + size_t i; + + if (!list) + return; + + for (i = 0; i < list->nItems; i++) + VIR_FREE(list->items[i]); + VIR_FREE(list); +} + + +/** + * virSecurityDACTransactionAppend: + * @path: Path to chown + * @src: disk source to chown + * @uid: user ID + * @gid: group ID + * + * Appends an entry onto transaction list. + * + * Returns: 1 in case of successful append + * 0 if there is no transaction enabled + * -1 otherwise. + */ +static int +virSecurityDACTransactionAppend(const char *path, + const virStorageSource *src, + uid_t uid, + gid_t gid) +{ + virSecurityDACChownListPtr list = virThreadLocalGet(&chownList); + if (!list) + return 0; + + if (virSecurityDACChownListAppend(list, path, src, uid, gid) < 0) + return -1; + + return 1; +} + + +static int virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, + const virStorageSource *src, + const char *path, + uid_t uid, + gid_t gid); + +/** + * virSecurityDACTransactionRun: + * @pid: process pid + * @opaque: opaque data + * + * This is the callback that runs in the same namespace as the domain we are + * relabelling. For given transaction (@opaque) it relabels all the paths on + * the list. + * + * Returns: 0 on success + * -1 otherwise. + */ +static int +virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED, + void *opaque) +{ + virSecurityDACChownListPtr list = opaque; + size_t i; + + for (i = 0; i < list->nItems; i++) { + virSecurityDACChownItemPtr item = list->items[i]; + + /* TODO Implement rollback */ + if (virSecurityDACSetOwnershipInternal(list->priv, + item->src, + item->path, + item->uid, + item->gid) < 0) + return -1; + } + + return 0; +} + + /* returns -1 on error, 0 on success */ int virSecurityDACSetUserAndGroup(virSecurityManagerPtr mgr, @@ -238,6 +369,13 @@ virSecurityDACProbe(const char *virtDriver ATTRIBUTE_UNUSED) static int virSecurityDACOpen(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) { + if (virThreadLocalInit(&chownList, + virSecurityDACChownListFree) < 0) { + virReportSystemError(errno, "%s", + _("Unable to initialize thread local variable")); + return -1; + } + return 0; } @@ -278,6 +416,113 @@ virSecurityDACPreFork(virSecurityManagerPtr mgr) return 0; } +/** + * virSecurityDACTransactionStart: + * @mgr: security manager + * + * Starts a new transaction. In transaction nothing is chown()-ed until + * TransactionCommit() is called. This is implemented as a list that is + * appended to whenever chown() would be called. Since secdriver APIs + * can be called from multiple threads (to work over different domains) + * the pointer to the list is stored in thread local variable. + * + * Returns 0 on success, + * -1 otherwise. + */ +static int +virSecurityDACTransactionStart(virSecurityManagerPtr mgr) +{ + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityDACChownListPtr list; + + list = virThreadLocalGet(&chownList); + if (list) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Another relabel transaction is already started")); + return -1; + } + + if (VIR_ALLOC(list) < 0) + return -1; + + list->priv = priv; + + if (virThreadLocalSet(&chownList, list) < 0) { + virReportSystemError(errno, "%s", + _("Unable to set thread local variable")); + VIR_FREE(list); + return -1; + } + + return 0; +} + +/** + * virSecurityDACTransactionCommit: + * @mgr: security manager + * @pid: domain's PID + * + * Enters the @pid namespace (usually @pid refers to a domain) and + * performs all the chown()-s on the 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. + */ +static int +virSecurityDACTransactionCommit(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + pid_t pid) +{ + virSecurityDACChownListPtr list; + int ret = -1; + + list = virThreadLocalGet(&chownList); + if (!list) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("No transaction is set")); + goto cleanup; + } + + if (virThreadLocalSet(&chownList, NULL) < 0) { + virReportSystemError(errno, "%s", + _("Unable to clear thread local variable")); + goto cleanup; + } + + if (virProcessRunInMountNamespace(pid, + virSecurityDACTransactionRun, + list) < 0) + goto cleanup; + + ret = 0; + cleanup: + virSecurityDACChownListFree(list); + return ret; +} + +/** + * virSecurityDACTransactionAbort: + * @mgr: security manager + * + * Cancels and frees any out standing transaction. + */ +static void +virSecurityDACTransactionAbort(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) +{ + virSecurityDACChownListPtr list; + + list = virThreadLocalGet(&chownList); + if (!list) + return; + + if (virThreadLocalSet(&chownList, NULL) < 0) + VIR_DEBUG("Unable to clear thread local variable"); + virSecurityDACChownListFree(list); +} + + static int virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, const virStorageSource *src, @@ -287,6 +532,14 @@ virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, { int rc; + /* Be aware that this function might run in a separate process. + * Therefore, any driver state changes would be thrown away. */ + + if ((rc = virSecurityDACTransactionAppend(path, src, uid, gid)) < 0) + return -1; + else if (rc > 0) + return 0; + VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'", NULLSTR(src ? src->path : path), (long) uid, (long) gid); @@ -1626,6 +1879,10 @@ virSecurityDriver virSecurityDriverDAC = { .preFork = virSecurityDACPreFork, + .transactionStart = virSecurityDACTransactionStart, + .transactionCommit = virSecurityDACTransactionCommit, + .transactionAbort = virSecurityDACTransactionAbort, + .domainSecurityVerify = virSecurityDACVerify, .domainSetSecurityDiskLabel = virSecurityDACSetDiskLabel, -- 2.11.0

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_selinux.c | 256 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 253 insertions(+), 3 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index fec19b98c..92267e8fc 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,115 @@ 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); +} + + +/** + * virSecuritySELinuxTransactionAppend: + * @path: Path to chown + * @tcon: target context + * @optional: true if setting @tcon is optional + * + * Appends an entry onto transaction list. + * + * Returns: 1 in case of successful append + * 0 if there is no transaction enabled + * -1 otherwise. + */ +static int +virSecuritySELinuxTransactionAppend(const char *path, + const char *tcon, + bool optional) +{ + virSecuritySELinuxContextListPtr list; + + list = virThreadLocalGet(&contextList); + if (!list) + return 0; + + if (virSecuritySELinuxContextListAppend(list, path, tcon, optional) < 0) + return -1; + + return 1; +} + + +static int virSecuritySELinuxSetFileconHelper(const char *path, + const char *tcon, + bool optional, + bool privileged); + +/** + * virSecuritySELinuxTransactionRun: + * @pid: process pid + * @opaque: opaque data + * + * This is the callback that runs in the same namespace as the domain we are + * relabelling. For given transaction (@opaque) it relabels all the paths on + * the list. + * + * Returns: 0 on success + * -1 otherwise. + */ +static int +virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED, + void *opaque) +{ + virSecuritySELinuxContextListPtr list = opaque; + size_t i; + + for (i = 0; i < list->nItems; i++) { + virSecuritySELinuxContextItemPtr item = list->items[i]; + + /* TODO Implement rollback */ + 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 +685,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 +976,110 @@ virSecuritySELinuxGetDOI(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) return SECURITY_SELINUX_VOID_DOI; } +/** + * virSecuritySELinuxTransactionStart: + * @mgr: security manager + * + * Starts a new transaction. In transaction nothing is changed context + * until TransactionCommit() is called. This is implemented as a list + * that is appended to whenever setfilecon() would be called. Since + * secdriver APIs can be called from multiple threads (to work over + * different domains) the pointer to the list is stored in thread local + * variable. + * + * Returns 0 on success, + * -1 otherwise. + */ +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; +} + +/** + * virSecuritySELinuxTransactionCommit: + * @mgr: security manager + * @pid: domain's PID + * + * Enters the @pid namespace (usually @pid refers to a domain) and + * performs all the sefilecon()-s on the 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. + */ +static int +virSecuritySELinuxTransactionCommit(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + pid_t pid) +{ + virSecuritySELinuxContextListPtr list; + int ret = 0; + + list = virThreadLocalGet(&contextList); + if (!list) + return 0; + + if (virThreadLocalSet(&contextList, NULL) < 0) { + virReportSystemError(errno, "%s", + _("Unable to clear thread local variable")); + goto cleanup; + } + + if (virProcessRunInMountNamespace(pid, + virSecuritySELinuxTransactionRun, + list) < 0) + goto cleanup; + + ret = 0; + cleanup: + virSecuritySELinuxContextListFree(list); + return ret; +} + +/** + * virSecuritySELinuxTransactionAbort: + * @mgr: security manager + * + * Cancels and frees any out standing transaction. + */ +static void +virSecuritySELinuxTransactionAbort(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) +{ + virSecuritySELinuxContextListPtr list; + + list = virThreadLocalGet(&contextList); + if (!list) + return; + + if (virThreadLocalSet(&contextList, NULL) < 0) + VIR_DEBUG("Unable to clear thread local variable"); + virSecuritySELinuxContextListFree(list); +} + static int virSecuritySELinuxGetProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr def ATTRIBUTE_UNUSED, @@ -885,10 +1122,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 +1191,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 +1199,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 +2913,10 @@ virSecurityDriver virSecurityDriverSELinux = { .getModel = virSecuritySELinuxGetModel, .getDOI = virSecuritySELinuxGetDOI, + .transactionStart = virSecuritySELinuxTransactionStart, + .transactionCommit = virSecuritySELinuxTransactionCommit, + .transactionAbort = virSecuritySELinuxTransactionAbort, + .domainSecurityVerify = virSecuritySELinuxVerify, .domainSetSecurityDiskLabel = virSecuritySELinuxSetDiskLabel, -- 2.11.0

So far if qemu is spawned under separate mount namespace in order to relabel everything it needs an access to the security driver is run in that namespace too. This has a very nasty down side - it is being run in a separate process, so any internal state transition is NOT reflected in the dameon. This can lead to many sleepless nights. Therefore, use the transaction APIs so that libvirt developers can sleep tight again. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_security.c | 100 ++++++++++++++--------------------------------- 1 file changed, 30 insertions(+), 70 deletions(-) diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index 9ab91e9f2..544feeb4a 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -40,66 +40,31 @@ struct qemuSecuritySetRestoreAllLabelData { }; -static int -qemuSecuritySetRestoreAllLabelHelper(pid_t pid, - void *opaque) -{ - struct qemuSecuritySetRestoreAllLabelData *data = opaque; - - virSecurityManagerPostFork(data->driver->securityManager); - - if (data->set) { - VIR_DEBUG("Setting up security labels inside namespace pid=%lld", - (long long) pid); - if (virSecurityManagerSetAllLabel(data->driver->securityManager, - data->vm->def, - data->stdin_path) < 0) - return -1; - } else { - VIR_DEBUG("Restoring security labels inside namespace pid=%lld", - (long long) pid); - if (virSecurityManagerRestoreAllLabel(data->driver->securityManager, - data->vm->def, - data->migrated) < 0) - return -1; - } - - return 0; -} - - int qemuSecuritySetAllLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, const char *stdin_path) { - struct qemuSecuritySetRestoreAllLabelData data; + int ret = -1; - memset(&data, 0, sizeof(data)); + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && + virSecurityManagerTransactionStart(driver->securityManager) < 0) + goto cleanup; - data.set = true; - data.driver = driver; - data.vm = vm; - data.stdin_path = stdin_path; + if (virSecurityManagerSetAllLabel(driver->securityManager, + vm->def, + stdin_path) < 0) + goto cleanup; - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) { - if (virSecurityManagerPreFork(driver->securityManager) < 0) - return -1; - if (virProcessRunInMountNamespace(vm->pid, - qemuSecuritySetRestoreAllLabelHelper, - &data) < 0) { - virSecurityManagerPostFork(driver->securityManager); - return -1; - } - virSecurityManagerPostFork(driver->securityManager); + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && + virSecurityManagerTransactionCommit(driver->securityManager, + vm->pid) < 0) + goto cleanup; - } else { - if (virSecurityManagerSetAllLabel(driver->securityManager, - vm->def, - stdin_path) < 0) - return -1; - } - return 0; + ret = 0; + cleanup: + virSecurityManagerTransactionAbort(driver->securityManager); + return ret; } @@ -108,27 +73,22 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, bool migrated) { - struct qemuSecuritySetRestoreAllLabelData data; + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && + virSecurityManagerTransactionStart(driver->securityManager) < 0) + goto cleanup; - memset(&data, 0, sizeof(data)); - - data.driver = driver; - data.vm = vm; - data.migrated = migrated; - - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) { - if (virSecurityManagerPreFork(driver->securityManager) < 0) - return; - - virProcessRunInMountNamespace(vm->pid, - qemuSecuritySetRestoreAllLabelHelper, - &data); - virSecurityManagerPostFork(driver->securityManager); - } else { - virSecurityManagerRestoreAllLabel(driver->securityManager, + if (virSecurityManagerRestoreAllLabel(driver->securityManager, vm->def, - migrated); - } + migrated) < 0) + goto cleanup; + + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && + virSecurityManagerTransactionCommit(driver->securityManager, + vm->pid) < 0) + goto cleanup; + + cleanup: + virSecurityManagerTransactionAbort(driver->securityManager); } -- 2.11.0

On 01/09/2017 07:58 AM, Michal Privoznik wrote:
So far if qemu is spawned under separate mount namespace in order to relabel everything it needs an access to the security driver is run in that namespace too. This has a very nasty down side -
s/is/to/
it is being run in a separate process, so any internal state transition is NOT reflected in the dameon. This can lead to many
s/dameon/daemon
sleepless nights. Therefore, use the transaction APIs so that libvirt developers can sleep tight again.
Having trouble sleeping lately? ;-)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_security.c | 100 ++++++++++++++--------------------------------- 1 file changed, 30 insertions(+), 70 deletions(-)
diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index 9ab91e9f2..544feeb4a 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -40,66 +40,31 @@ struct qemuSecuritySetRestoreAllLabelData { };
-static int -qemuSecuritySetRestoreAllLabelHelper(pid_t pid, - void *opaque) -{ - struct qemuSecuritySetRestoreAllLabelData *data = opaque; - - virSecurityManagerPostFork(data->driver->securityManager); - - if (data->set) { - VIR_DEBUG("Setting up security labels inside namespace pid=%lld", - (long long) pid); - if (virSecurityManagerSetAllLabel(data->driver->securityManager, - data->vm->def, - data->stdin_path) < 0) - return -1; - } else { - VIR_DEBUG("Restoring security labels inside namespace pid=%lld", - (long long) pid); - if (virSecurityManagerRestoreAllLabel(data->driver->securityManager, - data->vm->def, - data->migrated) < 0) - return -1; - } - - return 0; -} - - int qemuSecuritySetAllLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, const char *stdin_path) { - struct qemuSecuritySetRestoreAllLabelData data; + int ret = -1;
- memset(&data, 0, sizeof(data)); + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && + virSecurityManagerTransactionStart(driver->securityManager) < 0) + goto cleanup;
- data.set = true; - data.driver = driver; - data.vm = vm; - data.stdin_path = stdin_path; + if (virSecurityManagerSetAllLabel(driver->securityManager, + vm->def, + stdin_path) < 0) + goto cleanup;
- if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) { - if (virSecurityManagerPreFork(driver->securityManager) < 0) - return -1;
Both paths have removed the PreFork/PostFork processing... Is that then no longer required? This is/was the only PreFork caller I think.
- if (virProcessRunInMountNamespace(vm->pid, - qemuSecuritySetRestoreAllLabelHelper, - &data) < 0) { - virSecurityManagerPostFork(driver->securityManager); - return -1; - } - virSecurityManagerPostFork(driver->securityManager); + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && + virSecurityManagerTransactionCommit(driver->securityManager, + vm->pid) < 0) + goto cleanup;
Once we've entered Commit - calling Abort is a noop since Commit takes the 'list', clears the thread local storage, and will free the list on exit.
- } else { - if (virSecurityManagerSetAllLabel(driver->securityManager, - vm->def, - stdin_path) < 0) - return -1; - } - return 0; + ret = 0; + cleanup: + virSecurityManagerTransactionAbort(driver->securityManager); + return ret; }
@@ -108,27 +73,22 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, bool migrated) { - struct qemuSecuritySetRestoreAllLabelData data; + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && + virSecurityManagerTransactionStart(driver->securityManager) < 0) + goto cleanup;
- memset(&data, 0, sizeof(data)); - - data.driver = driver; - data.vm = vm; - data.migrated = migrated; - - if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) { - if (virSecurityManagerPreFork(driver->securityManager) < 0) - return; - - virProcessRunInMountNamespace(vm->pid, - qemuSecuritySetRestoreAllLabelHelper, - &data); - virSecurityManagerPostFork(driver->securityManager); - } else { - virSecurityManagerRestoreAllLabel(driver->securityManager, + if (virSecurityManagerRestoreAllLabel(driver->securityManager, vm->def, - migrated); - } + migrated) < 0) + goto cleanup;
This would seemingly be the only place where goto cleanup is necessary John
+ + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && + virSecurityManagerTransactionCommit(driver->securityManager, + vm->pid) < 0) + goto cleanup; + + cleanup: + virSecurityManagerTransactionAbort(driver->securityManager); }

On 01/09/2017 11:18 PM, John Ferlan wrote:
On 01/09/2017 07:58 AM, Michal Privoznik wrote:
So far if qemu is spawned under separate mount namespace in order to relabel everything it needs an access to the security driver is run in that namespace too. This has a very nasty down side -
s/is/to/
it is being run in a separate process, so any internal state transition is NOT reflected in the dameon. This can lead to many
s/dameon/daemon
sleepless nights. Therefore, use the transaction APIs so that libvirt developers can sleep tight again.
Having trouble sleeping lately? ;-)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_security.c | 100 ++++++++++++++--------------------------------- 1 file changed, 30 insertions(+), 70 deletions(-)
diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index 9ab91e9f2..544feeb4a 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -40,66 +40,31 @@ struct qemuSecuritySetRestoreAllLabelData { };
-static int -qemuSecuritySetRestoreAllLabelHelper(pid_t pid, - void *opaque) -{ - struct qemuSecuritySetRestoreAllLabelData *data = opaque; - - virSecurityManagerPostFork(data->driver->securityManager); - - if (data->set) { - VIR_DEBUG("Setting up security labels inside namespace pid=%lld", - (long long) pid); - if (virSecurityManagerSetAllLabel(data->driver->securityManager, - data->vm->def, - data->stdin_path) < 0) - return -1; - } else { - VIR_DEBUG("Restoring security labels inside namespace pid=%lld", - (long long) pid); - if (virSecurityManagerRestoreAllLabel(data->driver->securityManager, - data->vm->def, - data->migrated) < 0) - return -1; - } - - return 0; -} - - int qemuSecuritySetAllLabel(virQEMUDriverPtr driver, virDomainObjPtr vm, const char *stdin_path) { - struct qemuSecuritySetRestoreAllLabelData data; + int ret = -1;
- memset(&data, 0, sizeof(data)); + if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && + virSecurityManagerTransactionStart(driver->securityManager) < 0) + goto cleanup;
- data.set = true; - data.driver = driver; - data.vm = vm; - data.stdin_path = stdin_path; + if (virSecurityManagerSetAllLabel(driver->securityManager, + vm->def, + stdin_path) < 0) + goto cleanup;
- if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) { - if (virSecurityManagerPreFork(driver->securityManager) < 0) - return -1;
Both paths have removed the PreFork/PostFork processing... Is that then no longer required? This is/was the only PreFork caller I think.
Yes, it is no longer required. There is no fork() happening in virSecurityManagerSetAllLabel() anymore. Michal

On 01/09/2017 07:58 AM, Michal Privoznik wrote:
v2 of:
https://www.redhat.com/archives/libvir-list/2016-December/msg00907.html
diff to v1: - Pushed 1/6 from the original series as it was ACKed - Added more comments - Fixed return value of virSecurityDACTransactionAppend and virSecuritySELinuxTransactionAppend - Dropped ignore_value() around virThreadLocalSet() - Unset thread local just in transactionCommit APIs
I have not implemented any rollback yet, but I've added comments where the implementation should go ;-)
Michal Privoznik (5): security_dac: Resolve virSecurityDACSetOwnershipInternal const correctness security driver: Introduce transaction APIs security_dac: Implement transaction APIs security_selinux: Implement transaction APIs qemu: Use transactions from security driver
src/libvirt_private.syms | 3 + src/qemu/qemu_driver.c | 25 ++-- src/qemu/qemu_security.c | 100 ++++--------- src/security/security_dac.c | 261 +++++++++++++++++++++++++++++++++- src/security/security_driver.h | 9 ++ src/security/security_manager.c | 68 +++++++++ src/security/security_manager.h | 7 +- src/security/security_selinux.c | 256 ++++++++++++++++++++++++++++++++- src/security/security_stack.c | 49 +++++++ 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 +- 16 files changed, 703 insertions(+), 95 deletions(-)
BTW: Perhaps an existing bug... If *Launch calls SetAllLabel and fails a -1 is returned... Once SetAllLabel succeeds, all returns afterwards are -2. When the caller detects a -2 return, then it forces the relabel in the exit path. So if we have a partial success/failure, then do we really restore anything? Should the setting of ret = -2 go before the call to qemuSecuritySetAllLabel? Not related to this set of changes, but since I was looking at the code... I guess for now not having Rollback will be no worse than what exists. It would seem to matter more on Set processing and much harder on Restore processing (other than ignoring errors and trying to restore as many as possible). I think the Rollback thoughts came mostly because I started thinking about other transaction applications which don't process "All", but rather process "Some". Not rolling back on Abort or forcing a caller to Rollback may be unexpected. Consider that the virSecurity*TransactionRun could "save" the status of the call to each list entry and then if there is a failure, go through each successful one, recall the saved data label data, and restore it (ok conceptually at least). Taking this path means not worrying about the partial failure path I noted above. If ignoring failures on Restore is important, then Start could save in list whether this was a "Set" or "Restore" operation... In any case, I'm not going to hold things up for a Rollback... As long as the notes from patch 1 and 5 are addressed, then ACK series. John
participants (2)
-
John Ferlan
-
Michal Privoznik