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