[PATCH 2/2] audit: restore AUDIT_LOGINUID unset ABI
Paul Moore
pmoore at redhat.com
Fri Dec 12 19:23:53 UTC 2014
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
More information about the Linux-audit
mailing list