Re: [PATCH] ls: with -Z, show SMACK context for each file (v3)

[Jarkko Sakkinen]

Short comment: good arguments and yes I do need to update m4. I'm
thinking should I also update associated man page? As I said in reply
to other patch I sent is that I will freeze the library to 1.0 version
as soon as changes for id and ls are accepted. 

/Jarkko

On Tue, Jun 4, 2013, at 2:18, Pádraig Brady wrote:
> On 06/03/2013 06:05 PM, Jarkko Sakkinen wrote:
> > Enable showing of file SMACK labels with -Z command-line switch.
> > 
> > * src/ls.c (gobble_file): Output the smack context if available.
> > * src/ls.c: New function getsmackcon_cache() for grabbing SMACK label.
> 
>     ls: with -Z, show SMACK context if available
> 
>     Enable showing of file SMACK labels with -Z command-line switch.
> 
>     * src/ls.c (gobble_file): Output the smack context if available.
>     * src/ls.c (getsmackcon_cache): New function for getting SMACK label.
> 
> > diff --git a/src/ls.c b/src/ls.c
> > @@ -1371,7 +1385,7 @@ main (int argc, char **argv)
> >  
> >    format_needs_stat = sort_type == sort_time || sort_type == sort_size
> >      || format == long_format
> > -    || print_scontext
> > +    || print_scontext != no_context
> 
> again these changes are redundant
> 
> > @@ -2801,25 +2819,37 @@ errno_unsupported (int err)
> >     Like getfilecon/lgetfilecon, but when F's st_dev says it's on a known-
> >     SELinux-challenged file system, fail with ENOTSUP immediately.  */
> >  static int
> > -getfilecon_cache (char const *file, struct fileinfo *f, bool deref)
> > +getfilecon_cache (char const *file, struct fileinfo *f, bool deref,
> > +                  enum scontext_type scontext)
> >  {
> >    /* st_dev of the most recently processed device for which we've
> >       found that [l]getfilecon fails indicating lack of support.  */
> >    static dev_t unsupported_device;
> >  
> > -  if (f->stat.st_dev == unsupported_device)
> > +  if (f->stat.st_dev == unsupported_device || scontext == no_context)
> > +    {
> > +      errno = ENOTSUP;
> > +      return -1;
> > +    }
> 
> We need to read the selinux/smack context with -l even without -Z
> so the appropriate "extra" perm bits can be displayed appropriately.
> 
> > +  int r = 0;
> > +  if (scontext == selinux_context)
> > +    r = (deref
> > +         ? getfilecon (file, &f->scontext)
> > +         : lgetfilecon (file, &f->scontext));
> > +  else if (scontext == smack_context)
> > +    r = smack_new_label_from_path (file, "security.SMACK64", deref,
> > +                                   &f->scontext);
> 
> This is a new function in libsmack.
> Shouldn't you key HAVE_SMACK on its availability in m4/jm-macros.m4 ?
> Also I'm a little worried by the mixing of security_context_t*
> and char*, specifically that freecon() is called on both.
> The _currently_ isn't an issue, but it's awkward and best avoided if
> possible.
> 
> >            if (err == 0)
> > -            have_selinux = ! STREQ ("unlabeled", f->scontext);
> > +            if (print_scontext == selinux_context)
> > +              have_context = ! STREQ ("unlabeled", f->scontext);
> > +            else if (print_scontext == smack_context)
> > +              have_context = ! STREQ ("_", f->scontext);
> 
> You need braces around the above to associate the if else clauses
> correctly.
> 
> thanks,
> Pádraig.
> 
>