[libvirt] [PATCH v3 28/28] security_dac: Lock domain metadata

[John Ferlan]

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 at 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;
>  }
>  
>  
>