simplify struct audit_krule reveals bug

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 <[email protected]>

---
 include/linux/audit.h |  1 -
 kernel/auditfilter.c  | 12 ++++++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

include/linux/audit.h +0 -1

index af410d9..64a3b0e 100644
@@ -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 */

kernel/auditfilter.c +0 -0

index 739a6d2..3343d1c 100644
@@ -220,7 +220,14 @@ static inline int audit_match_class_bits(int class, u32 *mask)
 
 static int audit_match_signal(struct audit_entry *entry)

kernel/auditfilter.c +0 -4

        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

Quoted 8 lines · 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.

Quoted 5 lines

Signed-off-by Richard Guy Briggs <[email protected]>

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

Quoted 27 lines

include/linux/audit.h +0 -1

index af410d9..64a3b0e 100644
@@ -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 */

kernel/auditfilter.c +6 -1

index 739a6d2..3343d1c 100644
@@ -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];

kernel/auditfilter.c static inline int audit_match_class_bits(int class, u32 *mask) · line 230

Quoted 2 lines

kernel/auditfilter.c +2 -0

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

Quoted 8 lines

kernel/auditfilter.c +0 -2

        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;

Quoted 6 lines

kernel/auditfilter.c +0 -2

-                       break;
                case AUDIT_SUBJ_USER:
                case AUDIT_SUBJ_ROLE:
                case AUDIT_SUBJ_TYPE:
--

1.8.3.1

paul moore

Quoted 8 lines · 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.

Quoted 5 lines

Signed-off-by Richard Guy Briggs <[email protected]>

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

Quoted 19 lines

include/linux/audit.h +0 -1

index af410d9..64a3b0e 100644
@@ -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 */

kernel/auditfilter.c +0 -0

index 739a6d2..3343d1c 100644
@@ -220,7 +220,14 @@ static inline int audit_match_class_bits(int class, u32 *mask)
 
 static int audit_match_signal(struct audit_entry *entry)

kernel/auditfilter.c static inline int audit_match_class_bits(int class, u32 *mask) · line 230

Quoted 10 lines

kernel/auditfilter.c +8 -1

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

Quoted 14 lines

kernel/auditfilter.c +0 -4

        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

Signature · 7 lines

paul moore
www.paul-moore.com

--
Linux-audit mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/linux-audit

Quoted 15 lines · Paul Moore

Quoted 13 lines · 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 <[email protected]>

---
 include/linux/audit.h |  1 -
 kernel/auditfilter.c  | 12 ++++++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

Quoted 2 lines

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

Quoted 16 lines · Richard Guy Briggs

Quoted 15 lines · Paul Moore

Quoted 13 lines · 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 <[email protected]>

---
 include/linux/audit.h |  1 -
 kernel/auditfilter.c  | 12 ++++++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

Quoted 6 lines

Quoted 2 lines

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

Quoted 24 lines · Paul Moore

Quoted 22 lines · Richard Guy Briggs

Quoted 9 lines · Paul Moore

Quoted 8 lines · 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.

Quoted 8 lines

Quoted 5 lines

Signed-off-by Richard Guy Briggs <[email protected]>

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

Quoted 1 line

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.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 kernel/auditfilter.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

kernel/auditfilter.c +3 -2

index 3343d1c..48dcb59 100644
@@ -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

Quoted 4 lines · Richard Guy Briggs

Signed-off-by Richard Guy Briggs <[email protected]>

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

Quoted 22 lines

kernel/auditfilter.c +2 -0

index 3343d1c..48dcb59 100644
@@ -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));
        }
 

Quoted 7 lines

kernel/auditfilter.c +1 -2

-       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