Re: [PATCH] audit: restore AUDIT_LOGINUID unset ABI

[Richard Guy Briggs <rgb@redhat.com>]

On 14/12/09, Paul Moore wrote:
> On Tuesday, December 09, 2014 11:30:14 AM Richard Guy Briggs wrote:
> > On 14/12/08, Paul Moore wrote:
> > > As I understand it, when old userspace would set a filter with
> > > AUDIT_LOGINUID but when it listed the audit rules in the kernel it would
> > > see AUDIT_LOGINUID_SET, yes?  This patch attempts to fix this by marking a
> > > legacy userspace with the AUDIT_LOGINUID_LEGACY bitmask on the internal
> > > kernel representation so that when the rules are dumped to userspace the
> > > AUDIT_LOGINUID_SET rule can be rewritten as AUDIT_LOGINUID, yes?
> > 
> > Correct.
> > 
> > > However, there are some things that are not immediately obvious to me:
> > > 
> > > * Why are we using a bit in audit_field->type to indicate the legacy
> > > nature of userspace?
> > 
> > Convenience.  Adding a new member to audit_field or audit_krule seemed
> > unnecessary memory overhead (however, it then complicates other code...).
> >
> > > * Why are we reusing the AUDIT_NEGATE bit in the type field to indicate a
> > > legacy userspace?
> > 
> > It wasn't reaped when commit 18900909 went through... (first introduced
> > with original audit in b7b0074c, 2004-04-11).  It would have been more
> > clear if I had sent a first patch to remove AUDIT_NEGATE altogether and
> > re-introduce it with a new name in this patch.
> 
> The problem is that AUDIT_NEGATE lives in the userspace visible header file 
> which means it needs to live there for pretty much forever.  While I would 
> like to see us remote it for clarity's sake, I think we're stuck with it.

Ok, fair enough, the same goes for AUDIT_{LIST,ADD,DEL} since they are
no longer used, but still remain in the API.  I can use another value.

> > > * Why are we not using something in audit_krule?  Without looking to in
> > > depth it would appear that there are multiple fields which might be
> > > useful, e.g. "vers_ops", "flags"?
> > 
> > audit_krule applies to the set of all fields for this rule.  I wanted
> > something that localized it very unambiguously to this one field.
> 
> You can only add or delete rules, right?  Not modify?  If you can only add or 
> delete a rule, then if one of the fields in that rule is sent from legacy 
> userspace I think it is safe to set an indicator in one of the audit_krule 
> fields.  I understand your point, but I'm not sure it is something to worry 
> too much about; I'd rather see the legacy indicator here than in the 
> audit_field->type field where we might have to contend with userspace usage at 
> some point.
> 
> I'd like to explore the idea of not using audit_field->type; I picked 
> "vers_ops" and "flags" since they seemed like reasonable places to start.  The 
> "vers_ops" field in particular appears to be almost unused in the current code 
> and it seems like a good way to track userspace versions perhaps, e.g. 1 = 
> legacy, 2 = now current, etc.?  I'm curious if this sounds reasonable to you.

vers_ops appears to be assigned, copied and never otherwise read.  And
in fact *that* should have been removed with commit 18900909.  I'll just
prepare a patch to rip it out.

flags looks like a better choice...  and I'll have to do some similar
filtering that I did for type and as has been already done for
AUDIT_FILTER_PREPEND...  In fact, the inverse of AUDIT_FILTER_PREPEND.
And this patch is simpler than the one you are critiquing.  Bonus.

> paul moore

- RGB

--
Richard Guy Briggs <rbriggs@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