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

[Paul Moore <pmoore@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 <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.

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