Re: [libvirt] [PATCH v3 28/28] security_dac: Lock domain metadata
On 08/27/2018 04:08 AM, Michal Privoznik wrote:
You have nothing to say here, wow we got this far and your speechless...
Or your fingers were just really tired from all those patches!
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/security/security_dac.c | 52 +++++++++++++++++++++++++++++++++++++++------
1 file changed, 46 insertions(+), 6 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 2115e00e07..818548dd22 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -638,6 +638,8 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
struct stat sb;
int rc;
+ bool locked = false;
+ int ret = -1;
if (!path && src && src->path &&
virStorageSourceIsLocalStorage(src))
@@ -657,14 +659,28 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
return -1;
}
+ if (!S_ISDIR(sb.st_mode)) {
Seems we leave ourselves open for a few other odd combinations we don't
want... Should this be restricted to certain things like S_ISREG?
+ if (virSecurityManagerMetadataLock(mgr, path) < 0)
+ return -1;
+ locked = true;
+ }
+
if (virSecurityDACRememberLabel(priv, path, sb.st_uid, sb.st_gid) < 0)
- return -1;
+ goto cleanup;
}
VIR_INFO("Setting DAC user and group on '%s' to
'%ld:%ld'",
NULLSTR(src ? src->path : path), (long)uid, (long)gid);
- return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid);
+ if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid) < 0)
+ goto cleanup;
+
+ ret = 0;
+ cleanup:
+ if (locked &&
+ virSecurityManagerMetadataUnlock(mgr, path) < 0)
+ VIR_WARN("Unable to unlock metadata on %s", path);
+ return ret;
}
@@ -677,6 +693,9 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr,
int rv;
uid_t uid = 0; /* By default return to root:root */
gid_t gid = 0;
+ struct stat sb;
+ bool locked;
Coverity says, please initialize this to false since it's used in cleanup
+ int ret = -1;
if (!path && src && src->path &&
virStorageSourceIsLocalStorage(src))
@@ -691,17 +710,38 @@ virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr,
return 0;
if (path) {
+ if (stat(path, &sb) < 0) {
+ virReportSystemError(errno, _("unable to stat: %s"), path);
+ return -1;
+ }
+
+ if (!S_ISDIR(sb.st_mode)) {
Similar S_ISREG
I'll wait for when there's an update before R_By, but I see nothing
inherently wrong that sticks out - I'm just in search of liquid
refreshment right now, so this is as good as it gets.
John
+ if (virSecurityManagerMetadataLock(mgr, path) < 0)
+ return -1;
+ locked = true;
+ }
+
rv = virSecurityDACRecallLabel(priv, path, &uid, &gid);
if (rv < 0)
- return -1;
- if (rv > 0)
- return 0;
+ goto cleanup;
+ if (rv > 0) {
+ ret = 0;
+ goto cleanup;
+ }
}
VIR_INFO("Restoring DAC user and group on '%s' to %ld:%ld",
NULLSTR(src ? src->path : path), (long)uid, (long)gid);
- return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid);
+ if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid) < 0)
+ goto cleanup;
+
+ ret = 0;
+ cleanup:
+ if (locked &&
+ virSecurityManagerMetadataUnlock(mgr, path) < 0)
+ VIR_WARN("Unable to unlock metadata on %s", path);
+ return ret;
}