[libvirt] [PATCH 1/1] libvirt patch to write a mcs translation file to /run/setrans directory

[Daniel P. Berrange]

On Wed, May 15, 2013 at 09:56:33AM -0400, dwalsh at redhat.com wrote:
> From: Dan Walsh <dwalsh at redhat.com>
> 
> mcstransd is a translation tool that can translate MCS Labels into human
> understandable code.  I have patched it to watch for translation files in the
> /run/setrans directory.  This allows us to run commands like ps -eZ and see
> system_u:system_r:svirt_t:Fedora18 rather then system_u:system_r:svirt_t:s0:c1,c2.
> When used with containers it would make an easy way to list all processes within
> a container using ps -eZ | grep Fedora18
> ---
>  src/security/security_selinux.c | 56 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 55 insertions(+), 1 deletion(-)
> 
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index 61ff1de..bf32755 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -82,6 +82,55 @@ virSecuritySELinuxRestoreSecurityTPMFileLabelInt(virSecurityManagerPtr mgr,
>                                                   virDomainTPMDefPtr tpm);
>  
>  
> +static int
> +virSecuritySELinuxAddMCSFile(const char *name, 
> +                             const char *label)
> +{
> +    int rc = -1;

s/rc/ret/

> +    char *tmp = NULL;
> +    context_t con = NULL;
> +
> +    if (virAsprintf(&tmp, "%s/%s", SELINUX_TRANS_DIR, name) == -1) {

Use  "< 0" here

> +        virReportOOMError();
> +        return -1;
> +    }
> +
> +    if (! (con = context_new(label))) {

Some bogus whitespace after the '!'

> +        virReportSystemError(errno, "%s",
> +                             _("unable to allocate security context"));
> +        goto done;
> +    }
> +    rc = virFileWriteStr(tmp, context_range_get(con), 0);
> +    if (rc) {

Change this to

  if (virFileWriteStr(tmp, context_range_get(con), 0) < 0)

> +        virReportSystemError(errno, 
> +                             _("unable to create MCS file %s"), tmp);

And add

    goto cleanup;


> +    }

And add

   ret = 0;

> +
> +done:

s/done/cleanup/ to comply with naming conventions on labelling

> +    VIR_FREE(tmp);
> +    context_free(con);
> +    return rc;



> +}
> +
> +static int
> +virSecuritySELinuxRemoveMCSFile(const char *name)
> +{
> +    char *tmp=NULL;
> +    int rc = 0;

s/0/-1/;

> +    if (virAsprintf(&tmp, "%s/%s", SELINUX_TRANS_DIR, name) == -1) {

s/== -1/< 0/;

> +        virReportOOMError();
> +        return -1;
> +    }
> +    rc = unlink(tmp);
> +    if (rc && errno != ENOENT) {

Change these two lihes to

   if (unlink(tmp) < 0 && errno != ENOENT) {


> +        virReportSystemError(errno,
> +                             _("Unable to remove MCS file %s"), tmp);
> +        rc = -1;

Remnove this line & add  'goto cleanup'

> +    }
> +    VIR_FREE(tmp);

Add

    ret = 0;

  cleanup:

> +    return rc;

s/rc/ret/

> +}
> +
>  /*
>   * Returns 0 on success, 1 if already reserved, or -1 on fatal error
>   */

> @@ -1977,6 +2026,11 @@ virSecuritySELinuxSetSecurityProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UN
>              return -1;
>      }
>  
> +    if (virSecuritySELinuxAddMCSFile(def->name, secdef->label)) {

Missing  '< 0'


> +        if (security_getenforce() == 1)
> +            return -1;

IMHO this is bogus - we should never ignore the errors in
creating these files. Just always return -1;

> +    }



> +
>      if (setexeccon_raw(secdef->label) == -1) {
>          virReportSystemError(errno,
>                               _("unable to set security context '%s'"),


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|