* [RFC PATCH 0/2] Extend AUDIT_EXE and AUDIT_DIR to more filter types @ 2018-05-30 8:45 Ondrej Mosnacek 2018-05-30 8:45 ` [RFC PATCH 1/2] audit: allow other filter list types for AUDIT_EXE Ondrej Mosnacek 2018-05-30 8:45 ` [RFC PATCH 2/2] [WIP] audit: allow other filter list types for AUDIT_DIR Ondrej Mosnacek 0 siblings, 2 replies; 15+ messages in thread From: Ondrej Mosnacek @ 2018-05-30 8:45 UTC (permalink / raw) To: linux-audit; +Cc: Richard Guy Briggs This patch set extends the previous AUDIT_EXE patch by also doing a similar thing with the AUDIT_DIR field. I am sending it as RFC since this change requires passing audit_context to audit_filter and I'm not sure if I should also pass it when doing the AUDIT_FILTER_USER filtering. The call site does not have the ctx variable, although I suppose it could be extracted from the current task somehow, but I'm not sure if it even makes sense to use it in that place. I am not enabling AUDIT_DIR for AUDIT_FILTER_USER in this patch, but if it makes sense I will do that in the final patch. Paul/Richard, please advise. See the FIXME in the second patch for the problematic location. Ondrej Mosnacek (2): audit: allow other filter list types for AUDIT_EXE [WIP] audit: allow other filter list types for AUDIT_DIR kernel/audit.c | 5 +++-- kernel/audit.h | 32 +++++++++++++++++++++++++++++++- kernel/audit_tree.c | 4 +++- kernel/auditfilter.c | 13 ++++++++++--- kernel/auditsc.c | 28 ---------------------------- 5 files changed, 47 insertions(+), 35 deletions(-) -- 2.17.0 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [RFC PATCH 1/2] audit: allow other filter list types for AUDIT_EXE 2018-05-30 8:45 [RFC PATCH 0/2] Extend AUDIT_EXE and AUDIT_DIR to more filter types Ondrej Mosnacek @ 2018-05-30 8:45 ` Ondrej Mosnacek 2018-05-31 16:29 ` Richard Guy Briggs ` (2 more replies) 2018-05-30 8:45 ` [RFC PATCH 2/2] [WIP] audit: allow other filter list types for AUDIT_DIR Ondrej Mosnacek 1 sibling, 3 replies; 15+ messages in thread From: Ondrej Mosnacek @ 2018-05-30 8:45 UTC (permalink / raw) To: linux-audit; +Cc: Richard Guy Briggs This patch removes the restriction of the AUDIT_EXE field to only SYSCALL filter and teaches audit_filter to recognize this field. This makes it possible to write rule lists such as: auditctl -a exit,always [some general rule] # Filter out events with executable name /bin/exe1 or /bin/exe2: auditctl -a exclude,always -F exe=/bin/exe1 auditctl -a exclude,always -F exe=/bin/exe2 See: https://github.com/linux-audit/audit-kernel/issues/54 Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> --- kernel/auditfilter.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c index eaa320148d97..6db9847ca031 100644 --- a/kernel/auditfilter.c +++ b/kernel/auditfilter.c @@ -428,8 +428,6 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f) case AUDIT_EXE: if (f->op != Audit_not_equal && f->op != Audit_equal) return -EINVAL; - if (entry->rule.listnr != AUDIT_FILTER_EXIT) - return -EINVAL; break; } return 0; @@ -1360,6 +1358,11 @@ int audit_filter(int msgtype, unsigned int listtype) f->type, f->op, f->lsm_rule, NULL); } break; + case AUDIT_EXE: + result = audit_exe_compare(current, e->rule.exe); + if (f->op == Audit_not_equal) + result = !result; + break; default: goto unlock_and_return; } -- 2.17.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 1/2] audit: allow other filter list types for AUDIT_EXE 2018-05-30 8:45 ` [RFC PATCH 1/2] audit: allow other filter list types for AUDIT_EXE Ondrej Mosnacek @ 2018-05-31 16:29 ` Richard Guy Briggs 2018-06-04 20:41 ` Paul Moore 2018-06-19 14:25 ` Paul Moore 2 siblings, 0 replies; 15+ messages in thread From: Richard Guy Briggs @ 2018-05-31 16:29 UTC (permalink / raw) To: Ondrej Mosnacek; +Cc: linux-audit On 2018-05-30 10:45, Ondrej Mosnacek wrote: > This patch removes the restriction of the AUDIT_EXE field to only > SYSCALL filter and teaches audit_filter to recognize this field. > > This makes it possible to write rule lists such as: > > auditctl -a exit,always [some general rule] > # Filter out events with executable name /bin/exe1 or /bin/exe2: > auditctl -a exclude,always -F exe=/bin/exe1 > auditctl -a exclude,always -F exe=/bin/exe2 > > See: https://github.com/linux-audit/audit-kernel/issues/54 > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> Reviewed-by: Richard Guy Briggs <rgb@redhat.com> > --- > kernel/auditfilter.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c > index eaa320148d97..6db9847ca031 100644 > --- a/kernel/auditfilter.c > +++ b/kernel/auditfilter.c > @@ -428,8 +428,6 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f) > case AUDIT_EXE: > if (f->op != Audit_not_equal && f->op != Audit_equal) > return -EINVAL; > - if (entry->rule.listnr != AUDIT_FILTER_EXIT) > - return -EINVAL; > break; > } > return 0; > @@ -1360,6 +1358,11 @@ int audit_filter(int msgtype, unsigned int listtype) > f->type, f->op, f->lsm_rule, NULL); > } > break; > + case AUDIT_EXE: > + result = audit_exe_compare(current, e->rule.exe); > + if (f->op == Audit_not_equal) > + result = !result; > + break; > default: > goto unlock_and_return; > } > -- > 2.17.0 > - 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 1/2] audit: allow other filter list types for AUDIT_EXE 2018-05-30 8:45 ` [RFC PATCH 1/2] audit: allow other filter list types for AUDIT_EXE Ondrej Mosnacek 2018-05-31 16:29 ` Richard Guy Briggs @ 2018-06-04 20:41 ` Paul Moore 2018-06-05 11:13 ` Ondrej Mosnacek 2018-06-19 14:25 ` Paul Moore 2 siblings, 1 reply; 15+ messages in thread From: Paul Moore @ 2018-06-04 20:41 UTC (permalink / raw) To: Ondrej Mosnacek; +Cc: Richard Guy Briggs, linux-audit On Wed, May 30, 2018 at 4:45 AM, Ondrej Mosnacek <omosnace@redhat.com> wrote: > This patch removes the restriction of the AUDIT_EXE field to only > SYSCALL filter and teaches audit_filter to recognize this field. > > This makes it possible to write rule lists such as: > > auditctl -a exit,always [some general rule] > # Filter out events with executable name /bin/exe1 or /bin/exe2: > auditctl -a exclude,always -F exe=/bin/exe1 > auditctl -a exclude,always -F exe=/bin/exe2 > > See: https://github.com/linux-audit/audit-kernel/issues/54 > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > --- > kernel/auditfilter.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) Thanks for your patience Ondrej. Having reflected a bit on things from the recent IMA audit discussion, my current thinking is to go ahead and merge this patch into audit/next once the merge window closes. > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c > index eaa320148d97..6db9847ca031 100644 > --- a/kernel/auditfilter.c > +++ b/kernel/auditfilter.c > @@ -428,8 +428,6 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f) > case AUDIT_EXE: > if (f->op != Audit_not_equal && f->op != Audit_equal) > return -EINVAL; > - if (entry->rule.listnr != AUDIT_FILTER_EXIT) > - return -EINVAL; > break; > } > return 0; > @@ -1360,6 +1358,11 @@ int audit_filter(int msgtype, unsigned int listtype) > f->type, f->op, f->lsm_rule, NULL); > } > break; > + case AUDIT_EXE: > + result = audit_exe_compare(current, e->rule.exe); > + if (f->op == Audit_not_equal) > + result = !result; > + break; > default: > goto unlock_and_return; > } > -- > 2.17.0 -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 1/2] audit: allow other filter list types for AUDIT_EXE 2018-06-04 20:41 ` Paul Moore @ 2018-06-05 11:13 ` Ondrej Mosnacek 0 siblings, 0 replies; 15+ messages in thread From: Ondrej Mosnacek @ 2018-06-05 11:13 UTC (permalink / raw) To: Paul Moore; +Cc: Richard Guy Briggs, Linux-Audit Mailing List 2018-06-04 22:41 GMT+02:00 Paul Moore <paul@paul-moore.com>: > On Wed, May 30, 2018 at 4:45 AM, Ondrej Mosnacek <omosnace@redhat.com> wrote: >> This patch removes the restriction of the AUDIT_EXE field to only >> SYSCALL filter and teaches audit_filter to recognize this field. >> >> This makes it possible to write rule lists such as: >> >> auditctl -a exit,always [some general rule] >> # Filter out events with executable name /bin/exe1 or /bin/exe2: >> auditctl -a exclude,always -F exe=/bin/exe1 >> auditctl -a exclude,always -F exe=/bin/exe2 >> >> See: https://github.com/linux-audit/audit-kernel/issues/54 >> >> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> >> --- >> kernel/auditfilter.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) > > Thanks for your patience Ondrej. > > Having reflected a bit on things from the recent IMA audit discussion, > my current thinking is to go ahead and merge this patch into > audit/next once the merge window closes. OK, feel free to merge it independently of the DIR patch, I sent them in series because they need to be applied in order (otherwise there would be merge conflicts). > >> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c >> index eaa320148d97..6db9847ca031 100644 >> --- a/kernel/auditfilter.c >> +++ b/kernel/auditfilter.c >> @@ -428,8 +428,6 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f) >> case AUDIT_EXE: >> if (f->op != Audit_not_equal && f->op != Audit_equal) >> return -EINVAL; >> - if (entry->rule.listnr != AUDIT_FILTER_EXIT) >> - return -EINVAL; >> break; >> } >> return 0; >> @@ -1360,6 +1358,11 @@ int audit_filter(int msgtype, unsigned int listtype) >> f->type, f->op, f->lsm_rule, NULL); >> } >> break; >> + case AUDIT_EXE: >> + result = audit_exe_compare(current, e->rule.exe); >> + if (f->op == Audit_not_equal) >> + result = !result; >> + break; >> default: >> goto unlock_and_return; >> } >> -- >> 2.17.0 > > -- > paul moore > www.paul-moore.com -- Ondrej Mosnacek <omosnace at redhat dot com> Associate Software Engineer, Security Technologies Red Hat, Inc. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 1/2] audit: allow other filter list types for AUDIT_EXE 2018-05-30 8:45 ` [RFC PATCH 1/2] audit: allow other filter list types for AUDIT_EXE Ondrej Mosnacek 2018-05-31 16:29 ` Richard Guy Briggs 2018-06-04 20:41 ` Paul Moore @ 2018-06-19 14:25 ` Paul Moore 2 siblings, 0 replies; 15+ messages in thread From: Paul Moore @ 2018-06-19 14:25 UTC (permalink / raw) To: omosnace; +Cc: rgb, linux-audit On Wed, May 30, 2018 at 4:45 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > This patch removes the restriction of the AUDIT_EXE field to only > SYSCALL filter and teaches audit_filter to recognize this field. > > This makes it possible to write rule lists such as: > > auditctl -a exit,always [some general rule] > # Filter out events with executable name /bin/exe1 or /bin/exe2: > auditctl -a exclude,always -F exe=/bin/exe1 > auditctl -a exclude,always -F exe=/bin/exe2 > > See: https://github.com/linux-audit/audit-kernel/issues/54 > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > --- > kernel/auditfilter.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) Merged, thanks. -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* [RFC PATCH 2/2] [WIP] audit: allow other filter list types for AUDIT_DIR 2018-05-30 8:45 [RFC PATCH 0/2] Extend AUDIT_EXE and AUDIT_DIR to more filter types Ondrej Mosnacek 2018-05-30 8:45 ` [RFC PATCH 1/2] audit: allow other filter list types for AUDIT_EXE Ondrej Mosnacek @ 2018-05-30 8:45 ` Ondrej Mosnacek 2018-05-31 20:52 ` Richard Guy Briggs 1 sibling, 1 reply; 15+ messages in thread From: Ondrej Mosnacek @ 2018-05-30 8:45 UTC (permalink / raw) To: linux-audit; +Cc: Richard Guy Briggs This patch allows the AUDIR_DIR field to be used also with the exclude filter. Not-yet-signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> --- kernel/audit.c | 5 +++-- kernel/audit.h | 32 +++++++++++++++++++++++++++++++- kernel/audit_tree.c | 4 +++- kernel/auditfilter.c | 6 +++++- kernel/auditsc.c | 28 ---------------------------- 5 files changed, 42 insertions(+), 33 deletions(-) diff --git a/kernel/audit.c b/kernel/audit.c index e7478cb58079..aac5b6ecc11d 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -1333,7 +1333,8 @@ 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(msg_type, AUDIT_FILTER_USER); + // FIXME: do we need to pass the context here? + err = audit_filter(msg_type, AUDIT_FILTER_USER, NULL); if (err == 1) { /* match or error */ err = 0; if (msg_type == AUDIT_USER_TTY) { @@ -1754,7 +1755,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_TYPE, ctx))) return NULL; /* NOTE: don't ever fail/sleep on these two conditions: diff --git a/kernel/audit.h b/kernel/audit.h index 214e14948370..43cfeba25802 100644 --- a/kernel/audit.h +++ b/kernel/audit.h @@ -324,13 +324,43 @@ extern void audit_kill_trees(struct list_head *list); #define audit_kill_trees(list) BUG() #endif +struct audit_tree_refs { + struct audit_tree_refs *next; + struct audit_chunk *c[31]; +}; + +/* A utility function to match tree refs: */ +static inline int match_tree_refs(struct audit_context *ctx, struct audit_tree *tree) +{ +#ifdef CONFIG_AUDIT_TREE + struct audit_tree_refs *p; + int n; + if (!tree) + return 0; + /* full ones */ + for (p = ctx->first_trees; p != ctx->trees; p = p->next) { + for (n = 0; n < 31; n++) + if (audit_tree_match(p->c[n], tree)) + return 1; + } + /* partial */ + if (p) { + for (n = ctx->tree_count; n < 31; n++) + if (audit_tree_match(p->c[n], tree)) + return 1; + } +#endif + return 0; +} + extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len); 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); +extern int audit_filter(int msgtype, unsigned int listtype, + struct audit_context *ctx); #ifdef CONFIG_AUDITSYSCALL extern int audit_signal_info(int sig, struct task_struct *t); diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c index 67e6956c0b61..d4d36389c3d7 100644 --- a/kernel/audit_tree.c +++ b/kernel/audit_tree.c @@ -675,9 +675,11 @@ void audit_trim_trees(void) int audit_make_tree(struct audit_krule *rule, char *pathname, u32 op) { + if (krule->listnr != AUDIT_FILTER_EXIT && + krule->listnr != AUDIT_FILTER_TYPE) + return -EINVAL; if (pathname[0] != '/' || - rule->listnr != AUDIT_FILTER_EXIT || op != Audit_equal || rule->inode_f || rule->watch || rule->tree) return -EINVAL; diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c index 6db9847ca031..e1d70cb77ea3 100644 --- a/kernel/auditfilter.c +++ b/kernel/auditfilter.c @@ -1309,7 +1309,7 @@ int audit_compare_dname_path(const char *dname, const char *path, int parentlen) return strncmp(p, dname, dlen); } -int audit_filter(int msgtype, unsigned int listtype) +int audit_filter(int msgtype, unsigned int listtype, struct audit_context *ctx) { struct audit_entry *e; int ret = 1; /* Audit by default */ @@ -1363,6 +1363,10 @@ int audit_filter(int msgtype, unsigned int listtype) if (f->op == Audit_not_equal) result = !result; break; + case AUDIT_DIR: + if (ctx) + result = match_tree_refs(ctx, e->rule.tree); + break; default: goto unlock_and_return; } diff --git a/kernel/auditsc.c b/kernel/auditsc.c index ceb1c4596c51..0d00b9354886 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -125,11 +125,6 @@ struct audit_aux_data_bprm_fcaps { struct audit_cap_data new_pcap; }; -struct audit_tree_refs { - struct audit_tree_refs *next; - struct audit_chunk *c[31]; -}; - static int audit_match_perm(struct audit_context *ctx, int mask) { unsigned n; @@ -286,29 +281,6 @@ static void free_tree_refs(struct audit_context *ctx) } } -static int match_tree_refs(struct audit_context *ctx, struct audit_tree *tree) -{ -#ifdef CONFIG_AUDIT_TREE - struct audit_tree_refs *p; - int n; - if (!tree) - return 0; - /* full ones */ - for (p = ctx->first_trees; p != ctx->trees; p = p->next) { - for (n = 0; n < 31; n++) - if (audit_tree_match(p->c[n], tree)) - return 1; - } - /* partial */ - if (p) { - for (n = ctx->tree_count; n < 31; n++) - if (audit_tree_match(p->c[n], tree)) - return 1; - } -#endif - return 0; -} - static int audit_compare_uid(kuid_t uid, struct audit_names *name, struct audit_field *f, -- 2.17.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 2/2] [WIP] audit: allow other filter list types for AUDIT_DIR 2018-05-30 8:45 ` [RFC PATCH 2/2] [WIP] audit: allow other filter list types for AUDIT_DIR Ondrej Mosnacek @ 2018-05-31 20:52 ` Richard Guy Briggs 2018-06-01 8:12 ` Ondrej Mosnacek 0 siblings, 1 reply; 15+ messages in thread From: Richard Guy Briggs @ 2018-05-31 20:52 UTC (permalink / raw) To: Ondrej Mosnacek; +Cc: linux-audit On 2018-05-30 10:45, Ondrej Mosnacek wrote: > This patch allows the AUDIR_DIR field to be used also with the exclude > filter. > > Not-yet-signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > --- > kernel/audit.c | 5 +++-- > kernel/audit.h | 32 +++++++++++++++++++++++++++++++- > kernel/audit_tree.c | 4 +++- > kernel/auditfilter.c | 6 +++++- > kernel/auditsc.c | 28 ---------------------------- > 5 files changed, 42 insertions(+), 33 deletions(-) > > diff --git a/kernel/audit.c b/kernel/audit.c > index e7478cb58079..aac5b6ecc11d 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -1333,7 +1333,8 @@ 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(msg_type, AUDIT_FILTER_USER); > + // FIXME: do we need to pass the context here? > + err = audit_filter(msg_type, AUDIT_FILTER_USER, NULL); > if (err == 1) { /* match or error */ > err = 0; > if (msg_type == AUDIT_USER_TTY) { > @@ -1754,7 +1755,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_TYPE, ctx))) > return NULL; > > /* NOTE: don't ever fail/sleep on these two conditions: > diff --git a/kernel/audit.h b/kernel/audit.h > index 214e14948370..43cfeba25802 100644 > --- a/kernel/audit.h > +++ b/kernel/audit.h > @@ -324,13 +324,43 @@ extern void audit_kill_trees(struct list_head *list); > #define audit_kill_trees(list) BUG() > #endif > > +struct audit_tree_refs { > + struct audit_tree_refs *next; > + struct audit_chunk *c[31]; > +}; > + > +/* A utility function to match tree refs: */ > +static inline int match_tree_refs(struct audit_context *ctx, struct audit_tree *tree) > +{ > +#ifdef CONFIG_AUDIT_TREE > + struct audit_tree_refs *p; > + int n; > + if (!tree) > + return 0; > + /* full ones */ > + for (p = ctx->first_trees; p != ctx->trees; p = p->next) { > + for (n = 0; n < 31; n++) > + if (audit_tree_match(p->c[n], tree)) > + return 1; > + } > + /* partial */ > + if (p) { > + for (n = ctx->tree_count; n < 31; n++) > + if (audit_tree_match(p->c[n], tree)) > + return 1; > + } > +#endif > + return 0; > +} > + > extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len); > > 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); > +extern int audit_filter(int msgtype, unsigned int listtype, > + struct audit_context *ctx); > > #ifdef CONFIG_AUDITSYSCALL > extern int audit_signal_info(int sig, struct task_struct *t); > diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c > index 67e6956c0b61..d4d36389c3d7 100644 > --- a/kernel/audit_tree.c > +++ b/kernel/audit_tree.c > @@ -675,9 +675,11 @@ void audit_trim_trees(void) > > int audit_make_tree(struct audit_krule *rule, char *pathname, u32 op) > { > + if (krule->listnr != AUDIT_FILTER_EXIT && > + krule->listnr != AUDIT_FILTER_TYPE) > + return -EINVAL; > > if (pathname[0] != '/' || > - rule->listnr != AUDIT_FILTER_EXIT || > op != Audit_equal || > rule->inode_f || rule->watch || rule->tree) > return -EINVAL; > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c > index 6db9847ca031..e1d70cb77ea3 100644 > --- a/kernel/auditfilter.c > +++ b/kernel/auditfilter.c > @@ -1309,7 +1309,7 @@ int audit_compare_dname_path(const char *dname, const char *path, int parentlen) > return strncmp(p, dname, dlen); > } > > -int audit_filter(int msgtype, unsigned int listtype) > +int audit_filter(int msgtype, unsigned int listtype, struct audit_context *ctx) > { > struct audit_entry *e; > int ret = 1; /* Audit by default */ > @@ -1363,6 +1363,10 @@ int audit_filter(int msgtype, unsigned int listtype) > if (f->op == Audit_not_equal) > result = !result; > break; > + case AUDIT_DIR: > + if (ctx) > + result = match_tree_refs(ctx, e->rule.tree); I don't see why you need to send a context to audit_filter, since the rest of the function assumes the current task you can just use audit_context() to hand to match_tree_refs(). You could even check that ctx isn't NULL in match_tree_refs() to hide that code from audit_filter(). > + break; > default: > goto unlock_and_return; > } > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index ceb1c4596c51..0d00b9354886 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -125,11 +125,6 @@ struct audit_aux_data_bprm_fcaps { > struct audit_cap_data new_pcap; > }; > > -struct audit_tree_refs { > - struct audit_tree_refs *next; > - struct audit_chunk *c[31]; > -}; > - > static int audit_match_perm(struct audit_context *ctx, int mask) > { > unsigned n; > @@ -286,29 +281,6 @@ static void free_tree_refs(struct audit_context *ctx) > } > } > > -static int match_tree_refs(struct audit_context *ctx, struct audit_tree *tree) > -{ > -#ifdef CONFIG_AUDIT_TREE > - struct audit_tree_refs *p; > - int n; > - if (!tree) > - return 0; > - /* full ones */ > - for (p = ctx->first_trees; p != ctx->trees; p = p->next) { > - for (n = 0; n < 31; n++) > - if (audit_tree_match(p->c[n], tree)) > - return 1; > - } > - /* partial */ > - if (p) { > - for (n = ctx->tree_count; n < 31; n++) > - if (audit_tree_match(p->c[n], tree)) > - return 1; > - } > -#endif > - return 0; > -} > - Why did you move match_tree_refs() out of auditsc.c? > static int audit_compare_uid(kuid_t uid, > struct audit_names *name, > struct audit_field *f, > -- > 2.17.0 > - 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 2/2] [WIP] audit: allow other filter list types for AUDIT_DIR 2018-05-31 20:52 ` Richard Guy Briggs @ 2018-06-01 8:12 ` Ondrej Mosnacek 2018-06-01 20:05 ` Richard Guy Briggs 2018-06-04 22:12 ` Paul Moore 0 siblings, 2 replies; 15+ messages in thread From: Ondrej Mosnacek @ 2018-06-01 8:12 UTC (permalink / raw) To: Richard Guy Briggs; +Cc: Linux-Audit Mailing List 2018-05-31 22:52 GMT+02:00 Richard Guy Briggs <rgb@redhat.com>: > On 2018-05-30 10:45, Ondrej Mosnacek wrote: >> This patch allows the AUDIR_DIR field to be used also with the exclude >> filter. >> >> Not-yet-signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> >> --- >> kernel/audit.c | 5 +++-- >> kernel/audit.h | 32 +++++++++++++++++++++++++++++++- >> kernel/audit_tree.c | 4 +++- >> kernel/auditfilter.c | 6 +++++- >> kernel/auditsc.c | 28 ---------------------------- >> 5 files changed, 42 insertions(+), 33 deletions(-) >> >> diff --git a/kernel/audit.c b/kernel/audit.c >> index e7478cb58079..aac5b6ecc11d 100644 >> --- a/kernel/audit.c >> +++ b/kernel/audit.c >> @@ -1333,7 +1333,8 @@ 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(msg_type, AUDIT_FILTER_USER); >> + // FIXME: do we need to pass the context here? >> + err = audit_filter(msg_type, AUDIT_FILTER_USER, NULL); >> if (err == 1) { /* match or error */ >> err = 0; >> if (msg_type == AUDIT_USER_TTY) { >> @@ -1754,7 +1755,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_TYPE, ctx))) >> return NULL; >> >> /* NOTE: don't ever fail/sleep on these two conditions: >> diff --git a/kernel/audit.h b/kernel/audit.h >> index 214e14948370..43cfeba25802 100644 >> --- a/kernel/audit.h >> +++ b/kernel/audit.h >> @@ -324,13 +324,43 @@ extern void audit_kill_trees(struct list_head *list); >> #define audit_kill_trees(list) BUG() >> #endif >> >> +struct audit_tree_refs { >> + struct audit_tree_refs *next; >> + struct audit_chunk *c[31]; >> +}; >> + >> +/* A utility function to match tree refs: */ >> +static inline int match_tree_refs(struct audit_context *ctx, struct audit_tree *tree) >> +{ >> +#ifdef CONFIG_AUDIT_TREE >> + struct audit_tree_refs *p; >> + int n; >> + if (!tree) >> + return 0; >> + /* full ones */ >> + for (p = ctx->first_trees; p != ctx->trees; p = p->next) { >> + for (n = 0; n < 31; n++) >> + if (audit_tree_match(p->c[n], tree)) >> + return 1; >> + } >> + /* partial */ >> + if (p) { >> + for (n = ctx->tree_count; n < 31; n++) >> + if (audit_tree_match(p->c[n], tree)) >> + return 1; >> + } >> +#endif >> + return 0; >> +} >> + >> extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len); >> >> 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); >> +extern int audit_filter(int msgtype, unsigned int listtype, >> + struct audit_context *ctx); >> >> #ifdef CONFIG_AUDITSYSCALL >> extern int audit_signal_info(int sig, struct task_struct *t); >> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c >> index 67e6956c0b61..d4d36389c3d7 100644 >> --- a/kernel/audit_tree.c >> +++ b/kernel/audit_tree.c >> @@ -675,9 +675,11 @@ void audit_trim_trees(void) >> >> int audit_make_tree(struct audit_krule *rule, char *pathname, u32 op) >> { >> + if (krule->listnr != AUDIT_FILTER_EXIT && >> + krule->listnr != AUDIT_FILTER_TYPE) >> + return -EINVAL; >> >> if (pathname[0] != '/' || >> - rule->listnr != AUDIT_FILTER_EXIT || >> op != Audit_equal || >> rule->inode_f || rule->watch || rule->tree) >> return -EINVAL; >> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c >> index 6db9847ca031..e1d70cb77ea3 100644 >> --- a/kernel/auditfilter.c >> +++ b/kernel/auditfilter.c >> @@ -1309,7 +1309,7 @@ int audit_compare_dname_path(const char *dname, const char *path, int parentlen) >> return strncmp(p, dname, dlen); >> } >> >> -int audit_filter(int msgtype, unsigned int listtype) >> +int audit_filter(int msgtype, unsigned int listtype, struct audit_context *ctx) >> { >> struct audit_entry *e; >> int ret = 1; /* Audit by default */ >> @@ -1363,6 +1363,10 @@ int audit_filter(int msgtype, unsigned int listtype) >> if (f->op == Audit_not_equal) >> result = !result; >> break; >> + case AUDIT_DIR: >> + if (ctx) >> + result = match_tree_refs(ctx, e->rule.tree); > > I don't see why you need to send a context to audit_filter, since the > rest of the function assumes the current task you can just use > audit_context() to hand to match_tree_refs(). You could even check that > ctx isn't NULL in match_tree_refs() to hide that code from > audit_filter(). I wasn't sure if I could do that, since audit_filter didn't need the context until now and it is called from two places: audit_log_start -- which accepts context as an argument (doesn't get it from task, and it can be called with ctx == NULL). audit_receive_msg -- this function doesn't work with context at all, so I wasn't sure if audit_filter should consider it being NULL or if it should get it from the current task. My hunch is the second option is the right one, but the first one is safer (AUDIT_DIR will simply never be checked here). I don't have such insight into the logic of audit_context's lifetime, so I need someone to tell me what makes more sense here. > >> + break; >> default: >> goto unlock_and_return; >> } >> diff --git a/kernel/auditsc.c b/kernel/auditsc.c >> index ceb1c4596c51..0d00b9354886 100644 >> --- a/kernel/auditsc.c >> +++ b/kernel/auditsc.c >> @@ -125,11 +125,6 @@ struct audit_aux_data_bprm_fcaps { >> struct audit_cap_data new_pcap; >> }; >> >> -struct audit_tree_refs { >> - struct audit_tree_refs *next; >> - struct audit_chunk *c[31]; >> -}; >> - >> static int audit_match_perm(struct audit_context *ctx, int mask) >> { >> unsigned n; >> @@ -286,29 +281,6 @@ static void free_tree_refs(struct audit_context *ctx) >> } >> } >> >> -static int match_tree_refs(struct audit_context *ctx, struct audit_tree *tree) >> -{ >> -#ifdef CONFIG_AUDIT_TREE >> - struct audit_tree_refs *p; >> - int n; >> - if (!tree) >> - return 0; >> - /* full ones */ >> - for (p = ctx->first_trees; p != ctx->trees; p = p->next) { >> - for (n = 0; n < 31; n++) >> - if (audit_tree_match(p->c[n], tree)) >> - return 1; >> - } >> - /* partial */ >> - if (p) { >> - for (n = ctx->tree_count; n < 31; n++) >> - if (audit_tree_match(p->c[n], tree)) >> - return 1; >> - } >> -#endif >> - return 0; >> -} >> - > > Why did you move match_tree_refs() out of auditsc.c? Because now it can be called from both 'audit_filter_rules()' (defined in auditsc.c) and 'audit_filter()' (defined in auditfilter.c). I'm actually playing with the idea of unifying the filtering logic in these two functions, where sharing this function wouldn't be necessary. However, that is quite a big change (a lot of LOC being moved around) so I'd prefer the simple & dirty approach now and keep the cleanup for a later patch. Thanks for the comments! > >> static int audit_compare_uid(kuid_t uid, >> struct audit_names *name, >> struct audit_field *f, >> -- >> 2.17.0 >> > > - 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 -- Ondrej Mosnacek <omosnace at redhat dot com> Associate Software Engineer, Security Technologies Red Hat, Inc. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 2/2] [WIP] audit: allow other filter list types for AUDIT_DIR 2018-06-01 8:12 ` Ondrej Mosnacek @ 2018-06-01 20:05 ` Richard Guy Briggs 2018-06-04 11:32 ` Ondrej Mosnacek 2018-06-04 22:19 ` Paul Moore 2018-06-04 22:12 ` Paul Moore 1 sibling, 2 replies; 15+ messages in thread From: Richard Guy Briggs @ 2018-06-01 20:05 UTC (permalink / raw) To: Ondrej Mosnacek; +Cc: Linux-Audit Mailing List On 2018-06-01 10:12, Ondrej Mosnacek wrote: > 2018-05-31 22:52 GMT+02:00 Richard Guy Briggs <rgb@redhat.com>: > > On 2018-05-30 10:45, Ondrej Mosnacek wrote: > >> This patch allows the AUDIR_DIR field to be used also with the exclude > >> filter. > >> > >> Not-yet-signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > >> --- > >> kernel/audit.c | 5 +++-- > >> kernel/audit.h | 32 +++++++++++++++++++++++++++++++- > >> kernel/audit_tree.c | 4 +++- > >> kernel/auditfilter.c | 6 +++++- > >> kernel/auditsc.c | 28 ---------------------------- > >> 5 files changed, 42 insertions(+), 33 deletions(-) > >> > >> diff --git a/kernel/audit.c b/kernel/audit.c > >> index e7478cb58079..aac5b6ecc11d 100644 > >> --- a/kernel/audit.c > >> +++ b/kernel/audit.c > >> @@ -1333,7 +1333,8 @@ 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(msg_type, AUDIT_FILTER_USER); > >> + // FIXME: do we need to pass the context here? > >> + err = audit_filter(msg_type, AUDIT_FILTER_USER, NULL); > >> if (err == 1) { /* match or error */ > >> err = 0; > >> if (msg_type == AUDIT_USER_TTY) { > >> @@ -1754,7 +1755,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_TYPE, ctx))) > >> return NULL; > >> > >> /* NOTE: don't ever fail/sleep on these two conditions: > >> diff --git a/kernel/audit.h b/kernel/audit.h > >> index 214e14948370..43cfeba25802 100644 > >> --- a/kernel/audit.h > >> +++ b/kernel/audit.h > >> @@ -324,13 +324,43 @@ extern void audit_kill_trees(struct list_head *list); > >> #define audit_kill_trees(list) BUG() > >> #endif > >> > >> +struct audit_tree_refs { > >> + struct audit_tree_refs *next; > >> + struct audit_chunk *c[31]; > >> +}; > >> + > >> +/* A utility function to match tree refs: */ > >> +static inline int match_tree_refs(struct audit_context *ctx, struct audit_tree *tree) > >> +{ > >> +#ifdef CONFIG_AUDIT_TREE > >> + struct audit_tree_refs *p; > >> + int n; > >> + if (!tree) > >> + return 0; > >> + /* full ones */ > >> + for (p = ctx->first_trees; p != ctx->trees; p = p->next) { > >> + for (n = 0; n < 31; n++) > >> + if (audit_tree_match(p->c[n], tree)) > >> + return 1; > >> + } > >> + /* partial */ > >> + if (p) { > >> + for (n = ctx->tree_count; n < 31; n++) > >> + if (audit_tree_match(p->c[n], tree)) > >> + return 1; > >> + } > >> +#endif > >> + return 0; > >> +} > >> + > >> extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len); > >> > >> 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); > >> +extern int audit_filter(int msgtype, unsigned int listtype, > >> + struct audit_context *ctx); > >> > >> #ifdef CONFIG_AUDITSYSCALL > >> extern int audit_signal_info(int sig, struct task_struct *t); > >> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c > >> index 67e6956c0b61..d4d36389c3d7 100644 > >> --- a/kernel/audit_tree.c > >> +++ b/kernel/audit_tree.c > >> @@ -675,9 +675,11 @@ void audit_trim_trees(void) > >> > >> int audit_make_tree(struct audit_krule *rule, char *pathname, u32 op) > >> { > >> + if (krule->listnr != AUDIT_FILTER_EXIT && > >> + krule->listnr != AUDIT_FILTER_TYPE) > >> + return -EINVAL; > >> > >> if (pathname[0] != '/' || > >> - rule->listnr != AUDIT_FILTER_EXIT || > >> op != Audit_equal || > >> rule->inode_f || rule->watch || rule->tree) > >> return -EINVAL; > >> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c > >> index 6db9847ca031..e1d70cb77ea3 100644 > >> --- a/kernel/auditfilter.c > >> +++ b/kernel/auditfilter.c > >> @@ -1309,7 +1309,7 @@ int audit_compare_dname_path(const char *dname, const char *path, int parentlen) > >> return strncmp(p, dname, dlen); > >> } > >> > >> -int audit_filter(int msgtype, unsigned int listtype) > >> +int audit_filter(int msgtype, unsigned int listtype, struct audit_context *ctx) > >> { > >> struct audit_entry *e; > >> int ret = 1; /* Audit by default */ > >> @@ -1363,6 +1363,10 @@ int audit_filter(int msgtype, unsigned int listtype) > >> if (f->op == Audit_not_equal) > >> result = !result; > >> break; > >> + case AUDIT_DIR: > >> + if (ctx) > >> + result = match_tree_refs(ctx, e->rule.tree); > > > > I don't see why you need to send a context to audit_filter, since the > > rest of the function assumes the current task you can just use > > audit_context() to hand to match_tree_refs(). You could even check that > > ctx isn't NULL in match_tree_refs() to hide that code from > > audit_filter(). > > I wasn't sure if I could do that, since audit_filter didn't need the > context until now and it is called from two places: > > audit_log_start -- which accepts context as an argument (doesn't get > it from task, and it can be called with ctx == NULL). Alright, if current has no context, then match_tree_refs() could test for a NULL context and return since there are no trees to check. Having said that, there are cases where audit_log_start() is deliberately handed a NULL context which should be honoured. It appears we don't have a choice but to pass in the context. > audit_receive_msg -- this function doesn't work with context at all, > so I wasn't sure if audit_filter should consider it being NULL or if > it should get it from the current task. My hunch is the second option > is the right one, but the first one is safer (AUDIT_DIR will simply > never be checked here). I don't have such insight into the logic of > audit_context's lifetime, so I need someone to tell me what makes more > sense here. That is starting to work with context. The recent FEATURE_CHANGE patch to connect records of the same event uses current->audit_context (now audit_context()) from audit_log_feature_change() called from audit_set_feature() called from audit_receive_msg(). There is also a work in progress to convert all the CONFIG_CHANGE records over. I'm just trying to track down all the instances. > >> + break; > >> default: > >> goto unlock_and_return; > >> } > >> diff --git a/kernel/auditsc.c b/kernel/auditsc.c > >> index ceb1c4596c51..0d00b9354886 100644 > >> --- a/kernel/auditsc.c > >> +++ b/kernel/auditsc.c > >> @@ -125,11 +125,6 @@ struct audit_aux_data_bprm_fcaps { > >> struct audit_cap_data new_pcap; > >> }; > >> > >> -struct audit_tree_refs { > >> - struct audit_tree_refs *next; > >> - struct audit_chunk *c[31]; > >> -}; > >> - > >> static int audit_match_perm(struct audit_context *ctx, int mask) > >> { > >> unsigned n; > >> @@ -286,29 +281,6 @@ static void free_tree_refs(struct audit_context *ctx) > >> } > >> } > >> > >> -static int match_tree_refs(struct audit_context *ctx, struct audit_tree *tree) > >> -{ > >> -#ifdef CONFIG_AUDIT_TREE > >> - struct audit_tree_refs *p; > >> - int n; > >> - if (!tree) > >> - return 0; > >> - /* full ones */ > >> - for (p = ctx->first_trees; p != ctx->trees; p = p->next) { > >> - for (n = 0; n < 31; n++) > >> - if (audit_tree_match(p->c[n], tree)) > >> - return 1; > >> - } > >> - /* partial */ > >> - if (p) { > >> - for (n = ctx->tree_count; n < 31; n++) > >> - if (audit_tree_match(p->c[n], tree)) > >> - return 1; > >> - } > >> -#endif > >> - return 0; > >> -} > >> - > > > > Why did you move match_tree_refs() out of auditsc.c? > > Because now it can be called from both 'audit_filter_rules()' (defined > in auditsc.c) and 'audit_filter()' (defined in auditfilter.c). So since kernel/audit.h is included in all kernel/audit*.c files, having the prototype in there from any kernel/audit*.c file should make the function available to all functions in kernel/audit*.c files. > I'm actually playing with the idea of unifying the filtering logic in > these two functions, where sharing this function wouldn't be > necessary. However, that is quite a big change (a lot of LOC being > moved around) so I'd prefer the simple & dirty approach now and keep > the cleanup for a later patch. Some of the filtering has already been refactored and merged. More wouldn't hurt. > Thanks for the comments! > > >> static int audit_compare_uid(kuid_t uid, > >> struct audit_names *name, > >> struct audit_field *f, > >> -- > >> 2.17.0 > >> > > > > - 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 > > -- > Ondrej Mosnacek <omosnace at redhat dot com> > Associate Software Engineer, Security Technologies > Red Hat, Inc. - 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 2/2] [WIP] audit: allow other filter list types for AUDIT_DIR 2018-06-01 20:05 ` Richard Guy Briggs @ 2018-06-04 11:32 ` Ondrej Mosnacek 2018-06-04 20:47 ` Richard Guy Briggs 2018-06-04 22:19 ` Paul Moore 1 sibling, 1 reply; 15+ messages in thread From: Ondrej Mosnacek @ 2018-06-04 11:32 UTC (permalink / raw) To: Richard Guy Briggs; +Cc: Linux-Audit Mailing List 2018-06-01 22:05 GMT+02:00 Richard Guy Briggs <rgb@redhat.com>: > On 2018-06-01 10:12, Ondrej Mosnacek wrote: >> 2018-05-31 22:52 GMT+02:00 Richard Guy Briggs <rgb@redhat.com>: >> > On 2018-05-30 10:45, Ondrej Mosnacek wrote: >> >> This patch allows the AUDIR_DIR field to be used also with the exclude >> >> filter. >> >> >> >> Not-yet-signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> >> >> --- >> >> kernel/audit.c | 5 +++-- >> >> kernel/audit.h | 32 +++++++++++++++++++++++++++++++- >> >> kernel/audit_tree.c | 4 +++- >> >> kernel/auditfilter.c | 6 +++++- >> >> kernel/auditsc.c | 28 ---------------------------- >> >> 5 files changed, 42 insertions(+), 33 deletions(-) >> >> >> >> diff --git a/kernel/audit.c b/kernel/audit.c >> >> index e7478cb58079..aac5b6ecc11d 100644 >> >> --- a/kernel/audit.c >> >> +++ b/kernel/audit.c >> >> @@ -1333,7 +1333,8 @@ 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(msg_type, AUDIT_FILTER_USER); >> >> + // FIXME: do we need to pass the context here? >> >> + err = audit_filter(msg_type, AUDIT_FILTER_USER, NULL); >> >> if (err == 1) { /* match or error */ >> >> err = 0; >> >> if (msg_type == AUDIT_USER_TTY) { >> >> @@ -1754,7 +1755,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_TYPE, ctx))) >> >> return NULL; >> >> >> >> /* NOTE: don't ever fail/sleep on these two conditions: >> >> diff --git a/kernel/audit.h b/kernel/audit.h >> >> index 214e14948370..43cfeba25802 100644 >> >> --- a/kernel/audit.h >> >> +++ b/kernel/audit.h >> >> @@ -324,13 +324,43 @@ extern void audit_kill_trees(struct list_head *list); >> >> #define audit_kill_trees(list) BUG() >> >> #endif >> >> >> >> +struct audit_tree_refs { >> >> + struct audit_tree_refs *next; >> >> + struct audit_chunk *c[31]; >> >> +}; >> >> + >> >> +/* A utility function to match tree refs: */ >> >> +static inline int match_tree_refs(struct audit_context *ctx, struct audit_tree *tree) >> >> +{ >> >> +#ifdef CONFIG_AUDIT_TREE >> >> + struct audit_tree_refs *p; >> >> + int n; >> >> + if (!tree) >> >> + return 0; >> >> + /* full ones */ >> >> + for (p = ctx->first_trees; p != ctx->trees; p = p->next) { >> >> + for (n = 0; n < 31; n++) >> >> + if (audit_tree_match(p->c[n], tree)) >> >> + return 1; >> >> + } >> >> + /* partial */ >> >> + if (p) { >> >> + for (n = ctx->tree_count; n < 31; n++) >> >> + if (audit_tree_match(p->c[n], tree)) >> >> + return 1; >> >> + } >> >> +#endif >> >> + return 0; >> >> +} >> >> + >> >> extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len); >> >> >> >> 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); >> >> +extern int audit_filter(int msgtype, unsigned int listtype, >> >> + struct audit_context *ctx); >> >> >> >> #ifdef CONFIG_AUDITSYSCALL >> >> extern int audit_signal_info(int sig, struct task_struct *t); >> >> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c >> >> index 67e6956c0b61..d4d36389c3d7 100644 >> >> --- a/kernel/audit_tree.c >> >> +++ b/kernel/audit_tree.c >> >> @@ -675,9 +675,11 @@ void audit_trim_trees(void) >> >> >> >> int audit_make_tree(struct audit_krule *rule, char *pathname, u32 op) >> >> { >> >> + if (krule->listnr != AUDIT_FILTER_EXIT && >> >> + krule->listnr != AUDIT_FILTER_TYPE) >> >> + return -EINVAL; >> >> >> >> if (pathname[0] != '/' || >> >> - rule->listnr != AUDIT_FILTER_EXIT || >> >> op != Audit_equal || >> >> rule->inode_f || rule->watch || rule->tree) >> >> return -EINVAL; >> >> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c >> >> index 6db9847ca031..e1d70cb77ea3 100644 >> >> --- a/kernel/auditfilter.c >> >> +++ b/kernel/auditfilter.c >> >> @@ -1309,7 +1309,7 @@ int audit_compare_dname_path(const char *dname, const char *path, int parentlen) >> >> return strncmp(p, dname, dlen); >> >> } >> >> >> >> -int audit_filter(int msgtype, unsigned int listtype) >> >> +int audit_filter(int msgtype, unsigned int listtype, struct audit_context *ctx) >> >> { >> >> struct audit_entry *e; >> >> int ret = 1; /* Audit by default */ >> >> @@ -1363,6 +1363,10 @@ int audit_filter(int msgtype, unsigned int listtype) >> >> if (f->op == Audit_not_equal) >> >> result = !result; >> >> break; >> >> + case AUDIT_DIR: >> >> + if (ctx) >> >> + result = match_tree_refs(ctx, e->rule.tree); >> > >> > I don't see why you need to send a context to audit_filter, since the >> > rest of the function assumes the current task you can just use >> > audit_context() to hand to match_tree_refs(). You could even check that >> > ctx isn't NULL in match_tree_refs() to hide that code from >> > audit_filter(). >> >> I wasn't sure if I could do that, since audit_filter didn't need the >> context until now and it is called from two places: >> >> audit_log_start -- which accepts context as an argument (doesn't get >> it from task, and it can be called with ctx == NULL). > > Alright, if current has no context, then match_tree_refs() could test > for a NULL context and return since there are no trees to check. Having > said that, there are cases where audit_log_start() is deliberately > handed a NULL context which should be honoured. It appears we don't > have a choice but to pass in the context. OK, so I'll leave that part as it is. > >> audit_receive_msg -- this function doesn't work with context at all, >> so I wasn't sure if audit_filter should consider it being NULL or if >> it should get it from the current task. My hunch is the second option >> is the right one, but the first one is safer (AUDIT_DIR will simply >> never be checked here). I don't have such insight into the logic of >> audit_context's lifetime, so I need someone to tell me what makes more >> sense here. > > That is starting to work with context. The recent FEATURE_CHANGE patch > to connect records of the same event uses current->audit_context (now > audit_context()) from audit_log_feature_change() called from > audit_set_feature() called from audit_receive_msg(). > > There is also a work in progress to convert all the CONFIG_CHANGE > records over. I'm just trying to track down all the instances. OK, so does that mean that I should just pass audit_context() instead of NULL in audit_receive_msg()? The part that calls audit_filter() only deals with user messages (AUDIT_USER/AUDIT_FIRST..LAST_USER_MESSAGE) -- does it make sense to use the task's context in AUDIT_FILTER_USER (right now just with AUDIT_DIR)? It already filters based on the task's credentials, so perhaps it does, but I don't know... > >> >> + break; >> >> default: >> >> goto unlock_and_return; >> >> } >> >> diff --git a/kernel/auditsc.c b/kernel/auditsc.c >> >> index ceb1c4596c51..0d00b9354886 100644 >> >> --- a/kernel/auditsc.c >> >> +++ b/kernel/auditsc.c >> >> @@ -125,11 +125,6 @@ struct audit_aux_data_bprm_fcaps { >> >> struct audit_cap_data new_pcap; >> >> }; >> >> >> >> -struct audit_tree_refs { >> >> - struct audit_tree_refs *next; >> >> - struct audit_chunk *c[31]; >> >> -}; >> >> - >> >> static int audit_match_perm(struct audit_context *ctx, int mask) >> >> { >> >> unsigned n; >> >> @@ -286,29 +281,6 @@ static void free_tree_refs(struct audit_context *ctx) >> >> } >> >> } >> >> >> >> -static int match_tree_refs(struct audit_context *ctx, struct audit_tree *tree) >> >> -{ >> >> -#ifdef CONFIG_AUDIT_TREE >> >> - struct audit_tree_refs *p; >> >> - int n; >> >> - if (!tree) >> >> - return 0; >> >> - /* full ones */ >> >> - for (p = ctx->first_trees; p != ctx->trees; p = p->next) { >> >> - for (n = 0; n < 31; n++) >> >> - if (audit_tree_match(p->c[n], tree)) >> >> - return 1; >> >> - } >> >> - /* partial */ >> >> - if (p) { >> >> - for (n = ctx->tree_count; n < 31; n++) >> >> - if (audit_tree_match(p->c[n], tree)) >> >> - return 1; >> >> - } >> >> -#endif >> >> - return 0; >> >> -} >> >> - >> > >> > Why did you move match_tree_refs() out of auditsc.c? >> >> Because now it can be called from both 'audit_filter_rules()' (defined >> in auditsc.c) and 'audit_filter()' (defined in auditfilter.c). > > So since kernel/audit.h is included in all kernel/audit*.c files, having > the prototype in there from any kernel/audit*.c file should make the > function available to all functions in kernel/audit*.c files. You're right, I could have just added a declaration and export the function from auditsc.c... I will need to add a fallback definition for when CONFIG_AUDITSYSCALL is not enabled, but in that case tasks never have audit_context set anyway, so it's fine. I will just export the function in the next version. > >> I'm actually playing with the idea of unifying the filtering logic in >> these two functions, where sharing this function wouldn't be >> necessary. However, that is quite a big change (a lot of LOC being >> moved around) so I'd prefer the simple & dirty approach now and keep >> the cleanup for a later patch. > > Some of the filtering has already been refactored and merged. More > wouldn't hurt. > >> Thanks for the comments! >> >> >> static int audit_compare_uid(kuid_t uid, >> >> struct audit_names *name, >> >> struct audit_field *f, >> >> -- >> >> 2.17.0 >> >> >> > >> > - 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 >> >> -- >> Ondrej Mosnacek <omosnace at redhat dot com> >> Associate Software Engineer, Security Technologies >> Red Hat, Inc. > > - 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 -- Ondrej Mosnacek <omosnace at redhat dot com> Associate Software Engineer, Security Technologies Red Hat, Inc. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 2/2] [WIP] audit: allow other filter list types for AUDIT_DIR 2018-06-04 11:32 ` Ondrej Mosnacek @ 2018-06-04 20:47 ` Richard Guy Briggs 0 siblings, 0 replies; 15+ messages in thread From: Richard Guy Briggs @ 2018-06-04 20:47 UTC (permalink / raw) To: Ondrej Mosnacek; +Cc: Linux-Audit Mailing List On 2018-06-04 13:32, Ondrej Mosnacek wrote: > 2018-06-01 22:05 GMT+02:00 Richard Guy Briggs <rgb@redhat.com>: > > On 2018-06-01 10:12, Ondrej Mosnacek wrote: > >> 2018-05-31 22:52 GMT+02:00 Richard Guy Briggs <rgb@redhat.com>: > >> > On 2018-05-30 10:45, Ondrej Mosnacek wrote: > >> >> This patch allows the AUDIR_DIR field to be used also with the exclude > >> >> filter. > >> >> > >> >> Not-yet-signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > >> >> --- > >> >> kernel/audit.c | 5 +++-- > >> >> kernel/audit.h | 32 +++++++++++++++++++++++++++++++- > >> >> kernel/audit_tree.c | 4 +++- > >> >> kernel/auditfilter.c | 6 +++++- > >> >> kernel/auditsc.c | 28 ---------------------------- > >> >> 5 files changed, 42 insertions(+), 33 deletions(-) > >> >> > >> >> diff --git a/kernel/audit.c b/kernel/audit.c > >> >> index e7478cb58079..aac5b6ecc11d 100644 > >> >> --- a/kernel/audit.c > >> >> +++ b/kernel/audit.c > >> >> @@ -1333,7 +1333,8 @@ 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(msg_type, AUDIT_FILTER_USER); > >> >> + // FIXME: do we need to pass the context here? > >> >> + err = audit_filter(msg_type, AUDIT_FILTER_USER, NULL); > >> >> if (err == 1) { /* match or error */ > >> >> err = 0; > >> >> if (msg_type == AUDIT_USER_TTY) { > >> >> @@ -1754,7 +1755,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_TYPE, ctx))) > >> >> return NULL; > >> >> > >> >> /* NOTE: don't ever fail/sleep on these two conditions: > >> >> diff --git a/kernel/audit.h b/kernel/audit.h > >> >> index 214e14948370..43cfeba25802 100644 > >> >> --- a/kernel/audit.h > >> >> +++ b/kernel/audit.h > >> >> @@ -324,13 +324,43 @@ extern void audit_kill_trees(struct list_head *list); > >> >> #define audit_kill_trees(list) BUG() > >> >> #endif > >> >> > >> >> +struct audit_tree_refs { > >> >> + struct audit_tree_refs *next; > >> >> + struct audit_chunk *c[31]; > >> >> +}; > >> >> + > >> >> +/* A utility function to match tree refs: */ > >> >> +static inline int match_tree_refs(struct audit_context *ctx, struct audit_tree *tree) > >> >> +{ > >> >> +#ifdef CONFIG_AUDIT_TREE > >> >> + struct audit_tree_refs *p; > >> >> + int n; > >> >> + if (!tree) > >> >> + return 0; > >> >> + /* full ones */ > >> >> + for (p = ctx->first_trees; p != ctx->trees; p = p->next) { > >> >> + for (n = 0; n < 31; n++) > >> >> + if (audit_tree_match(p->c[n], tree)) > >> >> + return 1; > >> >> + } > >> >> + /* partial */ > >> >> + if (p) { > >> >> + for (n = ctx->tree_count; n < 31; n++) > >> >> + if (audit_tree_match(p->c[n], tree)) > >> >> + return 1; > >> >> + } > >> >> +#endif > >> >> + return 0; > >> >> +} > >> >> + > >> >> extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len); > >> >> > >> >> 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); > >> >> +extern int audit_filter(int msgtype, unsigned int listtype, > >> >> + struct audit_context *ctx); > >> >> > >> >> #ifdef CONFIG_AUDITSYSCALL > >> >> extern int audit_signal_info(int sig, struct task_struct *t); > >> >> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c > >> >> index 67e6956c0b61..d4d36389c3d7 100644 > >> >> --- a/kernel/audit_tree.c > >> >> +++ b/kernel/audit_tree.c > >> >> @@ -675,9 +675,11 @@ void audit_trim_trees(void) > >> >> > >> >> int audit_make_tree(struct audit_krule *rule, char *pathname, u32 op) > >> >> { > >> >> + if (krule->listnr != AUDIT_FILTER_EXIT && > >> >> + krule->listnr != AUDIT_FILTER_TYPE) > >> >> + return -EINVAL; > >> >> > >> >> if (pathname[0] != '/' || > >> >> - rule->listnr != AUDIT_FILTER_EXIT || > >> >> op != Audit_equal || > >> >> rule->inode_f || rule->watch || rule->tree) > >> >> return -EINVAL; > >> >> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c > >> >> index 6db9847ca031..e1d70cb77ea3 100644 > >> >> --- a/kernel/auditfilter.c > >> >> +++ b/kernel/auditfilter.c > >> >> @@ -1309,7 +1309,7 @@ int audit_compare_dname_path(const char *dname, const char *path, int parentlen) > >> >> return strncmp(p, dname, dlen); > >> >> } > >> >> > >> >> -int audit_filter(int msgtype, unsigned int listtype) > >> >> +int audit_filter(int msgtype, unsigned int listtype, struct audit_context *ctx) > >> >> { > >> >> struct audit_entry *e; > >> >> int ret = 1; /* Audit by default */ > >> >> @@ -1363,6 +1363,10 @@ int audit_filter(int msgtype, unsigned int listtype) > >> >> if (f->op == Audit_not_equal) > >> >> result = !result; > >> >> break; > >> >> + case AUDIT_DIR: > >> >> + if (ctx) > >> >> + result = match_tree_refs(ctx, e->rule.tree); > >> > > >> > I don't see why you need to send a context to audit_filter, since the > >> > rest of the function assumes the current task you can just use > >> > audit_context() to hand to match_tree_refs(). You could even check that > >> > ctx isn't NULL in match_tree_refs() to hide that code from > >> > audit_filter(). > >> > >> I wasn't sure if I could do that, since audit_filter didn't need the > >> context until now and it is called from two places: > >> > >> audit_log_start -- which accepts context as an argument (doesn't get > >> it from task, and it can be called with ctx == NULL). > > > > Alright, if current has no context, then match_tree_refs() could test > > for a NULL context and return since there are no trees to check. Having > > said that, there are cases where audit_log_start() is deliberately > > handed a NULL context which should be honoured. It appears we don't > > have a choice but to pass in the context. > > OK, so I'll leave that part as it is. > > > > >> audit_receive_msg -- this function doesn't work with context at all, > >> so I wasn't sure if audit_filter should consider it being NULL or if > >> it should get it from the current task. My hunch is the second option > >> is the right one, but the first one is safer (AUDIT_DIR will simply > >> never be checked here). I don't have such insight into the logic of > >> audit_context's lifetime, so I need someone to tell me what makes more > >> sense here. > > > > That is starting to work with context. The recent FEATURE_CHANGE patch > > to connect records of the same event uses current->audit_context (now > > audit_context()) from audit_log_feature_change() called from > > audit_set_feature() called from audit_receive_msg(). > > > > There is also a work in progress to convert all the CONFIG_CHANGE > > records over. I'm just trying to track down all the instances. > > OK, so does that mean that I should just pass audit_context() instead > of NULL in audit_receive_msg()? Reluctantly, yes. > The part that calls audit_filter() > only deals with user messages > (AUDIT_USER/AUDIT_FIRST..LAST_USER_MESSAGE) -- does it make sense to > use the task's context in AUDIT_FILTER_USER (right now just with > AUDIT_DIR)? It already filters based on the task's credentials, so > perhaps it does, but I don't know... I've got opinions both ways on USER messages. I don't see the additional information value of linking USER messages to SYSCALL records, but I do see the single event consolidation advantage. > >> >> + break; > >> >> default: > >> >> goto unlock_and_return; > >> >> } > >> >> diff --git a/kernel/auditsc.c b/kernel/auditsc.c > >> >> index ceb1c4596c51..0d00b9354886 100644 > >> >> --- a/kernel/auditsc.c > >> >> +++ b/kernel/auditsc.c > >> >> @@ -125,11 +125,6 @@ struct audit_aux_data_bprm_fcaps { > >> >> struct audit_cap_data new_pcap; > >> >> }; > >> >> > >> >> -struct audit_tree_refs { > >> >> - struct audit_tree_refs *next; > >> >> - struct audit_chunk *c[31]; > >> >> -}; > >> >> - > >> >> static int audit_match_perm(struct audit_context *ctx, int mask) > >> >> { > >> >> unsigned n; > >> >> @@ -286,29 +281,6 @@ static void free_tree_refs(struct audit_context *ctx) > >> >> } > >> >> } > >> >> > >> >> -static int match_tree_refs(struct audit_context *ctx, struct audit_tree *tree) > >> >> -{ > >> >> -#ifdef CONFIG_AUDIT_TREE > >> >> - struct audit_tree_refs *p; > >> >> - int n; > >> >> - if (!tree) > >> >> - return 0; > >> >> - /* full ones */ > >> >> - for (p = ctx->first_trees; p != ctx->trees; p = p->next) { > >> >> - for (n = 0; n < 31; n++) > >> >> - if (audit_tree_match(p->c[n], tree)) > >> >> - return 1; > >> >> - } > >> >> - /* partial */ > >> >> - if (p) { > >> >> - for (n = ctx->tree_count; n < 31; n++) > >> >> - if (audit_tree_match(p->c[n], tree)) > >> >> - return 1; > >> >> - } > >> >> -#endif > >> >> - return 0; > >> >> -} > >> >> - > >> > > >> > Why did you move match_tree_refs() out of auditsc.c? > >> > >> Because now it can be called from both 'audit_filter_rules()' (defined > >> in auditsc.c) and 'audit_filter()' (defined in auditfilter.c). > > > > So since kernel/audit.h is included in all kernel/audit*.c files, having > > the prototype in there from any kernel/audit*.c file should make the > > function available to all functions in kernel/audit*.c files. > > You're right, I could have just added a declaration and export the > function from auditsc.c... I will need to add a fallback definition > for when CONFIG_AUDITSYSCALL is not enabled, but in that case tasks > never have audit_context set anyway, so it's fine. I will just export > the function in the next version. > > > > >> I'm actually playing with the idea of unifying the filtering logic in > >> these two functions, where sharing this function wouldn't be > >> necessary. However, that is quite a big change (a lot of LOC being > >> moved around) so I'd prefer the simple & dirty approach now and keep > >> the cleanup for a later patch. > > > > Some of the filtering has already been refactored and merged. More > > wouldn't hurt. > > > >> Thanks for the comments! > >> > >> >> static int audit_compare_uid(kuid_t uid, > >> >> struct audit_names *name, > >> >> struct audit_field *f, > >> >> -- > >> >> 2.17.0 > >> >> > >> > > >> > - 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 > >> > >> -- > >> Ondrej Mosnacek <omosnace at redhat dot com> > >> Associate Software Engineer, Security Technologies > >> Red Hat, Inc. > > > > - 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 > > -- > Ondrej Mosnacek <omosnace at redhat dot com> > Associate Software Engineer, Security Technologies > Red Hat, Inc. - 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 2/2] [WIP] audit: allow other filter list types for AUDIT_DIR 2018-06-01 20:05 ` Richard Guy Briggs 2018-06-04 11:32 ` Ondrej Mosnacek @ 2018-06-04 22:19 ` Paul Moore 2018-06-05 11:23 ` Ondrej Mosnacek 1 sibling, 1 reply; 15+ messages in thread From: Paul Moore @ 2018-06-04 22:19 UTC (permalink / raw) To: Richard Guy Briggs, Ondrej Mosnacek; +Cc: Linux-Audit Mailing List On Fri, Jun 1, 2018 at 4:05 PM, Richard Guy Briggs <rgb@redhat.com> wrote: > On 2018-06-01 10:12, Ondrej Mosnacek wrote: ... >> audit_receive_msg -- this function doesn't work with context at all, >> so I wasn't sure if audit_filter should consider it being NULL or if >> it should get it from the current task. My hunch is the second option >> is the right one, but the first one is safer (AUDIT_DIR will simply >> never be checked here). I don't have such insight into the logic of >> audit_context's lifetime, so I need someone to tell me what makes more >> sense here. Given the nature of audit_receive_msg(), would it ever match on an AUDIT_DIR field? I don't think it would since there aren't really any vfs accesses that occur in the source of sending a netlink message down to the kernel ... am I missing something? > That is starting to work with context. The recent FEATURE_CHANGE patch > to connect records of the same event uses current->audit_context (now > audit_context()) from audit_log_feature_change() called from > audit_set_feature() called from audit_receive_msg(). > > There is also a work in progress to convert all the CONFIG_CHANGE > records over. I'm just trying to track down all the instances. This will be a nice improvement. -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 2/2] [WIP] audit: allow other filter list types for AUDIT_DIR 2018-06-04 22:19 ` Paul Moore @ 2018-06-05 11:23 ` Ondrej Mosnacek 0 siblings, 0 replies; 15+ messages in thread From: Ondrej Mosnacek @ 2018-06-05 11:23 UTC (permalink / raw) To: Paul Moore; +Cc: Richard Guy Briggs, Linux-Audit Mailing List 2018-06-05 0:19 GMT+02:00 Paul Moore <paul@paul-moore.com>: > On Fri, Jun 1, 2018 at 4:05 PM, Richard Guy Briggs <rgb@redhat.com> wrote: >> On 2018-06-01 10:12, Ondrej Mosnacek wrote: > > ... > >>> audit_receive_msg -- this function doesn't work with context at all, >>> so I wasn't sure if audit_filter should consider it being NULL or if >>> it should get it from the current task. My hunch is the second option >>> is the right one, but the first one is safer (AUDIT_DIR will simply >>> never be checked here). I don't have such insight into the logic of >>> audit_context's lifetime, so I need someone to tell me what makes more >>> sense here. > > Given the nature of audit_receive_msg(), would it ever match on an > AUDIT_DIR field? I don't think it would since there aren't really any > vfs accesses that occur in the source of sending a netlink message > down to the kernel ... am I missing something? Probably not, but we still need to decide whether to pass the current task's context or not. Both options (NULL and audit_context()) seem to be benign for now, but I need you to pick one of those that you prefer so I can send a final patch. > >> That is starting to work with context. The recent FEATURE_CHANGE patch >> to connect records of the same event uses current->audit_context (now >> audit_context()) from audit_log_feature_change() called from >> audit_set_feature() called from audit_receive_msg(). >> >> There is also a work in progress to convert all the CONFIG_CHANGE >> records over. I'm just trying to track down all the instances. > > This will be a nice improvement. > > -- > paul moore > www.paul-moore.com -- Ondrej Mosnacek <omosnace at redhat dot com> Associate Software Engineer, Security Technologies Red Hat, Inc. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 2/2] [WIP] audit: allow other filter list types for AUDIT_DIR 2018-06-01 8:12 ` Ondrej Mosnacek 2018-06-01 20:05 ` Richard Guy Briggs @ 2018-06-04 22:12 ` Paul Moore 1 sibling, 0 replies; 15+ messages in thread From: Paul Moore @ 2018-06-04 22:12 UTC (permalink / raw) To: Ondrej Mosnacek; +Cc: Richard Guy Briggs, Linux-Audit Mailing List On Fri, Jun 1, 2018 at 4:12 AM, Ondrej Mosnacek <omosnace@redhat.com> wrote: > I'm actually playing with the idea of unifying the filtering logic in > these two functions, where sharing this function wouldn't be > necessary. However, that is quite a big change (a lot of LOC being > moved around) so I'd prefer the simple & dirty approach now and keep > the cleanup for a later patch. [Resend as I forgot the reply-all - oops] I've had similar thoughts in the past, and deferred the work for exactly the same reason. If I recall correctly, I believe part of the reason may stem from the fact that some fields are simply not always valid when the filter is run and this may have been a crude effort at optimization (smaller function size). Regardless, we might preserve some of this idea by creating some helper functions (two?) with the different filter fields in each (no overlap) and a couple of wrapper functions which call the appropriate helpers ... or that just might be extra work for no noticeable perf advantage :) -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-06-19 14:26 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-30 8:45 [RFC PATCH 0/2] Extend AUDIT_EXE and AUDIT_DIR to more filter types Ondrej Mosnacek 2018-05-30 8:45 ` [RFC PATCH 1/2] audit: allow other filter list types for AUDIT_EXE Ondrej Mosnacek 2018-05-31 16:29 ` Richard Guy Briggs 2018-06-04 20:41 ` Paul Moore 2018-06-05 11:13 ` Ondrej Mosnacek 2018-06-19 14:25 ` Paul Moore 2018-05-30 8:45 ` [RFC PATCH 2/2] [WIP] audit: allow other filter list types for AUDIT_DIR Ondrej Mosnacek 2018-05-31 20:52 ` Richard Guy Briggs 2018-06-01 8:12 ` Ondrej Mosnacek 2018-06-01 20:05 ` Richard Guy Briggs 2018-06-04 11:32 ` Ondrej Mosnacek 2018-06-04 20:47 ` Richard Guy Briggs 2018-06-04 22:19 ` Paul Moore 2018-06-05 11:23 ` Ondrej Mosnacek 2018-06-04 22:12 ` Paul Moore
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.