diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 137daf5d28..4b4afef18a 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -751,6 +751,7 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, bool remember) { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virErrorPtr origerr; struct stat sb; int refcount; int rc; @@ -784,12 +785,13 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, * is @refcount domains using the @path. Do not * change the label (as it would almost certainly * cause the other domains to lose access to the - * @path). */ + * @path). However, the refcounter was incremented in + * XATTRs so decrease it. */ if (sb.st_uid != uid || sb.st_gid != gid) { virReportError(VIR_ERR_OPERATION_INVALID, _("Setting different DAC user or group on %s " "which is already in use"), path); - return -1; + goto error; } } } @@ -797,25 +799,26 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'", NULLSTR(src ? src->path : path), (long)uid, (long)gid); - if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid) < 0) { - virErrorPtr origerr; - - virErrorPreserveLast(&origerr); - /* Try to restore the label. This is done so that XATTRs - * are left in the same state as when the control entered - * this function. However, if our attempt fails, there's - * not much we can do. XATTRs refcounting is fubar'ed and - * the only option we have is warn users. */ - if (virSecurityDACRestoreFileLabelInternal(mgr, src, path, remember) < 0) - VIR_WARN("Unable to restore label on '%s'. " - "XATTRs might have been left in inconsistent state.", - NULLSTR(src ? src->path : path)); - - virErrorRestore(&origerr); - return -1; - } + if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid) < 0) + goto error; return 0; + + error: + virErrorPreserveLast(&origerr); + /* Try to restore the label. This is done so that XATTRs + * are left in the same state as when the control entered + * this function. However, if our attempt fails, there's + * not much we can do. XATTRs refcounting is fubar'ed and + * the only option we have is warn users. */ + if (virSecurityDACRestoreFileLabelInternal(mgr, src, path, remember) < 0) + VIR_WARN("Unable to restore label on '%s'. " + "XATTRs might have been left in inconsistent state.", + NULLSTR(src ? src->path : path)); + + virErrorRestore(&origerr); + + return -1; } diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index ea20373a90..9857223bbf 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1334,6 +1334,7 @@ virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, security_context_t econ = NULL; int refcount; int rc; + bool rollback = false; int ret = -1; if ((rc = virSecuritySELinuxTransactionAppend(path, tcon, @@ -1353,6 +1354,8 @@ virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, if (econ) { refcount = virSecuritySELinuxRememberLabel(path, econ); + if (refcount > 0) + rollback = true; if (refcount == -2) { /* Not supported. Don't error though. */ } else if (refcount < 0) { @@ -1362,7 +1365,8 @@ virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, * is @refcount domains using the @path. Do not * change the label (as it would almost certainly * cause the other domains to lose access to the - * @path). */ + * @path). However, the refcounter was + * incremented in XATTRs so decrease it. */ if (STRNEQ(econ, tcon)) { virReportError(VIR_ERR_OPERATION_INVALID, _("Setting different SELinux label on %s " @@ -1373,7 +1377,12 @@ virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, } } - if (virSecuritySELinuxSetFileconImpl(path, tcon, optional, privileged) < 0) { + if (virSecuritySELinuxSetFileconImpl(path, tcon, optional, privileged) < 0) + goto cleanup; + + ret = 0; + cleanup: + if (ret < 0 && rollback) { virErrorPtr origerr; virErrorPreserveLast(&origerr); @@ -1388,11 +1397,8 @@ virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr, path); virErrorRestore(&origerr); - goto cleanup; - } - ret = 0; - cleanup: + } freecon(econ); return ret; }