From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paul Moore Subject: Re: [PATCH 2/2] audit: restore AUDIT_LOGINUID unset ABI Date: Fri, 12 Dec 2014 11:39:06 -0500 Message-ID: <1560037.8CbAgy6kZ9@sifl> References: <38fc654e414944ee7ff05d74a7d091b2227633dc.1418360973.git.rgb@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <38fc654e414944ee7ff05d74a7d091b2227633dc.1418360973.git.rgb@redhat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com To: Richard Guy Briggs Cc: linux-audit@redhat.com, ebiederm@xmission.com, eparis@parisplace.org List-Id: linux-audit@redhat.com On Friday, December 12, 2014 12:20:16 AM Richard Guy Briggs wrote: > 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@vger.kernel.org # v3.10-rc1+ > Signed-off-by: Richard Guy Briggs > --- > 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; Argh! I missed that the audit_krule->flags end up in audit_rule_data->flags. 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. * 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 ... > 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; > } -- paul moore security and virtualization @ redhat