Re: [PATCH 2/2] audit: restore AUDIT_LOGINUID unset ABI

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

On 14/12/12, Paul Moore wrote:
> 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 <rgb@redhat.com>
> > ---
> >  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.

Well, it came in that way...

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

I thought it was going to be messier, but I like how it turned out
cleaner because of the way it was already used.

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

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.

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

- 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