[PATCH 1/2] audit: remove vestiges of vers_ops

[PATCH 1/2] audit: remove vestiges of vers_ops

[Richard Guy Briggs]

Should have been removed with 18900909.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 include/linux/audit.h |    1 -
 kernel/auditfilter.c  |    2 --
 2 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 36dffec..eefc39a 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -46,7 +46,6 @@ struct audit_tree;
 struct sk_buff;
 
 struct audit_krule {
-	int			vers_ops;
 	u32			flags;
 	u32			listnr;
 	u32			action;
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 40ed981..fb4d2df 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -425,7 +425,6 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
 		goto exit_nofree;
 
 	bufp = data->buf;
-	entry->rule.vers_ops = 2;
 	for (i = 0; i < data->field_count; i++) {
 		struct audit_field *f = &entry->rule.fields[i];
 
@@ -762,7 +761,6 @@ struct audit_entry *audit_dupe_rule(struct audit_krule *old)
 		return ERR_PTR(-ENOMEM);
 
 	new = &entry->rule;
-	new->vers_ops = old->vers_ops;
 	new->flags = old->flags;
 	new->listnr = old->listnr;
 	new->action = old->action;
-- 
1.7.1

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

[Richard Guy Briggs]

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

Re: [PATCH 1/2] audit: remove vestiges of vers_ops

[Paul Moore]

On Friday, December 12, 2014 12:20:15 AM Richard Guy Briggs wrote:
> Should have been removed with 18900909.
> 
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  include/linux/audit.h |    1 -
>  kernel/auditfilter.c  |    2 --
>  2 files changed, 0 insertions(+), 3 deletions(-)

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

> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 36dffec..eefc39a 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -46,7 +46,6 @@ struct audit_tree;
>  struct sk_buff;
> 
>  struct audit_krule {
> -	int			vers_ops;
>  	u32			flags;
>  	u32			listnr;
>  	u32			action;
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 40ed981..fb4d2df 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -425,7 +425,6 @@ static struct audit_entry *audit_data_to_entry(struct
> audit_rule_data *data, goto exit_nofree;
> 
>  	bufp = data->buf;
> -	entry->rule.vers_ops = 2;
>  	for (i = 0; i < data->field_count; i++) {
>  		struct audit_field *f = &entry->rule.fields[i];
> 
> @@ -762,7 +761,6 @@ struct audit_entry *audit_dupe_rule(struct audit_krule
> *old) return ERR_PTR(-ENOMEM);
> 
>  	new = &entry->rule;
> -	new->vers_ops = old->vers_ops;
>  	new->flags = old->flags;
>  	new->listnr = old->listnr;
>  	new->action = old->action;

-- 
paul moore
security and virtualization @ redhat

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

[Paul Moore]

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

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

[Richard Guy Briggs]

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

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

[Paul Moore]

On Friday, December 12, 2014 11:44:50 AM Richard Guy Briggs wrote:
> On 14/12/12, Paul Moore wrote:
> > On Friday, December 12, 2014 12:20:16 AM Richard Guy Briggs wrote:

...

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

Yes, it does, my mistake.  I was probably just looking at the structure 
definition, saw it wasn't exported to userspace, and thought the "flags" field 
seemed promising.
 
> > 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.

Yes, I think using audit_krule->flags is an improvement over the previous 
patch, but I think we are better served using a field that doesn't interfere 
with the userspace API.

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

The question I was trying to ask, perhaps rhetorically at this point, is if 
there is much/any advantage to spliting the public API flags into the private 
flags/listnr field.  It's probably not worth worrying about in the context of 
this fix, just something that popped into my head when looking at this fix.  
In retrospect I probably shouldn't have muddled the discussion with this idea.

> I thought it made sense to internally add it to the flags field.

I would still like us to use an internal field for tracking things that aren't 
part of the API.

-- 
paul moore
security and virtualization @ redhat

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

[Richard Guy Briggs]

On 14/12/12, Paul Moore wrote:
> On Friday, December 12, 2014 11:44:50 AM Richard Guy Briggs wrote:
> > On 14/12/12, Paul Moore wrote:
> > > On Friday, December 12, 2014 12:20:16 AM Richard Guy Briggs wrote:
> 
> ...
> 
> > > > 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...
> 
> Yes, it does, my mistake.  I was probably just looking at the structure 
> definition, saw it wasn't exported to userspace, and thought the "flags" field 
> seemed promising.
>  
> > > 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.
> 
> Yes, I think using audit_krule->flags is an improvement over the previous 
> patch, but I think we are better served using a field that doesn't interfere 
> with the userspace API.

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()?

> > > * 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.
> 
> The question I was trying to ask, perhaps rhetorically at this point, is if 
> there is much/any advantage to spliting the public API flags into the private 
> flags/listnr field.  It's probably not worth worrying about in the context of 
> this fix, just something that popped into my head when looking at this fix.  
> In retrospect I probably shouldn't have muddled the discussion with this idea.

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.

> > I thought it made sense to internally add it to the flags field.
> 
> I would still like us to use an internal field for tracking things
> that aren't part of the API.

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.

> 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

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

[Paul Moore]

On Tuesday, December 16, 2014 02:20:04 PM Richard Guy Briggs wrote:
> On 14/12/12, Paul Moore wrote:
> > On Friday, December 12, 2014 11:44:50 AM Richard Guy Briggs wrote:
> > > On 14/12/12, Paul Moore wrote:
> > > > On Friday, December 12, 2014 12:20:16 AM Richard Guy Briggs wrote:
> > ...
> > 
> > > > > 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...
> > 
> > Yes, it does, my mistake.  I was probably just looking at the structure
> > definition, saw it wasn't exported to userspace, and thought the "flags"
> > field seemed promising.
> > 
> > > > 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.
> > 
> > Yes, I think using audit_krule->flags is an improvement over the previous
> > patch, but I think we are better served using a field that doesn't
> > interfere with the userspace API.
> 
> 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.

Maybe I'm reading the code wrong, but it is taking bits away from a bitfield 
which is part of the audit API is it not?  This is why I don't like this, or 
the previous approaches, and why I would prefer to track the "legacy" state in 
another private field.

> 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()?

If the solution involves filtering out anything before returning the data to 
userspace I'm not in favor of the solution (see above).

> > > > * 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.
> > 
> > The question I was trying to ask, perhaps rhetorically at this point, is
> > if there is much/any advantage to spliting the public API flags into the
> > private flags/listnr field.  It's probably not worth worrying about in
> > the context of this fix, just something that popped into my head when
> > looking at this fix. In retrospect I probably shouldn't have muddled the
> > discussion with this idea.
>
> 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.

Personally I see it more as a performance issue, rather than an obfuscation 
issue.  However, as I said earlier, let's not worry about this now; I'd much 
rather us stay focused on resolving the LOGINUID issue.  It was a mistake on 
my part to bring this up in this thread.
 
> > > I thought it made sense to internally add it to the flags field.
> > 
> > I would still like us to use an internal field for tracking things
> > that aren't part of the API.
> 
> It *is* and internal field.

Yes, and no.  Yes it is internal, but it shares its value with the API.  This 
is my problem with these patches (see above).

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

Considering we just reclaimed a 32-bit field I consider this a zero-sum 
change.  Perhaps we'll do things differently in the future, this is a private 
state flag after all, but right now I'm more comfortable creating a new 
private flag field.

> Is it worth turning it into a bitfield to avoid the confusion?

Yes.  At least that's my opinion at this point.

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

The cost of stealing a bit from the public API can be huge.

-- 
paul moore
security and virtualization @ redhat