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

[PATCH 1/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.  Create a new
private flags audit_krule field (pflags) to store it that won't interact with
the public one from the API.

Cc: stable@vger.kernel.org # v3.10-rc1+
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 include/linux/audit.h |    4 ++++
 kernel/auditfilter.c  |   10 ++++++++++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index eefc39a..b481779 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -46,6 +46,7 @@ struct audit_tree;
 struct sk_buff;
 
 struct audit_krule {
+	u32			pflags;
 	u32			flags;
 	u32			listnr;
 	u32			action;
@@ -63,6 +64,9 @@ struct audit_krule {
 	u64			prio;
 };
 
+/* Flag to indicate legacy AUDIT_LOGINUID unset usage */
+#define AUDIT_LOGINUID_LEGACY		0x1
+
 struct audit_field {
 	u32				type;
 	union {
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index fb4d2df..ecb05d3 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.pflags |= AUDIT_LOGINUID_LEGACY;
 		}
 
 		if ((f->type == AUDIT_PID) || (f->type == AUDIT_PPID)) {
@@ -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->pflags & 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;
 		}
@@ -645,6 +653,7 @@ static int audit_compare_rule(struct audit_krule *a, struct audit_krule *b)
 	int i;
 
 	if (a->flags != b->flags ||
+	    a->pflags != b->pflags ||
 	    a->listnr != b->listnr ||
 	    a->action != b->action ||
 	    a->field_count != b->field_count)
@@ -762,6 +771,7 @@ struct audit_entry *audit_dupe_rule(struct audit_krule *old)
 
 	new = &entry->rule;
 	new->flags = old->flags;
+	new->pflags = old->pflags;
 	new->listnr = old->listnr;
 	new->action = old->action;
 	for (i = 0; i < AUDIT_BITMASK_SIZE; i++)
-- 
1.7.1

[PATCH 2/2] audit: shrink audit_krule by using smaller bitfields

[Richard Guy Briggs]

Replace five 32-bit fields with one.  Move a nearby 32-bit field to enable
64-bit alignment.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 include/linux/audit.h |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index b481779..bd06f92 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -46,13 +46,14 @@ struct audit_tree;
 struct sk_buff;
 
 struct audit_krule {
-	u32			pflags;
-	u32			flags;
-	u32			listnr;
-	u32			action;
-	u32			mask[AUDIT_BITMASK_SIZE];
+	u32			listnr:4,
+				flags:5,
+				action:2,
+				pflags:1,
+				field_count:7,
+				reserved:13;
 	u32			buflen; /* for data alloc on list rules */
-	u32			field_count;
+	u32			mask[AUDIT_BITMASK_SIZE];
 	char			*filterkey; /* ties events to rules */
 	struct audit_field	*fields;
 	struct audit_field	*arch_f; /* quick access to arch field */
-- 
1.7.1

Re: [PATCH 2/2] audit: shrink audit_krule by using smaller bitfields

[Paris, Eric]

It's probably not a problem, but one needs to remember that all
updates to all bit fields need to be under the same lock/protection.
Bitfields + concurrent access is dangerous, but may well be
appropriate in this case.

-Eric

On Tue, Dec 23, 2014 at 1:20 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> Replace five 32-bit fields with one.  Move a nearby 32-bit field to enable
> 64-bit alignment.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  include/linux/audit.h |   13 +++++++------
>  1 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index b481779..bd06f92 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -46,13 +46,14 @@ struct audit_tree;
>  struct sk_buff;
>
>  struct audit_krule {
> -       u32                     pflags;
> -       u32                     flags;
> -       u32                     listnr;
> -       u32                     action;
> -       u32                     mask[AUDIT_BITMASK_SIZE];
> +       u32                     listnr:4,
> +                               flags:5,
> +                               action:2,
> +                               pflags:1,
> +                               field_count:7,
> +                               reserved:13;
>         u32                     buflen; /* for data alloc on list rules */
> -       u32                     field_count;
> +       u32                     mask[AUDIT_BITMASK_SIZE];
>         char                    *filterkey; /* ties events to rules */
>         struct audit_field      *fields;
>         struct audit_field      *arch_f; /* quick access to arch field */
> --
> 1.7.1
>

Re: [PATCH 2/2] audit: shrink audit_krule by using smaller bitfields

[Steve Grubb]

On Tuesday, December 23, 2014 01:20:15 PM Richard Guy Briggs wrote:
> Replace five 32-bit fields with one.  Move a nearby 32-bit field to enable
> 64-bit alignment.

Are there performance impacts from handling bit fields? The syscall audit rules 
affect each syscall of each program running. Also, setting bitfields so small 
kind of boxes in any future capability enhancements.

-Steve

> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  include/linux/audit.h |   13 +++++++------
>  1 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index b481779..bd06f92 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -46,13 +46,14 @@ struct audit_tree;
>  struct sk_buff;
> 
>  struct audit_krule {
> -	u32			pflags;
> -	u32			flags;
> -	u32			listnr;
> -	u32			action;
> -	u32			mask[AUDIT_BITMASK_SIZE];
> +	u32			listnr:4,
> +				flags:5,
> +				action:2,
> +				pflags:1,
> +				field_count:7,
> +				reserved:13;
>  	u32			buflen; /* for data alloc on list rules */
> -	u32			field_count;
> +	u32			mask[AUDIT_BITMASK_SIZE];
>  	char			*filterkey; /* ties events to rules */
>  	struct audit_field	*fields;
>  	struct audit_field	*arch_f; /* quick access to arch field */

Re: [PATCH 2/2] audit: shrink audit_krule by using smaller bitfields

[Richard Guy Briggs]

On 14/12/23, Paris, Eric wrote:
> It's probably not a problem, but one needs to remember that all
> updates to all bit fields need to be under the same lock/protection.
> Bitfields + concurrent access is dangerous, but may well be
> appropriate in this case.

Good point.  I'll go back and check for any issues, but as you point
out, in this case it may be appropriate.  All these fields should only
change during a rule addition in a newly allocated struct (under
audit_cmd_mutex) before they are referenceable by other parts of the
code.

Thanks, Eric.

> -Eric
> 
> On Tue, Dec 23, 2014 at 1:20 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > Replace five 32-bit fields with one.  Move a nearby 32-bit field to enable
> > 64-bit alignment.
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  include/linux/audit.h |   13 +++++++------
> >  1 files changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index b481779..bd06f92 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -46,13 +46,14 @@ struct audit_tree;
> >  struct sk_buff;
> >
> >  struct audit_krule {
> > -       u32                     pflags;
> > -       u32                     flags;
> > -       u32                     listnr;
> > -       u32                     action;
> > -       u32                     mask[AUDIT_BITMASK_SIZE];
> > +       u32                     listnr:4,
> > +                               flags:5,
> > +                               action:2,
> > +                               pflags:1,
> > +                               field_count:7,
> > +                               reserved:13;
> >         u32                     buflen; /* for data alloc on list rules */
> > -       u32                     field_count;
> > +       u32                     mask[AUDIT_BITMASK_SIZE];
> >         char                    *filterkey; /* ties events to rules */
> >         struct audit_field      *fields;
> >         struct audit_field      *arch_f; /* quick access to arch field */
> > --
> > 1.7.1

- 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: shrink audit_krule by using smaller bitfields

[Richard Guy Briggs]

On 14/12/23, Steve Grubb wrote:
> On Tuesday, December 23, 2014 01:20:15 PM Richard Guy Briggs wrote:
> > Replace five 32-bit fields with one.  Move a nearby 32-bit field to enable
> > 64-bit alignment.
> 
> Are there performance impacts from handling bit fields? The syscall audit rules 
> affect each syscall of each program running.

Quite possibly.  I honestly don't know.  I agree testing this could be
quite insightful.  Given the size of the mask field, this optimization
does seem minor.

> Also, setting bitfields so small kind of boxes in any future
> capability enhancements.

I don't see any problem there since there are 13 bits left over in the
reserved field from which we can pull should there be any future needs.
Since these are internal structures, enlarging them in the future should
present no special problems.  listnr is already limited to 4 bits by the
prepend flag value of 0x10 passed in with it.  field_count is already
taking 7 bits to represent 64 (rather than 6 bits for 63) so that one
could go to 127...  action still has one value left before needing
expansion.  flags could use the bottom 4 bits, but that seems
unnecessary since expanding it bitwidth is trivial.

> -Steve
> 
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  include/linux/audit.h |   13 +++++++------
> >  1 files changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index b481779..bd06f92 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -46,13 +46,14 @@ struct audit_tree;
> >  struct sk_buff;
> > 
> >  struct audit_krule {
> > -	u32			pflags;
> > -	u32			flags;
> > -	u32			listnr;
> > -	u32			action;
> > -	u32			mask[AUDIT_BITMASK_SIZE];
> > +	u32			listnr:4,
> > +				flags:5,
> > +				action:2,
> > +				pflags:1,
> > +				field_count:7,
> > +				reserved:13;
> >  	u32			buflen; /* for data alloc on list rules */
> > -	u32			field_count;
> > +	u32			mask[AUDIT_BITMASK_SIZE];
> >  	char			*filterkey; /* ties events to rules */
> >  	struct audit_field	*fields;
> >  	struct audit_field	*arch_f; /* quick access to arch field */
> 

- 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 1/2] audit: restore AUDIT_LOGINUID unset ABI

[Paul Moore]

On Tuesday, December 23, 2014 01:02:04 PM 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.  Create a new
> private flags audit_krule field (pflags) to store it that won't interact
> with the public one from the API.
> 
> Cc: stable@vger.kernel.org # v3.10-rc1+
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  include/linux/audit.h |    4 ++++
>  kernel/auditfilter.c  |   10 ++++++++++
>  2 files changed, 14 insertions(+), 0 deletions(-)

Applied, thanks.

-- 
paul moore
security @ redhat

Re: [PATCH 2/2] audit: shrink audit_krule by using smaller bitfields

[Paul Moore]

On Tuesday, December 23, 2014 01:20:15 PM Richard Guy Briggs wrote:
> Replace five 32-bit fields with one.  Move a nearby 32-bit field to enable
> 64-bit alignment.
> 
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  include/linux/audit.h |   13 +++++++------
>  1 files changed, 7 insertions(+), 6 deletions(-)

Let's leave this alone for right now.  I expect to be doing some work on 
cleaning up audit in the near-ish future so let's hold off until we have a 
better idea of how things are going to look.

-- 
paul moore
security @ redhat

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

[Paris, Eric]

Me likie much more.

On Tue, Dec 23, 2014 at 4:26 PM, Paul Moore <pmoore@redhat.com> wrote:
> On Tuesday, December 23, 2014 01:02:04 PM 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.  Create a new
>> private flags audit_krule field (pflags) to store it that won't interact
>> with the public one from the API.
>>
>> Cc: stable@vger.kernel.org # v3.10-rc1+
>> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>> ---
>>  include/linux/audit.h |    4 ++++
>>  kernel/auditfilter.c  |   10 ++++++++++
>>  2 files changed, 14 insertions(+), 0 deletions(-)
>
> Applied, thanks.
>
> --
> paul moore
> security @ redhat
>

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

[Paul Moore]

On Tuesday, December 23, 2014 04:41:42 PM Paris, Eric wrote:
> Me likie much more.

Yeah, me too.

> On Tue, Dec 23, 2014 at 4:26 PM, Paul Moore <pmoore@redhat.com> wrote:
> > On Tuesday, December 23, 2014 01:02:04 PM 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.  Create a new
> >> private flags audit_krule field (pflags) to store it that won't interact
> >> with the public one from the API.
> >> 
> >> Cc: stable@vger.kernel.org # v3.10-rc1+
> >> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> >> ---
> >> 
> >>  include/linux/audit.h |    4 ++++
> >>  kernel/auditfilter.c  |   10 ++++++++++
> >>  2 files changed, 14 insertions(+), 0 deletions(-)
> > 
> > Applied, thanks.
> > 
> > --
> > paul moore
> > security @ redhat

-- 
paul moore
security @ redhat

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

[Richard Guy Briggs]

On 14/12/23, Paul Moore wrote:
> On Tuesday, December 23, 2014 04:41:42 PM Paris, Eric wrote:
> > Me likie much more.
> 
> Yeah, me too.

Good, thanks for your patience guys.

> > On Tue, Dec 23, 2014 at 4:26 PM, Paul Moore <pmoore@redhat.com> wrote:
> > > On Tuesday, December 23, 2014 01:02:04 PM 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.  Create a new
> > >> private flags audit_krule field (pflags) to store it that won't interact
> > >> with the public one from the API.
> > >> 
> > >> Cc: stable@vger.kernel.org # v3.10-rc1+
> > >> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > >> ---
> > >> 
> > >>  include/linux/audit.h |    4 ++++
> > >>  kernel/auditfilter.c  |   10 ++++++++++
> > >>  2 files changed, 14 insertions(+), 0 deletions(-)
> > > 
> > > Applied, thanks.
> > > 
> > > paul moore
> 
> 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