[RFC PATCH 0/3] simplify struct audit_krule reveals bug

[RFC PATCH 0/3] simplify struct audit_krule reveals bug

[Richard Guy Briggs]

In the process of trying to track down a potential bug altering the
registered arch for a syscall rule, a simplification of struct
audit_krule that removes a seemingly unnecessary member has revealed a
surprising NULL pointer dereference.

The struct audit_field *arch_f member should not be necessary since it
is the first field present if it is present at all, and is only
necessary for syscall rules, so iterating over the fields to find it is
simple and only happens when adding or deleting a rule.  Shrinking the
struct audit_krule seemed to be a good idea, but appears to have openned
a can of worms.  The first patch triggered this OOPS:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000009
IP: audit_match_signal+0x42/0x120
PGD 0 P4D 0
Oops: 0000 [#1] SMP PTI
Modules linked in: sunrpc 8139too i2c_piix4 pcspkr virtio_balloon 8139cp i2c_core mii sch_fq_codel floppy serio_raw ata_generic pata_acpi
CPU: 1 PID: 325 Comm: auditctl Not tainted 4.15.0-bz1462178-arch-changed+ #636
Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
RIP: 0010:audit_match_signal+0x42/0x120
RSP: 0018:ffffc900003dfc08 EFLAGS: 00010202
RAX: 0000000000000003 RBX: ffff880036588000 RCX: 0000000000000003
RDX: ffff88003c7f02e0 RSI: ffff88003c7f02a0 RDI: ffff880036588000
RBP: ffff88003671de00 R08: 0000000000000001 R09: 0000000000000000
R10: ffff880036a0b190 R11: 0000000000000000 R12: 0000000000000000
R13: ffff880036588178 R14: ffff880036588000 R15: ffffffff8247f880
FS:  00007fa53c6d9740(0000) GS:ffff88003e400000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000009 CR3: 00000000347ba000 CR4: 00000000000006e0
Call Trace:
 audit_rule_change+0xb32/0xce0
 audit_receive_msg+0x163/0x1090
 ? netlink_deliver_tap+0x90/0x350
 ? kvm_sched_clock_read+0x5/0x10
 ? sched_clock+0x5/0x10
 audit_receive+0x4d/0xa0
 netlink_unicast+0x195/0x250
 netlink_sendmsg+0x2fe/0x3f0
 sock_sendmsg+0x32/0x60
 SYSC_sendto+0xda/0x140
 ? syscall_trace_enter+0x2dc/0x400
 ? return_from_SYSCALL_64+0x10/0x75
 do_syscall_64+0x83/0x360
 entry_SYSCALL64_slow_path+0x25/0x25
RIP: 0033:0x7fa53bbb1607
RSP: 002b:00007fff33f48c78 EFLAGS: 00000246 ORIG_RAX: 000000000000002c
RAX: ffffffffffffffda RBX: 0000000000000444 RCX: 00007fa53bbb1607
RDX: 0000000000000444 RSI: 00007fff33f48cb0 RDI: 0000000000000003
RBP: 0000000000000431 R08: 00007fff33f48c9c R09: 000000000000000c
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000003
R13: 00007fff33f48cb0 R14: 00007fff33f48c9c R15: 00000000000003f3
Code: 01 00 00 83 3e 0b 0f 84 ef 00 00 00 31 c0 eb 0f 48 63 d0 48 c1 e2 05 48 01 f2 83 3a 0b 74 7d 83 c0 01 39 c8 75 ea 4d 85 c0 74 79 <41> 8b 78 08 e8 25 ff ed ff 85 c0 74 31 83 f8 01 75 58 48 8b 0d
RIP: audit_match_signal+0x42/0x120 RSP: ffffc900003dfc08
CR2: 0000000000000009

The second patch surprisingly fixes the OOPS.

Adding debug output, the OOPS is consistently happenning in the 7th STIG rule
that includes an arch parameter, but the value that causes the OOPS
dereferences, copies and prints out fine:
-a always,exit -F arch=b32 -S adjtimex,settimeofday,stime -F key=time-change
ams_: i=0 f=00000000e5612893 type=11 op=0 val=40000003 key="time-change"
-a always,exit -F arch=b64 -S adjtimex,settimeofday -F key=time-change
ams_: i=0 f=00000000cf222aca type=11 op=0 val=c000003e key="time-change"
-a always,exit -F arch=b32 -S clock_settime -F a0=0x0 -F key=time-change
ams_: i=0 f=00000000ad39bfc6 type=11 op=0 val=40000003 key="time-change"
-a always,exit -F arch=b64 -S clock_settime -F a0=0x0 -F key=time-change
ams_: i=0 f=00000000c9f83209 type=11 op=0 val=c000003e key="time-change"
-a always,exit -F arch=b32 -S sethostname,setdomainname -F key=system-locale
ams_: i=0 f=000000005a19d216 type=11 op=0 val=40000003 key="system-locale"
-a always,exit -F arch=b64 -S sethostname,setdomainname -F key=system-locale
ams_: i=0 f=000000003280e47a type=11 op=0 val=c000003e key="system-locale"
OOPS
-a always,exit -F arch=b32 -S chmod,fchmod,fchmodat -F auid>=1000 -F auid!=4294967295 -F key=perm_mod
ams_: i=0 f=000000008368170a type=11 op=0 val=40000003 key="perm_mod"

I'd let sleeping dogs lie, but I haven't tracked down the source of the
original rule that changes arch between addition and listing (nor reproduced it
yet since I don't have access to that HW arch), and it seems to reveal
potentially another bug.

Help!  Any observations or hints?

Richard Guy Briggs (3):
  audit: remove arch_f pointer from struct audit_krule
  fixup! audit: remove arch_f pointer from struct audit_krule
  debug! audit: remove arch_f pointer from struct audit_krule

 include/linux/audit.h |  1 -
 kernel/auditfilter.c  | 18 +++++++++++++-----
 2 files changed, 13 insertions(+), 6 deletions(-)

-- 
1.8.3.1

[RFC PATCH 1/3] audit: remove arch_f pointer from struct audit_krule

[Richard Guy Briggs]

The arch_f pointer was added to the struct audit_krule in commit:
e54dc2431d740a79a6bd013babade99d71b1714f ("audit signal recipients")

This is only used on addition and deletion of rules which isn't time
critical and the arch field is likely to be one of the first fields,
easily found iterating over the field type.  This isn't worth the
additional complexity and storage.  Delete the field.

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

diff --git a/include/linux/audit.h b/include/linux/audit.h
index af410d9..64a3b0e 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -58,7 +58,6 @@ struct audit_krule {
 	u32			field_count;
 	char			*filterkey; /* ties events to rules */
 	struct audit_field	*fields;
-	struct audit_field	*arch_f; /* quick access to arch field */
 	struct audit_field	*inode_f; /* quick access to an inode field */
 	struct audit_watch	*watch;	/* associated watch */
 	struct audit_tree	*tree;	/* associated watched tree */
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 739a6d2..3343d1c 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -220,7 +220,14 @@ static inline int audit_match_class_bits(int class, u32 *mask)
 
 static int audit_match_signal(struct audit_entry *entry)
 {
-	struct audit_field *arch = entry->rule.arch_f;
+	int i;
+	struct audit_field *arch;
+
+	for (i = 0; i < entry->rule.field_count; i++)
+		if (entry->rule.fields[i].type == AUDIT_ARCH) {
+			arch = &entry->rule.fields[i];
+			break;
+		}
 
 	if (!arch) {
 		/* When arch is unspecified, we must check both masks on biarch
@@ -496,9 +503,6 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
 			if (!gid_valid(f->gid))
 				goto exit_free;
 			break;
-		case AUDIT_ARCH:
-			entry->rule.arch_f = f;
-			break;
 		case AUDIT_SUBJ_USER:
 		case AUDIT_SUBJ_ROLE:
 		case AUDIT_SUBJ_TYPE:
-- 
1.8.3.1

[RFC PATCH 1/3] audit: remove arch_f pointer from struct audit_krule

[Richard Guy Briggs]

The arch_f pointer was added to the struct audit_krule in commit:
e54dc2431d740a79a6bd013babade99d71b1714f ("audit signal recipients")

This is only used on addition and deletion of rules which isn't time
critical and the arch field is likely to be one of the first fields,
easily found iterating over the field type.  This isn't worth the
additional complexity and storage.  Delete the field.

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

diff --git a/include/linux/audit.h b/include/linux/audit.h
index af410d9..64a3b0e 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -58,7 +58,6 @@ struct audit_krule {
 	u32			field_count;
 	char			*filterkey; /* ties events to rules */
 	struct audit_field	*fields;
-	struct audit_field	*arch_f; /* quick access to arch field */
 	struct audit_field	*inode_f; /* quick access to an inode field */
 	struct audit_watch	*watch;	/* associated watch */
 	struct audit_tree	*tree;	/* associated watched tree */
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 739a6d2..3343d1c 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -220,7 +220,14 @@ static inline int audit_match_class_bits(int class, u32 *mask)
 
 static int audit_match_signal(struct audit_entry *entry)
 {
-	struct audit_field *arch = entry->rule.arch_f;
+	int i;
+	struct audit_field *arch;
+
+	for (i = 0; i < entry->rule.field_count; i++)
+		if (entry->rule.fields[i].type == AUDIT_ARCH) {
+			arch = &entry->rule.fields[i];
+			break;
+		}
 
 	if (!arch) {
 		/* When arch is unspecified, we must check both masks on biarch
@@ -496,9 +503,6 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
 			if (!gid_valid(f->gid))
 				goto exit_free;
 			break;
-		case AUDIT_ARCH:
-			entry->rule.arch_f = f;
-			break;
 		case AUDIT_SUBJ_USER:
 		case AUDIT_SUBJ_ROLE:
 		case AUDIT_SUBJ_TYPE:
-- 
1.8.3.1

[RFC PATCH 2/3] fixup! audit: remove arch_f pointer from struct audit_krule

[Richard Guy Briggs]

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 kernel/auditfilter.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 3343d1c..48dcb59 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -221,11 +221,13 @@ static inline int audit_match_class_bits(int class, u32 *mask)
 static int audit_match_signal(struct audit_entry *entry)
 {
 	int i;
+	u32 archval;
 	struct audit_field *arch;
 
 	for (i = 0; i < entry->rule.field_count; i++)
 		if (entry->rule.fields[i].type == AUDIT_ARCH) {
 			arch = &entry->rule.fields[i];
+			archval = arch->val;
 			break;
 		}
 
@@ -238,7 +240,7 @@ static int audit_match_signal(struct audit_entry *entry)
 					       entry->rule.mask));
 	}
 
-	switch(audit_classify_arch(arch->val)) {
+	switch(audit_classify_arch(archval)) {
 	case 0: /* native */
 		return (audit_match_class_bits(AUDIT_CLASS_SIGNAL,
 					       entry->rule.mask));
-- 
1.8.3.1

[RFC PATCH 2/3] fixup! audit: remove arch_f pointer from struct audit_krule

[Richard Guy Briggs]

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 kernel/auditfilter.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 3343d1c..48dcb59 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -221,11 +221,13 @@ static inline int audit_match_class_bits(int class, u32 *mask)
 static int audit_match_signal(struct audit_entry *entry)
 {
 	int i;
+	u32 archval;
 	struct audit_field *arch;
 
 	for (i = 0; i < entry->rule.field_count; i++)
 		if (entry->rule.fields[i].type == AUDIT_ARCH) {
 			arch = &entry->rule.fields[i];
+			archval = arch->val;
 			break;
 		}
 
@@ -238,7 +240,7 @@ static int audit_match_signal(struct audit_entry *entry)
 					       entry->rule.mask));
 	}
 
-	switch(audit_classify_arch(arch->val)) {
+	switch(audit_classify_arch(archval)) {
 	case 0: /* native */
 		return (audit_match_class_bits(AUDIT_CLASS_SIGNAL,
 					       entry->rule.mask));
-- 
1.8.3.1

[RFC PATCH 3/3] debug! audit: remove arch_f pointer from struct audit_krule

[Richard Guy Briggs]

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 kernel/auditfilter.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 48dcb59..3938ad2c 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -228,6 +228,8 @@ static int audit_match_signal(struct audit_entry *entry)
 		if (entry->rule.fields[i].type == AUDIT_ARCH) {
 			arch = &entry->rule.fields[i];
 			archval = arch->val;
+			pr_info("ams_: i=%d arch_f=%px type=%d op=%d val=%x key=\"%s\"",
+				i, arch, arch->type, arch->op, arch->val, entry->rule.filterkey ?: "");
 			break;
 		}
 
-- 
1.8.3.1

Re: [RFC PATCH 1/3] audit: remove arch_f pointer from struct audit_krule

[Paul Moore]

On Mon, Feb 12, 2018 at 7:29 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> The arch_f pointer was added to the struct audit_krule in commit:
> e54dc2431d740a79a6bd013babade99d71b1714f ("audit signal recipients")
>
> This is only used on addition and deletion of rules which isn't time
> critical and the arch field is likely to be one of the first fields,
> easily found iterating over the field type.  This isn't worth the
> additional complexity and storage.  Delete the field.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  include/linux/audit.h |  1 -
>  kernel/auditfilter.c  | 12 ++++++++----
>  2 files changed, 8 insertions(+), 5 deletions(-)

I haven't decided if I like the removal of arch_f or not, but I think
I might know where your oops/panic is coming from, thoughts below ...

> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index af410d9..64a3b0e 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -58,7 +58,6 @@ struct audit_krule {
>         u32                     field_count;
>         char                    *filterkey; /* ties events to rules */
>         struct audit_field      *fields;
> -       struct audit_field      *arch_f; /* quick access to arch field */
>         struct audit_field      *inode_f; /* quick access to an inode field */
>         struct audit_watch      *watch; /* associated watch */
>         struct audit_tree       *tree;  /* associated watched tree */
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 739a6d2..3343d1c 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -220,7 +220,14 @@ static inline int audit_match_class_bits(int class, u32 *mask)
>
>  static int audit_match_signal(struct audit_entry *entry)
>  {
> -       struct audit_field *arch = entry->rule.arch_f;
> +       int i;
> +       struct audit_field *arch;
> +
> +       for (i = 0; i < entry->rule.field_count; i++)
> +               if (entry->rule.fields[i].type == AUDIT_ARCH) {
> +                       arch = &entry->rule.fields[i];
> +                       break;
> +               }

In the original code arch_f was initialized to NULL via the allocator
so the arch local variable was guaranteed to have a valid value or
NULL.  Unfortunately, in your code if there is no AUDIT_ARCH field
arch could remain uninitialized which I believe could lead to the
oops/panic you are seeing.

>         if (!arch) {
>                 /* When arch is unspecified, we must check both masks on biarch
> @@ -496,9 +503,6 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
>                         if (!gid_valid(f->gid))
>                                 goto exit_free;
>                         break;
> -               case AUDIT_ARCH:
> -                       entry->rule.arch_f = f;
> -                       break;
>                 case AUDIT_SUBJ_USER:
>                 case AUDIT_SUBJ_ROLE:
>                 case AUDIT_SUBJ_TYPE:
> --
> 1.8.3.1

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH 2/3] fixup! audit: remove arch_f pointer from struct audit_krule

[Paul Moore]

On Mon, Feb 12, 2018 at 7:29 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  kernel/auditfilter.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

I realize this is an RFC patchset, but considering recent patchsets I
feel some clarification might be helpful to prevent future delays ...
when submitting patchsets for merging please do not submit "fixup!"
patches which fix problems in patches that are submitted earlier in
the patchset, simply merge/squash the "fixup!" patches before
submitting.

> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 3343d1c..48dcb59 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -221,11 +221,13 @@ static inline int audit_match_class_bits(int class, u32 *mask)
>  static int audit_match_signal(struct audit_entry *entry)
>  {
>         int i;
> +       u32 archval;
>         struct audit_field *arch;
>
>         for (i = 0; i < entry->rule.field_count; i++)
>                 if (entry->rule.fields[i].type == AUDIT_ARCH) {
>                         arch = &entry->rule.fields[i];
> +                       archval = arch->val;
>                         break;
>                 }
>
> @@ -238,7 +240,7 @@ static int audit_match_signal(struct audit_entry *entry)
>                                                entry->rule.mask));
>         }
>
> -       switch(audit_classify_arch(arch->val)) {
> +       switch(audit_classify_arch(archval)) {
>         case 0: /* native */
>                 return (audit_match_class_bits(AUDIT_CLASS_SIGNAL,
>                                                entry->rule.mask));
> --
> 1.8.3.1
>



-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH 2/3] fixup! audit: remove arch_f pointer from struct audit_krule

[Richard Guy Briggs]

On 2018-02-15 15:43, Paul Moore wrote:
> On Mon, Feb 12, 2018 at 7:29 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  kernel/auditfilter.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> I realize this is an RFC patchset, but considering recent patchsets I
> feel some clarification might be helpful to prevent future delays ...
> when submitting patchsets for merging please do not submit "fixup!"
> patches which fix problems in patches that are submitted earlier in
> the patchset, simply merge/squash the "fixup!" patches before
> submitting.

Yeah, the only reason this is a "fixup!" patch is to clearly show what
remedial step was (surprisingly) needed to fix the bug.

Same with the following "debug!" annotation.  Clearly not intended for
upstream to Linus.

> > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > index 3343d1c..48dcb59 100644
> > --- a/kernel/auditfilter.c
> > +++ b/kernel/auditfilter.c
> > @@ -221,11 +221,13 @@ static inline int audit_match_class_bits(int class, u32 *mask)
> >  static int audit_match_signal(struct audit_entry *entry)
> >  {
> >         int i;
> > +       u32 archval;
> >         struct audit_field *arch;
> >
> >         for (i = 0; i < entry->rule.field_count; i++)
> >                 if (entry->rule.fields[i].type == AUDIT_ARCH) {
> >                         arch = &entry->rule.fields[i];
> > +                       archval = arch->val;
> >                         break;
> >                 }
> >
> > @@ -238,7 +240,7 @@ static int audit_match_signal(struct audit_entry *entry)
> >                                                entry->rule.mask));
> >         }
> >
> > -       switch(audit_classify_arch(arch->val)) {
> > +       switch(audit_classify_arch(archval)) {
> >         case 0: /* native */
> >                 return (audit_match_class_bits(AUDIT_CLASS_SIGNAL,
> >                                                entry->rule.mask));
> > --
> > 1.8.3.1
> 
> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [RFC PATCH 1/3] audit: remove arch_f pointer from struct audit_krule

[Richard Guy Briggs]

On 2018-02-15 15:42, Paul Moore wrote:
> On Mon, Feb 12, 2018 at 7:29 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > The arch_f pointer was added to the struct audit_krule in commit:
> > e54dc2431d740a79a6bd013babade99d71b1714f ("audit signal recipients")
> >
> > This is only used on addition and deletion of rules which isn't time
> > critical and the arch field is likely to be one of the first fields,
> > easily found iterating over the field type.  This isn't worth the
> > additional complexity and storage.  Delete the field.
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  include/linux/audit.h |  1 -
> >  kernel/auditfilter.c  | 12 ++++++++----
> >  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> I haven't decided if I like the removal of arch_f or not, but I think
> I might know where your oops/panic is coming from, thoughts below ...
> 
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index af410d9..64a3b0e 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -58,7 +58,6 @@ struct audit_krule {
> >         u32                     field_count;
> >         char                    *filterkey; /* ties events to rules */
> >         struct audit_field      *fields;
> > -       struct audit_field      *arch_f; /* quick access to arch field */
> >         struct audit_field      *inode_f; /* quick access to an inode field */
> >         struct audit_watch      *watch; /* associated watch */
> >         struct audit_tree       *tree;  /* associated watched tree */
> > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > index 739a6d2..3343d1c 100644
> > --- a/kernel/auditfilter.c
> > +++ b/kernel/auditfilter.c
> > @@ -220,7 +220,14 @@ static inline int audit_match_class_bits(int class, u32 *mask)
> >
> >  static int audit_match_signal(struct audit_entry *entry)
> >  {
> > -       struct audit_field *arch = entry->rule.arch_f;
> > +       int i;
> > +       struct audit_field *arch;
> > +
> > +       for (i = 0; i < entry->rule.field_count; i++)
> > +               if (entry->rule.fields[i].type == AUDIT_ARCH) {
> > +                       arch = &entry->rule.fields[i];
> > +                       break;
> > +               }
> 
> In the original code arch_f was initialized to NULL via the allocator
> so the arch local variable was guaranteed to have a valid value or
> NULL.  Unfortunately, in your code if there is no AUDIT_ARCH field
> arch could remain uninitialized which I believe could lead to the
> oops/panic you are seeing.

Ok, so the function local stack variable allocator doesn't initialze
local variables, and what was happenning was it was skipping the "if
(!arch)" clause (due to the spurrious "9" value of arch) jumping
straight in to the arch->val evaluation with a bogus arch pointer.  I
was fairly certain it was happenning on a rule that had an arch field,
but in fact it was happenning on one that didn't.  I was hinging on the
assumption local variables getting initialized to zero (or NULL).

Problem solved.  Thank you.

> >         if (!arch) {
> >                 /* When arch is unspecified, we must check both masks on biarch
> > @@ -496,9 +503,6 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
> >                         if (!gid_valid(f->gid))
> >                                 goto exit_free;
> >                         break;
> > -               case AUDIT_ARCH:
> > -                       entry->rule.arch_f = f;
> > -                       break;
> >                 case AUDIT_SUBJ_USER:
> >                 case AUDIT_SUBJ_ROLE:
> >                 case AUDIT_SUBJ_TYPE:
> > --
> > 1.8.3.1
> 
> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [RFC PATCH 1/3] audit: remove arch_f pointer from struct audit_krule

[Richard Guy Briggs]

On 2018-02-15 15:42, Paul Moore wrote:
> On Mon, Feb 12, 2018 at 7:29 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > The arch_f pointer was added to the struct audit_krule in commit:
> > e54dc2431d740a79a6bd013babade99d71b1714f ("audit signal recipients")
> >
> > This is only used on addition and deletion of rules which isn't time
> > critical and the arch field is likely to be one of the first fields,
> > easily found iterating over the field type.  This isn't worth the
> > additional complexity and storage.  Delete the field.
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  include/linux/audit.h |  1 -
> >  kernel/auditfilter.c  | 12 ++++++++----
> >  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> I haven't decided if I like the removal of arch_f or not, but I think
> I might know where your oops/panic is coming from, thoughts below ...

Have you decided yet if you like the removal of the arch_f pointer or
not?  An updated v2 was provided the following day:
	https://www.redhat.com/archives/linux-audit/2018-February/msg00059.html

I will send an updated patch if it seems worthwhile.

> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index af410d9..64a3b0e 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -58,7 +58,6 @@ struct audit_krule {
> >         u32                     field_count;
> >         char                    *filterkey; /* ties events to rules */
> >         struct audit_field      *fields;
> > -       struct audit_field      *arch_f; /* quick access to arch field */
> >         struct audit_field      *inode_f; /* quick access to an inode field */
> >         struct audit_watch      *watch; /* associated watch */
> >         struct audit_tree       *tree;  /* associated watched tree */
> > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > index 739a6d2..3343d1c 100644
> > --- a/kernel/auditfilter.c
> > +++ b/kernel/auditfilter.c
> > @@ -220,7 +220,14 @@ static inline int audit_match_class_bits(int class, u32 *mask)
> >
> >  static int audit_match_signal(struct audit_entry *entry)
> >  {
> > -       struct audit_field *arch = entry->rule.arch_f;
> > +       int i;
> > +       struct audit_field *arch;
> > +
> > +       for (i = 0; i < entry->rule.field_count; i++)
> > +               if (entry->rule.fields[i].type == AUDIT_ARCH) {
> > +                       arch = &entry->rule.fields[i];
> > +                       break;
> > +               }
> 
> In the original code arch_f was initialized to NULL via the allocator
> so the arch local variable was guaranteed to have a valid value or
> NULL.  Unfortunately, in your code if there is no AUDIT_ARCH field
> arch could remain uninitialized which I believe could lead to the
> oops/panic you are seeing.
> 
> >         if (!arch) {
> >                 /* When arch is unspecified, we must check both masks on biarch
> > @@ -496,9 +503,6 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
> >                         if (!gid_valid(f->gid))
> >                                 goto exit_free;
> >                         break;
> > -               case AUDIT_ARCH:
> > -                       entry->rule.arch_f = f;
> > -                       break;
> >                 case AUDIT_SUBJ_USER:
> >                 case AUDIT_SUBJ_ROLE:
> >                 case AUDIT_SUBJ_TYPE:
> > --
> > 1.8.3.1
> 
> -- 
> paul moore
> www.paul-moore.com
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [RFC PATCH 1/3] audit: remove arch_f pointer from struct audit_krule

[Paul Moore]

On Sun, Nov 25, 2018 at 12:11 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-02-15 15:42, Paul Moore wrote:
> > On Mon, Feb 12, 2018 at 7:29 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > > The arch_f pointer was added to the struct audit_krule in commit:
> > > e54dc2431d740a79a6bd013babade99d71b1714f ("audit signal recipients")
> > >
> > > This is only used on addition and deletion of rules which isn't time
> > > critical and the arch field is likely to be one of the first fields,
> > > easily found iterating over the field type.  This isn't worth the
> > > additional complexity and storage.  Delete the field.
> > >
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > ---
> > >  include/linux/audit.h |  1 -
> > >  kernel/auditfilter.c  | 12 ++++++++----
> > >  2 files changed, 8 insertions(+), 5 deletions(-)
> >
> > I haven't decided if I like the removal of arch_f or not, but I think
> > I might know where your oops/panic is coming from, thoughts below ...
>
> Have you decided yet if you like the removal of the arch_f pointer or
> not?  An updated v2 was provided the following day:
>         https://www.redhat.com/archives/linux-audit/2018-February/msg00059.html

I still think I'd like to keep it as-is for now.

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH 1/3] audit: remove arch_f pointer from struct audit_krule

[Richard Guy Briggs]

On 2018-11-26 11:37, Paul Moore wrote:
> On Sun, Nov 25, 2018 at 12:11 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2018-02-15 15:42, Paul Moore wrote:
> > > On Mon, Feb 12, 2018 at 7:29 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > The arch_f pointer was added to the struct audit_krule in commit:
> > > > e54dc2431d740a79a6bd013babade99d71b1714f ("audit signal recipients")
> > > >
> > > > This is only used on addition and deletion of rules which isn't time
> > > > critical and the arch field is likely to be one of the first fields,
> > > > easily found iterating over the field type.  This isn't worth the
> > > > additional complexity and storage.  Delete the field.
> > > >
> > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > ---
> > > >  include/linux/audit.h |  1 -
> > > >  kernel/auditfilter.c  | 12 ++++++++----
> > > >  2 files changed, 8 insertions(+), 5 deletions(-)
> > >
> > > I haven't decided if I like the removal of arch_f or not, but I think
> > > I might know where your oops/panic is coming from, thoughts below ...
> >
> > Have you decided yet if you like the removal of the arch_f pointer or
> > not?  An updated v2 was provided the following day:
> >         https://www.redhat.com/archives/linux-audit/2018-February/msg00059.html
> 
> I still think I'd like to keep it as-is for now.

Can you explain why you'd prefer to keep it as-is for now?  Is there a
factor I'm not aware of that might make it acceptable later?  arch_f
appears to make the code noisier than needed and use extra memory that
is a convenience at best only when adding or deleting rules.

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [RFC PATCH 1/3] audit: remove arch_f pointer from struct audit_krule

[Paul Moore]

On Mon, Nov 26, 2018 at 2:21 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-11-26 11:37, Paul Moore wrote:
> > On Sun, Nov 25, 2018 at 12:11 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > On 2018-02-15 15:42, Paul Moore wrote:
> > > > On Mon, Feb 12, 2018 at 7:29 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > > The arch_f pointer was added to the struct audit_krule in commit:
> > > > > e54dc2431d740a79a6bd013babade99d71b1714f ("audit signal recipients")
> > > > >
> > > > > This is only used on addition and deletion of rules which isn't time
> > > > > critical and the arch field is likely to be one of the first fields,
> > > > > easily found iterating over the field type.  This isn't worth the
> > > > > additional complexity and storage.  Delete the field.
> > > > >
> > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > > ---
> > > > >  include/linux/audit.h |  1 -
> > > > >  kernel/auditfilter.c  | 12 ++++++++----
> > > > >  2 files changed, 8 insertions(+), 5 deletions(-)
> > > >
> > > > I haven't decided if I like the removal of arch_f or not, but I think
> > > > I might know where your oops/panic is coming from, thoughts below ...
> > >
> > > Have you decided yet if you like the removal of the arch_f pointer or
> > > not?  An updated v2 was provided the following day:
> > >         https://www.redhat.com/archives/linux-audit/2018-February/msg00059.html
> >
> > I still think I'd like to keep it as-is for now.
>
> Can you explain why you'd prefer to keep it as-is for now?  Is there a
> factor I'm not aware of that might make it acceptable later?  arch_f
> appears to make the code noisier than needed and use extra memory that
> is a convenience at best only when adding or deleting rules.

Personal preference for the existing approach, status quo, gut
feeling, etc.  Some things are simply judgement calls.

Please stop trying to find a four page, black-and-white explanation
reason for every single patch that is rejected; it is getting very
tiring.  I typically provide reasons when I don't merge a patch, and
often on these smaller, trade-off patches it falls into the "judgement
call" category and there simply isn't a detailed response to be given.

-- 
paul moore
www.paul-moore.com