Re: [PATCH] audit: restore AUDIT_LOGINUID unset ABI

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