On 2018-05-31 17:29, Steve Grubb wrote:
> On Thursday, May 31, 2018 4:23:09 PM EDT Richard Guy Briggs wrote:
> > The AUDIT_FILTER_TYPE name is vague and misleading due to not describing
> > where or when the filter is applied and obsolete due to its available
> > filter fields having been expanded.
> > 
> > Userspace has already renamed it from AUDIT_FILTER_TYPE to
> > AUDIT_FILTER_EXCLUDE without checking if it already exists.
> 
> Historically speaking, this is not why it is the way it is. But I think it 
> doesn't mean that you cannot do something like this:
> 
> #define AUDIT_FILTER_EXCLUDE    AUDIT_FILTER_TYPE

I was originally hoping to do that, but that then causes a build error
on any previous version of audit userspace.

> It's easy then to add a #ifndef to the userspace code so that there is an 
> easy migration. I also do not see any compiler warnings with the above in 
> both /usr/include/linux/audit.h and /usr/include/libaudit.h.

I know it would be easy to fix and going forward userspace could adopt
the kernel one with

        #ifndef AUDIT_FILTER_EXCL
        #define AUDIT_FILTER_EXCL AUDIT_FILTER_TYPE
        #endif

I do have a minor preference for the shorter AUDIT_FILTER_EXCL.

The only place in userspace that AUDIT_FILTER_TYPE is used is grabbing
the value from the kernel to set AUDIT_FILTER_EXCLUDE, and in two bits
of documentation (since that is the published API for libaudit).

Might it make sense to still make AUDIT_FILTER_TYPE available for
libaudit users, but change over the documentation to use one of the
newer forms?

> -Steve
> 
> > In order to
> > not cause userspace compile problems from duplicate definitions and to
> > more accurately and inclusively rename it in the kernel, while providing
> > a migration path for userspace, rename it to AUDIT_FILTER_EXCL.
> > 
> > See: https://github.com/linux-audit/audit-kernel/issues/89
> > 
> > Signed-off-by: Richard Guy Briggs <r...@redhat.com>
> > ---
> >  include/uapi/linux/audit.h |  3 ++-
> >  kernel/audit.c             |  2 +-
> >  kernel/auditfilter.c       | 10 +++++-----
> >  3 files changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index 04f9bd2..45dd7ef 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -156,8 +156,9 @@
> >  #define AUDIT_FILTER_ENTRY 0x02    /* Apply rule at syscall entry */
> >  #define AUDIT_FILTER_WATCH 0x03    /* Apply rule to file system watches */
> >  #define AUDIT_FILTER_EXIT  0x04    /* Apply rule at syscall exit */
> > -#define AUDIT_FILTER_TYPE  0x05    /* Apply rule at audit_log_start */
> > +#define AUDIT_FILTER_EXCL  0x05    /* Apply rule at audit_log_start */
> >  #define AUDIT_FILTER_FS            0x06    /* Apply rule at 
> > __audit_inode_child */
> > +#define AUDIT_FILTER_TYPE  AUDIT_FILTER_EXCL /* obsolete misleading naming
> > */
> > 
> >  #define AUDIT_NR_FILTERS   7
> > 
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 3a18e59..089cede 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -1754,7 +1754,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, AUDIT_FILTER_TYPE)))
> > +   if (unlikely(!audit_filter(type, AUDIT_FILTER_EXCL)))
> >             return NULL;
> > 
> >     /* NOTE: don't ever fail/sleep on these two conditions:
> > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > index eaa3201..f17a42f5 100644
> > --- a/kernel/auditfilter.c
> > +++ b/kernel/auditfilter.c
> > @@ -264,7 +264,7 @@ static inline struct audit_entry
> > *audit_to_entry_common(struct audit_rule_data * case AUDIT_FILTER_TASK:
> >  #endif
> >     case AUDIT_FILTER_USER:
> > -   case AUDIT_FILTER_TYPE:
> > +   case AUDIT_FILTER_EXCL:
> >     case AUDIT_FILTER_FS:
> >             ;
> >     }
> > @@ -337,7 +337,7 @@ static int audit_field_valid(struct audit_entry *entry,
> > struct audit_field *f) {
> >     switch(f->type) {
> >     case AUDIT_MSGTYPE:
> > -           if (entry->rule.listnr != AUDIT_FILTER_TYPE &&
> > +           if (entry->rule.listnr != AUDIT_FILTER_EXCL &&
> >                 entry->rule.listnr != AUDIT_FILTER_USER)
> >                     return -EINVAL;
> >             break;
> > @@ -931,7 +931,7 @@ static inline int audit_add_rule(struct audit_entry
> > *entry) /* If any of these, don't count towards total */
> >     switch(entry->rule.listnr) {
> >     case AUDIT_FILTER_USER:
> > -   case AUDIT_FILTER_TYPE:
> > +   case AUDIT_FILTER_EXCL:
> >     case AUDIT_FILTER_FS:
> >             dont_count = 1;
> >     }
> > @@ -1013,7 +1013,7 @@ int audit_del_rule(struct audit_entry *entry)
> >     /* If any of these, don't count towards total */
> >     switch(entry->rule.listnr) {
> >     case AUDIT_FILTER_USER:
> > -   case AUDIT_FILTER_TYPE:
> > +   case AUDIT_FILTER_EXCL:
> >     case AUDIT_FILTER_FS:
> >             dont_count = 1;
> >     }
> > @@ -1369,7 +1369,7 @@ int audit_filter(int msgtype, unsigned int listtype)
> >                             break;
> >             }
> >             if (result > 0) {
> > -                   if (e->rule.action == AUDIT_NEVER || listtype == 
> > AUDIT_FILTER_TYPE)
> > +                   if (e->rule.action == AUDIT_NEVER || listtype == 
> > AUDIT_FILTER_EXCL)
> >                             ret = 0;
> >                     break;
> >             }
> 
> 
> 
> 

- RGB

--
Richard Guy Briggs <r...@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

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Reply via email to