On Wed, Aug 14, 2019 at 16:33:23 +0200, Michal Privoznik wrote:
It may happen that we leave some XATTRs behind. For instance, on a sudden power loss, the host just shuts down without calling restore on domain paths. This creates a problem, because when the host starts up again, the XATTRs are there but they don't reflect the true state and this may result in libvirt denying start of a domain.
To solve this, save a unique timestamp among with our XATTRs. The timestamp consists of host UUID + boot timestamp.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_util.c | 202 ++++++++++++++++++++++++++++++++++- tests/qemusecuritymock.c | 12 +++ 2 files changed, 213 insertions(+), 1 deletion(-)
diff --git a/src/security/security_util.c b/src/security/security_util.c index 365b2dd2d6..d063f526be 100644 --- a/src/security/security_util.c +++ b/src/security/security_util.c @@ -22,11 +22,16 @@ #include "virfile.h" #include "virstring.h" #include "virerror.h" +#include "virlog.h" +#include "viruuid.h" +#include "virhostuptime.h"
#include "security_util.h"
#define VIR_FROM_THIS VIR_FROM_SECURITY
+VIR_LOG_INIT("security.security_util"); + /* There are four namespaces available on Linux (xattr(7)): * * user - can be modified by anybody, @@ -83,6 +88,157 @@ virSecurityGetRefCountAttrName(const char *name ATTRIBUTE_UNUSED) }
+static char * +virSecurityGetTimestampAttrName(const char *name ATTRIBUTE_UNUSED) +{ + char *ret = NULL; +#ifdef XATTR_NAMESPACE + ignore_value(virAsprintf(&ret, XATTR_NAMESPACE".libvirt.security.timestamp_%s", name)); +#else + errno = ENOSYS; + virReportSystemError(errno, "%s", + _("Extended attributes are not supported on this system")); +#endif + return ret; +}
Again, put #ifdef outside, please.
+ + +/* This global timestamp holds combination of host UUID + boot time so that we + * can detect stale XATTRs. For instance, on a sudden power loss, XATTRs are + * not going to change (nobody will call restoreLabel()) and thus they reflect + * state from just before the power loss and if there was a machine running, + * then XATTRs there are stale and no one will ever remove them. They don't + * reflect the true state (most notably the ref counter). + */ +static char *timestamp; + + +static int +virSecurityEnsureTimestamp(void) +{ + unsigned char uuid[VIR_UUID_BUFLEN] = {0}; + char uuidstr[VIR_UUID_STRING_BUFLEN] = {0}; + unsigned long long boottime = 0; + + if (timestamp) + return 0; + + if (virGetHostUUID(uuid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot get the host uuid")); + return -1; + } + + virUUIDFormat(uuid, uuidstr); + + if (virHostGetBootTime(&boottime) < 0) { + virReportSystemError(errno, "%s", + _("Unable to get host boot time")); + return -1; + } + + if (virAsprintf(×tamp, "%s-%llu", uuidstr, boottime) < 0) + return -1; + + return 0; +} + + +/** + * virSecurityValidateTimestamp: + * @name: security driver name + * @path: file name + * + * Check if remembered label on @path for security driver @name + * is valid, i.e. the label has been set since the last boot. If + * the label was set in previous runs, all XATTRs related to + * @name are removed so that clean slate is restored. + * + * Returns: 0 if remembered label is valid, + * 1 if remembered label was not valid, + * -1 otherwise. + */ +static int +virSecurityValidateTimestamp(const char *name, + const char *path) +{ + VIR_AUTOFREE(char *) timestamp_name = NULL; + VIR_AUTOFREE(char *) value = NULL; + + if (virSecurityEnsureTimestamp() < 0) + return -1; + + if (!(timestamp_name = virSecurityGetTimestampAttrName(name))) + return -1; + + errno = 0; + if (virFileGetXAttrQuiet(path, timestamp_name, &value) < 0) { + if (errno == ENOSYS || errno == ENOTSUP) { + /* XATTRs are not supported. */
Redundant comment.
+ return -1; + } else if (errno != ENODATA) { + virReportSystemError(errno, + _("Unable to get XATTR %s on %s"), + timestamp_name, + path); + return -1; + } + + /* Timestamp is missing. We can continue and claim a valid timestamp. + * But then we would never remove stale XATTRs. Therefore, claim it + * invalid and have the code below remove all XATTRs. This of course + * means that we will not restore the original owner, but the plus side + * is that we reset refcounter which will represent the true state. + */ + } + + if (STREQ_NULLABLE(value, timestamp)) { + /* Hooray, XATTRs are valid. */
Redundant comment.
+ VIR_DEBUG("XATTRs on %s secdriver=%s are valid", path, name); + return 0; + }
I believe the reason for having UUID in the timestamp is to be able to detect when the label was remembered on a different host. But here, you completely ignore this and always remove the remembered labels when it was remembered on a different host or after a reboot. And since you will need to check the UUID separately from the timestamp, I think it would make sense to store them separately rather than combining them in a single value.
+ + VIR_WARN("Invalid XATTR timestamp detected on %s secdriver=%s", path, name); + + if (virSecurityMoveRememberedLabel(name, path, NULL) < 0) + return -1; + + return 1; +}
Jirka