Re: [PATCH v2] audit: add fields to exclude filter by reusing user filter

[Richard Guy Briggs]

On 2016-06-16 16:54, Paul Moore wrote:
> On Tue, Jun 14, 2016 at 5:04 PM, Richard Guy Briggs <rgb@xxxxxxxxxx> wrote:
> > RFE: add additional fields for use in audit filter exclude rules
> > https://github.com/linux-audit/audit-kernel/issues/5
> >
> > Re-factor and combine audit_filter_type() with audit_filter_user() to
> > use audit_filter_user_rules() to enable the exclude filter to
> > additionally filter on PID, UID, GID, AUID, LOGINUID_SET, SUBJ_*.
> >
> > The process of combining the similar audit_filter_user() and
> > audit_filter_type() functions, required inverting the meaning and
> > including the ALWAYS action of the latter.
> >
> > Keep the check to quit early if the list is empty.
> >
> > Signed-off-by: Richard Guy Briggs <rgb@xxxxxxxxxx>
> > ---
> > v2: combine audit_filter_user() and audit_filter_type() into
> > audit_filter().
> > ---
> >
> >  include/linux/audit.h |    2 --
> >  kernel/audit.c        |    4 ++--
> >  kernel/audit.h        |    2 ++
> >  kernel/auditfilter.c  |   46 ++++++++++------------------------------------
> >  4 files changed, 14 insertions(+), 40 deletions(-)
> 
> Patches that remove more code than the add always make me happy :)

Ok, see v3, instead of -26, how about -41?

> Comment below ...

...

> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 32cdafb..539c1d9 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -160,8 +160,6 @@ extern void audit_log_task_info(struct audit_buffer *ab,
> >  extern int                 audit_update_lsm_rules(void);
> >
> >                                 /* Private API (for audit.c only) */
> > -extern int audit_filter_user(int type);
> > -extern int audit_filter_type(int type);
> >  extern int audit_rule_change(int type, __u32 portid, int seq,
> >                                 void *data, size_t datasz);
> >  extern int audit_list_rules_send(struct sk_buff *request_skb, int seq);
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 384374a..2dfaa19 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -914,7 +914,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> >                 if (!audit_enabled && msg_type != AUDIT_USER_AVC)
> >                         return 0;
> >
> > -               err = audit_filter_user(msg_type);
> > +               err = audit_filter(msg_type, AUDIT_FILTER_USER);
> >                 if (err == 1) { /* match or error */
> >                         err = 0;
> >                         if (msg_type == AUDIT_USER_TTY) {
> > @@ -1362,7 +1362,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
> >         if (audit_initialized != AUDIT_INITIALIZED)
> >                 return NULL;
> >
> > -       if (unlikely(audit_filter_type(type)))
> > +       if (unlikely(!audit_filter(type, AUDIT_FILTER_TYPE)))
> >                 return NULL;
> >
> >         if (gfp_mask & __GFP_DIRECT_RECLAIM) {
> > diff --git a/kernel/audit.h b/kernel/audit.h
> > index cbbe6bb..1879f02 100644
> > --- a/kernel/audit.h
> > +++ b/kernel/audit.h
> > @@ -327,6 +327,8 @@ extern pid_t audit_sig_pid;
> >  extern kuid_t audit_sig_uid;
> >  extern u32 audit_sig_sid;
> >
> > +extern int audit_filter(int msgtype, unsigned int listtype);
> > +
> >  #ifdef CONFIG_AUDITSYSCALL
> >  extern int __audit_signal_info(int sig, struct task_struct *t);
> >  static inline int audit_signal_info(int sig, struct task_struct *t)
> > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > index 96c9a1b..f90c042 100644
> > --- a/kernel/auditfilter.c
> > +++ b/kernel/auditfilter.c
> > @@ -1349,50 +1349,24 @@ static int audit_filter_user_rules(struct audit_krule *rule, int type,
> >         return 1;
> >  }
> >
> > -int audit_filter_user(int type)
> > +int audit_filter(int msgtype, unsigned int listtype)
> >  {
> >         enum audit_state state = AUDIT_DISABLED;
> >         struct audit_entry *e;
> > -       int rc, ret;
> > -
> > -       ret = 1; /* Audit by default */
> > +       int rc, result = 1; /* Audit by default */
> >
> >         rcu_read_lock();
> > -       list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_USER], list) {
> > -               rc = audit_filter_user_rules(&e->rule, type, &state);
> > -               if (rc) {
> > -                       if (rc > 0 && state == AUDIT_DISABLED)
> > -                               ret = 0;
> > -                       break;
> > -               }
> > -       }
> > -       rcu_read_unlock();
> > -
> > -       return ret;
> > -}
> > -
> > -int audit_filter_type(int type)
> > -{
> > -       struct audit_entry *e;
> > -       int result = 0;
> > -
> > -       rcu_read_lock();
> > -       if (list_empty(&audit_filter_list[AUDIT_FILTER_TYPE]))
> > +       if (list_empty(&audit_filter_list[listtype]))
> >                 goto unlock_and_return;
> >
> > -       list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_TYPE],
> > -                               list) {
> > -               int i;
> > -               for (i = 0; i < e->rule.field_count; i++) {
> > -                       struct audit_field *f = &e->rule.fields[i];
> > -                       if (f->type == AUDIT_MSGTYPE) {
> > -                               result = audit_comparator(type, f->op, f->val);
> > -                               if (!result)
> > -                                       break;
> > -                       }
> > +       list_for_each_entry_rcu(e, &audit_filter_list[listtype], list) {
> > +               rc = audit_filter_user_rules(&e->rule, msgtype, &state);
> 
> Any reason not to do away with audit_filter_user_rules() and just
> insert the code here?  As far as I can tell there are no other callers
> ...

Effort?  It was a bit messy pulling it in, but with a bit of effort to
clear out some accumulated cruft the result appears clearer to me.  Was
it worth it?

> > +               if (rc) {
> > +                       if (rc > 0 && ((state == AUDIT_DISABLED) ||
> > +                                      (listtype == AUDIT_FILTER_TYPE)))
> > +                               result = 0;
> > +                       break;
> >                 }
> > -               if (result)
> > -                       goto unlock_and_return;
> >         }
> >  unlock_and_return:
> >         rcu_read_unlock();
> 
> paul moore

- RGB

--
Richard Guy Briggs <rgb@xxxxxxxxxx>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635