A regression was caused by commit 780a7654cee8: audit: Make testing for a valid loginuid explicit. (which in turn attempted to fix a regression caused by e1760bd) When audit_krule_to_data() fills in the rules to get a listing, there was a missing clause to convert back from AUDIT_LOGINUID_SET to AUDIT_LOGINUID. This broke userspace by not returning the same information that was sent and expected. The rule: auditctl -a exit,never -F auid=-1 gives: auditctl -l LIST_RULES: exit,never f24=0 syscall=all when it should give: LIST_RULES: exit,never auid=-1 (0xffffffff) syscall=all Tag it so that it is reported the same way it was set. Cc: stable(a)vger.kernel.org # v3.10-rc1+ Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; --- include/linux/audit.h | 3 +++ kernel/auditfilter.c | 10 +++++++++- 2 files changed, 12 insertions(+), 1 deletions(-) diff --git a/include/linux/audit.h b/include/linux/audit.h index eefc39a..d905832 100644 --- a/include/linux/audit.h +++ b/include/linux/audit.h @@ -63,6 +63,9 @@ struct audit_krule { u64 prio; }; +/* Flag to indicate legacy AUDIT_LOGINUID unset usage */ +#define AUDIT_LOGINUID_LEGACY 0x80000000 + struct audit_field { u32 type; union { 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; data->action = krule->action; data->field_count = krule->field_count; bufp = data->buf; @@ -629,6 +630,13 @@ static struct audit_rule_data *audit_krule_to_data(struct audit_krule *krule) data->buflen += data->values[i] = audit_pack_string(&bufp, krule->filterkey); break; + case AUDIT_LOGINUID_SET: + if (krule->flags & AUDIT_LOGINUID_LEGACY && !f->val) { + data->fields[i] = AUDIT_LOGINUID; + data->values[i] = AUDIT_UID_UNSET; + break; + } + /* fallthrough if set */ default: data->values[i] = f->val; } -- 1.7.1

On 14/12/12, Paul Moore wrote: Well, it came in that way... I thought it was going to be messier, but I like how it turned out cleaner because of the way it was already used. 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. I thought it made sense to internally add it to the flags field. - RGB -- Richard Guy Briggs <rbriggs(a)redhat.com&gt; 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

On 14/12/12, Paul Moore wrote: But it doesn't interfere. A field is passed in from userspace that is multiplexed and has to be filtered anyways into its components in the internal representation. It is then combined again on output to userspace from more than one source. Arguably, the field passed in from userspace it mis-named, since it isn't strictly flags, but a list number from zero to five with a flag added just larger than any of the list numbers. The userspace API is not affected. Would you prefer if I filter the internal flags field with "& AUDIT_FILTER_PREPEND" rather than "& ~AUDIT_LOGINUID_LEGACY" to make it more clear what is being recombined in audit_krule_to_data()? Ok, I hadn't understood that you were contemplating leaving that field passed from userspace as it was passed in, in the internal representation and simply filtering it for necessary information at the point of use. That has some merit, but that listnr value is used at least a dozen and a half places and would need filtering in each of those, slightly complicating/obfuscating the code. It *is* and internal field. Internally, the "flags" field is only used for *one* bit, which seems like a waste to add another 32-bit field to accomodate another single-bit flag. Is it worth turning it into a bitfield to avoid the confusion? The overhead of doing the filtering on rule creation and deletion seems pretty minimal compared to adding a 32-bit field that stays with every entry. - RGB -- Richard Guy Briggs <rbriggs(a)redhat.com&gt; 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

On Friday, December 12, 2014 12:20:15 AM Richard Guy Briggs wrote: Thanks. I still need to review 2/2 of this patchset, but this patch is standalone from my perspective so I'm merging it now. One note for future submissions, instead of saying "removed with XXXX", a little more description can be helpful, e.g. "removed by commit XXXX ("audit: make it suck less")". Documentation/SubmittingPatches has more text on this in section 2. I myself am not perfect on this either so I'll rarely refuse a patch for this reason, but it makes me happy when I see it done correctly :) -- paul moore security and virtualization @ redhat