Re: [PATCH] audit: restore AUDIT_LOGINUID unset ABI

[Paul Moore <pmoore@redhat.com>]

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.

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

-- 
paul moore
security and virtualization @ redhat