Firstly, there's no reason to enumerate all XATTRs since they differ only in the prefix and we can construct them in a loop. Secondly, and more importantly, the script was still looking for just one prefix "trusted.libvirt.security" even on FreeBSD. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- tools/libvirt_recover_xattrs.sh | 49 +++++++++++++++++---------------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/tools/libvirt_recover_xattrs.sh b/tools/libvirt_recover_xattrs.sh index 69dfca0160..3302fca60e 100755 --- a/tools/libvirt_recover_xattrs.sh +++ b/tools/libvirt_recover_xattrs.sh @@ -23,14 +23,17 @@ EOF QUIET=0 DRY_RUN=0 -P="/" +DIR="/" # So far only qemu and lxc drivers use security driver. URI=("qemu:///system" "qemu:///session" "lxc:///system") -LIBVIRT_XATTR_PREFIX="trusted.libvirt.security" +# On Linux we use 'trusted' namespace, on FreeBSD we use 'system' +# as there is no 'trusted'. +LIBVIRT_XATTR_PREFIXES=("trusted.libvirt.security" + "system.libvirt.security") if [ `whoami` != "root" ]; then die "Must be run as root" @@ -57,7 +60,7 @@ done shift $((OPTIND - 1)) if [ $# -gt 0 ]; then - P=$1 + DIR=$1 fi if [ ${DRY_RUN} -eq 0 ]; then @@ -69,28 +72,26 @@ if [ ${DRY_RUN} -eq 0 ]; then fi -# On Linux we use 'trusted' namespace, on FreeBSD we use 'system' -# as there is no 'trusted'. -XATTRS=("trusted.libvirt.security.dac" - "trusted.libvirt.security.ref_dac" - "trusted.libvirt.security.selinux" - "trusted.libvirt.security.ref_selinux", - "system.libvirt.security.dac" - "system.libvirt.security.ref_dac" - "system.libvirt.security.selinux" - "system.libvirt.security.ref_selinux") +declare -a XATTRS +for i in "dac" "selinux"; do + for p in ${LIBVIRT_XATTR_PREFIXES[@]}; do + XATTRS+=("$p.$i" "$p.ref_$i") + done +done -for i in $(getfattr -R -d -m ${LIBVIRT_XATTR_PREFIX} --absolute-names ${P} 2>/dev/null | grep "^# file:" | cut -d':' -f 2); do - if [ ${DRY_RUN} -ne 0 ]; then - echo $i - getfattr -d -m ${LIBVIRT_XATTR_PREFIX} $i - continue - fi +for p in ${LIBVIRT_XATTR_PREFIXES[*]}; do + for i in $(getfattr -R -d -m ${p} --absolute-names ${DIR} 2>/dev/null | grep "^# file:" | cut -d':' -f 2); do + echo $i; + if [ ${DRY_RUN} -ne 0 ]; then + getfattr -d -m $p --absolute-names $i | grep -v "^# file:" + continue + fi - if [ ${QUIET} -eq 0 ]; then - echo "Fixing $i"; - fi - for x in ${XATTRS[*]}; do - setfattr -x $x $i + if [ ${QUIET} -eq 0 ]; then + echo "Fixing $i"; + fi + for x in ${XATTRS[*]}; do + setfattr -x $x $i + done done done -- 2.19.2

Just like it's DAC counterpart is doing, virSecuritySELinuxRestoreAllLabel() could print @migrated in the debug message. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/security_selinux.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 2fceb547b4..fb631cd321 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2597,7 +2597,7 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManagerPtr mgr, size_t i; int rc = 0; - VIR_DEBUG("Restoring security label on %s", def->name); + VIR_DEBUG("Restoring security label on %s migrated=%d", def->name, migrated); secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); -- 2.19.2

The way that security drivers use XATTR is kind of verbose. If error reporting was left for caller then the caller would end up even more verbose. There are two places where we do not want to report error if virFileGetXAttr fails. Therefore virFileGetXAttrQuiet is introduced as an alternative that doesn't report errors. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/security/security_util.c | 4 ++-- src/util/virfile.c | 42 ++++++++++++++++++++++++++++++------ src/util/virfile.h | 5 +++++ tests/qemusecuritymock.c | 6 +++--- 5 files changed, 46 insertions(+), 12 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 73ef24d66f..8792155312 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1866,6 +1866,7 @@ virFileGetHugepageSize; virFileGetMountReverseSubtree; virFileGetMountSubtree; virFileGetXAttr; +virFileGetXAttrQuiet; virFileInData; virFileIsAbsPath; virFileIsCDROM; diff --git a/src/security/security_util.c b/src/security/security_util.c index bfa78c6cca..f09a18a623 100644 --- a/src/security/security_util.c +++ b/src/security/security_util.c @@ -123,7 +123,7 @@ virSecurityGetRememberedLabel(const char *name, if (!(ref_name = virSecurityGetRefCountAttrName(name))) goto cleanup; - if (virFileGetXAttr(path, ref_name, &value) < 0) { + if (virFileGetXAttrQuiet(path, ref_name, &value) < 0) { if (errno == ENOSYS || errno == ENODATA || errno == ENOTSUP) { ret = 0; } else { @@ -208,7 +208,7 @@ virSecuritySetRememberedLabel(const char *name, if (!(ref_name = virSecurityGetRefCountAttrName(name))) goto cleanup; - if (virFileGetXAttr(path, ref_name, &value) < 0) { + if (virFileGetXAttrQuiet(path, ref_name, &value) < 0) { if (errno == ENOSYS || errno == ENOTSUP) { ret = 0; goto cleanup; diff --git a/src/util/virfile.c b/src/util/virfile.c index ec8d85929c..7ce4b1dbc2 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -4338,7 +4338,7 @@ virFileWaitForExists(const char *path, #if HAVE_LIBATTR /** - * virFileGetXAttr; + * virFileGetXAttrQuiet; * @path: a filename * @name: name of xattr * @value: read value @@ -4350,9 +4350,9 @@ virFileWaitForExists(const char *path, * -1 otherwise (with errno set). */ int -virFileGetXAttr(const char *path, - const char *name, - char **value) +virFileGetXAttrQuiet(const char *path, + const char *name, + char **value) { char *buf = NULL; int ret = -1; @@ -4425,9 +4425,9 @@ virFileRemoveXAttr(const char *path, #else /* !HAVE_LIBATTR */ int -virFileGetXAttr(const char *path ATTRIBUTE_UNUSED, - const char *name ATTRIBUTE_UNUSED, - char **value ATTRIBUTE_UNUSED) +virFileGetXAttrQuiet(const char *path ATTRIBUTE_UNUSED, + const char *name ATTRIBUTE_UNUSED, + char **value ATTRIBUTE_UNUSED) { errno = ENOSYS; return -1; @@ -4451,3 +4451,31 @@ virFileRemoveXAttr(const char *path ATTRIBUTE_UNUSED, } #endif /* HAVE_LIBATTR */ + +/** + * virFileGetXAttr; + * @path: a filename + * @name: name of xattr + * @value: read value + * + * Reads xattr with @name for given @path and stores it into + * @value. Caller is responsible for freeing @value. + * + * Returns: 0 on success, + * -1 otherwise (with errno set AND error reported). + */ +int +virFileGetXAttr(const char *path, + const char *name, + char **value) +{ + int ret; + + if ((ret = virFileGetXAttrQuiet(path, name, value)) < 0) { + virReportSystemError(errno, + _("Unable to get XATTR %s on %s"), + name, path); + } + + return ret; +} diff --git a/src/util/virfile.h b/src/util/virfile.h index 3dedb7666a..099743f7f0 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -381,6 +381,11 @@ int virFileGetXAttr(const char *path, char **value) ATTRIBUTE_NOINLINE; +int virFileGetXAttrQuiet(const char *path, + const char *name, + char **value) + ATTRIBUTE_NOINLINE; + int virFileSetXAttr(const char *path, const char *name, const char *value) diff --git a/tests/qemusecuritymock.c b/tests/qemusecuritymock.c index d1b17d8aa4..a54e5d426e 100644 --- a/tests/qemusecuritymock.c +++ b/tests/qemusecuritymock.c @@ -131,9 +131,9 @@ get_key(const char *path, int -virFileGetXAttr(const char *path, - const char *name, - char **value) +virFileGetXAttrQuiet(const char *path, + const char *name, + char **value) { int ret = -1; char *key; -- 2.19.2

It's better to have the function report errors, because none of the callers does. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/virfile.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 7ce4b1dbc2..fbcab404e7 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -4395,14 +4395,21 @@ virFileGetXAttrQuiet(const char *path, * Sets xattr of @name and @value on @path. * * Returns: 0 on success, - * -1 otherwise (with errno set). + * -1 otherwise (with errno set AND error reported). */ int virFileSetXAttr(const char *path, const char *name, const char *value) { - return setxattr(path, name, value, strlen(value), 0); + if (setxattr(path, name, value, strlen(value), 0) < 0) { + virReportSystemError(errno, + _("Unable to set XATTR %s on %s"), + name, path); + return -1; + } + + return 0; } /** @@ -4434,11 +4441,14 @@ virFileGetXAttrQuiet(const char *path ATTRIBUTE_UNUSED, } int -virFileSetXAttr(const char *path ATTRIBUTE_UNUSED, - const char *name ATTRIBUTE_UNUSED, +virFileSetXAttr(const char *path, + const char *name, const char *value ATTRIBUTE_UNUSED) { errno = ENOSYS; + virReportSystemError(errno, + _("Unable to set XATTR %s on %s"), + name, path); return -1; } -- 2.19.2

It's better to have the function report errors, because none of the callers does. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/virfile.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index fbcab404e7..7e1452c6f2 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -4420,13 +4420,20 @@ virFileSetXAttr(const char *path, * Remove xattr of @name on @path. * * Returns: 0 on success, - * -1 otherwise (with errno set). + * -1 otherwise (with errno set AND error reported). */ int virFileRemoveXAttr(const char *path, const char *name) { - return removexattr(path, name); + if (removexattr(path, name) < 0) { + virReportSystemError(errno, + _("Unable to remove XATTR %s on %s"), + name, path); + return -1; + } + + return 0; } #else /* !HAVE_LIBATTR */ @@ -4453,10 +4460,13 @@ virFileSetXAttr(const char *path, } int -virFileRemoveXAttr(const char *path ATTRIBUTE_UNUSED, - const char *name ATTRIBUTE_UNUSED) +virFileRemoveXAttr(const char *path, + const char *name) { errno = ENOSYS; + virReportSystemError(errno, + _("Unable to remove XATTR %s on %s"), + name, path); return -1; } -- 2.19.2

The way that virSecurityDACRecallLabel is currently written is that if XATTRs are not supported for given path to the caller this is not different than if the path is still in use. The value of 1 is returned which makes secdrivers skip label restore. This is clearly a bug as we are not restoring labels on say NFS even though previously we were. Strictly speaking, changes to virSecurityDACRememberLabel are not needed, but they are done anyway so that getter and setter behave in the same fashion. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/security_dac.c | 18 ++++++++++++------ src/security/security_selinux.c | 21 +++++++++++++++------ src/security/security_util.c | 6 ++++-- 3 files changed, 31 insertions(+), 14 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 6f8ca8cd54..72026646cf 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -458,10 +458,11 @@ virSecurityDACRecallLabel(virSecurityDACDataPtr priv ATTRIBUTE_UNUSED, { char *label; int ret = -1; + int rv; - if (virSecurityGetRememberedLabel(SECURITY_DAC_NAME, - path, &label) < 0) - goto cleanup; + rv = virSecurityGetRememberedLabel(SECURITY_DAC_NAME, path, &label); + if (rv < 0) + return rv; if (!label) return 1; @@ -760,7 +761,9 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, } refcount = virSecurityDACRememberLabel(priv, path, sb.st_uid, sb.st_gid); - if (refcount < 0) { + if (refcount == -2) { + /* Not supported. Don't error though. */ + } else if (refcount < 0) { return -1; } else if (refcount > 1) { /* Refcount is greater than 1 which means that there @@ -827,10 +830,13 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr, if (recall && path) { rv = virSecurityDACRecallLabel(priv, path, &uid, &gid); - if (rv < 0) + if (rv == -2) { + /* Not supported. Don't error though. */ + } else if (rv < 0) { return -1; - if (rv > 0) + } else if (rv > 0) { return 0; + } } VIR_INFO("Restoring DAC user and group on '%s' to %ld:%ld", diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index fb631cd321..667ad0fbd4 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -207,9 +207,11 @@ static int virSecuritySELinuxRecallLabel(const char *path, security_context_t *con) { - if (virSecurityGetRememberedLabel(SECURITY_SELINUX_NAME, - path, con) < 0) - return -1; + int rv; + + rv = virSecurityGetRememberedLabel(SECURITY_SELINUX_NAME, path, con); + if (rv < 0) + return rv; if (!*con) return 1; @@ -1337,7 +1339,9 @@ virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, if (econ) { refcount = virSecuritySELinuxRememberLabel(path, econ); - if (refcount < 0) { + if (refcount == -2) { + /* Not supported. Don't error though. */ + } else if (refcount < 0) { goto cleanup; } else if (refcount > 1) { /* Refcount is greater than 1 which means that there @@ -1485,13 +1489,18 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr, } if (recall) { - if ((rc = virSecuritySELinuxRecallLabel(newpath, &fcon)) < 0) { + rc = virSecuritySELinuxRecallLabel(newpath, &fcon); + if (rc == -2) { + /* Not supported. Lookup the default label below. */ + } else if (rc < 0) { goto cleanup; } else if (rc > 0) { ret = 0; goto cleanup; } - } else { + } + + if (!recall || rc == -2) { if (stat(newpath, &buf) != 0) { VIR_WARN("cannot stat %s: %s", newpath, virStrerror(errno, ebuf, sizeof(ebuf))); diff --git a/src/security/security_util.c b/src/security/security_util.c index f09a18a623..3c24d7cded 100644 --- a/src/security/security_util.c +++ b/src/security/security_util.c @@ -105,6 +105,7 @@ virSecurityGetRefCountAttrName(const char *name ATTRIBUTE_UNUSED) * zero) and returns zero. * * Returns: 0 on success, + * -2 if underlying file system doesn't support XATTRs, * -1 otherwise (with error reported) */ int @@ -125,7 +126,7 @@ virSecurityGetRememberedLabel(const char *name, if (virFileGetXAttrQuiet(path, ref_name, &value) < 0) { if (errno == ENOSYS || errno == ENODATA || errno == ENOTSUP) { - ret = 0; + ret = -2; } else { virReportSystemError(errno, _("Unable to get XATTR %s on %s"), @@ -192,6 +193,7 @@ virSecurityGetRememberedLabel(const char *name, * See also virSecurityGetRememberedLabel. * * Returns: the new refcount value on success, + * -2 if underlying file system doesn't support XATTRs, * -1 otherwise (with error reported) */ int @@ -210,7 +212,7 @@ virSecuritySetRememberedLabel(const char *name, if (virFileGetXAttrQuiet(path, ref_name, &value) < 0) { if (errno == ENOSYS || errno == ENOTSUP) { - ret = 0; + ret = -2; goto cleanup; } else if (errno != ENODATA) { virReportSystemError(errno, -- 2.19.2

Both DAC and SELinux drivers support transactions. Each item on the transaction list consists of various variables and @restore is one of them. Document it so that as the list of variables grow it's easier to spot which variable does what. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/security_dac.c | 5 ++++- src/security/security_selinux.c | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 72026646cf..03c7f8363b 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -79,7 +79,7 @@ struct _virSecurityDACChownItem { const virStorageSource *src; uid_t uid; gid_t gid; - bool restore; + bool restore; /* Whether current operation is set or restore */ }; typedef struct _virSecurityDACChownList virSecurityDACChownList; @@ -155,8 +155,11 @@ virSecurityDACChownListFree(void *opaque) * @src: disk source to chown * @uid: user ID * @gid: group ID + * @restore: if current operation is set or restore * * Appends an entry onto transaction list. + * The @restore should be true if the operation is restoring + * seclabel and false otherwise. * * Returns: 1 in case of successful append * 0 if there is no transaction enabled diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 667ad0fbd4..3cb7e1b3bc 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -82,7 +82,7 @@ struct _virSecuritySELinuxContextItem { char *path; char *tcon; bool optional; - bool restore; + bool restore; /* Whether current operation is set or restore */ }; typedef struct _virSecuritySELinuxContextList virSecuritySELinuxContextList; @@ -168,8 +168,11 @@ virSecuritySELinuxContextListFree(void *opaque) * @path: Path to chown * @tcon: target context * @optional: true if setting @tcon is optional + * @restore: if current operation is set or restore * * Appends an entry onto transaction list. + * The @restore should be true if the operation is restoring + * seclabel and false otherwise. * * Returns: 1 in case of successful append * 0 if there is no transaction enabled -- 2.19.2

One caller in particular (virSecurityDACSetImageLabelInternal) will want to have the feature turned on only in some cases. Introduce @remember member to _virSecurityDACChownItem to track whether caller wants to do owner remembering or not. The actual remembering is then enabled if both caller wanted it and the feature is turned on in the config file. Technically, we could skip over paths that don't have remember enabled when creating a list of paths to lock. We won't touch their XATTRs after all. Well, I rather play it safe and keep them on the locking list for now. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/security_dac.c | 63 ++++++++++++++++++++++--------------- 1 file changed, 37 insertions(+), 26 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 03c7f8363b..e47f0343e7 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -79,6 +79,7 @@ struct _virSecurityDACChownItem { const virStorageSource *src; uid_t uid; gid_t gid; + bool remember; /* Whether owner remembering should be done for @path/@src */ bool restore; /* Whether current operation is set or restore */ }; @@ -100,6 +101,7 @@ virSecurityDACChownListAppend(virSecurityDACChownListPtr list, const virStorageSource *src, uid_t uid, gid_t gid, + bool remember, bool restore) { int ret = -1; @@ -116,6 +118,7 @@ virSecurityDACChownListAppend(virSecurityDACChownListPtr list, item->src = src; item->uid = uid; item->gid = gid; + item->remember = remember; item->restore = restore; if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0) @@ -155,9 +158,12 @@ virSecurityDACChownListFree(void *opaque) * @src: disk source to chown * @uid: user ID * @gid: group ID + * @remember: if the original owner should be recorded/recalled * @restore: if current operation is set or restore * * Appends an entry onto transaction list. + * The @remember should be true if caller wishes to record/recall + * the original owner of @path/@src. * The @restore should be true if the operation is restoring * seclabel and false otherwise. * @@ -170,13 +176,15 @@ virSecurityDACTransactionAppend(const char *path, const virStorageSource *src, uid_t uid, gid_t gid, + bool remember, bool restore) { virSecurityDACChownListPtr list = virThreadLocalGet(&chownList); if (!list) return 0; - if (virSecurityDACChownListAppend(list, path, src, uid, gid, restore) < 0) + if (virSecurityDACChownListAppend(list, path, src, + uid, gid, remember, restore) < 0) return -1; return 1; @@ -235,6 +243,7 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED, for (i = 0; i < list->nItems; i++) { virSecurityDACChownItemPtr item = list->items[i]; + const bool remember = item->remember && list->lock; if (!item->restore) { rv = virSecurityDACSetOwnership(list->manager, @@ -242,12 +251,12 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED, item->path, item->uid, item->gid, - list->lock); + remember); } else { rv = virSecurityDACRestoreFileLabelInternal(list->manager, item->src, item->path, - list->lock); + remember); } if (rv < 0) @@ -256,12 +265,13 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED, for (; rv < 0 && i > 0; i--) { virSecurityDACChownItemPtr item = list->items[i - 1]; + const bool remember = item->remember && list->lock; if (!item->restore) { virSecurityDACRestoreFileLabelInternal(list->manager, item->src, item->path, - list->lock); + remember); } else { VIR_WARN("Ignoring failed restore attempt on %s", NULLSTR(item->src ? item->src->path : item->path)); @@ -752,7 +762,8 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, /* Be aware that this function might run in a separate process. * Therefore, any driver state changes would be thrown away. */ - if ((rc = virSecurityDACTransactionAppend(path, src, uid, gid, false)) < 0) + if ((rc = virSecurityDACTransactionAppend(path, src, + uid, gid, remember, false)) < 0) return -1; else if (rc > 0) return 0; @@ -826,7 +837,7 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr, /* Be aware that this function might run in a separate process. * Therefore, any driver state changes would be thrown away. */ - if ((rv = virSecurityDACTransactionAppend(path, src, uid, gid, true)) < 0) + if ((rv = virSecurityDACTransactionAppend(path, src, uid, gid, recall, true)) < 0) return -1; else if (rv > 0) return 0; @@ -853,7 +864,7 @@ static int virSecurityDACRestoreFileLabel(virSecurityManagerPtr mgr, const char *path) { - return virSecurityDACRestoreFileLabelInternal(mgr, NULL, path, false); + return virSecurityDACRestoreFileLabelInternal(mgr, NULL, path, true); } @@ -900,7 +911,7 @@ virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr, return -1; } - return virSecurityDACSetOwnership(mgr, src, NULL, user, group, false); + return virSecurityDACSetOwnership(mgr, src, NULL, user, group, true); } @@ -967,7 +978,7 @@ virSecurityDACRestoreImageLabelInt(virSecurityManagerPtr mgr, } } - return virSecurityDACRestoreFileLabelInternal(mgr, src, NULL, false); + return virSecurityDACRestoreFileLabelInternal(mgr, src, NULL, true); } @@ -995,7 +1006,7 @@ virSecurityDACSetHostdevLabelHelper(const char *file, if (virSecurityDACGetIds(secdef, priv, &user, &group, NULL, NULL) < 0) return -1; - return virSecurityDACSetOwnership(mgr, NULL, file, user, group, false); + return virSecurityDACSetOwnership(mgr, NULL, file, user, group, true); } @@ -1371,7 +1382,7 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, case VIR_DOMAIN_CHR_TYPE_FILE: ret = virSecurityDACSetOwnership(mgr, NULL, dev_source->data.file.path, - user, group, false); + user, group, true); break; case VIR_DOMAIN_CHR_TYPE_PIPE: @@ -1379,12 +1390,12 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, virAsprintf(&out, "%s.out", dev_source->data.file.path) < 0) goto done; if (virFileExists(in) && virFileExists(out)) { - if (virSecurityDACSetOwnership(mgr, NULL, in, user, group, false) < 0 || - virSecurityDACSetOwnership(mgr, NULL, out, user, group, false) < 0) + if (virSecurityDACSetOwnership(mgr, NULL, in, user, group, true) < 0 || + virSecurityDACSetOwnership(mgr, NULL, out, user, group, true) < 0) goto done; } else if (virSecurityDACSetOwnership(mgr, NULL, dev_source->data.file.path, - user, group, false) < 0) { + user, group, true) < 0) { goto done; } ret = 0; @@ -1399,7 +1410,7 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, * and passed via FD */ if (virSecurityDACSetOwnership(mgr, NULL, dev_source->data.nix.path, - user, group, false) < 0) + user, group, true) < 0) goto done; } ret = 0; @@ -1582,7 +1593,7 @@ virSecurityDACSetGraphicsLabel(virSecurityManagerPtr mgr, if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL) < 0) return -1; - if (virSecurityDACSetOwnership(mgr, NULL, rendernode, user, group, false) < 0) + if (virSecurityDACSetOwnership(mgr, NULL, rendernode, user, group, true) < 0) return -1; return 0; @@ -1625,7 +1636,7 @@ virSecurityDACSetInputLabel(virSecurityManagerPtr mgr, ret = virSecurityDACSetOwnership(mgr, NULL, input->source.evdev, - user, group, false); + user, group, true); break; case VIR_DOMAIN_INPUT_TYPE_MOUSE: @@ -1830,7 +1841,7 @@ virSecurityDACSetMemoryLabel(virSecurityManagerPtr mgr, ret = virSecurityDACSetOwnership(mgr, NULL, mem->nvdimmPath, - user, group, false); + user, group, true); break; case VIR_DOMAIN_MEMORY_MODEL_DIMM: @@ -1867,7 +1878,7 @@ virSecurityDACSetSEVLabel(virSecurityManagerPtr mgr, return -1; if (virSecurityDACSetOwnership(mgr, NULL, DEV_SEV, - user, group, false) < 0) + user, group, true) < 0) return -1; return 0; @@ -1954,31 +1965,31 @@ virSecurityDACSetAllLabel(virSecurityManagerPtr mgr, if (def->os.loader && def->os.loader->nvram && virSecurityDACSetOwnership(mgr, NULL, def->os.loader->nvram, - user, group, false) < 0) + user, group, true) < 0) return -1; if (def->os.kernel && virSecurityDACSetOwnership(mgr, NULL, def->os.kernel, - user, group, false) < 0) + user, group, true) < 0) return -1; if (def->os.initrd && virSecurityDACSetOwnership(mgr, NULL, def->os.initrd, - user, group, false) < 0) + user, group, true) < 0) return -1; if (def->os.dtb && virSecurityDACSetOwnership(mgr, NULL, def->os.dtb, - user, group, false) < 0) + user, group, true) < 0) return -1; if (def->os.slic_table && virSecurityDACSetOwnership(mgr, NULL, def->os.slic_table, - user, group, false) < 0) + user, group, true) < 0) return -1; return 0; @@ -2000,7 +2011,7 @@ virSecurityDACSetSavedStateLabel(virSecurityManagerPtr mgr, if (virSecurityDACGetImageIds(secdef, priv, &user, &group) < 0) return -1; - return virSecurityDACSetOwnership(mgr, NULL, savefile, user, group, false); + return virSecurityDACSetOwnership(mgr, NULL, savefile, user, group, true); } @@ -2320,7 +2331,7 @@ virSecurityDACDomainSetPathLabel(virSecurityManagerPtr mgr, if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL) < 0) return -1; - return virSecurityDACSetOwnership(mgr, NULL, path, user, group, false); + return virSecurityDACSetOwnership(mgr, NULL, path, user, group, true); } virSecurityDriver virSecurityDriverDAC = { -- 2.19.2

Just like previous commit allowed to enable or disable owner remembering for each individual path, do the same for SELinux driver. This is going to be needed in the next commit. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/security_selinux.c | 163 ++++++++++++++++++-------------- 1 file changed, 94 insertions(+), 69 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 3cb7e1b3bc..e696311b09 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -82,6 +82,7 @@ struct _virSecuritySELinuxContextItem { char *path; char *tcon; bool optional; + bool remember; /* Whether owner remembering should be done for @path/@src */ bool restore; /* Whether current operation is set or restore */ }; @@ -122,6 +123,7 @@ virSecuritySELinuxContextListAppend(virSecuritySELinuxContextListPtr list, const char *path, const char *tcon, bool optional, + bool remember, bool restore) { int ret = -1; @@ -134,6 +136,7 @@ virSecuritySELinuxContextListAppend(virSecuritySELinuxContextListPtr list, goto cleanup; item->optional = optional; + item->remember = remember; item->restore = restore; if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0) @@ -168,9 +171,12 @@ virSecuritySELinuxContextListFree(void *opaque) * @path: Path to chown * @tcon: target context * @optional: true if setting @tcon is optional + * @remember: if the original owner should be recorded/recalled * @restore: if current operation is set or restore * * Appends an entry onto transaction list. + * The @remember should be true if caller wishes to record/recall + * the original owner of @path/@src. * The @restore should be true if the operation is restoring * seclabel and false otherwise. * @@ -182,6 +188,7 @@ static int virSecuritySELinuxTransactionAppend(const char *path, const char *tcon, bool optional, + bool remember, bool restore) { virSecuritySELinuxContextListPtr list; @@ -190,7 +197,8 @@ virSecuritySELinuxTransactionAppend(const char *path, if (!list) return 0; - if (virSecuritySELinuxContextListAppend(list, path, tcon, optional, restore) < 0) + if (virSecuritySELinuxContextListAppend(list, path, tcon, + optional, remember, restore) < 0) return -1; return 1; @@ -276,17 +284,18 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED, rv = 0; for (i = 0; i < list->nItems; i++) { virSecuritySELinuxContextItemPtr item = list->items[i]; + const bool remember = item->remember && list->lock; if (!item->restore) { rv = virSecuritySELinuxSetFileconHelper(list->manager, item->path, item->tcon, item->optional, - list->lock); + remember); } else { rv = virSecuritySELinuxRestoreFileLabel(list->manager, item->path, - list->lock); + remember); } if (rv < 0) @@ -295,11 +304,12 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED, for (; rv < 0 && i > 0; i--) { virSecuritySELinuxContextItemPtr item = list->items[i - 1]; + const bool remember = item->remember && list->lock; if (!item->restore) { virSecuritySELinuxRestoreFileLabel(list->manager, item->path, - list->lock); + remember); } else { VIR_WARN("Ignoring failed restore attempt on %s", item->path); } @@ -1326,7 +1336,8 @@ virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, int rc; int ret = -1; - if ((rc = virSecuritySELinuxTransactionAppend(path, tcon, optional, false)) < 0) + if ((rc = virSecuritySELinuxTransactionAppend(path, tcon, + optional, remember, false)) < 0) return -1; else if (rc > 0) return 0; @@ -1389,16 +1400,20 @@ virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, static int virSecuritySELinuxSetFileconOptional(virSecurityManagerPtr mgr, - const char *path, const char *tcon) + const char *path, + const char *tcon, + bool remember) { - return virSecuritySELinuxSetFileconHelper(mgr, path, tcon, true, false); + return virSecuritySELinuxSetFileconHelper(mgr, path, tcon, true, remember); } static int virSecuritySELinuxSetFilecon(virSecurityManagerPtr mgr, - const char *path, const char *tcon) + const char *path, + const char *tcon, + bool remember) { - return virSecuritySELinuxSetFileconHelper(mgr, path, tcon, false, false); + return virSecuritySELinuxSetFileconHelper(mgr, path, tcon, false, remember); } static int @@ -1484,7 +1499,8 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr, goto cleanup; } - if ((rc = virSecuritySELinuxTransactionAppend(path, NULL, false, true)) < 0) { + if ((rc = virSecuritySELinuxTransactionAppend(path, NULL, + false, recall, true)) < 0) { goto cleanup; } else if (rc > 0) { ret = 0; @@ -1545,7 +1561,7 @@ virSecuritySELinuxSetInputLabel(virSecurityManagerPtr mgr, switch ((virDomainInputType)input->type) { case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH: if (virSecuritySELinuxSetFilecon(mgr, input->source.evdev, - seclabel->imagelabel) < 0) + seclabel->imagelabel, true) < 0) return -1; break; @@ -1574,7 +1590,7 @@ virSecuritySELinuxRestoreInputLabel(virSecurityManagerPtr mgr, switch ((virDomainInputType)input->type) { case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH: - rc = virSecuritySELinuxRestoreFileLabel(mgr, input->source.evdev, false); + rc = virSecuritySELinuxRestoreFileLabel(mgr, input->source.evdev, true); break; case VIR_DOMAIN_INPUT_TYPE_MOUSE: @@ -1602,7 +1618,7 @@ virSecuritySELinuxSetMemoryLabel(virSecurityManagerPtr mgr, return 0; if (virSecuritySELinuxSetFilecon(mgr, mem->nvdimmPath, - seclabel->imagelabel) < 0) + seclabel->imagelabel, true) < 0) return -1; break; @@ -1630,7 +1646,7 @@ virSecuritySELinuxRestoreMemoryLabel(virSecurityManagerPtr mgr, if (!seclabel || !seclabel->relabel) return 0; - ret = virSecuritySELinuxRestoreFileLabel(mgr, mem->nvdimmPath, false); + ret = virSecuritySELinuxRestoreFileLabel(mgr, mem->nvdimmPath, true); break; case VIR_DOMAIN_MEMORY_MODEL_DIMM: @@ -1661,14 +1677,14 @@ virSecuritySELinuxSetTPMFileLabel(virSecurityManagerPtr mgr, switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: tpmdev = tpm->data.passthrough.source.data.file.path; - rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel); + rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel, true); if (rc < 0) return -1; if ((cancel_path = virTPMCreateCancelPath(tpmdev)) != NULL) { rc = virSecuritySELinuxSetFilecon(mgr, cancel_path, - seclabel->imagelabel); + seclabel->imagelabel, true); VIR_FREE(cancel_path); if (rc < 0) { virSecuritySELinuxRestoreTPMFileLabelInt(mgr, def, tpm); @@ -1680,7 +1696,7 @@ virSecuritySELinuxSetTPMFileLabel(virSecurityManagerPtr mgr, break; case VIR_DOMAIN_TPM_TYPE_EMULATOR: tpmdev = tpm->data.emulator.source.data.nix.path; - rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel); + rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel, true); if (rc < 0) return -1; break; @@ -1709,10 +1725,10 @@ virSecuritySELinuxRestoreTPMFileLabelInt(virSecurityManagerPtr mgr, switch (tpm->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: tpmdev = tpm->data.passthrough.source.data.file.path; - rc = virSecuritySELinuxRestoreFileLabel(mgr, tpmdev, false); + rc = virSecuritySELinuxRestoreFileLabel(mgr, tpmdev, true); if ((cancel_path = virTPMCreateCancelPath(tpmdev)) != NULL) { - if (virSecuritySELinuxRestoreFileLabel(mgr, cancel_path, false) < 0) + if (virSecuritySELinuxRestoreFileLabel(mgr, cancel_path, true) < 0) rc = -1; VIR_FREE(cancel_path); } @@ -1779,7 +1795,7 @@ virSecuritySELinuxRestoreImageLabelInt(virSecurityManagerPtr mgr, } } - return virSecuritySELinuxRestoreFileLabel(mgr, src->path, false); + return virSecuritySELinuxRestoreFileLabel(mgr, src->path, true); } @@ -1822,32 +1838,38 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr, if (!disk_seclabel->relabel) return 0; - ret = virSecuritySELinuxSetFilecon(mgr, src->path, disk_seclabel->label); + ret = virSecuritySELinuxSetFilecon(mgr, src->path, + disk_seclabel->label, true); } else if (parent_seclabel && (!parent_seclabel->relabel || parent_seclabel->label)) { if (!parent_seclabel->relabel) return 0; - ret = virSecuritySELinuxSetFilecon(mgr, src->path, parent_seclabel->label); + ret = virSecuritySELinuxSetFilecon(mgr, src->path, + parent_seclabel->label, true); } else if (!parent || parent == src) { if (src->shared) { ret = virSecuritySELinuxSetFileconOptional(mgr, src->path, - data->file_context); + data->file_context, + true); } else if (src->readonly) { ret = virSecuritySELinuxSetFileconOptional(mgr, src->path, - data->content_context); + data->content_context, + true); } else if (secdef->imagelabel) { ret = virSecuritySELinuxSetFileconOptional(mgr, src->path, - secdef->imagelabel); + secdef->imagelabel, + true); } else { ret = 0; } } else { ret = virSecuritySELinuxSetFileconOptional(mgr, src->path, - data->content_context); + data->content_context, + true); } if (ret == 1 && !disk_seclabel) { @@ -1900,7 +1922,7 @@ virSecuritySELinuxSetHostdevLabelHelper(const char *file, void *opaque) secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); if (secdef == NULL) return 0; - return virSecuritySELinuxSetFilecon(mgr, file, secdef->imagelabel); + return virSecuritySELinuxSetFilecon(mgr, file, secdef->imagelabel, true); } static int @@ -1932,13 +1954,13 @@ virSecuritySELinuxSetSCSILabel(virSCSIDevicePtr dev, if (virSCSIDeviceGetShareable(dev)) return virSecuritySELinuxSetFileconOptional(mgr, file, - data->file_context); + data->file_context, true); else if (virSCSIDeviceGetReadonly(dev)) return virSecuritySELinuxSetFileconOptional(mgr, file, - data->content_context); + data->content_context, true); else return virSecuritySELinuxSetFileconOptional(mgr, file, - secdef->imagelabel); + secdef->imagelabel, true); } static int @@ -2093,7 +2115,7 @@ virSecuritySELinuxSetHostdevCapsLabel(virSecurityManagerPtr mgr, if (VIR_STRDUP(path, dev->source.caps.u.storage.block) < 0) return -1; } - ret = virSecuritySELinuxSetFilecon(mgr, path, secdef->imagelabel); + ret = virSecuritySELinuxSetFilecon(mgr, path, secdef->imagelabel, true); VIR_FREE(path); break; } @@ -2107,7 +2129,7 @@ virSecuritySELinuxSetHostdevCapsLabel(virSecurityManagerPtr mgr, if (VIR_STRDUP(path, dev->source.caps.u.misc.chardev) < 0) return -1; } - ret = virSecuritySELinuxSetFilecon(mgr, path, secdef->imagelabel); + ret = virSecuritySELinuxSetFilecon(mgr, path, secdef->imagelabel, true); VIR_FREE(path); break; } @@ -2153,7 +2175,7 @@ virSecuritySELinuxRestorePCILabel(virPCIDevicePtr dev ATTRIBUTE_UNUSED, { virSecurityManagerPtr mgr = opaque; - return virSecuritySELinuxRestoreFileLabel(mgr, file, false); + return virSecuritySELinuxRestoreFileLabel(mgr, file, true); } static int @@ -2163,7 +2185,7 @@ virSecuritySELinuxRestoreUSBLabel(virUSBDevicePtr dev ATTRIBUTE_UNUSED, { virSecurityManagerPtr mgr = opaque; - return virSecuritySELinuxRestoreFileLabel(mgr, file, false); + return virSecuritySELinuxRestoreFileLabel(mgr, file, true); } @@ -2180,7 +2202,7 @@ virSecuritySELinuxRestoreSCSILabel(virSCSIDevicePtr dev, if (virSCSIDeviceGetShareable(dev) || virSCSIDeviceGetReadonly(dev)) return 0; - return virSecuritySELinuxRestoreFileLabel(mgr, file, false); + return virSecuritySELinuxRestoreFileLabel(mgr, file, true); } static int @@ -2190,7 +2212,7 @@ virSecuritySELinuxRestoreHostLabel(virSCSIVHostDevicePtr dev ATTRIBUTE_UNUSED, { virSecurityManagerPtr mgr = opaque; - return virSecuritySELinuxRestoreFileLabel(mgr, file, false); + return virSecuritySELinuxRestoreFileLabel(mgr, file, true); } @@ -2294,7 +2316,7 @@ virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManagerPtr mgr, if (!(vfiodev = virMediatedDeviceGetIOMMUGroupDev(mdevsrc->uuidstr))) goto done; - ret = virSecuritySELinuxRestoreFileLabel(mgr, vfiodev, false); + ret = virSecuritySELinuxRestoreFileLabel(mgr, vfiodev, true); VIR_FREE(vfiodev); break; @@ -2328,7 +2350,7 @@ virSecuritySELinuxRestoreHostdevCapsLabel(virSecurityManagerPtr mgr, if (VIR_STRDUP(path, dev->source.caps.u.storage.block) < 0) return -1; } - ret = virSecuritySELinuxRestoreFileLabel(mgr, path, false); + ret = virSecuritySELinuxRestoreFileLabel(mgr, path, true); VIR_FREE(path); break; } @@ -2342,7 +2364,7 @@ virSecuritySELinuxRestoreHostdevCapsLabel(virSecurityManagerPtr mgr, if (VIR_STRDUP(path, dev->source.caps.u.misc.chardev) < 0) return -1; } - ret = virSecuritySELinuxRestoreFileLabel(mgr, path, false); + ret = virSecuritySELinuxRestoreFileLabel(mgr, path, true); VIR_FREE(path); break; } @@ -2420,14 +2442,16 @@ virSecuritySELinuxSetChardevLabel(virSecurityManagerPtr mgr, case VIR_DOMAIN_CHR_TYPE_FILE: ret = virSecuritySELinuxSetFilecon(mgr, dev_source->data.file.path, - imagelabel); + imagelabel, + true); break; case VIR_DOMAIN_CHR_TYPE_UNIX: if (!dev_source->data.nix.listen) { if (virSecuritySELinuxSetFilecon(mgr, dev_source->data.nix.path, - imagelabel) < 0) + imagelabel, + true) < 0) goto done; } ret = 0; @@ -2438,13 +2462,14 @@ virSecuritySELinuxSetChardevLabel(virSecurityManagerPtr mgr, (virAsprintf(&out, "%s.out", dev_source->data.file.path) < 0)) goto done; if (virFileExists(in) && virFileExists(out)) { - if ((virSecuritySELinuxSetFilecon(mgr, in, imagelabel) < 0) || - (virSecuritySELinuxSetFilecon(mgr, out, imagelabel) < 0)) { + if ((virSecuritySELinuxSetFilecon(mgr, in, imagelabel, true) < 0) || + (virSecuritySELinuxSetFilecon(mgr, out, imagelabel, true) < 0)) { goto done; } } else if (virSecuritySELinuxSetFilecon(mgr, dev_source->data.file.path, - imagelabel) < 0) { + imagelabel, + true) < 0) { goto done; } ret = 0; @@ -2492,7 +2517,7 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr, case VIR_DOMAIN_CHR_TYPE_FILE: if (virSecuritySELinuxRestoreFileLabel(mgr, dev_source->data.file.path, - false) < 0) + true) < 0) goto done; ret = 0; break; @@ -2501,7 +2526,7 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr, if (!dev_source->data.nix.listen) { if (virSecuritySELinuxRestoreFileLabel(mgr, dev_source->data.file.path, - false) < 0) + true) < 0) goto done; } ret = 0; @@ -2512,13 +2537,13 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr, (virAsprintf(&in, "%s.in", dev_source->data.file.path) < 0)) goto done; if (virFileExists(in) && virFileExists(out)) { - if ((virSecuritySELinuxRestoreFileLabel(mgr, out, false) < 0) || - (virSecuritySELinuxRestoreFileLabel(mgr, in, false) < 0)) { + if ((virSecuritySELinuxRestoreFileLabel(mgr, out, true) < 0) || + (virSecuritySELinuxRestoreFileLabel(mgr, in, true) < 0)) { goto done; } } else if (virSecuritySELinuxRestoreFileLabel(mgr, dev_source->data.file.path, - false) < 0) { + true) < 0) { goto done; } ret = 0; @@ -2570,7 +2595,7 @@ virSecuritySELinuxRestoreSecuritySmartcardCallback(virDomainDefPtr def, database = dev->data.cert.database; if (!database) database = VIR_DOMAIN_SMARTCARD_DEFAULT_DATABASE; - return virSecuritySELinuxRestoreFileLabel(mgr, database, false); + return virSecuritySELinuxRestoreFileLabel(mgr, database, true); case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH: return virSecuritySELinuxRestoreChardevLabel(mgr, def, @@ -2665,23 +2690,23 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManagerPtr mgr, rc = -1; if (def->os.loader && def->os.loader->nvram && - virSecuritySELinuxRestoreFileLabel(mgr, def->os.loader->nvram, false) < 0) + virSecuritySELinuxRestoreFileLabel(mgr, def->os.loader->nvram, true) < 0) rc = -1; if (def->os.kernel && - virSecuritySELinuxRestoreFileLabel(mgr, def->os.kernel, false) < 0) + virSecuritySELinuxRestoreFileLabel(mgr, def->os.kernel, true) < 0) rc = -1; if (def->os.initrd && - virSecuritySELinuxRestoreFileLabel(mgr, def->os.initrd, false) < 0) + virSecuritySELinuxRestoreFileLabel(mgr, def->os.initrd, true) < 0) rc = -1; if (def->os.dtb && - virSecuritySELinuxRestoreFileLabel(mgr, def->os.dtb, false) < 0) + virSecuritySELinuxRestoreFileLabel(mgr, def->os.dtb, true) < 0) rc = -1; if (def->os.slic_table && - virSecuritySELinuxRestoreFileLabel(mgr, def->os.slic_table, false) < 0) + virSecuritySELinuxRestoreFileLabel(mgr, def->os.slic_table, true) < 0) rc = -1; return rc; @@ -2726,7 +2751,7 @@ virSecuritySELinuxSetSavedStateLabel(virSecurityManagerPtr mgr, if (!secdef || !secdef->relabel) return 0; - return virSecuritySELinuxSetFilecon(mgr, savefile, secdef->imagelabel); + return virSecuritySELinuxSetFilecon(mgr, savefile, secdef->imagelabel, true); } @@ -2741,7 +2766,7 @@ virSecuritySELinuxRestoreSavedStateLabel(virSecurityManagerPtr mgr, if (!secdef || !secdef->relabel) return 0; - return virSecuritySELinuxRestoreFileLabel(mgr, savefile, false); + return virSecuritySELinuxRestoreFileLabel(mgr, savefile, true); } @@ -2984,7 +3009,7 @@ virSecuritySELinuxSetSecuritySmartcardCallback(virDomainDefPtr def, database = dev->data.cert.database; if (!database) database = VIR_DOMAIN_SMARTCARD_DEFAULT_DATABASE; - return virSecuritySELinuxSetFilecon(mgr, database, data->content_context); + return virSecuritySELinuxSetFilecon(mgr, database, data->content_context, true); case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH: return virSecuritySELinuxSetChardevLabel(mgr, def, @@ -3075,32 +3100,32 @@ virSecuritySELinuxSetAllLabel(virSecurityManagerPtr mgr, if (def->os.loader && def->os.loader->nvram && secdef && secdef->imagelabel && virSecuritySELinuxSetFilecon(mgr, def->os.loader->nvram, - secdef->imagelabel) < 0) + secdef->imagelabel, true) < 0) return -1; if (def->os.kernel && virSecuritySELinuxSetFilecon(mgr, def->os.kernel, - data->content_context) < 0) + data->content_context, true) < 0) return -1; if (def->os.initrd && virSecuritySELinuxSetFilecon(mgr, def->os.initrd, - data->content_context) < 0) + data->content_context, true) < 0) return -1; if (def->os.dtb && virSecuritySELinuxSetFilecon(mgr, def->os.dtb, - data->content_context) < 0) + data->content_context, true) < 0) return -1; if (def->os.slic_table && virSecuritySELinuxSetFilecon(mgr, def->os.slic_table, - data->content_context) < 0) + data->content_context, true) < 0) return -1; if (stdin_path && virSecuritySELinuxSetFilecon(mgr, stdin_path, - data->content_context) < 0) + data->content_context, true) < 0) return -1; return 0; @@ -3259,7 +3284,7 @@ virSecuritySELinuxDomainSetPathLabel(virSecurityManagerPtr mgr, if (!seclabel || !seclabel->relabel) return 0; - return virSecuritySELinuxSetFilecon(mgr, path, seclabel->imagelabel); + return virSecuritySELinuxSetFilecon(mgr, path, seclabel->imagelabel, true); } @@ -3284,7 +3309,7 @@ virSecuritySELinuxSetFileLabels(virSecurityManagerPtr mgr, char *filename = NULL; DIR *dir; - if ((ret = virSecuritySELinuxSetFilecon(mgr, path, seclabel->imagelabel))) + if ((ret = virSecuritySELinuxSetFilecon(mgr, path, seclabel->imagelabel, true))) return ret; if (!virFileIsDir(path)) @@ -3302,7 +3327,7 @@ virSecuritySELinuxSetFileLabels(virSecurityManagerPtr mgr, break; } ret = virSecuritySELinuxSetFilecon(mgr, filename, - seclabel->imagelabel); + seclabel->imagelabel, true); VIR_FREE(filename); if (ret < 0) break; @@ -3336,7 +3361,7 @@ virSecuritySELinuxRestoreFileLabels(virSecurityManagerPtr mgr, char *filename = NULL; DIR *dir; - if ((ret = virSecuritySELinuxRestoreFileLabel(mgr, path, false))) + if ((ret = virSecuritySELinuxRestoreFileLabel(mgr, path, true))) return ret; if (!virFileIsDir(path)) @@ -3353,7 +3378,7 @@ virSecuritySELinuxRestoreFileLabels(virSecurityManagerPtr mgr, ret = -1; break; } - ret = virSecuritySELinuxRestoreFileLabel(mgr, filename, false); + ret = virSecuritySELinuxRestoreFileLabel(mgr, filename, true); VIR_FREE(filename); if (ret < 0) break; -- 2.19.2

Here is the problem: If all disks had XATTRs (i.e. domains using them were started with owner remembering turned on) then refcounting implemented in XATTRs would work nicely and we could set the whole backing chain and restore it later. But world is not that simple. As soon as there is one domain that was started with the feature turned off (or simply by older libvirt), the XATTR refounting does not reflect the actual number of uses by running domains and therefore any attempt to restore might cut off the old domain. There is no simple way around this. Except artificially turning the feature off for the rest of the backing chain. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/security_dac.c | 3 ++- src/security/security_selinux.c | 13 +++++++------ 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index e47f0343e7..91e91e378e 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -911,7 +911,8 @@ virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr, return -1; } - return virSecurityDACSetOwnership(mgr, src, NULL, user, group, true); + /* Remember label only for the top level image. */ + return virSecurityDACSetOwnership(mgr, src, NULL, user, group, src == parent); } diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index e696311b09..10585e9f8c 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1819,6 +1819,7 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr, virSecurityLabelDefPtr secdef; virSecurityDeviceLabelDefPtr disk_seclabel; virSecurityDeviceLabelDefPtr parent_seclabel = NULL; + const bool remember = src == parent; int ret; if (!src->path || !virStorageSourceIsLocalStorage(src)) @@ -1839,29 +1840,29 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr, return 0; ret = virSecuritySELinuxSetFilecon(mgr, src->path, - disk_seclabel->label, true); + disk_seclabel->label, remember); } else if (parent_seclabel && (!parent_seclabel->relabel || parent_seclabel->label)) { if (!parent_seclabel->relabel) return 0; ret = virSecuritySELinuxSetFilecon(mgr, src->path, - parent_seclabel->label, true); + parent_seclabel->label, remember); } else if (!parent || parent == src) { if (src->shared) { ret = virSecuritySELinuxSetFileconOptional(mgr, src->path, data->file_context, - true); + remember); } else if (src->readonly) { ret = virSecuritySELinuxSetFileconOptional(mgr, src->path, data->content_context, - true); + remember); } else if (secdef->imagelabel) { ret = virSecuritySELinuxSetFileconOptional(mgr, src->path, secdef->imagelabel, - true); + remember); } else { ret = 0; } @@ -1869,7 +1870,7 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr, ret = virSecuritySELinuxSetFileconOptional(mgr, src->path, data->content_context, - true); + remember); } if (ret == 1 && !disk_seclabel) { -- 2.19.2

The purpose of this API is to allow caller move XATTRs (or remove them) from one file to another. This will be needed when moving top level of disk chain (either by introducing new HEAD or removing it). Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/security/security_driver.h | 5 +++++ src/security/security_manager.c | 39 +++++++++++++++++++++++++++++++++ src/security/security_manager.h | 4 ++++ src/security/security_nop.c | 10 +++++++++ src/security/security_stack.c | 20 +++++++++++++++++ 6 files changed, 79 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8792155312..7b2a876ad4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1381,6 +1381,7 @@ virSecurityManagerGetModel; virSecurityManagerGetMountOptions; virSecurityManagerGetNested; virSecurityManagerGetProcessLabel; +virSecurityManagerMoveImageMetadata; virSecurityManagerNew; virSecurityManagerNewDAC; virSecurityManagerNewStack; diff --git a/src/security/security_driver.h b/src/security/security_driver.h index 36cf9da037..998fe9697c 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -120,6 +120,10 @@ typedef int (*virSecurityDomainRestoreImageLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, virStorageSourcePtr src, virSecurityDomainImageLabelFlags flags); +typedef int (*virSecurityDomainMoveImageMetadata) (virSecurityManagerPtr mgr, + pid_t pid, + virStorageSourcePtr src, + virStorageSourcePtr dst); typedef int (*virSecurityDomainSetMemoryLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainMemoryDefPtr mem); @@ -170,6 +174,7 @@ struct _virSecurityDriver { virSecurityDomainSetImageLabel domainSetSecurityImageLabel; virSecurityDomainRestoreImageLabel domainRestoreSecurityImageLabel; + virSecurityDomainMoveImageMetadata domainMoveImageMetadata; virSecurityDomainSetMemoryLabel domainSetSecurityMemoryLabel; virSecurityDomainRestoreMemoryLabel domainRestoreSecurityMemoryLabel; diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 74ab0d0dd3..c205c3bf17 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -432,6 +432,45 @@ virSecurityManagerRestoreImageLabel(virSecurityManagerPtr mgr, } +/** + * virSecurityManagerMoveImageMetadata: + * @mgr: security manager + * @pid: domain's PID + * @src: source of metadata + * @dst: destination to move metadata to + * + * For given source @src, metadata is moved to destination @dst. + * + * If @dst is NULL then metadata is removed from @src and not + * stored anywhere. + * + * If @pid is not -1 enther the @pid mount namespace (usually + * @pid refers to a domain) and perform the move from there. If + * @pid is -1 then the move is performed from the caller's + * namespace. + * + * Returns: 0 on success, + * -1 otherwise. + */ +int +virSecurityManagerMoveImageMetadata(virSecurityManagerPtr mgr, + pid_t pid, + virStorageSourcePtr src, + virStorageSourcePtr dst) +{ + if (mgr->drv->domainMoveImageMetadata) { + int ret; + virObjectLock(mgr); + ret = mgr->drv->domainMoveImageMetadata(mgr, pid, src, dst); + virObjectUnlock(mgr); + return ret; + } + + virReportUnsupportedError(); + return -1; +} + + int virSecurityManagerSetDaemonSocketLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm) diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 7e174a33ee..33e79b2095 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -160,6 +160,10 @@ int virSecurityManagerRestoreImageLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, virStorageSourcePtr src, virSecurityDomainImageLabelFlags flags); +int virSecurityManagerMoveImageMetadata(virSecurityManagerPtr mgr, + pid_t pid, + virStorageSourcePtr src, + virStorageSourcePtr dst); int virSecurityManagerSetMemoryLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, diff --git a/src/security/security_nop.c b/src/security/security_nop.c index 9b3263ad77..966b9d41a1 100644 --- a/src/security/security_nop.c +++ b/src/security/security_nop.c @@ -224,6 +224,15 @@ virSecurityDomainSetImageLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, return 0; } +static int +virSecurityDomainMoveImageMetadataNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + pid_t pid ATTRIBUTE_UNUSED, + virStorageSourcePtr src ATTRIBUTE_UNUSED, + virStorageSourcePtr dst ATTRIBUTE_UNUSED) +{ + return 0; +} + static int virSecurityDomainSetMemoryLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr def ATTRIBUTE_UNUSED, @@ -280,6 +289,7 @@ virSecurityDriver virSecurityDriverNop = { .domainSetSecurityImageLabel = virSecurityDomainSetImageLabelNop, .domainRestoreSecurityImageLabel = virSecurityDomainRestoreImageLabelNop, + .domainMoveImageMetadata = virSecurityDomainMoveImageMetadataNop, .domainSetSecurityMemoryLabel = virSecurityDomainSetMemoryLabelNop, .domainRestoreSecurityMemoryLabel = virSecurityDomainRestoreMemoryLabelNop, diff --git a/src/security/security_stack.c b/src/security/security_stack.c index eba918e257..d445c0773e 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -599,6 +599,25 @@ virSecurityStackRestoreImageLabel(virSecurityManagerPtr mgr, return rc; } +static int +virSecurityStackMoveImageMetadata(virSecurityManagerPtr mgr, + pid_t pid, + virStorageSourcePtr src, + virStorageSourcePtr dst) +{ + virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityStackItemPtr item = priv->itemsHead; + int rc = 0; + + for (; item; item = item->next) { + if (virSecurityManagerMoveImageMetadata(item->securityManager, + pid, src, dst) < 0) + rc = -1; + } + + return rc; +} + static int virSecurityStackSetMemoryLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, @@ -785,6 +804,7 @@ virSecurityDriver virSecurityDriverStack = { .domainSetSecurityImageLabel = virSecurityStackSetImageLabel, .domainRestoreSecurityImageLabel = virSecurityStackRestoreImageLabel, + .domainMoveImageMetadata = virSecurityStackMoveImageMetadata, .domainSetSecurityMemoryLabel = virSecurityStackSetMemoryLabel, .domainRestoreSecurityMemoryLabel = virSecurityStackRestoreMemoryLabel, -- 2.19.2

On 3/28/19 11:04 AM, Michal Privoznik wrote: This and all the other error reporting is redundant after patches #2-4 - Cole

Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/security/security_selinux.c | 57 +++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 10585e9f8c..08933664da 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1912,6 +1912,62 @@ virSecuritySELinuxSetImageLabel(virSecurityManagerPtr mgr, } +struct virSecuritySELinuxMoveImageMetadataData { + virSecurityManagerPtr mgr; + const char *src; + const char *dst; +}; + + +static int +virSecuritySELinuxMoveImageMetadataHelper(pid_t pid ATTRIBUTE_UNUSED, + void *opaque) +{ + struct virSecuritySELinuxMoveImageMetadataData *data = opaque; + const char *paths[2] = { data->src, data->dst }; + virSecurityManagerMetadataLockStatePtr state; + int ret; + + if (!(state = virSecurityManagerMetadataLock(data->mgr, paths, ARRAY_CARDINALITY(paths)))) + return -1; + + ret = virSecurityMoveRememberedLabel(SECURITY_SELINUX_NAME, data->src, data->dst); + virSecurityManagerMetadataUnlock(data->mgr, &state); + return ret; +} + + +static int +virSecuritySELinuxMoveImageMetadata(virSecurityManagerPtr mgr, + pid_t pid, + virStorageSourcePtr src, + virStorageSourcePtr dst) +{ + struct virSecuritySELinuxMoveImageMetadataData data = { .mgr = mgr, 0 }; + int rc; + + if (src && virStorageSourceIsLocalStorage(src)) + data.src = src->path; + + if (dst && virStorageSourceIsLocalStorage(dst)) + data.dst = dst->path; + + if (!data.src) + return 0; + + if (pid == -1) { + rc = virProcessRunInFork(virSecuritySELinuxMoveImageMetadataHelper, + &data); + } else { + rc = virProcessRunInMountNamespace(pid, + virSecuritySELinuxMoveImageMetadataHelper, + &data); + } + + return rc; +} + + static int virSecuritySELinuxSetHostdevLabelHelper(const char *file, void *opaque) { @@ -3467,6 +3523,7 @@ virSecurityDriver virSecurityDriverSELinux = { .domainSetSecurityImageLabel = virSecuritySELinuxSetImageLabel, .domainRestoreSecurityImageLabel = virSecuritySELinuxRestoreImageLabel, + .domainMoveImageMetadata = virSecuritySELinuxMoveImageMetadata, .domainSetSecurityMemoryLabel = virSecuritySELinuxSetMemoryLabel, .domainRestoreSecurityMemoryLabel = virSecuritySELinuxRestoreMemoryLabel, -- 2.19.2

Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_blockjob.c | 6 ++++++ src/qemu/qemu_driver.c | 17 ++++++++++++++--- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index fa7e4c8625..1b4e30ba01 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -37,6 +37,7 @@ #include "locking/domain_lock.h" #include "viralloc.h" #include "virstring.h" +#include "qemu_security.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -275,6 +276,11 @@ qemuBlockJobEventProcessLegacyCompleted(virQEMUDriverPtr driver, * want to only revoke the non-shared portion of the chain); so for * now, we leak the access to the original. */ virDomainLockImageDetach(driver->lockManager, vm, disk->src); + + /* Move secret driver metadata */ + if (qemuSecurityMoveImageMetadata(driver, vm, disk->src, disk->mirror) < 0) + VIR_WARN("Unable to move disk metadata on vm %s", vm->def->name); + virObjectUnref(disk->src); disk->src = disk->mirror; } else { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 62d8d977c5..1af6272c71 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15173,22 +15173,33 @@ qemuDomainSnapshotUpdateDiskSourcesRenumber(virStorageSourcePtr src) /** * qemuDomainSnapshotUpdateDiskSources: + * @driver: QEMU driver + * @vm: domain object * @dd: snapshot disk data object * @persist: set to true if persistent config of the VM was changed * * Updates disk definition after a successful snapshot. */ static void -qemuDomainSnapshotUpdateDiskSources(qemuDomainSnapshotDiskDataPtr dd, +qemuDomainSnapshotUpdateDiskSources(virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuDomainSnapshotDiskDataPtr dd, bool *persist) { - if (!dd->src) + if (!dd->src) { + /* Remove old metadata */ + if (qemuSecurityMoveImageMetadata(driver, vm, dd->disk->src, NULL) < 0) + VIR_WARN("Unable to remove disk metadata on vm %s", vm->def->name); return; + } /* storage driver access won'd be needed */ if (dd->initialized) virStorageFileDeinit(dd->src); + if (qemuSecurityMoveImageMetadata(driver, vm, dd->disk->src, dd->src) < 0) + VIR_WARN("Unable to move disk metadata on vm %s", vm->def->name); + /* the old disk image is now readonly */ dd->disk->src->readonly = true; @@ -15313,7 +15324,7 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, virDomainAuditDisk(vm, dd->disk->src, dd->src, "snapshot", ret >= 0); if (ret == 0) - qemuDomainSnapshotUpdateDiskSources(dd, &persist); + qemuDomainSnapshotUpdateDiskSources(driver, vm, dd, &persist); } if (ret < 0) -- 2.19.2