[libvirt] [PATCH 0/6] Don't run whole sec driver in namespace
In eadaa97548 I've tried to solve the issue of setting seclabels on private /dev/* entries. While my approach works, it has tiny flaw - anything that happens in the namespace stays in the namespace. I mean, if there's a internal state change occurring on relabel operation (it should not, and it doesn't nowadays, but it's no guarantee), this change is not reflected in the daemon. This is because when entering the namespace, the daemon forks, enters the namespace and then executes the RelabelAll() function. This imperfection is: a) very easy to forget b) very hard to debug Therefore, we may have transaction APIs as suggested here [1]. On transactionBegin() the sec driver will record [path. seclabel] somewhere instead of applying the label. Then on transactionCommit() new process is forked, enters the namespace and perform previously recorded changes. This way it is only the minimal code that runs in the namespace. Moreover, it runs over constant data thus there can be no internal state transition. 1: https://www.redhat.com/archives/libvir-list/2016-December/msg00254.html Michal Privoznik (6): security_selinux: s/virSecuritySELinuxSecurity/virSecuritySELinux/ 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 | 28 +++-- src/qemu/qemu_security.c | 98 +++++---------- src/security/security_dac.c | 197 +++++++++++++++++++++++++++++- src/security/security_driver.h | 9 ++ src/security/security_manager.c | 38 ++++++ src/security/security_manager.h | 7 +- src/security/security_selinux.c | 219 +++++++++++++++++++++++++++++++--- 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, 561 insertions(+), 107 deletions(-) -- 2.11.0
It doesn't make much sense to have two different prefix for functions within the same driver. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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
On 12/19/2016 10:57 AM, Michal Privoznik wrote:
It doesn't make much sense to have two different prefix for functions within the same driver.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_selinux.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)
ACK John
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 | 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
On 12/19/2016 10:57 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 | 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))
src is NULL here...
return 0;
+ if (!(src = virStorageSourceCopy(storage, false))) + goto cleanup; +
If the copy is only needed for remote, why not special case that...
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; }
Theoretically this could be an } else { then allocate source "cpy" here with the explanation that FileInit will create/write src->drv for remote usage... Use "cpy" instead of 'src' for the remote copy
/* 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;
No reason for if (save_errno) - besides it probably should be "if (save_errno == 0)" John
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);
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@redhat.com> --- 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:
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.
Extra CR/LF between paragraphs
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
s/forge/forget
step now while this limitation is still freshly in our minds.
s/freshly/fresh
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.
So what happens if one is halfway through a commit? No chance to rollback the already committed and we drop the yet to be committed. I wonder if there should be a Rollback() API as well... That would mean Commit would need to provide more context upon return though.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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 +++++++++++++++++++++++++++++++++++++++++
I know qemu doesn't used it, but would _apparmor also benefit from similar functionality? What's here is OK, but concerns raised in patch 4 make me pause on ACK. John
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,
On 01/07/2017 03:04 PM, John Ferlan wrote:
On 12/19/2016 10:57 AM, Michal Privoznik wrote:
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.
Extra CR/LF between paragraphs
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
s/forge/forget
step now while this limitation is still freshly in our minds.
s/freshly/fresh
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.
So what happens if one is halfway through a commit? No chance to rollback the already committed and we drop the yet to be committed.
Ah, you mean when an error occurs during Commit()? Well, nothing special - we don't do any Rollback() now either. The caller calls secdriver's Restore() API eventually. Also, what would happen if an error occurs during Rollback()? I'm not saying we shouldn't have it some day, but I don't see much need for it today. This is no worse than what we currently have.
I wonder if there should be a Rollback() API as well... That would mean Commit would need to provide more context upon return though.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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 +++++++++++++++++++++++++++++++++++++++++
I know qemu doesn't used it, but would _apparmor also benefit from similar functionality?
Not sure. Frankly, apparmor is terra incognita for me. But my gut feeling is that whenever there's a problem in a secdriver we fix DAC and SELinux but very occasionally apparmor. It just works :-)
What's here is OK, but concerns raised in patch 4 make me pause on ACK.
Fair enough. Let me check. Michal
John
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 193 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 193 insertions(+) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index b6f75fe1d..0208c1751 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -68,6 +68,117 @@ 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); +} + + +static int +virSecurityDACTransactionAppend(const char *path, + const virStorageSource *src, + uid_t uid, + gid_t gid) +{ + virSecurityDACChownListPtr list; + int ret = -1; + + list = virThreadLocalGet(&chownList); + if (!list) + return 0; + + if (virSecurityDACChownListAppend(list, path, src, uid, gid) < 0) + goto cleanup; + + ret = 0; + cleanup: + return ret; +} + + +static int virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, + const virStorageSource *src, + const char *path, + uid_t uid, + gid_t gid); + +static int +virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED, + void *opaque) +{ + virSecurityDACChownListPtr list = opaque; + size_t i; + + ignore_value(virThreadLocalSet(&chownList, NULL)); + for (i = 0; i < list->nItems; i++) { + virSecurityDACChownItemPtr item = list->items[i]; + + 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 +349,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 +396,69 @@ virSecurityDACPreFork(virSecurityManagerPtr mgr) return 0; } +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; +} + +static int +virSecurityDACTransactionCommit(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + pid_t pid) +{ + virSecurityDACChownListPtr list; + int ret = 0; + + list = virThreadLocalGet(&chownList); + if (!list) + return 0; + + if (virProcessRunInMountNamespace(pid, + virSecurityDACTransactionRun, + list) < 0) + ret = -1; + + ignore_value(virThreadLocalSet(&chownList, NULL)); + virSecurityDACChownListFree(list); + return ret; +} + +static void +virSecurityDACTransactionAbort(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) +{ + virSecurityDACChownListPtr list; + + list = virThreadLocalGet(&chownList); + if (!list) + return; + + ignore_value(virThreadLocalSet(&chownList, NULL)); + virSecurityDACChownListFree(list); +} + + static int virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, const virStorageSource *src, @@ -287,6 +468,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 +1815,10 @@ virSecurityDriver virSecurityDriverDAC = { .preFork = virSecurityDACPreFork, + .transactionStart = virSecurityDACTransactionStart, + .transactionCommit = virSecurityDACTransactionCommit, + .transactionAbort = virSecurityDACTransactionAbort, + .domainSecurityVerify = virSecurityDACVerify, .domainSetSecurityDiskLabel = virSecurityDACSetDiskLabel, -- 2.11.0
On 12/19/2016 10:57 AM, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 193 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 193 insertions(+)
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.
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index b6f75fe1d..0208c1751 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -68,6 +68,117 @@ 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); +} + +
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...
+static int +virSecurityDACTransactionAppend(const char *path, + const virStorageSource *src, + uid_t uid, + gid_t gid) +{ + virSecurityDACChownListPtr list; + int ret = -1; + + list = virThreadLocalGet(&chownList); + if (!list) + return 0; + + if (virSecurityDACChownListAppend(list, path, src, uid, gid) < 0) + goto cleanup; + + ret = 0;
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.
+ cleanup: + return ret; +} + + +static int virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, + const virStorageSource *src, + const char *path, + uid_t uid, + gid_t gid); +
Some comments here would be nice.
+static int +virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED, + void *opaque) +{ + virSecurityDACChownListPtr list = opaque; + size_t i; + + ignore_value(virThreadLocalSet(&chownList, NULL));
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
+ for (i = 0; i < list->nItems; i++) { + virSecurityDACChownItemPtr item = list->items[i]; + + if (virSecurityDACSetOwnershipInternal(list->priv, + item->src, + item->path, + item->uid, + item->gid) < 0) + return -1;
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.
+ } + + return 0; +} + + /* returns -1 on error, 0 on success */ int virSecurityDACSetUserAndGroup(virSecurityManagerPtr mgr, @@ -238,6 +349,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 +396,69 @@ virSecurityDACPreFork(virSecurityManagerPtr mgr) return 0; }
+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; +} + +static int +virSecurityDACTransactionCommit(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + pid_t pid) +{ + virSecurityDACChownListPtr list; + int ret = 0; + + list = virThreadLocalGet(&chownList); + if (!list) + return 0;
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?
+ + if (virProcessRunInMountNamespace(pid, + virSecurityDACTransactionRun, + list) < 0) + ret = -1; + + ignore_value(virThreadLocalSet(&chownList, NULL));
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.
+ virSecurityDACChownListFree(list); + return ret; +} + +static void +virSecurityDACTransactionAbort(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) +{ + virSecurityDACChownListPtr list; + + list = virThreadLocalGet(&chownList); + if (!list) + return; + + ignore_value(virThreadLocalSet(&chownList, NULL));
Not that it should fail, but part of me wonders if a VIR_DEBUG should issued...
+ virSecurityDACChownListFree(list); +} + + static int virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, const virStorageSource *src, @@ -287,6 +468,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; +
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
VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'", NULLSTR(src ? src->path : path), (long) uid, (long) gid);
@@ -1626,6 +1815,10 @@ virSecurityDriver virSecurityDriverDAC = {
.preFork = virSecurityDACPreFork,
+ .transactionStart = virSecurityDACTransactionStart, + .transactionCommit = virSecurityDACTransactionCommit, + .transactionAbort = virSecurityDACTransactionAbort, + .domainSecurityVerify = virSecurityDACVerify,
.domainSetSecurityDiskLabel = virSecurityDACSetDiskLabel,
On 01/07/2017 03:05 PM, John Ferlan wrote:
On 12/19/2016 10:57 AM, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 193 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 193 insertions(+)
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.
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index b6f75fe1d..0208c1751 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -68,6 +68,117 @@ 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); +} + +
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...
+static int +virSecurityDACTransactionAppend(const char *path, + const virStorageSource *src, + uid_t uid, + gid_t gid) +{ + virSecurityDACChownListPtr list; + int ret = -1; + + list = virThreadLocalGet(&chownList); + if (!list) + return 0; + + if (virSecurityDACChownListAppend(list, path, src, uid, gid) < 0) + goto cleanup; + + ret = 0;
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.
Oh right. I AM a giddy goat.
+ cleanup: + return ret; +} + + +static int virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, + const virStorageSource *src, + const char *path, + uid_t uid, + gid_t gid); +
Some comments here would be nice.
+static int +virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED, + void *opaque) +{ + virSecurityDACChownListPtr list = opaque; + size_t i; + + ignore_value(virThreadLocalSet(&chownList, NULL));
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
No. For couple of reasons: 1) this function is run from a separate process so unsetting this has no affect on the parent process (libvirtd). 2) We have to unset the thread local otherwise virSecurityDACSetOwnershipInternal() would still see the thread local variable and think there's a transaction set up. Thus instead of doing real chown() it would just append items to the list. But okay, I will put this into the comment. And also check for retval of virThreadLocalSet().
+ for (i = 0; i < list->nItems; i++) { + virSecurityDACChownItemPtr item = list->items[i]; + + if (virSecurityDACSetOwnershipInternal(list->priv, + item->src, + item->path, + item->uid, + item->gid) < 0) + return -1;
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.
Exactly. BTW: current code has the same limitation. If an error occurs in SetSecurityAllLabel() (called from qemuProcessLaunch for instance), all callers eventually call RestoreSecurityAllLabel() (called rom qemuProcessStop).
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.
Agreed.
+ } + + return 0; +} + + /* returns -1 on error, 0 on success */ int virSecurityDACSetUserAndGroup(virSecurityManagerPtr mgr, @@ -238,6 +349,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 +396,69 @@ virSecurityDACPreFork(virSecurityManagerPtr mgr) return 0; }
+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; +} + +static int +virSecurityDACTransactionCommit(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + pid_t pid) +{ + virSecurityDACChownListPtr list; + int ret = 0; + + list = virThreadLocalGet(&chownList); + if (!list) + return 0;
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?
It should if we want callers to be allowed to call TransactionCommit() only after prior TransactionStart() call. If you look at the very last patch of mine there's the following pattern: if (namespaceEnabled) transactionStart(); securityDriverCall(); transactionCommit(); transactionAbort(); If the namespace is not enabled both transactionCommit() and transactionAbort() are no-ops basically. But okay, I can make this function return an error in this case and thus change pattern to: if (namespaceEnabled) transactionStart(); securityDriverCall(); if (namespaceEnabled) transactionCommit(); transactionAbort(); I just thought that my current approach saves couple of LOC.
+ + if (virProcessRunInMountNamespace(pid, + virSecurityDACTransactionRun, + list) < 0) + ret = -1; + + ignore_value(virThreadLocalSet(&chownList, NULL));
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?
Ah, good point. If I do this before calling RunInMountNamespace() I don't have to do it there either.
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.
+ virSecurityDACChownListFree(list); + return ret; +} + +static void +virSecurityDACTransactionAbort(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) +{ + virSecurityDACChownListPtr list; + + list = virThreadLocalGet(&chownList); + if (!list) + return; + + ignore_value(virThreadLocalSet(&chownList, NULL));
Not that it should fail, but part of me wonders if a VIR_DEBUG should issued...
Yep. Will change that here and in all the other lines too.
+ virSecurityDACChownListFree(list); +} + + static int virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv, const virStorageSource *src, @@ -287,6 +468,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; +
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...
Yeah, for some reason my code was prepared for the behaviour you describe in the review, but my code does not implement it in the end. Michal
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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
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 | 98 ++++++++++++++---------------------------------- 1 file changed, 28 insertions(+), 70 deletions(-) diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index 9ab91e9f2..c0a991650 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -40,66 +40,30 @@ 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 (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 +72,21 @@ 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 (virSecurityManagerTransactionCommit(driver->securityManager, + vm->pid) < 0) + goto cleanup; + + cleanup: + virSecurityManagerTransactionAbort(driver->securityManager); } -- 2.11.0
participants (2)
-
John Ferlan -
Michal Privoznik