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

[Paul Moore <pmoore@redhat.com>]

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.

> > * 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.

> 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.

-- 
paul moore
security and virtualization @ redhat