[PATCH 2/2] audit: restore AUDIT_LOGINUID unset ABI

[Richard Guy Briggs]

On 14/12/12, Paul Moore wrote:
> On Friday, December 12, 2014 11:44:50 AM Richard Guy Briggs wrote:
> > On 14/12/12, Paul Moore wrote:
> > > On Friday, December 12, 2014 12:20:16 AM Richard Guy Briggs wrote:
> 
> ...
> 
> > > > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > > > index fb4d2df..ea62c7b 100644
> > > > --- a/kernel/auditfilter.c
> > > > +++ b/kernel/auditfilter.c
> > > > @@ -441,6 +441,7 @@ static struct audit_entry
> > > > *audit_data_to_entry(struct
> > > > audit_rule_data *data, if ((f->type == AUDIT_LOGINUID) && (f->val ==
> > > > AUDIT_UID_UNSET)) { f->type = AUDIT_LOGINUID_SET;
> > > > 
> > > >  			f->val = 0;
> > > > 
> > > > +			entry->rule.flags |= AUDIT_LOGINUID_LEGACY;
> > > > 
> > > >  		}
> > > >  		
> > > >  		if ((f->type == AUDIT_PID) || (f->type == AUDIT_PPID)) {
> > > > 
> > > > @@ -592,7 +593,7 @@ static struct audit_rule_data
> > > > *audit_krule_to_data(struct audit_krule *krule) return NULL;
> > > > 
> > > >  	memset(data, 0, sizeof(*data));
> > > > 
> > > > -	data->flags = krule->flags | krule->listnr;
> > > > +	data->flags = (krule->flags & ~AUDIT_LOGINUID_LEGACY) |
> > > >                  krule->listnr;
> > > 
> > > Argh!  I missed that the audit_krule->flags end up in
> > > audit_rule_data->flags.
> >
> > Well, it came in that way...
> 
> Yes, it does, my mistake.  I was probably just looking at the structure 
> definition, saw it wasn't exported to userspace, and thought the "flags" field 
> seemed promising.
>  
> > > Bummer.
> > > 
> > > Some thoughts:
> > > 
> > > * Your 1/2 patch saved 32-bits in audit_krule, what are your thoughts on
> > > adding a new 32-bit bitmap, say "private", which could be used internally
> > > to track things like this?  I'm not a big fan of overloading parts of the
> > > public API for use by internal mechanisms, it almost always gets messy.
> > 
> > I thought it was going to be messier, but I like how it turned out
> > cleaner because of the way it was already used.
> 
> Yes, I think using audit_krule->flags is an improvement over the previous 
> patch, but I think we are better served using a field that doesn't interfere 
> with the userspace API.

But it doesn't interfere.  A field is passed in from userspace that is
multiplexed and has to be filtered anyways into its components in the
internal representation.  It is then combined again on output to
userspace from more than one source.  Arguably, the field passed in from
userspace it mis-named, since it isn't strictly flags, but a list number
from zero to five with a flag added just larger than any of the list
numbers.

The userspace API is not affected.

Would you prefer if I filter the internal flags field with "&
AUDIT_FILTER_PREPEND" rather than "& ~AUDIT_LOGINUID_LEGACY" to make it
more clear what is being recombined in audit_krule_to_data()?

> > > * Also, why is there both an audit_krule->flags and audit_krule->listnr
> > > field? With the exception of the AUDIT_FILTER_PREPEND bit are they always
> > > going to be the same?  I wonder if some more cleanup could be done here
> > > ...
> > 
> > This is part of the API.  The flags field is used to hand in the list
> > number and its intended position on the list.  Once it gets transferred
> > from a user data blob to a kernel entry, it is split into listnr and
> > flags.
> 
> The question I was trying to ask, perhaps rhetorically at this point, is if 
> there is much/any advantage to spliting the public API flags into the private 
> flags/listnr field.  It's probably not worth worrying about in the context of 
> this fix, just something that popped into my head when looking at this fix.  
> In retrospect I probably shouldn't have muddled the discussion with this idea.

Ok, I hadn't understood that you were contemplating leaving that field
passed from userspace as it was passed in, in the internal
representation and simply filtering it for necessary information at the
point of use.  That has some merit, but that listnr value is used at
least a dozen and a half places and would need filtering in each of
those, slightly complicating/obfuscating the code.

> > I thought it made sense to internally add it to the flags field.
> 
> I would still like us to use an internal field for tracking things
> that aren't part of the API.

It *is* and internal field.
Internally, the "flags" field is only used for *one* bit, which seems
like a waste to add another 32-bit field to accomodate another
single-bit flag.  Is it worth turning it into a bitfield to avoid the
confusion?  The overhead of doing the filtering on rule creation and
deletion seems pretty minimal compared to adding a 32-bit field that
stays with every entry.

> paul moore

- RGB

--
Richard Guy Briggs <rbriggs at redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545