[libvirt] [PATCH v1 00/23] Keep original seclabel
So, you may be familiar with this already. Well, I've tried to get these patches in like a year ago (or even more). Point is, these ones are new, written from scratch. However, still based on idea, that virtlockd will keep the track of the original seclabels. So far only DAC driver is fixed, but the infrastructure I'm proposing here is easily extensible to other drivers too. Even if there's some disagreement on the design, the first few patches fix some bugs, so they should make it in. Michal Privoznik (23): virtlockd: Don't SIGSEGV on SIGUSR1 security_dac: Fix TODO marks virSecurityDACSetOwnershipInternal: Don't chown so often security_dac: Introduce remember/recall stubs virSecurityDACSetOwnership: Pass virSecurityDACDataPtr virSecurityDACRestoreSecurityFileLabel: Pass virSecurityDACDataPtr security_dac: Limit usage of virSecurityDACSetOwnershipInternal security_dac: Plug in remember/recall APIs lock_protocol: Add two new remote procedures lock_daemon: Switch to wrapper locking functions locking: Introduce virSeclabelSpace virtlockd: Work virSeclabelSpace in virLockDriver: Introduce virLockDriverRemember and virLockDriverRecall lock_driver_nop: Implement remember and recall APIs lock_driver_lockd: Implement remember and recall APIs lock_manager: Implement remember & recall APIs locking: Favour enum type over int lock_driver: Introduce VIR_LOCK_MANAGER_OBJECT_TYPE_SECLABEL virSecurityManagerNewDAC: Pass locking plugin in security_dac: Remember security labels security_dac: Restore original owner more often security: Introduce virSecurityManagerDomainRestoreDirLabel qemuProcessStop: Restore seclabels on dirs too cfg.mk | 2 +- po/POTFILES.in | 1 + src/Makefile.am | 3 + src/libvirt_private.syms | 3 + src/lock_protocol-structs | 15 + src/locking/lock_daemon.c | 69 ++++- src/locking/lock_daemon.h | 8 + src/locking/lock_daemon_dispatch.c | 68 +++++ src/locking/lock_daemon_seclabels.c | 545 ++++++++++++++++++++++++++++++++++++ src/locking/lock_daemon_seclabels.h | 43 +++ src/locking/lock_driver.h | 42 ++- src/locking/lock_driver_lockd.c | 103 ++++++- src/locking/lock_driver_nop.c | 28 +- src/locking/lock_driver_sanlock.c | 4 +- src/locking/lock_manager.c | 36 ++- src/locking/lock_manager.h | 13 +- src/locking/lock_protocol.x | 29 +- src/qemu/qemu_driver.c | 3 +- src/qemu/qemu_process.c | 22 +- src/security/security_dac.c | 288 +++++++++++++++---- src/security/security_dac.h | 2 + src/security/security_driver.h | 5 +- src/security/security_manager.c | 20 +- src/security/security_manager.h | 7 +- src/security/security_selinux.c | 16 ++ src/security/security_stack.c | 20 ++ 26 files changed, 1310 insertions(+), 85 deletions(-) create mode 100644 src/locking/lock_daemon_seclabels.c create mode 100644 src/locking/lock_daemon_seclabels.h -- 2.4.9
So we have this mechanism that on SIGUSR1 the virtlockd dumps its internal state into a JSON file, reexec itself and the reloads the internal state back. However, there's a bug in our implementation: (gdb) signal SIGUSR1 Continuing with signal SIGUSR1. [Thread 0x7fd094f7b700 (LWP 10602) exited] process 10600 is executing new program: /home/zippy/work/libvirt/libvirt.git/src/virtlockd warning: Could not load shared library symbols for linux-vdso.so.1. Do you need "set solib-search-path" or "set sysroot"? [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib64/libthread_db.so.1". [New Thread 0x7fb28bc3c700 (LWP 14501)] Program received signal SIGSEGV, Segmentation fault. 0x00007fb29133d530 in virExpandN (ptrptr=0x70, size=8, countptr=0x68, add=1, report=true, domcode=7, filename=0x7fb29138aeab "rpc/virnetserver.c", funcname=0x7fb29138b680 <__FUNCTION__.15821> "virNetServerAddProgram", linenr=661) at util/viralloc.c:288 288 if (*countptr + add < *countptr) { (gdb) bt #0 0x00007fb29133d530 in virExpandN (ptrptr=0x70, size=8, countptr=0x68, add=1, report=true, domcode=7, filename=0x7fb29138aeab "rpc/virnetserver.c", funcname=0x7fb29138b680 <__FUNCTION__.15821> "virNetServerAddProgram", linenr=661) at util/viralloc.c:288 #1 0x00007fb29132a267 in virNetServerAddProgram (srv=0x0, prog=0x7fb2915d08b0) at rpc/virnetserver.c:661 #2 0x00007fb29131f27f in main (argc=1, argv=0x7fff8f771298) at locking/lock_daemon.c:1445 Notice the NULL @srv passed to frame 2? Usually, the @srv variable is initialized on fresh start. However, in case of daemon reload, the code path that is responsible for initializing the value was not triggered and therefore we crashed immediately. Fix this by always setting the variable. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_daemon.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index 55fc0c3..56db959 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -1366,6 +1366,8 @@ int main(int argc, char **argv) { ret = VIR_LOCK_DAEMON_ERR_NETWORK; goto cleanup; } + } else if (rv == 1) { + srv = virNetDaemonGetServer(lockDaemon->dmn, 0); } if (timeout != -1) { -- 2.4.9
On 10/12/2015 06:25 AM, Michal Privoznik wrote:
So we have this mechanism that on SIGUSR1 the virtlockd dumps its internal state into a JSON file, reexec itself and the reloads the internal state back. However, there's a bug in our implementation:
(gdb) signal SIGUSR1 Continuing with signal SIGUSR1. [Thread 0x7fd094f7b700 (LWP 10602) exited] process 10600 is executing new program: /home/zippy/work/libvirt/libvirt.git/src/virtlockd warning: Could not load shared library symbols for linux-vdso.so.1. Do you need "set solib-search-path" or "set sysroot"? [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib64/libthread_db.so.1". [New Thread 0x7fb28bc3c700 (LWP 14501)]
Program received signal SIGSEGV, Segmentation fault. 0x00007fb29133d530 in virExpandN (ptrptr=0x70, size=8, countptr=0x68, add=1, report=true, domcode=7, filename=0x7fb29138aeab "rpc/virnetserver.c", funcname=0x7fb29138b680 <__FUNCTION__.15821> "virNetServerAddProgram", linenr=661) at util/viralloc.c:288 288 if (*countptr + add < *countptr) { (gdb) bt #0 0x00007fb29133d530 in virExpandN (ptrptr=0x70, size=8, countptr=0x68, add=1, report=true, domcode=7, filename=0x7fb29138aeab "rpc/virnetserver.c", funcname=0x7fb29138b680 <__FUNCTION__.15821> "virNetServerAddProgram", linenr=661) at util/viralloc.c:288 #1 0x00007fb29132a267 in virNetServerAddProgram (srv=0x0, prog=0x7fb2915d08b0) at rpc/virnetserver.c:661 #2 0x00007fb29131f27f in main (argc=1, argv=0x7fff8f771298) at locking/lock_daemon.c:1445
Notice the NULL @srv passed to frame 2? Usually, the @srv variable is initialized on fresh start. However, in case of daemon reload, the code path that is responsible for initializing the value was not triggered and therefore we crashed immediately. Fix this by always setting the variable.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_daemon.c | 2 ++ 1 file changed, 2 insertions(+)
ACK - although you may want to considering altering the comment after the virLockDaemonPostExecRestart call to indicate that getting the need to get the srv pointer again... John
Correctly mark the places where we need to remember and recall file ownership. We don't want to mislead any potential developer. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 864d75b..0dfe570 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -312,6 +312,7 @@ virSecurityDACSetOwnershipInternal(virSecurityDACDataPtr priv, static int virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid) { + /* XXX record previous ownership */ return virSecurityDACSetOwnershipInternal(NULL, NULL, path, uid, gid); } @@ -324,7 +325,7 @@ virSecurityDACRestoreSecurityFileLabelInternal(virSecurityDACDataPtr priv, VIR_INFO("Restoring DAC user and group on '%s'", NULLSTR(src ? src->path : path)); - /* XXX record previous ownership */ + /* XXX recall previous ownership */ return virSecurityDACSetOwnershipInternal(priv, src, path, 0, 0); } -- 2.4.9
On 10/12/2015 06:25 AM, Michal Privoznik wrote:
Correctly mark the places where we need to remember and recall file ownership. We don't want to mislead any potential developer.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
ACK John
It's better if we stat() file that we are about to chown() at first and check if there's something we need to change. Not that it would make much difference, but for the upcoming patches we need to be doing stat() anyway. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 0dfe570..9b079e0 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -271,17 +271,18 @@ virSecurityDACSetOwnershipInternal(virSecurityDACDataPtr priv, path = src->path; } + if (stat(path, &sb) < 0) { + virReportSystemError(errno, _("unable to stat: %s"), path); + return -1; + } + + if (sb.st_uid == uid && sb.st_gid == gid) { + /* nothing to chown */ + return 0; + } + rc = chown(path, uid, gid); chown_errno = errno; - - if (rc < 0 && - stat(path, &sb) >= 0) { - if (sb.st_uid == uid && - sb.st_gid == gid) { - /* It's alright, there's nothing to change anyway. */ - return 0; - } - } } if (rc < 0) { -- 2.4.9
On 10/12/2015 06:25 AM, Michal Privoznik wrote:
It's better if we stat() file that we are about to chown() at first and check if there's something we need to change. Not that it would make much difference, but for the upcoming patches we need to be doing stat() anyway.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 0dfe570..9b079e0 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -271,17 +271,18 @@ virSecurityDACSetOwnershipInternal(virSecurityDACDataPtr priv, path = src->path; }
+ if (stat(path, &sb) < 0) { + virReportSystemError(errno, _("unable to stat: %s"), path); + return -1; + } + + if (sb.st_uid == uid && sb.st_gid == gid) { + /* nothing to chown */ + return 0; + } + rc = chown(path, uid, gid); chown_errno = errno; - - if (rc < 0 && - stat(path, &sb) >= 0) { - if (sb.st_uid == uid && - sb.st_gid == gid) { - /* It's alright, there's nothing to change anyway. */ - return 0; - } - }
I don't believe chown_errno is necessary anymore either since a failure in stat() won't overwrite errno. Your call to keep it or not since it's also part of the "if" portion (although that path doesn't overwrite either). ACK - John
}
if (rc < 0) {
On Mon, Oct 12, 2015 at 12:25:48 +0200, Michal Privoznik wrote:
It's better if we stat() file that we are about to chown() at first and check if there's something we need to change. Not that it would make much difference, but for the upcoming patches we need to be doing stat() anyway.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 0dfe570..9b079e0 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -271,17 +271,18 @@ virSecurityDACSetOwnershipInternal(virSecurityDACDataPtr priv, path = src->path; }
+ if (stat(path, &sb) < 0) { + virReportSystemError(errno, _("unable to stat: %s"), path); + return -1; + }
I'd like to see a more specific error message in terms of what is happening at the point stat was called. Otherwise the message might be too confusing at this point since it might have failed due to lack of permissions actually. Peter
These stubs will be worked in later. They merely lay out the structure of the feature. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 9b079e0..9b53332 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -184,6 +184,51 @@ virSecurityDACGetImageIds(virSecurityLabelDefPtr seclabel, return 0; } +/** + * virSecurityDACRememberLabel: + * @priv: driver's private data + * @path: path to the file + * @uid: user owning the @path + * @gid: group owning the @path + * + * Remember the owner of @path (represented by @uid:@gid). + * + * Returns: 0 on success, -1 on failure + */ +static int +ATTRIBUTE_UNUSED +virSecurityDACRememberLabel(virSecurityDACDataPtr priv ATTRIBUTE_UNUSED, + const char *path ATTRIBUTE_UNUSED, + uid_t uid ATTRIBUTE_UNUSED, + gid_t gid ATTRIBUTE_UNUSED) +{ + return 0; +} + +/** + * virSecurityDACRecallLabel: + * @priv: driver's private data + * @path: path to the file + * @uid: user owning the @path + * @gid: group owning the @path + * + * Recall the previously recorded owner for the @path. However, it may happen + * that @path is still in use (e.g. by another domain). In that case, 1 is + * returned and caller should not relabel the @path. + * + * Returns: 1 if @path is still in use (@uid and @gid not touched) + * 0 if @path should be restored (@uid and @gid set) + * -1 on failure (@uid and @gid not touched) + */ +static int +ATTRIBUTE_UNUSED +virSecurityDACRecallLabel(virSecurityDACDataPtr priv ATTRIBUTE_UNUSED, + const char *path ATTRIBUTE_UNUSED, + uid_t *uid ATTRIBUTE_UNUSED, + gid_t *gid ATTRIBUTE_UNUSED) +{ + return 0; +} static virSecurityDriverStatus virSecurityDACProbe(const char *virtDriver ATTRIBUTE_UNUSED) -- 2.4.9
On 10/12/2015 06:25 AM, Michal Privoznik wrote:
These stubs will be worked in later. They merely lay out the structure of the feature.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 9b079e0..9b53332 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -184,6 +184,51 @@ virSecurityDACGetImageIds(virSecurityLabelDefPtr seclabel, return 0; }
+/** + * virSecurityDACRememberLabel: + * @priv: driver's private data + * @path: path to the file + * @uid: user owning the @path + * @gid: group owning the @path + * + * Remember the owner of @path (represented by @uid:@gid). + * + * Returns: 0 on success, -1 on failure + */ +static int +ATTRIBUTE_UNUSED
Not that they stick around long, but didn't we have a recent bug/issue with putting this in static functions? yeah - you wrote the patch about apibuild.py, but I see that has to do with ATTRIBUTE_NONNULL(1) I dunno, but at least I was paying attention to recent patches ;-) ACK (whether you need to remove or not) John
+virSecurityDACRememberLabel(virSecurityDACDataPtr priv ATTRIBUTE_UNUSED, + const char *path ATTRIBUTE_UNUSED, + uid_t uid ATTRIBUTE_UNUSED, + gid_t gid ATTRIBUTE_UNUSED) +{ + return 0; +} + +/** + * virSecurityDACRecallLabel: + * @priv: driver's private data + * @path: path to the file + * @uid: user owning the @path + * @gid: group owning the @path + * + * Recall the previously recorded owner for the @path. However, it may happen + * that @path is still in use (e.g. by another domain). In that case, 1 is + * returned and caller should not relabel the @path. + * + * Returns: 1 if @path is still in use (@uid and @gid not touched) + * 0 if @path should be restored (@uid and @gid set) + * -1 on failure (@uid and @gid not touched) + */ +static int +ATTRIBUTE_UNUSED +virSecurityDACRecallLabel(virSecurityDACDataPtr priv ATTRIBUTE_UNUSED, + const char *path ATTRIBUTE_UNUSED, + uid_t *uid ATTRIBUTE_UNUSED, + gid_t *gid ATTRIBUTE_UNUSED) +{ + return 0; +}
static virSecurityDriverStatus virSecurityDACProbe(const char *virtDriver ATTRIBUTE_UNUSED)
On Mon, Oct 12, 2015 at 12:25:49 +0200, Michal Privoznik wrote:
These stubs will be worked in later. They merely lay out the structure of the feature.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 9b079e0..9b53332 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -184,6 +184,51 @@ virSecurityDACGetImageIds(virSecurityLabelDefPtr seclabel, return 0; }
+/** + * virSecurityDACRememberLabel: + * @priv: driver's private data + * @path: path to the file + * @uid: user owning the @path + * @gid: group owning the @path + * + * Remember the owner of @path (represented by @uid:@gid). + * + * Returns: 0 on success, -1 on failure + */ +static int +ATTRIBUTE_UNUSED
I'm not a fan of unused static functions. Since the patch also doesn't contain any explanation in the commit message I'd suggest you merge it to the patch that calls the functions.
+virSecurityDACRememberLabel(virSecurityDACDataPtr priv ATTRIBUTE_UNUSED, + const char *path ATTRIBUTE_UNUSED,
I'm afraid you will eventually need a more complex data type than just a string. We are already doing DAC ownership manipulation on gluster volumes via the chown callback so we probably want to do the same thing there too at least for disk images.
+ uid_t uid ATTRIBUTE_UNUSED, + gid_t gid ATTRIBUTE_UNUSED) +{ + return 0; +}
Peter
On 16.10.2015 07:43, Peter Krempa wrote:
On Mon, Oct 12, 2015 at 12:25:49 +0200, Michal Privoznik wrote:
These stubs will be worked in later. They merely lay out the structure of the feature.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 9b079e0..9b53332 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -184,6 +184,51 @@ virSecurityDACGetImageIds(virSecurityLabelDefPtr seclabel, return 0; }
+/** + * virSecurityDACRememberLabel: + * @priv: driver's private data + * @path: path to the file + * @uid: user owning the @path + * @gid: group owning the @path + * + * Remember the owner of @path (represented by @uid:@gid). + * + * Returns: 0 on success, -1 on failure + */ +static int +ATTRIBUTE_UNUSED
I'm not a fan of unused static functions. Since the patch also doesn't contain any explanation in the commit message I'd suggest you merge it to the patch that calls the functions.
Fair enough.
+virSecurityDACRememberLabel(virSecurityDACDataPtr priv ATTRIBUTE_UNUSED, + const char *path ATTRIBUTE_UNUSED,
I'm afraid you will eventually need a more complex data type than just a string. We are already doing DAC ownership manipulation on gluster volumes via the chown callback so we probably want to do the same thing there too at least for disk images.
"more complex data type than just a string" - you mean for @path? Well, that one is not interpreted by virtlockd. So maybe we can store there "$domain.name:$disk.target" or something. However, since domain XML can change over time I rather avoid using disk targets as it's files (or gluster volumes) what we chown() really. Any bright idea? Michal
This is pure code adjustment. The structure is going to be needed later as it will hold a reference that will be used to talk to virtlockd. However, so far this is no functional change just code preparation. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 9b53332..d951e21 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -356,10 +356,13 @@ virSecurityDACSetOwnershipInternal(virSecurityDACDataPtr priv, static int -virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid) +virSecurityDACSetOwnership(virSecurityDACDataPtr priv, + const char *path, + uid_t uid, + gid_t gid) { /* XXX record previous ownership */ - return virSecurityDACSetOwnershipInternal(NULL, NULL, path, uid, gid); + return virSecurityDACSetOwnershipInternal(priv, NULL, path, uid, gid); } @@ -522,7 +525,7 @@ virSecurityDACSetSecurityHostdevLabelHelper(const char *file, if (virSecurityDACGetIds(secdef, priv, &user, &group, NULL, NULL)) return -1; - return virSecurityDACSetOwnership(file, user, group); + return virSecurityDACSetOwnership(priv, file, user, group); } @@ -816,7 +819,7 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, switch ((virDomainChrType) dev_source->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: - ret = virSecurityDACSetOwnership(dev_source->data.file.path, + ret = virSecurityDACSetOwnership(priv, dev_source->data.file.path, user, group); break; @@ -825,11 +828,11 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, (virAsprintf(&out, "%s.out", dev_source->data.file.path) < 0)) goto done; if (virFileExists(in) && virFileExists(out)) { - if ((virSecurityDACSetOwnership(in, user, group) < 0) || - (virSecurityDACSetOwnership(out, user, group) < 0)) { + if ((virSecurityDACSetOwnership(priv, in, user, group) < 0) || + (virSecurityDACSetOwnership(priv, out, user, group) < 0)) { goto done; } - } else if (virSecurityDACSetOwnership(dev_source->data.file.path, + } else if (virSecurityDACSetOwnership(priv, dev_source->data.file.path, user, group) < 0) { goto done; } @@ -838,7 +841,7 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, case VIR_DOMAIN_CHR_TYPE_UNIX: if (!dev_source->data.nix.listen) { - if (virSecurityDACSetOwnership(dev_source->data.nix.path, + if (virSecurityDACSetOwnership(priv, dev_source->data.nix.path, user, group) < 0) goto done; } @@ -1103,19 +1106,19 @@ virSecurityDACSetSecurityAllLabel(virSecurityManagerPtr mgr, return -1; if (def->os.loader && def->os.loader->nvram && - virSecurityDACSetOwnership(def->os.loader->nvram, user, group) < 0) + virSecurityDACSetOwnership(priv, def->os.loader->nvram, user, group) < 0) return -1; if (def->os.kernel && - virSecurityDACSetOwnership(def->os.kernel, user, group) < 0) + virSecurityDACSetOwnership(priv, def->os.kernel, user, group) < 0) return -1; if (def->os.initrd && - virSecurityDACSetOwnership(def->os.initrd, user, group) < 0) + virSecurityDACSetOwnership(priv, def->os.initrd, user, group) < 0) return -1; if (def->os.dtb && - virSecurityDACSetOwnership(def->os.dtb, user, group) < 0) + virSecurityDACSetOwnership(priv, def->os.dtb, user, group) < 0) return -1; return 0; @@ -1137,7 +1140,7 @@ virSecurityDACSetSavedStateLabel(virSecurityManagerPtr mgr, if (virSecurityDACGetImageIds(secdef, priv, &user, &group) < 0) return -1; - return virSecurityDACSetOwnership(savefile, user, group); + return virSecurityDACSetOwnership(priv, savefile, user, group); } @@ -1456,7 +1459,7 @@ virSecurityDACDomainSetDirLabel(virSecurityManagerPtr mgr, if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL) < 0) return -1; - return virSecurityDACSetOwnership(path, user, group); + return virSecurityDACSetOwnership(priv, path, user, group); } virSecurityDriver virSecurityDriverDAC = { -- 2.4.9
On Mon, Oct 12, 2015 at 12:25:50 +0200, Michal Privoznik wrote:
This is pure code adjustment. The structure is going to be needed later as it will hold a reference that will be used to talk to virtlockd. However, so far this is no functional change just code preparation.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-)
Since you are changing every caller you might as well as get rid of the 'virSecurityDACSetOwnership' wrapper completely and rename virSecurityDACSetOwnershipInternal. The original purpose of virSecurityDACSetOwnership was to avoid changing every caller. Peter
This is pure code adjustment. The structure is going to be needed later as it will hold a reference that will be used to talk to virtlockd. However, so far this is no functional change just code preparation. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 44 ++++++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 18 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index d951e21..ccd9261 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -380,9 +380,10 @@ virSecurityDACRestoreSecurityFileLabelInternal(virSecurityDACDataPtr priv, static int -virSecurityDACRestoreSecurityFileLabel(const char *path) +virSecurityDACRestoreSecurityFileLabel(virSecurityDACDataPtr priv, + const char *path) { - return virSecurityDACRestoreSecurityFileLabelInternal(NULL, NULL, path); + return virSecurityDACRestoreSecurityFileLabelInternal(priv, NULL, path); } @@ -664,27 +665,33 @@ virSecurityDACSetSecurityHostdevLabel(virSecurityManagerPtr mgr, static int virSecurityDACRestoreSecurityPCILabel(virPCIDevicePtr dev ATTRIBUTE_UNUSED, const char *file, - void *opaque ATTRIBUTE_UNUSED) + void *opaque) { - return virSecurityDACRestoreSecurityFileLabel(file); + virSecurityManagerPtr mgr = opaque; + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + return virSecurityDACRestoreSecurityFileLabel(priv, file); } static int virSecurityDACRestoreSecurityUSBLabel(virUSBDevicePtr dev ATTRIBUTE_UNUSED, const char *file, - void *opaque ATTRIBUTE_UNUSED) + void *opaque) { - return virSecurityDACRestoreSecurityFileLabel(file); + virSecurityManagerPtr mgr = opaque; + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + return virSecurityDACRestoreSecurityFileLabel(priv, file); } static int virSecurityDACRestoreSecuritySCSILabel(virSCSIDevicePtr dev ATTRIBUTE_UNUSED, const char *file, - void *opaque ATTRIBUTE_UNUSED) + void *opaque) { - return virSecurityDACRestoreSecurityFileLabel(file); + virSecurityManagerPtr mgr = opaque; + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + return virSecurityDACRestoreSecurityFileLabel(priv, file); } @@ -869,11 +876,12 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, } static int -virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, +virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr, virDomainDefPtr def ATTRIBUTE_UNUSED, virDomainChrDefPtr dev, virDomainChrSourceDefPtr dev_source) { + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityDeviceLabelDefPtr chr_seclabel = NULL; char *in = NULL, *out = NULL; int ret = -1; @@ -888,7 +896,7 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, switch ((virDomainChrType) dev_source->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: - ret = virSecurityDACRestoreSecurityFileLabel(dev_source->data.file.path); + ret = virSecurityDACRestoreSecurityFileLabel(priv, dev_source->data.file.path); break; case VIR_DOMAIN_CHR_TYPE_PIPE: @@ -896,11 +904,11 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, (virAsprintf(&in, "%s.in", dev_source->data.file.path) < 0)) goto done; if (virFileExists(in) && virFileExists(out)) { - if ((virSecurityDACRestoreSecurityFileLabel(out) < 0) || - (virSecurityDACRestoreSecurityFileLabel(in) < 0)) { + if ((virSecurityDACRestoreSecurityFileLabel(priv, out) < 0) || + (virSecurityDACRestoreSecurityFileLabel(priv, in) < 0)) { goto done; } - } else if (virSecurityDACRestoreSecurityFileLabel(dev_source->data.file.path) < 0) { + } else if (virSecurityDACRestoreSecurityFileLabel(priv, dev_source->data.file.path) < 0) { goto done; } ret = 0; @@ -1026,19 +1034,19 @@ virSecurityDACRestoreSecurityAllLabel(virSecurityManagerPtr mgr, } if (def->os.loader && def->os.loader->nvram && - virSecurityDACRestoreSecurityFileLabel(def->os.loader->nvram) < 0) + virSecurityDACRestoreSecurityFileLabel(priv, def->os.loader->nvram) < 0) rc = -1; if (def->os.kernel && - virSecurityDACRestoreSecurityFileLabel(def->os.kernel) < 0) + virSecurityDACRestoreSecurityFileLabel(priv, def->os.kernel) < 0) rc = -1; if (def->os.initrd && - virSecurityDACRestoreSecurityFileLabel(def->os.initrd) < 0) + virSecurityDACRestoreSecurityFileLabel(priv, def->os.initrd) < 0) rc = -1; if (def->os.dtb && - virSecurityDACRestoreSecurityFileLabel(def->os.dtb) < 0) + virSecurityDACRestoreSecurityFileLabel(priv, def->os.dtb) < 0) rc = -1; return rc; @@ -1154,7 +1162,7 @@ virSecurityDACRestoreSavedStateLabel(virSecurityManagerPtr mgr, if (!priv->dynamicOwnership) return 0; - return virSecurityDACRestoreSecurityFileLabel(savefile); + return virSecurityDACRestoreSecurityFileLabel(priv, savefile); } -- 2.4.9
On Mon, Oct 12, 2015 at 12:25:51 +0200, Michal Privoznik wrote:
This is pure code adjustment. The structure is going to be needed later as it will hold a reference that will be used to talk to virtlockd. However, so far this is no functional change just code preparation.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 44 ++++++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 18 deletions(-)
Same as with previous patch. virSecurityDACRestoreSecurityFileLabel was purely introduced to avoid changing all callers. It can be dropped since you modify the callers and virSecurityDACRestoreSecurityFileLabelInternal renamed. Peter
This function should really be called only when we want to change ownership of a file (or disk source). Lets switch to calling a wrapper function which will eventually record the current owner of the file and call virSecurityDACSetOwnershipInternal subsequently. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 36 ++++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index ccd9261..a38c46c 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -357,12 +357,13 @@ virSecurityDACSetOwnershipInternal(virSecurityDACDataPtr priv, static int virSecurityDACSetOwnership(virSecurityDACDataPtr priv, + virStorageSourcePtr src, const char *path, uid_t uid, gid_t gid) { /* XXX record previous ownership */ - return virSecurityDACSetOwnershipInternal(priv, NULL, path, uid, gid); + return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid); } @@ -418,7 +419,7 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr, return -1; } - return virSecurityDACSetOwnershipInternal(priv, src, NULL, user, group); + return virSecurityDACSetOwnership(priv, src, NULL, user, group); } @@ -526,7 +527,7 @@ virSecurityDACSetSecurityHostdevLabelHelper(const char *file, if (virSecurityDACGetIds(secdef, priv, &user, &group, NULL, NULL)) return -1; - return virSecurityDACSetOwnership(priv, file, user, group); + return virSecurityDACSetOwnership(priv, NULL, file, user, group); } @@ -826,7 +827,8 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, switch ((virDomainChrType) dev_source->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: - ret = virSecurityDACSetOwnership(priv, dev_source->data.file.path, + ret = virSecurityDACSetOwnership(priv, NULL, + dev_source->data.file.path, user, group); break; @@ -835,11 +837,12 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, (virAsprintf(&out, "%s.out", dev_source->data.file.path) < 0)) goto done; if (virFileExists(in) && virFileExists(out)) { - if ((virSecurityDACSetOwnership(priv, in, user, group) < 0) || - (virSecurityDACSetOwnership(priv, out, user, group) < 0)) { + if ((virSecurityDACSetOwnership(priv, NULL, in, user, group) < 0) || + (virSecurityDACSetOwnership(priv, NULL, out, user, group) < 0)) { goto done; } - } else if (virSecurityDACSetOwnership(priv, dev_source->data.file.path, + } else if (virSecurityDACSetOwnership(priv, NULL, + dev_source->data.file.path, user, group) < 0) { goto done; } @@ -848,7 +851,8 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, case VIR_DOMAIN_CHR_TYPE_UNIX: if (!dev_source->data.nix.listen) { - if (virSecurityDACSetOwnership(priv, dev_source->data.nix.path, + if (virSecurityDACSetOwnership(priv, NULL, + dev_source->data.nix.path, user, group) < 0) goto done; } @@ -1114,19 +1118,23 @@ virSecurityDACSetSecurityAllLabel(virSecurityManagerPtr mgr, return -1; if (def->os.loader && def->os.loader->nvram && - virSecurityDACSetOwnership(priv, def->os.loader->nvram, user, group) < 0) + virSecurityDACSetOwnership(priv, NULL, + def->os.loader->nvram, user, group) < 0) return -1; if (def->os.kernel && - virSecurityDACSetOwnership(priv, def->os.kernel, user, group) < 0) + virSecurityDACSetOwnership(priv, NULL, + def->os.kernel, user, group) < 0) return -1; if (def->os.initrd && - virSecurityDACSetOwnership(priv, def->os.initrd, user, group) < 0) + virSecurityDACSetOwnership(priv, NULL, + def->os.initrd, user, group) < 0) return -1; if (def->os.dtb && - virSecurityDACSetOwnership(priv, def->os.dtb, user, group) < 0) + virSecurityDACSetOwnership(priv, NULL, + def->os.dtb, user, group) < 0) return -1; return 0; @@ -1148,7 +1156,7 @@ virSecurityDACSetSavedStateLabel(virSecurityManagerPtr mgr, if (virSecurityDACGetImageIds(secdef, priv, &user, &group) < 0) return -1; - return virSecurityDACSetOwnership(priv, savefile, user, group); + return virSecurityDACSetOwnership(priv, NULL, savefile, user, group); } @@ -1467,7 +1475,7 @@ virSecurityDACDomainSetDirLabel(virSecurityManagerPtr mgr, if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL) < 0) return -1; - return virSecurityDACSetOwnership(priv, path, user, group); + return virSecurityDACSetOwnership(priv, NULL, path, user, group); } virSecurityDriver virSecurityDriverDAC = { -- 2.4.9
On Mon, Oct 12, 2015 at 12:25:52 +0200, Michal Privoznik wrote:
This function should really be called only when we want to change ownership of a file (or disk source). Lets switch to calling a wrapper function which will eventually record the current owner of the file and call virSecurityDACSetOwnershipInternal subsequently.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 36 ++++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 14 deletions(-)
Ah, you want to create a full wrapper so that you can extend it afterwards. Fair enough. ACK to the previous ones too. Peter
Even though the APIs are not implemented yet, they create a skeleton that can be filled in later. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 38 +++++++++++++++++++++++++++++++++----- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index a38c46c..6c4e351 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -196,7 +196,6 @@ virSecurityDACGetImageIds(virSecurityLabelDefPtr seclabel, * Returns: 0 on success, -1 on failure */ static int -ATTRIBUTE_UNUSED virSecurityDACRememberLabel(virSecurityDACDataPtr priv ATTRIBUTE_UNUSED, const char *path ATTRIBUTE_UNUSED, uid_t uid ATTRIBUTE_UNUSED, @@ -221,7 +220,6 @@ virSecurityDACRememberLabel(virSecurityDACDataPtr priv ATTRIBUTE_UNUSED, * -1 on failure (@uid and @gid not touched) */ static int -ATTRIBUTE_UNUSED virSecurityDACRecallLabel(virSecurityDACDataPtr priv ATTRIBUTE_UNUSED, const char *path ATTRIBUTE_UNUSED, uid_t *uid ATTRIBUTE_UNUSED, @@ -362,7 +360,22 @@ virSecurityDACSetOwnership(virSecurityDACDataPtr priv, uid_t uid, gid_t gid) { - /* XXX record previous ownership */ + struct stat sb; + + if (!path && src && src->path && + virStorageSourceIsLocalStorage(src)) + path = src->path; + + if (path) { + if (stat(path, &sb) < 0) { + virReportSystemError(errno, _("unable to stat: %s"), path); + return -1; + } + + if (virSecurityDACRememberLabel(priv, path, sb.st_uid, sb.st_gid) < 0) + return -1; + } + return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid); } @@ -372,11 +385,26 @@ virSecurityDACRestoreSecurityFileLabelInternal(virSecurityDACDataPtr priv, virStorageSourcePtr src, const char *path) { + int rv; + uid_t uid = 0; /* By default return to root:root */ + gid_t gid = 0; + VIR_INFO("Restoring DAC user and group on '%s'", NULLSTR(src ? src->path : path)); - /* XXX recall previous ownership */ - return virSecurityDACSetOwnershipInternal(priv, src, path, 0, 0); + if (!path && src && src->path && + virStorageSourceIsLocalStorage(src)) + path = src->path; + + if (path) { + rv = virSecurityDACRecallLabel(priv, path, &uid, &gid); + if (rv < 0) + return -1; + if (rv > 0) + return 0; + } + + return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid); } -- 2.4.9
On 10/12/2015 06:25 AM, Michal Privoznik wrote:
Even though the APIs are not implemented yet, they create a skeleton that can be filled in later.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 38 +++++++++++++++++++++++++++++++++----- 1 file changed, 33 insertions(+), 5 deletions(-)
ACK 5 - 8 John (I'll continue with this tomorrow - figured I'd at least get a start)
On Mon, Oct 12, 2015 at 12:25:53 +0200, Michal Privoznik wrote:
Even though the APIs are not implemented yet, they create a skeleton that can be filled in later.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 38 +++++++++++++++++++++++++++++++++----- 1 file changed, 33 insertions(+), 5 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index a38c46c..6c4e351 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -196,7 +196,6 @@ virSecurityDACGetImageIds(virSecurityLabelDefPtr seclabel, * Returns: 0 on success, -1 on failure */ static int -ATTRIBUTE_UNUSED virSecurityDACRememberLabel(virSecurityDACDataPtr priv ATTRIBUTE_UNUSED, const char *path ATTRIBUTE_UNUSED, uid_t uid ATTRIBUTE_UNUSED,
I'd suggest you fold the addition of the stub into this patch. This patch either does not do much explaining so the separate commits don't make sense. Peter
These procedures will be used to store and bring back security labels. So far, the idea is that tuple (path, model, label) is enough. Well, certainly for DAC and SELinux. The functions are: VIR_LOCK_SPACE_PROTOCOL_PROC_REMEMBER_SECLABEL VIR_LOCK_SPACE_PROTOCOL_PROC_RECALL_SECLABEL Yeah, they really need that VIR_LOCK_SPACE_PROTOCOL_PROC prefix due to way we call gendispatch.pl. So the former will take the whole tuple and remember it. The latter will then take just pair of (path, model) and return label stored previously. Moreover, the return value of recall will be important: value greater than zero means @path is still in use, don't relabel it. Value of zero means @path is no longer used, and a negative value means an error (e.g. @path not found, OOM, etc.). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/lock_protocol-structs | 15 +++++++++++++++ src/locking/lock_daemon_dispatch.c | 21 +++++++++++++++++++++ src/locking/lock_protocol.x | 29 ++++++++++++++++++++++++++++- 3 files changed, 64 insertions(+), 1 deletion(-) diff --git a/src/lock_protocol-structs b/src/lock_protocol-structs index 8e8b84f..c45086b 100644 --- a/src/lock_protocol-structs +++ b/src/lock_protocol-structs @@ -43,6 +43,19 @@ struct virLockSpaceProtocolReleaseResourceArgs { struct virLockSpaceProtocolCreateLockSpaceArgs { virLockSpaceProtocolNonNullString path; }; +struct virLockSpaceProtocolRememberSeclabelArgs { + virLockSpaceProtocolNonNullString model; + virLockSpaceProtocolNonNullString path; + virLockSpaceProtocolNonNullString label; +}; +struct virLockSpaceProtocolRecallSeclabelArgs { + virLockSpaceProtocolNonNullString model; + virLockSpaceProtocolNonNullString path; +}; +struct virLockSpaceProtocolRecallSeclabelRet { + virLockSpaceProtocolString label; + u_int ret; +}; enum virLockSpaceProtocolProcedure { VIR_LOCK_SPACE_PROTOCOL_PROC_REGISTER = 1, VIR_LOCK_SPACE_PROTOCOL_PROC_RESTRICT = 2, @@ -52,4 +65,6 @@ enum virLockSpaceProtocolProcedure { VIR_LOCK_SPACE_PROTOCOL_PROC_ACQUIRE_RESOURCE = 6, VIR_LOCK_SPACE_PROTOCOL_PROC_RELEASE_RESOURCE = 7, VIR_LOCK_SPACE_PROTOCOL_PROC_CREATE_LOCKSPACE = 8, + VIR_LOCK_SPACE_PROTOCOL_PROC_REMEMBER_SECLABEL = 9, + VIR_LOCK_SPACE_PROTOCOL_PROC_RECALL_SECLABEL = 10, }; diff --git a/src/locking/lock_daemon_dispatch.c b/src/locking/lock_daemon_dispatch.c index 1b479db..2d0bd81 100644 --- a/src/locking/lock_daemon_dispatch.c +++ b/src/locking/lock_daemon_dispatch.c @@ -430,3 +430,24 @@ virLockSpaceProtocolDispatchCreateLockSpace(virNetServerPtr server ATTRIBUTE_UNU virMutexUnlock(&priv->lock); return rv; } + +static int +virLockSpaceProtocolDispatchRememberSeclabel(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED, + virLockSpaceProtocolRememberSeclabelArgs *args ATTRIBUTE_UNUSED) +{ + return 0; +} + +static int +virLockSpaceProtocolDispatchRecallSeclabel(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED, + virLockSpaceProtocolRecallSeclabelArgs *args ATTRIBUTE_UNUSED, + virLockSpaceProtocolRecallSeclabelRet *ret ATTRIBUTE_UNUSED) +{ + return 0; +} diff --git a/src/locking/lock_protocol.x b/src/locking/lock_protocol.x index a77a784..bac4f0c 100644 --- a/src/locking/lock_protocol.x +++ b/src/locking/lock_protocol.x @@ -71,6 +71,21 @@ struct virLockSpaceProtocolCreateLockSpaceArgs { virLockSpaceProtocolNonNullString path; }; +struct virLockSpaceProtocolRememberSeclabelArgs { + virLockSpaceProtocolNonNullString model; + virLockSpaceProtocolNonNullString path; + virLockSpaceProtocolNonNullString label; +}; + +struct virLockSpaceProtocolRecallSeclabelArgs { + virLockSpaceProtocolNonNullString model; + virLockSpaceProtocolNonNullString path; +}; + +struct virLockSpaceProtocolRecallSeclabelRet { + virLockSpaceProtocolString label; + unsigned int ret; +}; /* Define the program number, protocol version and procedure numbers here. */ const VIR_LOCK_SPACE_PROTOCOL_PROGRAM = 0xEA7BEEF; @@ -149,5 +164,17 @@ enum virLockSpaceProtocolProcedure { * @generate: none * @acl: none */ - VIR_LOCK_SPACE_PROTOCOL_PROC_CREATE_LOCKSPACE = 8 + VIR_LOCK_SPACE_PROTOCOL_PROC_CREATE_LOCKSPACE = 8, + + /** + * @generate: none + * @acl: none + */ + VIR_LOCK_SPACE_PROTOCOL_PROC_REMEMBER_SECLABEL = 9, + + /** + * @generate: none + * @acl: none + */ + VIR_LOCK_SPACE_PROTOCOL_PROC_RECALL_SECLABEL = 10 }; -- 2.4.9
On Mon, Oct 12, 2015 at 12:25:54 +0200, Michal Privoznik wrote:
These procedures will be used to store and bring back security labels. So far, the idea is that tuple (path, model, label) is enough. Well, certainly for DAC and SELinux. The functions are:
I'm afraid that a 'path' per-se isn't enough to express storage locations in a unique way. Said that a string is fine though, but I'd rather call it 'identifier' or something similar and explicitly document some formats for possibly other storage systems that might use security labeling but don't have a local representation in the host. I'd imagine something like: STORAGE_SYSTEM_UUID:VOLUME_UUID and perhaps HOST_UUID:/path for local files One other thing to consider is that if this approach will be used across multiple hosts the paths although equal string-wise might not result being the same file. Not honoring that would result into security implications.
VIR_LOCK_SPACE_PROTOCOL_PROC_REMEMBER_SECLABEL VIR_LOCK_SPACE_PROTOCOL_PROC_RECALL_SECLABEL
Yeah, they really need that VIR_LOCK_SPACE_PROTOCOL_PROC prefix due to way we call gendispatch.pl.
So the former will take the whole tuple and remember it. The latter will then take just pair of (path, model) and return label stored previously. Moreover, the return value of recall will be important: value greater than zero means @path is still in use, don't relabel it. Value of zero means @path is no longer used, and a negative value means an error (e.g. @path not found, OOM, etc.).
I wanted to suggest other constraints but found out that they are actually documented in the patch that adds the API for this RPC. Peter
On 16.10.2015 09:17, Peter Krempa wrote:
On Mon, Oct 12, 2015 at 12:25:54 +0200, Michal Privoznik wrote:
These procedures will be used to store and bring back security labels. So far, the idea is that tuple (path, model, label) is enough. Well, certainly for DAC and SELinux. The functions are:
I'm afraid that a 'path' per-se isn't enough to express storage locations in a unique way. Said that a string is fine though, but I'd rather call it 'identifier' or something similar and explicitly document some formats for possibly other storage systems that might use security labeling but don't have a local representation in the host.
I'd imagine something like: STORAGE_SYSTEM_UUID:VOLUME_UUID
I can imagine this, okay.
and perhaps HOST_UUID:/path for local files
Hm... what good it will have to store UUID among with the path? I mean, we can't know if two paths from two different hosts is the same file or not anyway.
One other thing to consider is that if this approach will be used across multiple hosts the paths although equal string-wise might not result being the same file. Not honoring that would result into security implications.
What security implications do you have in mind? On the other hand, I just realized that this approach will not fly. I mean, virtlockd is running per host. So consider a file on NFS share, and to make things more complicated assume it's accessible under different paths from two different hosts. There is still a race between those two virtlockd-s - they will not mutually exclude each other and therefore can store already chown()-ed owership info. For instance: host1: stat() host1: store ownership (e.g. 100:20) host1: chown (e.g. to 20:20) host2: stat() host2: store ownership (20:20) host1: recall ownership (100:20) host1: restore ownership (100:20) host2: recall ownership (20:20) host2: restore ownership (20:20) Both hosts think that they are the last ones to use the file, therefore they restore ownerships. However, after all the original ownership is not preserved. One way out of this could be that we already require virtlockd in order to work properly that lockspace is on a shared mountpoint accessible by both hosts. So instead of keeping seclabel space in memory, we can load & store it within a file on that mountpoint and guard accesses via flock(). But this is rather unpleasant as we would have to parse and format the whole seclabel space just to update a refcounter to some entry. The other approach would be to have a seclabel space as a directory on the shared mountpoint and create some file (with internal structure) per path remembered. The internal structure of the file would then contain tuple (model, seclabel, refcount) for each model. When updating the refcount, only the file would need to be flock()-ed getting us much higher throughput. But we would still need to parse and format a file on each access (even though the file would be much smaller). I'm out of ideas here. Anybody has some? Michal
On 10/16/2015 10:54 AM, Michal Privoznik wrote:
On 16.10.2015 09:17, Peter Krempa wrote:
On Mon, Oct 12, 2015 at 12:25:54 +0200, Michal Privoznik wrote:
These procedures will be used to store and bring back security labels. So far, the idea is that tuple (path, model, label) is enough. Well, certainly for DAC and SELinux. The functions are:
I'm afraid that a 'path' per-se isn't enough to express storage locations in a unique way. Said that a string is fine though, but I'd rather call it 'identifier' or something similar and explicitly document some formats for possibly other storage systems that might use security labeling but don't have a local representation in the host.
I'd imagine something like: STORAGE_SYSTEM_UUID:VOLUME_UUID
I can imagine this, okay.
and perhaps HOST_UUID:/path for local files
Hm... what good it will have to store UUID among with the path? I mean, we can't know if two paths from two different hosts is the same file or not anyway.
One other thing to consider is that if this approach will be used across multiple hosts the paths although equal string-wise might not result being the same file. Not honoring that would result into security implications.
What security implications do you have in mind?
On the other hand, I just realized that this approach will not fly. I mean, virtlockd is running per host. So consider a file on NFS share, and to make things more complicated assume it's accessible under different paths from two different hosts. There is still a race between those two virtlockd-s - they will not mutually exclude each other and therefore can store already chown()-ed owership info. For instance:
host1: stat() host1: store ownership (e.g. 100:20) host1: chown (e.g. to 20:20)
host2: stat() host2: store ownership (20:20)
host1: recall ownership (100:20) host1: restore ownership (100:20)
host2: recall ownership (20:20) host2: restore ownership (20:20)
Both hosts think that they are the last ones to use the file, therefore they restore ownerships. However, after all the original ownership is not preserved.
Ah - the age old problem of networked and shared storage... And it's compounded by two hosts that don't know they are modifying the same resource. If realistically the server is the 'only' one to know, then who's problem is this to solve? Say nothing of choosing NFS... For this one you have to also consider root_squash vs. non root squash environment. The attempt to change anything is managed by the server, but for the sake of your argument it's set up to allow "no_root_squash" - meaning clients can have more control... In your scenario, H2 never chown()'s... So was it trying set to (20:20) - in which case, why would it do that since the stat it would get would show (20:20)... Or does it try to set say (40:20). If the latter, then H1 is going to have access problems as soon as H2 is successful. If H1 then goes to restore and finds the current stat of (40:20), but it believes it set (20:20), then should it restore? Or does it "try to" restore anyway and then let virtlockd hold onto the restore until H2 is done? Eventually H2 will restore to (20:20) at which time whatever is waiting to restore H1's value finds the (20:20) and can finally successfully restore back to (100:20)... Conversely if H2 did save and chown (20:20), but when it goes to restore (20:20) and finds (100:20), you wouldn't want some thread hanging around... However, in this case if the "store" == "restore" and the current is something else, then perhaps the restore gets dropped. Maybe a few drinks will help the thought process more ;-) John
One way out of this could be that we already require virtlockd in order to work properly that lockspace is on a shared mountpoint accessible by both hosts. So instead of keeping seclabel space in memory, we can load & store it within a file on that mountpoint and guard accesses via flock(). But this is rather unpleasant as we would have to parse and format the whole seclabel space just to update a refcounter to some entry.
The other approach would be to have a seclabel space as a directory on the shared mountpoint and create some file (with internal structure) per path remembered. The internal structure of the file would then contain tuple (model, seclabel, refcount) for each model. When updating the refcount, only the file would need to be flock()-ed getting us much higher throughput. But we would still need to parse and format a file on each access (even though the file would be much smaller).
I'm out of ideas here. Anybody has some?
Michal
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 16.10.2015 21:58, John Ferlan wrote:
On 10/16/2015 10:54 AM, Michal Privoznik wrote:
On 16.10.2015 09:17, Peter Krempa wrote:
On Mon, Oct 12, 2015 at 12:25:54 +0200, Michal Privoznik wrote:
These procedures will be used to store and bring back security labels. So far, the idea is that tuple (path, model, label) is enough. Well, certainly for DAC and SELinux. The functions are:
I'm afraid that a 'path' per-se isn't enough to express storage locations in a unique way. Said that a string is fine though, but I'd rather call it 'identifier' or something similar and explicitly document some formats for possibly other storage systems that might use security labeling but don't have a local representation in the host.
I'd imagine something like: STORAGE_SYSTEM_UUID:VOLUME_UUID
I can imagine this, okay.
and perhaps HOST_UUID:/path for local files
Hm... what good it will have to store UUID among with the path? I mean, we can't know if two paths from two different hosts is the same file or not anyway.
One other thing to consider is that if this approach will be used across multiple hosts the paths although equal string-wise might not result being the same file. Not honoring that would result into security implications.
What security implications do you have in mind?
On the other hand, I just realized that this approach will not fly. I mean, virtlockd is running per host. So consider a file on NFS share, and to make things more complicated assume it's accessible under different paths from two different hosts. There is still a race between those two virtlockd-s - they will not mutually exclude each other and therefore can store already chown()-ed owership info. For instance:
host1: stat() host1: store ownership (e.g. 100:20) host1: chown (e.g. to 20:20)
host2: stat() host2: store ownership (20:20)
host2: chown (e.g. to 20:40)
host1: recall ownership (100:20) host1: restore ownership (100:20)
(this will cut off host2)
host2: recall ownership (20:20) host2: restore ownership (20:20)
Both hosts think that they are the last ones to use the file, therefore they restore ownerships. However, after all the original ownership is not preserved.
Ah - the age old problem of networked and shared storage... And it's compounded by two hosts that don't know they are modifying the same resource. If realistically the server is the 'only' one to know, then who's problem is this to solve?
Say nothing of choosing NFS... For this one you have to also consider root_squash vs. non root squash environment. The attempt to change anything is managed by the server, but for the sake of your argument it's set up to allow "no_root_squash" - meaning clients can have more control...
In your scenario, H2 never chown()'s... So was it trying set to (20:20) - in which case, why would it do that since the stat it would get would show (20:20)... Or does it try to set say (40:20). If the latter, then H1 is going to have access problems as soon as H2 is successful. If H1 then goes to restore and finds the current stat of (40:20), but it believes it set (20:20), then should it restore? Or does it "try to" restore anyway and then let virtlockd hold onto the restore until H2 is done? Eventually H2 will restore to (20:20) at which time whatever is waiting to restore H1's value finds the (20:20) and can finally successfully restore back to (100:20)... Conversely if H2 did save and chown (20:20), but when it goes to restore (20:20) and finds (100:20), you wouldn't want some thread hanging around... However, in this case if the "store" == "restore" and the current is something else, then perhaps the restore gets dropped.
Oh, yeah. H2 should chown() too. But not necessarily cutting off H1 (it still can have access via user while H2 via group part of the permission). And we should not consider just DAC labels here. The framework should be capable of storing SELinux labels too. In that world, different labels (or context ranges) do not necessarily mean EPERM like in DAC. Therefore I think we should not check if current label is the one we set earlier. Moreover, even if we do check on restore if the seclabel has changed since the time we set it and if it has do not restore - in my scenario the file would has the wrong ownership at the end anyway, wouldn't it? I mean, at restore, h1 finds the seclabel has changed (it's 20:40 and it has set it to 20:20), so it leaves the file alone. Then, h2 comes around and finds the seclabel has NOT changed, so it restores it to what it thinks was the original label (20:20).
Maybe a few drinks will help the thought process more ;-)
John
One way out of this could be that we already require virtlockd in order to work properly that lockspace is on a shared mountpoint accessible by both hosts. So instead of keeping seclabel space in memory, we can load & store it within a file on that mountpoint and guard accesses via flock(). But this is rather unpleasant as we would have to parse and format the whole seclabel space just to update a refcounter to some entry.
The other approach would be to have a seclabel space as a directory on the shared mountpoint and create some file (with internal structure) per path remembered. The internal structure of the file would then contain tuple (model, seclabel, refcount) for each model. When updating the refcount, only the file would need to be flock()-ed getting us much higher throughput. But we would still need to parse and format a file on each access (even though the file would be much smaller).
I've given this some thoughts and even after few beers (reaching Ballmer peak) it still seems like good idea. Michal
On 10/19/2015 01:49 AM, Michal Privoznik wrote:
On 16.10.2015 21:58, John Ferlan wrote:
On 10/16/2015 10:54 AM, Michal Privoznik wrote:
On 16.10.2015 09:17, Peter Krempa wrote:
On Mon, Oct 12, 2015 at 12:25:54 +0200, Michal Privoznik wrote:
These procedures will be used to store and bring back security labels. So far, the idea is that tuple (path, model, label) is enough. Well, certainly for DAC and SELinux. The functions are:
I'm afraid that a 'path' per-se isn't enough to express storage locations in a unique way. Said that a string is fine though, but I'd rather call it 'identifier' or something similar and explicitly document some formats for possibly other storage systems that might use security labeling but don't have a local representation in the host.
I'd imagine something like: STORAGE_SYSTEM_UUID:VOLUME_UUID
I can imagine this, okay.
and perhaps HOST_UUID:/path for local files
Hm... what good it will have to store UUID among with the path? I mean, we can't know if two paths from two different hosts is the same file or not anyway.
One other thing to consider is that if this approach will be used across multiple hosts the paths although equal string-wise might not result being the same file. Not honoring that would result into security implications.
What security implications do you have in mind?
On the other hand, I just realized that this approach will not fly. I mean, virtlockd is running per host. So consider a file on NFS share, and to make things more complicated assume it's accessible under different paths from two different hosts. There is still a race between those two virtlockd-s - they will not mutually exclude each other and therefore can store already chown()-ed owership info. For instance:
host1: stat() host1: store ownership (e.g. 100:20) host1: chown (e.g. to 20:20)
host2: stat() host2: store ownership (20:20)
host2: chown (e.g. to 20:40)
host1: recall ownership (100:20) host1: restore ownership (100:20)
(this will cut off host2)
host2: recall ownership (20:20) host2: restore ownership (20:20)
Both hosts think that they are the last ones to use the file, therefore they restore ownerships. However, after all the original ownership is not preserved.
Ah - the age old problem of networked and shared storage... And it's compounded by two hosts that don't know they are modifying the same resource. If realistically the server is the 'only' one to know, then who's problem is this to solve?
Say nothing of choosing NFS... For this one you have to also consider root_squash vs. non root squash environment. The attempt to change anything is managed by the server, but for the sake of your argument it's set up to allow "no_root_squash" - meaning clients can have more control...
In your scenario, H2 never chown()'s... So was it trying set to (20:20) - in which case, why would it do that since the stat it would get would show (20:20)... Or does it try to set say (40:20). If the latter, then H1 is going to have access problems as soon as H2 is successful. If H1 then goes to restore and finds the current stat of (40:20), but it believes it set (20:20), then should it restore? Or does it "try to" restore anyway and then let virtlockd hold onto the restore until H2 is done? Eventually H2 will restore to (20:20) at which time whatever is waiting to restore H1's value finds the (20:20) and can finally successfully restore back to (100:20)... Conversely if H2 did save and chown (20:20), but when it goes to restore (20:20) and finds (100:20), you wouldn't want some thread hanging around... However, in this case if the "store" == "restore" and the current is something else, then perhaps the restore gets dropped.
Oh, yeah. H2 should chown() too. But not necessarily cutting off H1 (it still can have access via user while H2 via group part of the permission). And we should not consider just DAC labels here. The framework should be capable of storing SELinux labels too. In that world, different labels (or context ranges) do not necessarily mean EPERM like in DAC. Therefore I think we should not check if current label is the one we set earlier.
Understood that my thoughts were DAC specific... Although a comparison routine for the specific driver could be called from a generic spot to determine if it's changed - it was just easier to type it thinking only DAC though. Also, consider that when using DAC with uid:gid mapping, the uid:gid on the client*s* would need to match the uid:gid on the server and the NFS server would need to be configured properly to allow client*s* to change. Otherwise, there's some perhaps unintended consequences.
Moreover, even if we do check on restore if the seclabel has changed since the time we set it and if it has do not restore - in my scenario the file would has the wrong ownership at the end anyway, wouldn't it? I mean, at restore, h1 finds the seclabel has changed (it's 20:40 and it has set it to 20:20), so it leaves the file alone. Then, h2 comes around and finds the seclabel has NOT changed, so it restores it to what it thinks was the original label (20:20).
The nuance for h1 is can it or should it 'wait' to find the label has reverted back to what it set it to? Where the wait thread lives I hadn't thought too much about, but perhaps would be "no different" than some other libvirt thread that waits for something and then sends an event when done. It's not a fool proof method for sure, but throwing spaghetti on the wall to see if anything sticks.
Maybe a few drinks will help the thought process more ;-)
John
One way out of this could be that we already require virtlockd in order to work properly that lockspace is on a shared mountpoint accessible by both hosts. So instead of keeping seclabel space in memory, we can load & store it within a file on that mountpoint and guard accesses via flock(). But this is rather unpleasant as we would have to parse and format the whole seclabel space just to update a refcounter to some entry.
The other approach would be to have a seclabel space as a directory on the shared mountpoint and create some file (with internal structure) per path remembered. The internal structure of the file would then contain tuple (model, seclabel, refcount) for each model. When updating the refcount, only the file would need to be flock()-ed getting us much higher throughput. But we would still need to parse and format a file on each access (even though the file would be much smaller).
I've given this some thoughts and even after few beers (reaching Ballmer peak) it still seems like good idea.
So a solution to the NFS problem (or other such shared filesystem) is to carve out space on that file system? If it's NFS and root squashed - under which uid:gid does that happen? How "large" is it? How do you "guarantee" something doesn't come along and reap it or change it's access? I would think it should be up to the networked file system to provide the mechanism for secure access, change, etc. IOW: Is this really libvirt's problem to solve? I'm not sure every nuance of every target server file system could be dealt with properly by some generic method. If someone wants to use NFS and they want this rollback, can we convince them to run virtlockd on their NFS Server? And then use that connection in order to manage things if they really care about this label rollback feature? Are we building something too complex to use and/or describe? Do we really want to be the "owner" of the security problem? John
On 10/12/2015 06:25 AM, Michal Privoznik wrote:
These procedures will be used to store and bring back security labels. So far, the idea is that tuple (path, model, label) is enough. Well, certainly for DAC and SELinux. The functions are:
VIR_LOCK_SPACE_PROTOCOL_PROC_REMEMBER_SECLABEL VIR_LOCK_SPACE_PROTOCOL_PROC_RECALL_SECLABEL
Yeah, they really need that VIR_LOCK_SPACE_PROTOCOL_PROC prefix due to way we call gendispatch.pl.
So the former will take the whole tuple and remember it. The latter will then take just pair of (path, model) and return label stored previously. Moreover, the return value of recall will be important: value greater than zero means @path is still in use, don't relabel it. Value of zero means @path is no longer used, and a negative value means an error (e.g. @path not found, OOM, etc.).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/lock_protocol-structs | 15 +++++++++++++++ src/locking/lock_daemon_dispatch.c | 21 +++++++++++++++++++++ src/locking/lock_protocol.x | 29 ++++++++++++++++++++++++++++- 3 files changed, 64 insertions(+), 1 deletion(-)
Disclaimer: My working knowledge of seclabels is limited. I know what they are, but never dug into the details. They remind me of an OpenVMS technology that I knew quite well years ago (known as ACL's)... However, it seems in general they are passed around as character strings, so that's my working idea... As I started thinking about this - essentially you're looking to generate a object in virtlockd that has a unique name for which you'd like to store/save of some character string context so that at some point in time that context could be restored. Without looking too far ahead, I'm assuming this concept is to handle the 'dynamic' seclabel type's and the static relabel='yes' types in order to allow a relabel after libvirt is done. So you're then saving the "model" (dac, selinux, apparmor) and provided "label" (specific to the model). I think Peter covered some issues around the 'path' which could/should be able to describe the 'identifier' object. Again, not looking forward and just thinking in terms of what's in a "<seclabel.../>"... Since it's possible to have multiple seclabels for each 'path'/'identifier' - I assume you must have built in some logic to first search on the identifier, then if found using the model in order to determine if something was already defined for that. If so, you're returning '1' for already in use; otherwise, you create and return '0' on success and '-1' on failure. So in a way, the code would seemingly already have a way to take a unique object name and search to determine if the 'payload' type already exists and make decisions from there. Since you've set @generate to none that says to me the virtlockd would then have some "built-in" knowledge about the format of what is being received so that it can make decisions about what to return. That got me to thinking about other uses... Although the design center is security labels for disks and the naming is centered around that, it could be a security label for anything, correct? Or thinking more generically, perhaps a MAC for a network? Some network filter? A secret for something? Or perhaps a resource (such as disk) which is currently being used by a pool in which case we don't want some other pool to use the same resource (yes, I have a bz on that and IIRC there's a different one dealing with disk usage across domains). Although with multiple uses of the same "object by name", it seems some sort of tagging on the payload would be required and a way to handle the "I don't know this type of 'payload'". I guess what I'm thinking we should avoid is code copy-n-paste bloat for the "next" remember/recall item. Although yes, easier to debug and perhaps easier to design if we don't have to think about other object uses or the future - I would think we have enough current examples that we could come up with something generic. In the end perhaps something along the lines of for remember. { *String unique_id; /* char string to uniquely id the object */ int payload; /* Payload type - enum {SECLABEL, ...} */ *String object1; /* Currently "model" - payload specific search */ *String object2; /* Currently "label" - whatever it is to store */ } John
diff --git a/src/lock_protocol-structs b/src/lock_protocol-structs index 8e8b84f..c45086b 100644 --- a/src/lock_protocol-structs +++ b/src/lock_protocol-structs @@ -43,6 +43,19 @@ struct virLockSpaceProtocolReleaseResourceArgs { struct virLockSpaceProtocolCreateLockSpaceArgs { virLockSpaceProtocolNonNullString path; }; +struct virLockSpaceProtocolRememberSeclabelArgs { + virLockSpaceProtocolNonNullString model; + virLockSpaceProtocolNonNullString path; + virLockSpaceProtocolNonNullString label; +}; +struct virLockSpaceProtocolRecallSeclabelArgs { + virLockSpaceProtocolNonNullString model; + virLockSpaceProtocolNonNullString path; +}; +struct virLockSpaceProtocolRecallSeclabelRet { + virLockSpaceProtocolString label; + u_int ret; +}; enum virLockSpaceProtocolProcedure { VIR_LOCK_SPACE_PROTOCOL_PROC_REGISTER = 1, VIR_LOCK_SPACE_PROTOCOL_PROC_RESTRICT = 2, @@ -52,4 +65,6 @@ enum virLockSpaceProtocolProcedure { VIR_LOCK_SPACE_PROTOCOL_PROC_ACQUIRE_RESOURCE = 6, VIR_LOCK_SPACE_PROTOCOL_PROC_RELEASE_RESOURCE = 7, VIR_LOCK_SPACE_PROTOCOL_PROC_CREATE_LOCKSPACE = 8, + VIR_LOCK_SPACE_PROTOCOL_PROC_REMEMBER_SECLABEL = 9, + VIR_LOCK_SPACE_PROTOCOL_PROC_RECALL_SECLABEL = 10, }; diff --git a/src/locking/lock_daemon_dispatch.c b/src/locking/lock_daemon_dispatch.c index 1b479db..2d0bd81 100644 --- a/src/locking/lock_daemon_dispatch.c +++ b/src/locking/lock_daemon_dispatch.c @@ -430,3 +430,24 @@ virLockSpaceProtocolDispatchCreateLockSpace(virNetServerPtr server ATTRIBUTE_UNU virMutexUnlock(&priv->lock); return rv; } + +static int +virLockSpaceProtocolDispatchRememberSeclabel(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED, + virLockSpaceProtocolRememberSeclabelArgs *args ATTRIBUTE_UNUSED) +{ + return 0; +} + +static int +virLockSpaceProtocolDispatchRecallSeclabel(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED, + virLockSpaceProtocolRecallSeclabelArgs *args ATTRIBUTE_UNUSED, + virLockSpaceProtocolRecallSeclabelRet *ret ATTRIBUTE_UNUSED) +{ + return 0; +} diff --git a/src/locking/lock_protocol.x b/src/locking/lock_protocol.x index a77a784..bac4f0c 100644 --- a/src/locking/lock_protocol.x +++ b/src/locking/lock_protocol.x @@ -71,6 +71,21 @@ struct virLockSpaceProtocolCreateLockSpaceArgs { virLockSpaceProtocolNonNullString path; };
+struct virLockSpaceProtocolRememberSeclabelArgs { + virLockSpaceProtocolNonNullString model; + virLockSpaceProtocolNonNullString path; + virLockSpaceProtocolNonNullString label; +}; + +struct virLockSpaceProtocolRecallSeclabelArgs { + virLockSpaceProtocolNonNullString model; + virLockSpaceProtocolNonNullString path; +}; + +struct virLockSpaceProtocolRecallSeclabelRet { + virLockSpaceProtocolString label; + unsigned int ret; +};
/* Define the program number, protocol version and procedure numbers here. */ const VIR_LOCK_SPACE_PROTOCOL_PROGRAM = 0xEA7BEEF; @@ -149,5 +164,17 @@ enum virLockSpaceProtocolProcedure { * @generate: none * @acl: none */ - VIR_LOCK_SPACE_PROTOCOL_PROC_CREATE_LOCKSPACE = 8 + VIR_LOCK_SPACE_PROTOCOL_PROC_CREATE_LOCKSPACE = 8, + + /** + * @generate: none + * @acl: none + */ + VIR_LOCK_SPACE_PROTOCOL_PROC_REMEMBER_SECLABEL = 9, + + /** + * @generate: none + * @acl: none + */ + VIR_LOCK_SPACE_PROTOCOL_PROC_RECALL_SECLABEL = 10 };
Lets use wrapper functions virLockDaemonLock and virLockDaemonUnlock instead of virMutexLock and virMutexUnlock. This has no functional impact, but it's easier to read (at least for me). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_daemon.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index 56db959..3984b4d 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -125,6 +125,17 @@ virLockDaemonFree(virLockDaemonPtr lockd) VIR_FREE(lockd); } +static void +virLockDaemonLock(virLockDaemonPtr lockd) +{ + virMutexLock(&lockd->lock); +} + +static void +virLockDaemonUnlock(virLockDaemonPtr lockd) +{ + virMutexUnlock(&lockd->lock); +} static void virLockDaemonLockSpaceDataFree(void *data, const void *key ATTRIBUTE_UNUSED) @@ -275,9 +286,9 @@ int virLockDaemonAddLockSpace(virLockDaemonPtr lockd, virLockSpacePtr lockspace) { int ret; - virMutexLock(&lockd->lock); + virLockDaemonLock(lockd); ret = virHashAddEntry(lockd->lockspaces, path, lockspace); - virMutexUnlock(&lockd->lock); + virLockDaemonUnlock(lockd); return ret; } @@ -285,12 +296,12 @@ virLockSpacePtr virLockDaemonFindLockSpace(virLockDaemonPtr lockd, const char *path) { virLockSpacePtr lockspace; - virMutexLock(&lockd->lock); + virLockDaemonLock(lockd); if (path && STRNEQ(path, "")) lockspace = virHashLookup(lockd->lockspaces, path); else lockspace = lockd->defaultLockspace; - virMutexUnlock(&lockd->lock); + virLockDaemonUnlock(lockd); return lockspace; } @@ -675,14 +686,14 @@ virLockDaemonClientFree(void *opaque) /* Release all locks associated with this * owner in all lockspaces */ - virMutexLock(&lockDaemon->lock); + virLockDaemonLock(lockDaemon); virHashForEach(lockDaemon->lockspaces, virLockDaemonClientReleaseLockspace, &data); virLockDaemonClientReleaseLockspace(lockDaemon->defaultLockspace, "", &data); - virMutexUnlock(&lockDaemon->lock); + virLockDaemonUnlock(lockDaemon); /* If the client had some active leases when it * closed the connection, we must kill it off -- 2.4.9
On Mon, Oct 12, 2015 at 12:25:55 +0200, Michal Privoznik wrote:
Lets use wrapper functions virLockDaemonLock and virLockDaemonUnlock instead of virMutexLock and virMutexUnlock. This has no functional impact, but it's easier to read (at least for me).
Actually we removed such helpers in favor of virObjectLock in most places. On the other hand, converting a singleton to virObject is a waste of resources.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_daemon.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)
Reluctant ACK if you add a 'inline' keyword. Peter
This module will handle all the security label remembering and recalling for virlockd. We need to record a tuple of (path, security model, security label). So I'm going with a hash table, where @path would be the key, and tuple of (model, label) is going to be the payload. Well, the payload tuple is wrapped into self inflating array (virSeclabelSpaceLabels). So for each path, we will have to traverse through array of various models to find the needed one. But since there's gonna be many paths and just a few security models, it's going to be the best approach. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- po/POTFILES.in | 1 + src/Makefile.am | 2 + src/locking/lock_daemon_seclabels.c | 545 ++++++++++++++++++++++++++++++++++++ src/locking/lock_daemon_seclabels.h | 43 +++ 4 files changed, 591 insertions(+) create mode 100644 src/locking/lock_daemon_seclabels.c create mode 100644 src/locking/lock_daemon_seclabels.h diff --git a/po/POTFILES.in b/po/POTFILES.in index 0cc5b99..04dbad3 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -74,6 +74,7 @@ src/libvirt-qemu.c src/locking/lock_daemon.c src/locking/lock_daemon_config.c src/locking/lock_daemon_dispatch.c +src/locking/lock_daemon_seclabels.c src/locking/lock_driver_lockd.c src/locking/lock_driver_sanlock.c src/locking/lock_manager.c diff --git a/src/Makefile.am b/src/Makefile.am index d5dd9fb..e706166 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -253,6 +253,8 @@ LOCK_DAEMON_SOURCES = \ locking/lock_daemon_config.c \ locking/lock_daemon_dispatch.c \ locking/lock_daemon_dispatch.h \ + locking/lock_daemon_seclabels.c \ + locking/lock_daemon_seclabels.h \ $(NULL) locking/lock_daemon_dispatch_stubs.h: $(LOCK_PROTOCOL) \ diff --git a/src/locking/lock_daemon_seclabels.c b/src/locking/lock_daemon_seclabels.c new file mode 100644 index 0000000..8c3e4a0 --- /dev/null +++ b/src/locking/lock_daemon_seclabels.c @@ -0,0 +1,545 @@ +/* + * lock_daemon_seclabels.c: Security label mgmt + * + * Copyright (C) 2015 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Michal Privoznik <mprivozn@redhat.com> + */ + +#include <config.h> + +#include "viralloc.h" +#include "virerror.h" +#include "virhash.h" +#include "virjson.h" +#include "virlog.h" +#include "virstring.h" + +#define VIR_FROM_THIS VIR_FROM_RPC + +VIR_LOG_INIT("locking.lock_daemon_dispatch"); + +#include "lock_daemon.h" +#include "lock_daemon_seclabels.h" + +typedef struct _virSeclabel virSeclabel; +typedef virSeclabel *virSeclabelPtr; + +struct _virSeclabel { + char *model; + char *label; + unsigned int refcount; +}; + + +typedef struct _virSeclabelSpaceLabels virSeclabelSpaceLabels; +typedef virSeclabelSpaceLabels *virSeclabelSpaceLabelsPtr; + +struct _virSeclabelSpaceLabels { + virSeclabelPtr *labels; + size_t nlabels; +}; + +struct _virSeclabelSpace { + virMutex lock; + + virHashTablePtr labels; +}; + + +static void +virSeclabelFree(virSeclabelPtr label) +{ + if (!label) + return; + + VIR_FREE(label->model); + VIR_FREE(label->label); + VIR_FREE(label); +} + + +static virSeclabelPtr +virSeclabelNew(const char *model, + const char *label) +{ + virSeclabelPtr ret; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + if (VIR_STRDUP(ret->model, model) < 0 || + VIR_STRDUP(ret->label, label) < 0) + goto error; + + ret->refcount = 1; + + return ret; + error: + virSeclabelFree(ret); + return NULL; +} + + +static void +virSeclabelSpaceLabelsFree(virSeclabelSpaceLabelsPtr labels) +{ + size_t i; + + if (!labels) + return; + + for (i = 0; i < labels->nlabels; i++) + virSeclabelFree(labels->labels[i]); + + VIR_FREE(labels->labels); + VIR_FREE(labels); +} + +static void +virSeclabelSpaceLabelsHashFree(void *payload, + const void *name ATTRIBUTE_UNUSED) +{ + virSeclabelSpaceLabelsFree(payload); +} + + +static void +virSeclabelSpaceLock(virSeclabelSpacePtr space) +{ + virMutexLock(&space->lock); +} + + +static void +virSeclabelSpaceUnlock(virSeclabelSpacePtr space) +{ + virMutexUnlock(&space->lock); +} + + +void +virSeclabelSpaceFree(virSeclabelSpacePtr space) +{ + if (!space) + return; + + virMutexDestroy(&space->lock); + virHashFree(space->labels); + VIR_FREE(space); +} + + +virSeclabelSpacePtr +virSeclabelSpaceNew(void) +{ + virSeclabelSpacePtr ret; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + if (virMutexInit(&ret->lock) < 0) { + virReportSystemError(errno, "%s", + _("Unable to init mutex")); + goto error; + } + + if (!(ret->labels = virHashCreate(10, virSeclabelSpaceLabelsHashFree))) + goto error; + + return ret; + error: + virSeclabelSpaceFree(ret); + return NULL; +} + + +static virSeclabelPtr +virSeclabelSpaceLookup(virSeclabelSpacePtr space, + const char *path, + const char *model) +{ + virSeclabelSpaceLabelsPtr labels; + virSeclabelPtr ret; + size_t i; + + if (!(labels = virHashLookup(space->labels, path))) + return NULL; + + for (i = 0; labels->nlabels; i++) { + ret = labels->labels[i]; + + if (STREQ(ret->model, model)) + return ret; + } + + return NULL; +} + + +static int +virSeclabelSpaceAdd(virSeclabelSpacePtr space, + const char *path, + virSeclabelPtr label) +{ + virSeclabelSpaceLabelsPtr labels; + virSeclabelPtr tmp; + size_t i; + int ret = -1; + + if (!(labels = virHashLookup(space->labels, path))) { + /* Add new */ + if (VIR_ALLOC(labels) < 0 || + VIR_ALLOC(labels->labels) < 0 || + virHashAddEntry(space->labels, path, labels) < 0) { + virSeclabelSpaceLabelsFree(labels); + goto cleanup; + } + + labels->labels[0] = label; + labels->nlabels = 1; + } else { + /* Update old */ + for (i = 0; i < labels->nlabels; i++) { + tmp = labels->labels[i]; + + if (STREQ(tmp->model, label->model)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("duplicate label for model '%s': old: '%s' new '%s'"), + tmp->model, tmp->label, label->label); + goto cleanup; + } + } + + if (VIR_APPEND_ELEMENT_COPY(labels->labels, labels->nlabels, label) < 0) + goto cleanup; + + if (virHashUpdateEntry(space->labels, path, labels) < 0) + goto cleanup; + } + + ret = 0; + cleanup: + return ret; +} + + +static void +virSeclabelSpaceRemove(virSeclabelSpacePtr space, + const char *path, + virSeclabelPtr seclabel) +{ + virSeclabelSpaceLabelsPtr labels; + virSeclabelPtr tmp; + size_t i; + + if (!(labels = virHashLookup(space->labels, path))) { + /* path not found */ + return; + } + + for (i = 0; i < labels->nlabels; i++) { + tmp = labels->labels[i]; + + if (STREQ(seclabel->model, tmp->model)) + break; + } + + if (i == labels->nlabels) { + /* model not found */ + return; + } + + tmp = labels->labels[i]; + + VIR_DELETE_ELEMENT(labels->labels, i, labels->nlabels); + + if (!labels->nlabels) + virHashRemoveEntry(space->labels, path); +} + + +struct virSeclabelSpaceIteratorData { + virJSONValuePtr labels; + bool error; +}; + + +static void +virSeclabelSpaceIterator(void *payload, + const void *name, + void *opaque) +{ + virSeclabelSpaceLabelsPtr labels = payload; + const char *path = name; + struct virSeclabelSpaceIteratorData *data = opaque; + virSeclabelPtr tmp; + virJSONValuePtr item, jsonLabels, jsonLabel; + size_t i; + + if (data->error) + return; + + if (!(item = virJSONValueNewObject())) + goto error; + + if (virJSONValueObjectAppendString(item, "path", path) < 0) + goto error; + + if (!(jsonLabels = virJSONValueNewArray())) + goto error; + + for (i = 0; i < labels->nlabels; i++) { + tmp = labels->labels[i]; + + if (!(jsonLabel = virJSONValueNewObject())) + goto error; + + if (virJSONValueObjectAppendString(jsonLabel, "model", tmp->model) < 0 || + virJSONValueObjectAppendString(jsonLabel, "label", tmp->label) < 0 || + virJSONValueObjectAppendNumberUint(jsonLabel, "refcount", tmp->refcount) < 0) + goto error; + + if (virJSONValueArrayAppend(jsonLabels, jsonLabel) < 0) + goto error; + jsonLabel = NULL; + } + + if (virJSONValueObjectAppend(item, "labels", jsonLabels) < 0) + goto error; + jsonLabels = NULL; + + if (virJSONValueArrayAppend(data->labels, item) < 0) + goto error; + item = NULL; + + return; + error: + virJSONValueFree(jsonLabel); + virJSONValueFree(jsonLabels); + virJSONValueFree(item); + data->error = true; +} + + +/** + * virSeclabelSpacePreExecRestart: + * + * @space: object to dump + * + * Dumps virSeclabel object into JSON. + * + * Returns: a non-NULL pointer to JSON object on success, + * NULL on error + */ +virJSONValuePtr +virSeclabelSpacePreExecRestart(virSeclabelSpacePtr space) +{ + virJSONValuePtr object = NULL; + virJSONValuePtr labels = NULL; + struct virSeclabelSpaceIteratorData data; + + if (!space) + return NULL; + + virSeclabelSpaceLock(space); + + if (!(object = virJSONValueNewObject())) + return NULL; + + if (!(labels = virJSONValueNewArray())) + goto error; + + data.labels = labels; + data.error = false; + if (virHashForEach(space->labels, virSeclabelSpaceIterator, &data) < 0) + goto error; + + if (virJSONValueObjectAppend(object, "seclabels", labels) < 0) + goto error; + /* From now on, @labels is contained in @object. Avoid double freeing it. */ + labels = NULL; + + virSeclabelSpaceUnlock(space); + return object; + error: + virSeclabelSpaceUnlock(space); + virJSONValueFree(labels); + virJSONValueFree(object); + return NULL; +} + + +/** + * virSeclabelSpacePostExecRestart: + * + * @object: JSON representation of internal state + * + * Read in JSON object, create new virSeclabelSpace object and restore its internal state from JSON. + * + * Returns: virSeclabelSpace object + * NULL on error + */ +virSeclabelSpacePtr +virSeclabelSpacePostExecRestart(virJSONValuePtr object) +{ + virSeclabelSpacePtr ret; + virJSONValuePtr labels; + size_t i, npaths; + + if (!(ret = virSeclabelSpaceNew())) + return NULL; + + if (!(labels = virJSONValueObjectGetArray(object, "seclabels"))) + goto error; + + npaths = virJSONValueArraySize(labels); + for (i = 0; i < npaths; i++) { + virJSONValuePtr item = virJSONValueArrayGet(labels, i); + virJSONValuePtr arr = virJSONValueObjectGetArray(item, "labels"); + const char *path = virJSONValueObjectGetString(item, "path"); + size_t j, nlabels; + + nlabels = virJSONValueArraySize(arr); + + for (j = 0; j < nlabels; j++) { + virJSONValuePtr arr_item = virJSONValueArrayGet(arr, j); + const char *model = virJSONValueObjectGetString(arr_item, "model"); + const char *label = virJSONValueObjectGetString(arr_item, "label"); + virSeclabelPtr seclabel; + + if (!(seclabel = virSeclabelNew(model, label))) + goto error; + + if (virJSONValueObjectGetNumberUint(arr_item, + "refcount", + &seclabel->refcount) < 0) { + virSeclabelFree(seclabel); + goto error; + } + + if (virSeclabelSpaceAdd(ret, path, seclabel) < 0) { + virSeclabelFree(seclabel); + goto error; + } + } + } + + return ret; + error: + virSeclabelSpaceFree(ret); + return NULL; +} + + +/** + * virSeclabelSpaceRemember: + * + * @space: seclabel space object + * @path: path to remember + * @model: security model of @label ("selinux", "dac", etc.) + * @label: actual value to hold (e.g. "root:root") + * + * This function should remember the original label for a @path on the first + * call. Any subsequent call over the same @path and @model just increments the + * refcounter for the label (actual value of @label is ignored in this case). + * + * Returns: 0 on success + * -1 otherwise + */ +int +virSeclabelSpaceRemember(virSeclabelSpacePtr space, + const char *path, + const char *model, + const char *label) +{ + virSeclabelPtr seclabel; + int ret = -1; + + virSeclabelSpaceLock(space); + if ((seclabel = virSeclabelSpaceLookup(space, path, model))) { + /* We don't really care about label here. There already is an existing + * record for the @path and @model, so @label is likely to not have the + * original label anyway. */ + seclabel->refcount++; + ret = 0; + goto cleanup; + } + + if (!(seclabel = virSeclabelNew(model, label))) + goto cleanup; + + if (virSeclabelSpaceAdd(space, path, seclabel) < 0) { + virSeclabelFree(seclabel); + goto cleanup; + } + + ret = 0; + cleanup: + virSeclabelSpaceUnlock(space); + return ret; +} + + +/** + * virSeclabelSpaceRecall: + * + * @space: seclabel space object + * @path: path to recall + * @model: model to recall + * @label: the original value remembered + * + * Counterpart for virSeclabelSpaceRemember. Upon successful return (retval == + * 0) @label is set to point to the original value remembered. The caller is + * responsible for freeing the @label when no longer needed. + * + * Returns: 1 if label found, but still in use (refcount > 1), @label not touched + * 0 if label found and not used anymore (refcount = 1), @label set + * -1 if no label was recorded with (@path, @model) tuple, @label not touched + */ +int +virSeclabelSpaceRecall(virSeclabelSpacePtr space, + const char *path, + const char *model, + char **label) +{ + int ret = -1; + virSeclabelPtr seclabel; + + virSeclabelSpaceLock(space); + + if (!(seclabel = virSeclabelSpaceLookup(space, path, model))) + goto cleanup; + + seclabel->refcount--; + if (seclabel->refcount) { + /* still in use */ + ret = 1; + goto cleanup; + } + + virSeclabelSpaceRemove(space, path, seclabel); + /* steal the label pointer before freeing */ + *label = seclabel->label; + seclabel->label = NULL; + virSeclabelFree(seclabel); + + ret = 0; + cleanup: + virSeclabelSpaceUnlock(space); + return ret; +} diff --git a/src/locking/lock_daemon_seclabels.h b/src/locking/lock_daemon_seclabels.h new file mode 100644 index 0000000..3164206 --- /dev/null +++ b/src/locking/lock_daemon_seclabels.h @@ -0,0 +1,43 @@ +/* + * lock_daemon_seclabels.h: Security label mgmt + * + * Copyright (C) 2015 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Michal Privoznik <mprivozn@redhat.com> + */ + +#ifndef __VIR_LOCK_DAEMON_SECLABELS_H__ +# define __VIR_LOCK_DAEMON_SECLABELS_H__ + +typedef struct _virSeclabelSpace virSeclabelSpace; +typedef virSeclabelSpace *virSeclabelSpacePtr; + +void virSeclabelSpaceFree(virSeclabelSpacePtr space); +virSeclabelSpacePtr virSeclabelSpaceNew(void); + +int virSeclabelSpaceRemember(virSeclabelSpacePtr space, + const char *path, + const char *model, + const char *label); +int virSeclabelSpaceRecall(virSeclabelSpacePtr space, + const char *path, + const char *model, + char **label); + +virJSONValuePtr virSeclabelSpacePreExecRestart(virSeclabelSpacePtr space); +virSeclabelSpacePtr virSeclabelSpacePostExecRestart(virJSONValuePtr object); +#endif /* __VIR_LOCK_DAEMON_SECLABELS_H__ */ -- 2.4.9
On 10/12/2015 06:25 AM, Michal Privoznik wrote:
This module will handle all the security label remembering and recalling for virlockd. We need to record a tuple of (path, security model, security label). So I'm going with a hash table, where @path would be the key, and tuple of (model, label) is going to be the payload. Well, the payload tuple is wrapped into self inflating array (virSeclabelSpaceLabels). So for each path, we will have to traverse through array of various models to find the needed one. But since there's gonna be many paths and just a few security models, it's going to be the best approach.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- po/POTFILES.in | 1 + src/Makefile.am | 2 + src/locking/lock_daemon_seclabels.c | 545 ++++++++++++++++++++++++++++++++++++ src/locking/lock_daemon_seclabels.h | 43 +++ 4 files changed, 591 insertions(+) create mode 100644 src/locking/lock_daemon_seclabels.c create mode 100644 src/locking/lock_daemon_seclabels.h
As a "first pass" - I ran this through Coverity and it found a couple issues... John ...
+ + +static void +virSeclabelSpaceIterator(void *payload, + const void *name, + void *opaque) +{ + virSeclabelSpaceLabelsPtr labels = payload; + const char *path = name; + struct virSeclabelSpaceIteratorData *data = opaque; + virSeclabelPtr tmp; + virJSONValuePtr item, jsonLabels, jsonLabel;
Coverity found: jsonLabel and jsonLabels should be initialized since one can jump to error and call virJSONValueFree before they are properly initialized. NOTE: Build fails because of this...
+ size_t i; + + if (data->error) + return; + + if (!(item = virJSONValueNewObject())) + goto error; + + if (virJSONValueObjectAppendString(item, "path", path) < 0) + goto error; + + if (!(jsonLabels = virJSONValueNewArray())) + goto error; + + for (i = 0; i < labels->nlabels; i++) { + tmp = labels->labels[i]; + + if (!(jsonLabel = virJSONValueNewObject())) + goto error; + + if (virJSONValueObjectAppendString(jsonLabel, "model", tmp->model) < 0 || + virJSONValueObjectAppendString(jsonLabel, "label", tmp->label) < 0 || + virJSONValueObjectAppendNumberUint(jsonLabel, "refcount", tmp->refcount) < 0) + goto error; + + if (virJSONValueArrayAppend(jsonLabels, jsonLabel) < 0) + goto error; + jsonLabel = NULL; + } + + if (virJSONValueObjectAppend(item, "labels", jsonLabels) < 0) + goto error; + jsonLabels = NULL; + + if (virJSONValueArrayAppend(data->labels, item) < 0) + goto error; + item = NULL; + + return; + error: + virJSONValueFree(jsonLabel); + virJSONValueFree(jsonLabels); + virJSONValueFree(item); + data->error = true; +} + +
...
+/** + * virSeclabelSpacePostExecRestart: + * + * @object: JSON representation of internal state + * + * Read in JSON object, create new virSeclabelSpace object and restore its internal state from JSON. + * + * Returns: virSeclabelSpace object + * NULL on error + */ +virSeclabelSpacePtr +virSeclabelSpacePostExecRestart(virJSONValuePtr object) +{ + virSeclabelSpacePtr ret; + virJSONValuePtr labels; + size_t i, npaths; + + if (!(ret = virSeclabelSpaceNew())) + return NULL; + + if (!(labels = virJSONValueObjectGetArray(object, "seclabels"))) + goto error; + + npaths = virJSONValueArraySize(labels);
Coverity found: if npaths is -1, then the following loop ends on an unsigned variable...
+ for (i = 0; i < npaths; i++) { + virJSONValuePtr item = virJSONValueArrayGet(labels, i); + virJSONValuePtr arr = virJSONValueObjectGetArray(item, "labels");
Coverity found: if 'arr' is NULL, then deref below won't end well.
+ const char *path = virJSONValueObjectGetString(item, "path"); + size_t j, nlabels; + + nlabels = virJSONValueArraySize(arr);
Coverity found: if nlabels is -1, then the following loop ends on an unsigned variable...
+ + for (j = 0; j < nlabels; j++) { + virJSONValuePtr arr_item = virJSONValueArrayGet(arr, j); + const char *model = virJSONValueObjectGetString(arr_item, "model"); + const char *label = virJSONValueObjectGetString(arr_item, "label"); + virSeclabelPtr seclabel; + + if (!(seclabel = virSeclabelNew(model, label))) + goto error; + + if (virJSONValueObjectGetNumberUint(arr_item, + "refcount", + &seclabel->refcount) < 0) { + virSeclabelFree(seclabel); + goto error; + } + + if (virSeclabelSpaceAdd(ret, path, seclabel) < 0) { + virSeclabelFree(seclabel); + goto error; + } + } + } + + return ret; + error: + virSeclabelSpaceFree(ret); + return NULL; +} + +
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_daemon.c | 44 ++++++++++++++++++++++++++ src/locking/lock_daemon.h | 8 +++++ src/locking/lock_daemon_dispatch.c | 65 ++++++++++++++++++++++++++++++++------ 3 files changed, 108 insertions(+), 9 deletions(-) diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index 3984b4d..3a0a7ff 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -49,6 +49,7 @@ #include "virstring.h" #include "locking/lock_daemon_dispatch.h" +#include "locking/lock_daemon_seclabels.h" #include "locking/lock_protocol.h" #include "configmake.h" @@ -64,6 +65,7 @@ struct _virLockDaemon { virNetDaemonPtr dmn; virHashTablePtr lockspaces; virLockSpacePtr defaultLockspace; + virSeclabelSpacePtr seclabelSpace; }; virLockDaemonPtr lockDaemon = NULL; @@ -121,6 +123,7 @@ virLockDaemonFree(virLockDaemonPtr lockd) virObjectUnref(lockd->dmn); virHashFree(lockd->lockspaces); virLockSpaceFree(lockd->defaultLockspace); + virSeclabelSpaceFree(lockd->seclabelSpace); VIR_FREE(lockd); } @@ -179,6 +182,9 @@ virLockDaemonNew(virLockDaemonConfigPtr config, bool privileged) if (!(lockd->defaultLockspace = virLockSpaceNew(NULL))) goto error; + if (!(lockd->seclabelSpace = virSeclabelSpaceNew())) + goto error; + return lockd; error: @@ -193,6 +199,7 @@ virLockDaemonNewPostExecRestart(virJSONValuePtr object, bool privileged) virLockDaemonPtr lockd; virJSONValuePtr child; virJSONValuePtr lockspaces; + virJSONValuePtr seclabelSpace; virNetServerPtr srv; size_t i; ssize_t n; @@ -248,6 +255,14 @@ virLockDaemonNewPostExecRestart(virJSONValuePtr object, bool privileged) } } + if (!(seclabelSpace = virJSONValueObjectGet(object, "seclabelSpace"))) { + /* It's okay, if there's not seclabel space info. */ + if (!(lockd->seclabelSpace = virSeclabelSpaceNew())) + goto error; + } else if (!(lockd->seclabelSpace = virSeclabelSpacePostExecRestart(seclabelSpace))) { + goto error; + } + if (virJSONValueObjectHasKey(object, "daemon")) { if (!(child = virJSONValueObjectGet(object, "daemon"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -281,6 +296,26 @@ virLockDaemonNewPostExecRestart(virJSONValuePtr object, bool privileged) } +int virLockDaemonRememberSeclabel(virLockDaemonPtr lockd, + const char *path, + const char *model, + const char *label) +{ + return virSeclabelSpaceRemember(lockd->seclabelSpace, + path, model, label); +} + + +int virLockDaemonRecallSeclabel(virLockDaemonPtr lockd, + const char *path, + const char *model, + char **label) +{ + return virSeclabelSpaceRecall(lockd->seclabelSpace, + path, model, label); +} + + int virLockDaemonAddLockSpace(virLockDaemonPtr lockd, const char *path, virLockSpacePtr lockspace) @@ -1005,6 +1040,7 @@ virLockDaemonPreExecRestart(const char *state_file, char *magic; virHashKeyValuePairPtr pairs = NULL, tmp; virJSONValuePtr lockspaces; + virJSONValuePtr seclabelSpace; VIR_DEBUG("Running pre-restart exec"); @@ -1050,6 +1086,14 @@ virLockDaemonPreExecRestart(const char *state_file, tmp++; } + if (!(seclabelSpace = virSeclabelSpacePreExecRestart(lockDaemon->seclabelSpace))) + goto cleanup; + + if (virJSONValueObjectAppend(object, "seclabelSpace", seclabelSpace) < 0) { + virJSONValueFree(seclabelSpace); + goto cleanup; + } + if (!(magic = virLockDaemonGetExecRestartMagic())) goto cleanup; diff --git a/src/locking/lock_daemon.h b/src/locking/lock_daemon.h index da62edc..fb72a73 100644 --- a/src/locking/lock_daemon.h +++ b/src/locking/lock_daemon.h @@ -53,4 +53,12 @@ int virLockDaemonAddLockSpace(virLockDaemonPtr lockd, virLockSpacePtr virLockDaemonFindLockSpace(virLockDaemonPtr lockd, const char *path); +int virLockDaemonRememberSeclabel(virLockDaemonPtr lockd, + const char *path, + const char *model, + const char *label); +int virLockDaemonRecallSeclabel(virLockDaemonPtr lockd, + const char *path, + const char *model, + char **label); #endif /* __VIR_LOCK_DAEMON_H__ */ diff --git a/src/locking/lock_daemon_dispatch.c b/src/locking/lock_daemon_dispatch.c index 2d0bd81..5ad7e11 100644 --- a/src/locking/lock_daemon_dispatch.c +++ b/src/locking/lock_daemon_dispatch.c @@ -24,6 +24,7 @@ #include "rpc/virnetdaemon.h" #include "rpc/virnetserverclient.h" +#include "viralloc.h" #include "virlog.h" #include "virstring.h" #include "lock_daemon.h" @@ -36,6 +37,7 @@ VIR_LOG_INIT("locking.lock_daemon_dispatch"); #include "lock_daemon_dispatch_stubs.h" +#include "lock_daemon_seclabels.h" static int virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server ATTRIBUTE_UNUSED, @@ -433,21 +435,66 @@ virLockSpaceProtocolDispatchCreateLockSpace(virNetServerPtr server ATTRIBUTE_UNU static int virLockSpaceProtocolDispatchRememberSeclabel(virNetServerPtr server ATTRIBUTE_UNUSED, - virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetServerClientPtr client, virNetMessagePtr msg ATTRIBUTE_UNUSED, - virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED, - virLockSpaceProtocolRememberSeclabelArgs *args ATTRIBUTE_UNUSED) + virNetMessageErrorPtr rerr, + virLockSpaceProtocolRememberSeclabelArgs *args) { - return 0; + int rv = -1; + virLockDaemonClientPtr priv = + virNetServerClientGetPrivateData(client); + + virMutexLock(&priv->lock); + + if (virLockDaemonRememberSeclabel(lockDaemon, + args->path, args->model, args->label) < 0) + goto cleanup; + + rv = 0; + cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + virMutexUnlock(&priv->lock); + return rv; } static int virLockSpaceProtocolDispatchRecallSeclabel(virNetServerPtr server ATTRIBUTE_UNUSED, - virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetServerClientPtr client, virNetMessagePtr msg ATTRIBUTE_UNUSED, - virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED, - virLockSpaceProtocolRecallSeclabelArgs *args ATTRIBUTE_UNUSED, - virLockSpaceProtocolRecallSeclabelRet *ret ATTRIBUTE_UNUSED) + virNetMessageErrorPtr rerr, + virLockSpaceProtocolRecallSeclabelArgs *args, + virLockSpaceProtocolRecallSeclabelRet *ret) { - return 0; + int rv = -1; + int funcRet; + virLockDaemonClientPtr priv = + virNetServerClientGetPrivateData(client); + char *label = NULL; + char **label_p = NULL; + + virMutexLock(&priv->lock); + + memset(ret, 0, sizeof(*ret)); + + funcRet = virLockDaemonRecallSeclabel(lockDaemon, + args->path, args->model, &label); + + if (funcRet == 0 && + (VIR_ALLOC(label_p) < 0 || + VIR_STRDUP(*label_p, label) < 0)) + goto cleanup; + + ret->label = label_p; + ret->ret = funcRet; + rv = 0; + + cleanup: + if (rv < 0) { + VIR_FREE(label_p); + virNetMessageSaveError(rerr); + } + virMutexUnlock(&priv->lock); + VIR_FREE(label); + return rv; } -- 2.4.9
On Mon, Oct 12, 2015 at 12:25:57 +0200, Michal Privoznik wrote: A rather sparse commit message.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_daemon.c | 44 ++++++++++++++++++++++++++ src/locking/lock_daemon.h | 8 +++++ src/locking/lock_daemon_dispatch.c | 65 ++++++++++++++++++++++++++++++++------ 3 files changed, 108 insertions(+), 9 deletions(-)
[...]
diff --git a/src/locking/lock_daemon_dispatch.c b/src/locking/lock_daemon_dispatch.c index 2d0bd81..5ad7e11 100644 --- a/src/locking/lock_daemon_dispatch.c +++ b/src/locking/lock_daemon_dispatch.c
[...]
@@ -433,21 +435,66 @@ virLockSpaceProtocolDispatchCreateLockSpace(virNetServerPtr server ATTRIBUTE_UNU
[...]
static int virLockSpaceProtocolDispatchRecallSeclabel(virNetServerPtr server ATTRIBUTE_UNUSED, - virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetServerClientPtr client, virNetMessagePtr msg ATTRIBUTE_UNUSED, - virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED, - virLockSpaceProtocolRecallSeclabelArgs *args ATTRIBUTE_UNUSED, - virLockSpaceProtocolRecallSeclabelRet *ret ATTRIBUTE_UNUSED) + virNetMessageErrorPtr rerr, + virLockSpaceProtocolRecallSeclabelArgs *args, + virLockSpaceProtocolRecallSeclabelRet *ret) { - return 0; + int rv = -1; + int funcRet; + virLockDaemonClientPtr priv = + virNetServerClientGetPrivateData(client); + char *label = NULL; + char **label_p = NULL; + + virMutexLock(&priv->lock); + + memset(ret, 0, sizeof(*ret)); + + funcRet = virLockDaemonRecallSeclabel(lockDaemon, + args->path, args->model, &label); + + if (funcRet == 0 && + (VIR_ALLOC(label_p) < 0 || + VIR_STRDUP(*label_p, label) < 0))
This looks rather weird? Why is the extra pointer necessary?
+ goto cleanup; + + ret->label = label_p; + ret->ret = funcRet; + rv = 0;
Peter
On 16.10.2015 09:32, Peter Krempa wrote:
On Mon, Oct 12, 2015 at 12:25:57 +0200, Michal Privoznik wrote:
A rather sparse commit message.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_daemon.c | 44 ++++++++++++++++++++++++++ src/locking/lock_daemon.h | 8 +++++ src/locking/lock_daemon_dispatch.c | 65 ++++++++++++++++++++++++++++++++------ 3 files changed, 108 insertions(+), 9 deletions(-)
[...]
diff --git a/src/locking/lock_daemon_dispatch.c b/src/locking/lock_daemon_dispatch.c index 2d0bd81..5ad7e11 100644 --- a/src/locking/lock_daemon_dispatch.c +++ b/src/locking/lock_daemon_dispatch.c
[...]
@@ -433,21 +435,66 @@ virLockSpaceProtocolDispatchCreateLockSpace(virNetServerPtr server ATTRIBUTE_UNU
[...]
static int virLockSpaceProtocolDispatchRecallSeclabel(virNetServerPtr server ATTRIBUTE_UNUSED, - virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetServerClientPtr client, virNetMessagePtr msg ATTRIBUTE_UNUSED, - virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED, - virLockSpaceProtocolRecallSeclabelArgs *args ATTRIBUTE_UNUSED, - virLockSpaceProtocolRecallSeclabelRet *ret ATTRIBUTE_UNUSED) + virNetMessageErrorPtr rerr, + virLockSpaceProtocolRecallSeclabelArgs *args, + virLockSpaceProtocolRecallSeclabelRet *ret) { - return 0; + int rv = -1; + int funcRet; + virLockDaemonClientPtr priv = + virNetServerClientGetPrivateData(client); + char *label = NULL; + char **label_p = NULL; + + virMutexLock(&priv->lock); + + memset(ret, 0, sizeof(*ret)); + + funcRet = virLockDaemonRecallSeclabel(lockDaemon, + args->path, args->model, &label); + + if (funcRet == 0 && + (VIR_ALLOC(label_p) < 0 || + VIR_STRDUP(*label_p, label) < 0))
This looks rather weird? Why is the extra pointer necessary?
It's due to way we handle remote strings. If that was remote NON NULL string, then a single pointer would be enough, but since I want it to be NULL sometimes I must do it this way. If you look at the remote procedures we generate automatically you'll find the same pattern (in fact I've copied the idea from there). Michal
These are just internal APIs that will pass the data back and forth to and from virtlockd. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver.h | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index f8fd38e..4a2bbe4 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -285,6 +285,39 @@ typedef int (*virLockDriverInquire)(virLockManagerPtr man, unsigned int flags); +/** + * virLockDriverRemember: + * @man: the lock manager context + * @path: path to the file + * @model: security label model + * @label: security label to remember + * + * Remember given security label. + * + * Returns 0 on success, or -1 on failure. + */ +typedef int (*virLockDriverRemember)(virLockManagerPtr man, + const char *path, + const char *model, + const char *label); + +/** + * virLockDriverRecall: + * @man: the lock manager context + * @path: path to the file + * @model: security label model + * @label: security label to restore + * + * Recall previously saved security label + * + * Returns: 1 if @path is still in use (@label untouched), + * 0 if @path seclabel should be restored (@label set) + * -1 on failure (e.g. @path not found in records) + */ +typedef int (*virLockDriverRecall)(virLockManagerPtr man, + const char *path, + const char *model, + char **label); struct _virLockManager { virLockDriverPtr driver; void *privateData; @@ -313,6 +346,9 @@ struct _virLockDriver { virLockDriverAcquire drvAcquire; virLockDriverRelease drvRelease; virLockDriverInquire drvInquire; + + virLockDriverRemember drvRemember; + virLockDriverRecall drvRecall; }; -- 2.4.9
On Mon, Oct 12, 2015 at 12:25:58 +0200, Michal Privoznik wrote:
These are just internal APIs that will pass the data back and forth to and from virtlockd.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver.h | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)
diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index f8fd38e..4a2bbe4 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -285,6 +285,39 @@ typedef int (*virLockDriverInquire)(virLockManagerPtr man, unsigned int flags);
+/** + * virLockDriverRemember: + * @man: the lock manager context + * @path: path to the file + * @model: security label model + * @label: security label to remember + * + * Remember given security label.
As top level functions these would deserve a bit more documentation.
+ * + * Returns 0 on success, or -1 on failure.
Peter
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver_nop.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/locking/lock_driver_nop.c b/src/locking/lock_driver_nop.c index 93a988e..89aad3a 100644 --- a/src/locking/lock_driver_nop.c +++ b/src/locking/lock_driver_nop.c @@ -101,6 +101,27 @@ static void virLockManagerNopFree(virLockManagerPtr lock ATTRIBUTE_UNUSED) { } +static int +virLockManagerNopRemember(virLockManagerPtr lock ATTRIBUTE_UNUSED, + const char *path ATTRIBUTE_UNUSED, + const char *model ATTRIBUTE_UNUSED, + const char *label ATTRIBUTE_UNUSED) +{ + return 0; +} + +static int +virLockManagerNopRecall(virLockManagerPtr lock ATTRIBUTE_UNUSED, + const char *path ATTRIBUTE_UNUSED, + const char *model ATTRIBUTE_UNUSED, + char **label) +{ + if (label) + *label = NULL; + + return 0; +} + virLockDriver virLockDriverNop = { .version = VIR_LOCK_MANAGER_VERSION, @@ -118,4 +139,7 @@ virLockDriver virLockDriverNop = .drvRelease = virLockManagerNopRelease, .drvInquire = virLockManagerNopInquire, + + .drvRemember = virLockManagerNopRemember, + .drvRecall = virLockManagerNopRecall, }; -- 2.4.9
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver_lockd.c | 87 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index c974d60..70ed45d 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -775,6 +775,90 @@ static int virLockManagerLockDaemonInquire(virLockManagerPtr lock ATTRIBUTE_UNUS return 0; } +static int +virLockManagerLockDaemonRemember(virLockManagerPtr lock, + const char *path, + const char *model, + const char *label) +{ + virNetClientPtr client = NULL; + virNetClientProgramPtr program = NULL; + virLockSpaceProtocolRememberSeclabelArgs args; + int counter = 0; + int rv = -1; + + memset(&args, 0, sizeof(args)); + + if (!(client = virLockManagerLockDaemonConnect(lock, &program, &counter))) + goto cleanup; + + args.path = (char *)path; + args.model = (char *)model; + args.label = (char *)label; + + if (virNetClientProgramCall(program, + client, + counter++, + VIR_LOCK_SPACE_PROTOCOL_PROC_REMEMBER_SECLABEL, + 0, NULL, NULL, NULL, + (xdrproc_t)xdr_virLockSpaceProtocolRememberSeclabelArgs, &args, + (xdrproc_t)xdr_void, NULL) < 0) + goto cleanup; + rv = 0; + + cleanup: + virNetClientClose(client); + virObjectUnref(client); + virObjectUnref(program); + + return rv; +} + +static int +virLockManagerLockDaemonRecall(virLockManagerPtr lock, + const char *path, + const char *model, + char **label) +{ + virNetClientPtr client = NULL; + virNetClientProgramPtr program = NULL; + virLockSpaceProtocolRecallSeclabelArgs args; + virLockSpaceProtocolRecallSeclabelRet ret; + int counter = 0; + int rv = -1; + + memset(&args, 0, sizeof(args)); + memset(&ret, 0, sizeof(ret)); + + if (!(client = virLockManagerLockDaemonConnect(lock, &program, &counter))) + goto cleanup; + + args.path = (char *)path; + args.model = (char *)model; + + if (virNetClientProgramCall(program, + client, + counter++, + VIR_LOCK_SPACE_PROTOCOL_PROC_RECALL_SECLABEL, + 0, NULL, NULL, NULL, + (xdrproc_t)xdr_virLockSpaceProtocolRecallSeclabelArgs, &args, + (xdrproc_t)xdr_virLockSpaceProtocolRecallSeclabelRet, &ret) < 0) + goto cleanup; + + if (ret.ret == 0 && label) + *label = *ret.label; + + rv = ret.ret; + + cleanup: + VIR_FREE(ret.label); + virNetClientClose(client); + virObjectUnref(client); + virObjectUnref(program); + + return rv; +} + virLockDriver virLockDriverImpl = { .version = VIR_LOCK_MANAGER_VERSION, @@ -792,4 +876,7 @@ virLockDriver virLockDriverImpl = .drvRelease = virLockManagerLockDaemonRelease, .drvInquire = virLockManagerLockDaemonInquire, + + .drvRemember = virLockManagerLockDaemonRemember, + .drvRecall = virLockManagerLockDaemonRecall, }; -- 2.4.9
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 2 ++ src/locking/lock_manager.c | 32 ++++++++++++++++++++++++++++++++ src/locking/lock_manager.h | 9 +++++++++ 3 files changed, 43 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index be6ee19..72dda41 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1000,7 +1000,9 @@ virLockManagerPluginNew; virLockManagerPluginRef; virLockManagerPluginUnref; virLockManagerPluginUsesState; +virLockManagerRecallSeclabel; virLockManagerRelease; +virLockManagerRememberSeclabel; # nodeinfo.h diff --git a/src/locking/lock_manager.c b/src/locking/lock_manager.c index a002ea8..8fc82c8 100644 --- a/src/locking/lock_manager.c +++ b/src/locking/lock_manager.c @@ -397,3 +397,35 @@ int virLockManagerFree(virLockManagerPtr lock) return 0; } + +int virLockManagerRememberSeclabel(virLockManagerPtr lock, + const char *path, + const char *model, + const char *label) +{ + VIR_DEBUG("lock=%p path=%s model=%s label=%s", + lock, path, model, label); + + if (!lock) + return 0; + + CHECK_MANAGER(drvRemember, 0); + + return lock->driver->drvRemember(lock, path, model, label); +} + +int virLockManagerRecallSeclabel(virLockManagerPtr lock, + const char *path, + const char *model, + char **label) +{ + VIR_DEBUG("lock=%p path=%s model=%s label=%p", + lock, path, model, label); + + if (!lock) + return 0; + + CHECK_MANAGER(drvRecall, 0); + + return lock->driver->drvRecall(lock, path, model, label); +} diff --git a/src/locking/lock_manager.h b/src/locking/lock_manager.h index 4189759..4c2236f 100644 --- a/src/locking/lock_manager.h +++ b/src/locking/lock_manager.h @@ -67,4 +67,13 @@ int virLockManagerInquire(virLockManagerPtr manager, int virLockManagerFree(virLockManagerPtr manager); +int virLockManagerRememberSeclabel(virLockManagerPtr manager, + const char *path, + const char *model, + const char *label); +int virLockManagerRecallSeclabel(virLockManagerPtr manager, + const char *path, + const char *model, + char **label); + #endif /* __VIR_LOCK_MANAGER_H__ */ -- 2.4.9
On Mon, Oct 12, 2015 at 12:26:01 +0200, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 2 ++ src/locking/lock_manager.c | 32 ++++++++++++++++++++++++++++++++ src/locking/lock_manager.h | 9 +++++++++ 3 files changed, 43 insertions(+)
[...]
diff --git a/src/locking/lock_manager.c b/src/locking/lock_manager.c index a002ea8..8fc82c8 100644 --- a/src/locking/lock_manager.c +++ b/src/locking/lock_manager.c @@ -397,3 +397,35 @@ int virLockManagerFree(virLockManagerPtr lock)
return 0; } + +int virLockManagerRememberSeclabel(virLockManagerPtr lock, + const char *path, + const char *model, + const char *label) +{ + VIR_DEBUG("lock=%p path=%s model=%s label=%s", + lock, path, model, label); + + if (!lock) + return 0; + + CHECK_MANAGER(drvRemember, 0);
Reporting an error but then returning success doesn't seem to be the right thing to do.
+ + return lock->driver->drvRemember(lock, path, model, label); +} + +int virLockManagerRecallSeclabel(virLockManagerPtr lock, + const char *path, + const char *model, + char **label) +{ + VIR_DEBUG("lock=%p path=%s model=%s label=%p", + lock, path, model, label); + + if (!lock) + return 0; + + CHECK_MANAGER(drvRecall, 0);
Same here.
+ + return lock->driver->drvRecall(lock, path, model, label); +}
Peter
This is not a public API where we have to care about size of data types. Moreover, I'll be adding new type to lock soon and it would help if compiler could point on all the locations I need to fix. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver.h | 4 ++-- src/locking/lock_driver_lockd.c | 4 ++-- src/locking/lock_driver_nop.c | 4 ++-- src/locking/lock_driver_sanlock.c | 4 ++-- src/locking/lock_manager.c | 4 ++-- src/locking/lock_manager.h | 4 ++-- 6 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index 4a2bbe4..e7a8371 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -171,7 +171,7 @@ typedef int (*virLockDriverDeinit)(void); * Returns 0 if successful initialized a new context, -1 on error */ typedef int (*virLockDriverNew)(virLockManagerPtr man, - unsigned int type, + virLockManagerObjectType type, size_t nparams, virLockManagerParamPtr params, unsigned int flags); @@ -218,7 +218,7 @@ typedef void (*virLockDriverFree)(virLockManagerPtr man); * Returns 0 on success, or -1 on failure */ typedef int (*virLockDriverAddResource)(virLockManagerPtr man, - unsigned int type, + virLockManagerResourceType type, const char *name, size_t nparams, virLockManagerParamPtr params, diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 70ed45d..a4a9a62 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -427,7 +427,7 @@ static void virLockManagerLockDaemonFree(virLockManagerPtr lock) static int virLockManagerLockDaemonNew(virLockManagerPtr lock, - unsigned int type, + virLockManagerObjectType type, size_t nparams, virLockManagerParamPtr params, unsigned int flags) @@ -492,7 +492,7 @@ static int virLockManagerLockDaemonNew(virLockManagerPtr lock, static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, - unsigned int type, + virLockManagerResourceType type, const char *name, size_t nparams, virLockManagerParamPtr params, diff --git a/src/locking/lock_driver_nop.c b/src/locking/lock_driver_nop.c index 89aad3a..5f72ee5 100644 --- a/src/locking/lock_driver_nop.c +++ b/src/locking/lock_driver_nop.c @@ -48,7 +48,7 @@ static int virLockManagerNopDeinit(void) static int virLockManagerNopNew(virLockManagerPtr lock ATTRIBUTE_UNUSED, - unsigned int type ATTRIBUTE_UNUSED, + virLockManagerObjectType type ATTRIBUTE_UNUSED, size_t nparams ATTRIBUTE_UNUSED, virLockManagerParamPtr params ATTRIBUTE_UNUSED, unsigned int flags_unused ATTRIBUTE_UNUSED) @@ -57,7 +57,7 @@ static int virLockManagerNopNew(virLockManagerPtr lock ATTRIBUTE_UNUSED, } static int virLockManagerNopAddResource(virLockManagerPtr lock ATTRIBUTE_UNUSED, - unsigned int type ATTRIBUTE_UNUSED, + virLockManagerResourceType type ATTRIBUTE_UNUSED, const char *name ATTRIBUTE_UNUSED, size_t nparams ATTRIBUTE_UNUSED, virLockManagerParamPtr params ATTRIBUTE_UNUSED, diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index e052875..2de9a55 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -445,7 +445,7 @@ static int virLockManagerSanlockDeinit(void) static int virLockManagerSanlockNew(virLockManagerPtr lock, - unsigned int type, + virLockManagerObjectType type, size_t nparams, virLockManagerParamPtr params, unsigned int flags) @@ -732,7 +732,7 @@ static int virLockManagerSanlockCreateLease(struct sanlk_resource *res) static int virLockManagerSanlockAddResource(virLockManagerPtr lock, - unsigned int type, + virLockManagerResourceType type, const char *name, size_t nparams, virLockManagerParamPtr params, diff --git a/src/locking/lock_manager.c b/src/locking/lock_manager.c index 8fc82c8..83684f2 100644 --- a/src/locking/lock_manager.c +++ b/src/locking/lock_manager.c @@ -295,7 +295,7 @@ virLockDriverPtr virLockManagerPluginGetDriver(virLockManagerPluginPtr plugin) * Returns a new lock manager context */ virLockManagerPtr virLockManagerNew(virLockDriverPtr driver, - unsigned int type, + virLockManagerObjectType type, size_t nparams, virLockManagerParamPtr params, unsigned int flags) @@ -322,7 +322,7 @@ virLockManagerPtr virLockManagerNew(virLockDriverPtr driver, int virLockManagerAddResource(virLockManagerPtr lock, - unsigned int type, + virLockManagerResourceType type, const char *name, size_t nparams, virLockManagerParamPtr params, diff --git a/src/locking/lock_manager.h b/src/locking/lock_manager.h index 4c2236f..0f85a7b 100644 --- a/src/locking/lock_manager.h +++ b/src/locking/lock_manager.h @@ -41,13 +41,13 @@ bool virLockManagerPluginUsesState(virLockManagerPluginPtr plugin); virLockDriverPtr virLockManagerPluginGetDriver(virLockManagerPluginPtr plugin); virLockManagerPtr virLockManagerNew(virLockDriverPtr driver, - unsigned int type, + virLockManagerObjectType type, size_t nparams, virLockManagerParamPtr params, unsigned int flags); int virLockManagerAddResource(virLockManagerPtr manager, - unsigned int type, + virLockManagerResourceType type, const char *name, size_t nparams, virLockManagerParamPtr params, -- 2.4.9
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver.h | 2 ++ src/locking/lock_driver_lockd.c | 12 +++++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index e7a8371..71feea3 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -42,6 +42,8 @@ typedef enum { typedef enum { /* The managed object is a virtual guest domain */ VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN = 0, + /* The managed object is a seclabel */ + VIR_LOCK_MANAGER_OBJECT_TYPE_SECLABEL, } virLockManagerObjectType; typedef enum { diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index a4a9a62..f9867a1 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -54,6 +54,8 @@ struct _virLockManagerLockDaemonResource { }; struct _virLockManagerLockDaemonPrivate { + virLockManagerObjectType type; + unsigned char uuid[VIR_UUID_BUFLEN]; char *name; int id; @@ -290,11 +292,13 @@ virLockManagerLockDaemonConnect(virLockManagerPtr lock, int *counter) { virNetClientPtr client; + virLockManagerLockDaemonPrivatePtr priv = lock->privateData; if (!(client = virLockManagerLockDaemonConnectionNew(geteuid() == 0, program))) return NULL; - if (virLockManagerLockDaemonConnectionRegister(lock, + if (priv->type == VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN && + virLockManagerLockDaemonConnectionRegister(lock, client, *program, counter) < 0) @@ -441,6 +445,8 @@ static int virLockManagerLockDaemonNew(virLockManagerPtr lock, return -1; lock->privateData = priv; + priv->type = type; + switch (type) { case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN: for (i = 0; i < nparams; i++) { @@ -480,6 +486,10 @@ static int virLockManagerLockDaemonNew(virLockManagerPtr lock, } break; + case VIR_LOCK_MANAGER_OBJECT_TYPE_SECLABEL: + /* nada */ + break; + default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown lock manager object type %d"), -- 2.4.9
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- cfg.mk | 2 +- src/qemu/qemu_driver.c | 3 ++- src/security/security_dac.c | 10 ++++++++++ src/security/security_dac.h | 2 ++ src/security/security_manager.c | 4 +++- src/security/security_manager.h | 4 +++- 6 files changed, 21 insertions(+), 4 deletions(-) diff --git a/cfg.mk b/cfg.mk index e436434..43a591f 100644 --- a/cfg.mk +++ b/cfg.mk @@ -783,7 +783,7 @@ sc_prohibit_cross_inclusion: access/ | conf/) safe="($$dir|conf|util)";; \ locking/) safe="($$dir|util|conf|rpc)";; \ cpu/| network/| node_device/| rpc/| security/| storage/) \ - safe="($$dir|util|conf|storage)";; \ + safe="($$dir|util|conf|storage|locking)";; \ xenapi/ | xenconfig/ ) safe="($$dir|util|conf|xen)";; \ *) safe="($$dir|$(mid_dirs)|util)";; \ esac; \ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8cd5ee3..5abccb0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -435,7 +435,8 @@ qemuSecurityInit(virQEMUDriverPtr driver) cfg->user, cfg->group, flags, - qemuSecurityChownCallback))) + qemuSecurityChownCallback, + driver->lockManager))) goto error; if (!stack) { if (!(stack = virSecurityManagerNewStack(mgr))) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 6c4e351..463b459 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -57,6 +57,7 @@ struct _virSecurityDACData { bool dynamicOwnership; char *baselabel; virSecurityManagerDACChownCallback chownCallback; + virLockManagerPluginPtr lockPlugin; }; typedef struct _virSecurityDACCallbackData virSecurityDACCallbackData; @@ -101,6 +102,15 @@ virSecurityDACSetChownCallback(virSecurityManagerPtr mgr, priv->chownCallback = chownCallback; } +void +virSecurityDACSetLockingPlugin(virSecurityManagerPtr mgr, + virLockManagerPluginPtr lockPlugin) +{ + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + priv->lockPlugin = lockPlugin; +} + + /* returns 1 if label isn't found, 0 on success, -1 on error */ static int ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) diff --git a/src/security/security_dac.h b/src/security/security_dac.h index 846cefb..aefc262 100644 --- a/src/security/security_dac.h +++ b/src/security/security_dac.h @@ -35,4 +35,6 @@ void virSecurityDACSetDynamicOwnership(virSecurityManagerPtr mgr, void virSecurityDACSetChownCallback(virSecurityManagerPtr mgr, virSecurityManagerDACChownCallback chownCallback); +void virSecurityDACSetLockingPlugin(virSecurityManagerPtr mgr, + virLockManagerPluginPtr lockPlugin); #endif /* __VIR_SECURITY_DAC */ diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 5b05a47..e41e761 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -141,7 +141,8 @@ virSecurityManagerNewDAC(const char *virtDriver, uid_t user, gid_t group, unsigned int flags, - virSecurityManagerDACChownCallback chownCallback) + virSecurityManagerDACChownCallback chownCallback, + virLockManagerPluginPtr lockPlugin) { virSecurityManagerPtr mgr; @@ -162,6 +163,7 @@ virSecurityManagerNewDAC(const char *virtDriver, virSecurityDACSetDynamicOwnership(mgr, flags & VIR_SECURITY_MANAGER_DYNAMIC_OWNERSHIP); virSecurityDACSetChownCallback(mgr, chownCallback); + virSecurityDACSetLockingPlugin(mgr, lockPlugin); return mgr; } diff --git a/src/security/security_manager.h b/src/security/security_manager.h index e534e31..96f7053 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -26,6 +26,7 @@ # include "domain_conf.h" # include "vircommand.h" # include "virstoragefile.h" +# include "locking/lock_manager.h" typedef struct _virSecurityManager virSecurityManager; typedef virSecurityManager *virSecurityManagerPtr; @@ -71,7 +72,8 @@ virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver, uid_t user, gid_t group, unsigned int flags, - virSecurityManagerDACChownCallback chownCallback); + virSecurityManagerDACChownCallback chownCallback, + virLockManagerPluginPtr lockPlugin); int virSecurityManagerPreFork(virSecurityManagerPtr mgr); void virSecurityManagerPostFork(virSecurityManagerPtr mgr); -- 2.4.9
https://bugzilla.redhat.com/show_bug.cgi?id=547546 So now that we have the whole infrastructure prepared, we can remember and recall the DAC security labels. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/Makefile.am | 1 + src/security/security_dac.c | 90 ++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 81 insertions(+), 10 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index e706166..51b4af0 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -2738,6 +2738,7 @@ libvirt_lxc_LDADD = \ libvirt-net-rpc-server.la \ libvirt-net-rpc.la \ libvirt_security_manager.la \ + libvirt_driver.la \ libvirt_conf.la \ libvirt_util.la \ ../gnulib/lib/libgnu.la diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 463b459..5c99dfa 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -206,12 +206,38 @@ virSecurityDACGetImageIds(virSecurityLabelDefPtr seclabel, * Returns: 0 on success, -1 on failure */ static int -virSecurityDACRememberLabel(virSecurityDACDataPtr priv ATTRIBUTE_UNUSED, - const char *path ATTRIBUTE_UNUSED, - uid_t uid ATTRIBUTE_UNUSED, - gid_t gid ATTRIBUTE_UNUSED) +virSecurityDACRememberLabel(virSecurityDACDataPtr priv, + const char *path, + uid_t uid, + gid_t gid) { - return 0; + int ret = -1; + virLockManagerPtr lockManager = NULL; + char *label; + + if (!priv->lockPlugin) + return 0; + + if (virAsprintf(&label, "+%u:+%u", + (unsigned int) uid, + (unsigned int) gid) < 0) + goto cleanup; + + lockManager = virLockManagerNew(virLockManagerPluginGetDriver(priv->lockPlugin), + VIR_LOCK_MANAGER_OBJECT_TYPE_SECLABEL, + 0, NULL, 0); + if (!lockManager) + goto cleanup; + + if (virLockManagerRememberSeclabel(lockManager, path, + SECURITY_DAC_NAME, label) < 0) + goto cleanup; + + ret = 0; + cleanup: + VIR_FREE(label); + virLockManagerFree(lockManager); + return ret; } /** @@ -230,12 +256,56 @@ virSecurityDACRememberLabel(virSecurityDACDataPtr priv ATTRIBUTE_UNUSED, * -1 on failure (@uid and @gid not touched) */ static int -virSecurityDACRecallLabel(virSecurityDACDataPtr priv ATTRIBUTE_UNUSED, - const char *path ATTRIBUTE_UNUSED, - uid_t *uid ATTRIBUTE_UNUSED, - gid_t *gid ATTRIBUTE_UNUSED) +virSecurityDACRecallLabel(virSecurityDACDataPtr priv, + const char *path, + uid_t *uid, + gid_t *gid) { - return 0; + int rv, ret = -1; + virLockManagerPtr lockManager = NULL; + char *label = NULL; + + if (!priv->lockPlugin) + return 0; + + lockManager = virLockManagerNew(virLockManagerPluginGetDriver(priv->lockPlugin), + VIR_LOCK_MANAGER_OBJECT_TYPE_SECLABEL, + 0, NULL, 0); + if (!lockManager) + goto cleanup; + + rv = virLockManagerRecallSeclabel(lockManager, path, + SECURITY_DAC_NAME, &label); + + VIR_DEBUG("path=%s label=%s", path, label); + + if (rv < 0) { + /* Okay, path was not found, or there was some other error. At any + * rate, claim success here. */ + ret = 0; + goto cleanup; + } else if (rv > 0) { + /* Okay, path was found, but is still in use. Notify caller so that we + * don't relabel anything. */ + ret = 1; + goto cleanup; + } + + if (!label) { + /* Okay, underlying lock driver does not know how to store seclabels. + * Fall back to defaults. */ + ret = 0; + goto cleanup; + } + + if (virParseOwnershipIds(label, uid, gid) < 0) + goto cleanup; + + ret = 0; + cleanup: + VIR_FREE(label); + virLockManagerFree(lockManager); + return ret; } static virSecurityDriverStatus -- 2.4.9
Now that we know what label we should restore and we do have reference counter to each seclabel, we restore the original seclabel only after the last domain is torn down. Therefore, we can safely try to restore labels even for RO or shared disks. The reference counter will take the care of everything. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 5c99dfa..59b16ef 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -561,14 +561,6 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, if (!priv->dynamicOwnership) return 0; - /* Don't restore labels on readoly/shared disks, because other VMs may - * still be accessing these. Alternatively we could iterate over all - * running domains and try to figure out if it is in use, but this would - * not work for clustered filesystems, since we can't see running VMs using - * the file on other nodes. Safest bet is thus to skip the restore step. */ - if (src->readonly || src->shared) - return 0; - secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); if (secdef && !secdef->relabel) return 0; -- 2.4.9
On Mon, Oct 12, 2015 at 12:26:06 +0200, Michal Privoznik wrote:
Now that we know what label we should restore and we do have reference counter to each seclabel, we restore the original seclabel only after the last domain is torn down. Therefore, we can safely try to restore labels even for RO or shared disks. The reference counter will take the care of everything.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_dac.c | 8 -------- 1 file changed, 8 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 5c99dfa..59b16ef 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -561,14 +561,6 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, if (!priv->dynamicOwnership) return 0;
- /* Don't restore labels on readoly/shared disks, because other VMs may - * still be accessing these. Alternatively we could iterate over all - * running domains and try to figure out if it is in use, but this would - * not work for clustered filesystems, since we can't see running VMs using - * the file on other nodes. Safest bet is thus to skip the restore step. */ - if (src->readonly || src->shared) - return 0; -
This will regress if you use the 'nop' driver for "storing" locks but still expect libvirt not to break your labelling. Peter
This is a counterpart of virSecurityManagerDomainSetDirLabel. We should restore the security labels on the directories where we've changed them. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/security/security_dac.c | 18 ++++++++++++++++++ src/security/security_driver.h | 5 ++++- src/security/security_manager.c | 16 ++++++++++++++++ src/security/security_manager.h | 3 +++ src/security/security_selinux.c | 16 ++++++++++++++++ src/security/security_stack.c | 20 ++++++++++++++++++++ 7 files changed, 78 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 72dda41..0e03c6c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1030,6 +1030,7 @@ virSecurityDriverLookup; # security/security_manager.h virSecurityManagerCheckAllLabel; virSecurityManagerClearSocketLabel; +virSecurityManagerDomainRestoreDirLabel; virSecurityManagerDomainSetDirLabel; virSecurityManagerGenLabel; virSecurityManagerGetBaseLabel; diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 59b16ef..043866e 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1578,6 +1578,23 @@ virSecurityDACDomainSetDirLabel(virSecurityManagerPtr mgr, return virSecurityDACSetOwnership(priv, NULL, path, user, group); } +static int +virSecurityDACDomainRestoreDirLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + const char *path) +{ + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityLabelDefPtr seclabel; + + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); + + if (!seclabel && !seclabel->relabel) + return 0; + + return virSecurityDACRestoreSecurityFileLabel(priv, path); +} + + virSecurityDriver virSecurityDriverDAC = { .privateDataLen = sizeof(virSecurityDACData), .name = SECURITY_DAC_NAME, @@ -1627,4 +1644,5 @@ virSecurityDriver virSecurityDriverDAC = { .getBaseLabel = virSecurityDACGetBaseLabel, .domainSetDirLabel = virSecurityDACDomainSetDirLabel, + .domainRestoreDirLabel = virSecurityDACDomainRestoreDirLabel, }; diff --git a/src/security/security_driver.h b/src/security/security_driver.h index 784b0de..2503831 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -121,7 +121,9 @@ typedef int (*virSecurityDomainRestoreImageLabel) (virSecurityManagerPtr mgr, typedef int (*virSecurityDomainSetDirLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, const char *path); - +typedef int (*virSecurityDomainRestoreDirLabel) (virSecurityManagerPtr mgr, + virDomainDefPtr def, + const char *path); struct _virSecurityDriver { size_t privateDataLen; @@ -173,6 +175,7 @@ struct _virSecurityDriver { virSecurityDriverGetBaseLabel getBaseLabel; virSecurityDomainSetDirLabel domainSetDirLabel; + virSecurityDomainRestoreDirLabel domainRestoreDirLabel; }; virSecurityDriverPtr virSecurityDriverLookup(const char *name, diff --git a/src/security/security_manager.c b/src/security/security_manager.c index e41e761..a5308ac 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -1003,3 +1003,19 @@ virSecurityManagerDomainSetDirLabel(virSecurityManagerPtr mgr, return 0; } + +int +virSecurityManagerDomainRestoreDirLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + const char *path) +{ + if (mgr->drv->domainRestoreDirLabel) { + int ret; + virObjectLock(mgr); + ret = mgr->drv->domainRestoreDirLabel(mgr, vm, path); + virObjectUnlock(mgr); + return ret; + } + + return 0; +} diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 96f7053..bfec4fa 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -165,5 +165,8 @@ int virSecurityManagerRestoreImageLabel(virSecurityManagerPtr mgr, int virSecurityManagerDomainSetDirLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, const char *path); +int virSecurityManagerDomainRestoreDirLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + const char *path); #endif /* VIR_SECURITY_MANAGER_H__ */ diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index c2464c2..3d04123 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2550,6 +2550,21 @@ virSecuritySELinuxDomainSetDirLabel(virSecurityManagerPtr mgr, return virSecuritySELinuxSetFilecon(mgr, path, seclabel->imagelabel); } +static int +virSecuritySELinuxDomainRestoreDirLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + const char *path) +{ + virSecurityLabelDefPtr seclabel; + + seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); + if (!seclabel || !seclabel->relabel) + return 0; + + return virSecuritySELinuxRestoreSecurityFileLabel(mgr, path); +} + + virSecurityDriver virSecurityDriverSELinux = { .privateDataLen = sizeof(virSecuritySELinuxData), .name = SECURITY_SELINUX_NAME, @@ -2596,4 +2611,5 @@ virSecurityDriver virSecurityDriverSELinux = { .getBaseLabel = virSecuritySELinuxGetBaseLabel, .domainSetDirLabel = virSecuritySELinuxDomainSetDirLabel, + .domainRestoreDirLabel = virSecuritySELinuxDomainRestoreDirLabel, }; diff --git a/src/security/security_stack.c b/src/security/security_stack.c index 8d9560d..5c9d27f 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -617,6 +617,25 @@ virSecurityStackDomainSetDirLabel(virSecurityManagerPtr mgr, return rc; } +static int +virSecurityStackDomainRestoreDirLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + const char *path) +{ + virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityStackItemPtr item = priv->itemsHead; + int rc = 0; + + for (; item; item = item->next) { + if (virSecurityManagerDomainRestoreDirLabel(item->securityManager, + vm, path) < 0) + rc = -1; + } + + return rc; +} + + virSecurityDriver virSecurityDriverStack = { .privateDataLen = sizeof(virSecurityStackData), .name = "stack", @@ -668,4 +687,5 @@ virSecurityDriver virSecurityDriverStack = { .getBaseLabel = virSecurityStackGetBaseLabel, .domainSetDirLabel = virSecurityStackDomainSetDirLabel, + .domainRestoreDirLabel = virSecurityStackDomainRestoreDirLabel, }; -- 2.4.9
This is a counterpart for f1f68ca33. While we set the security labels on various directories that domain is going to use (e.g. channel target directory) we don't restore the permissions back. Leaving a hanging entry in virtlockd. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_process.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 8aa9efc..e2968ea 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5243,15 +5243,21 @@ void qemuProcessStop(virQEMUDriverPtr driver, priv->monConfig = NULL; } - ignore_value(virAsprintf(&tmppath, "%s/domain-%s", - cfg->libDir, vm->def->name)); - virFileDeleteTree(tmppath); - VIR_FREE(tmppath); + if (virAsprintf(&tmppath, "%s/domain-%s", + cfg->libDir, vm->def->name) > 0) { + virSecurityManagerDomainRestoreDirLabel(driver->securityManager, + vm->def, tmppath); + virFileDeleteTree(tmppath); + VIR_FREE(tmppath); + } - ignore_value(virAsprintf(&tmppath, "%s/domain-%s", - cfg->channelTargetDir, vm->def->name)); - virFileDeleteTree(tmppath); - VIR_FREE(tmppath); + if (virAsprintf(&tmppath, "%s/domain-%s", + cfg->channelTargetDir, vm->def->name) > 0) { + virSecurityManagerDomainRestoreDirLabel(driver->securityManager, + vm->def, tmppath); + virFileDeleteTree(tmppath); + VIR_FREE(tmppath); + } ignore_value(virDomainChrDefForeach(vm->def, false, -- 2.4.9
On 12.10.2015 12:25, Michal Privoznik wrote:
So, you may be familiar with this already. Well, I've tried to get these patches in like a year ago (or even more). Point is, these ones are new, written from scratch. However, still based on idea, that virtlockd will keep the track of the original seclabels. So far only DAC driver is fixed, but the infrastructure I'm proposing here is easily extensible to other drivers too.
Even if there's some disagreement on the design, the first few patches fix some bugs, so they should make it in.
Michal Privoznik (23): virtlockd: Don't SIGSEGV on SIGUSR1 security_dac: Fix TODO marks virSecurityDACSetOwnershipInternal: Don't chown so often security_dac: Introduce remember/recall stubs virSecurityDACSetOwnership: Pass virSecurityDACDataPtr virSecurityDACRestoreSecurityFileLabel: Pass virSecurityDACDataPtr security_dac: Limit usage of virSecurityDACSetOwnershipInternal security_dac: Plug in remember/recall APIs lock_protocol: Add two new remote procedures lock_daemon: Switch to wrapper locking functions locking: Introduce virSeclabelSpace virtlockd: Work virSeclabelSpace in virLockDriver: Introduce virLockDriverRemember and virLockDriverRecall lock_driver_nop: Implement remember and recall APIs lock_driver_lockd: Implement remember and recall APIs lock_manager: Implement remember & recall APIs locking: Favour enum type over int lock_driver: Introduce VIR_LOCK_MANAGER_OBJECT_TYPE_SECLABEL virSecurityManagerNewDAC: Pass locking plugin in security_dac: Remember security labels security_dac: Restore original owner more often security: Introduce virSecurityManagerDomainRestoreDirLabel qemuProcessStop: Restore seclabels on dirs too
cfg.mk | 2 +- po/POTFILES.in | 1 + src/Makefile.am | 3 + src/libvirt_private.syms | 3 + src/lock_protocol-structs | 15 + src/locking/lock_daemon.c | 69 ++++- src/locking/lock_daemon.h | 8 + src/locking/lock_daemon_dispatch.c | 68 +++++ src/locking/lock_daemon_seclabels.c | 545 ++++++++++++++++++++++++++++++++++++ src/locking/lock_daemon_seclabels.h | 43 +++ src/locking/lock_driver.h | 42 ++- src/locking/lock_driver_lockd.c | 103 ++++++- src/locking/lock_driver_nop.c | 28 +- src/locking/lock_driver_sanlock.c | 4 +- src/locking/lock_manager.c | 36 ++- src/locking/lock_manager.h | 13 +- src/locking/lock_protocol.x | 29 +- src/qemu/qemu_driver.c | 3 +- src/qemu/qemu_process.c | 22 +- src/security/security_dac.c | 288 +++++++++++++++---- src/security/security_dac.h | 2 + src/security/security_driver.h | 5 +- src/security/security_manager.c | 20 +- src/security/security_manager.h | 7 +- src/security/security_selinux.c | 16 ++ src/security/security_stack.c | 20 ++ 26 files changed, 1310 insertions(+), 85 deletions(-) create mode 100644 src/locking/lock_daemon_seclabels.c create mode 100644 src/locking/lock_daemon_seclabels.h
So, I've fixed all the findings up to 08/23 (inclusive) and pushed it up till that point. Those patches mostly clean up security driver, fix some bugs, and prepare the security driver to whatever remembering framework we come up. Thank you both guys! Michal
participants (3)
-
John Ferlan -
Michal Privoznik -
Peter Krempa