Re: [PATCH 2/2] audit: restore AUDIT_LOGINUID unset ABI
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