Re: [PATCH ghak111 V1] audit: deliver siginfo regarless of syscall

On Thu, Apr 18, 2019 at 11:16 AM Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > On 2019-04-18 10:59, Paul Moore wrote: > > On Mon, Apr 8, 2019 at 11:53 PM Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > > > When a process signals the audit daemon (shutdown, rotate, resume, > > > reconfig) but syscall auditing is not enabled, we still want to know the > > > identity of the process sending the signal to the audit daemon. > > > > > > Move audit_signal_info() out of syscall auditing to general auditing but > > > create a new function audit_signal_info_syscall() to take care of the > > > syscall dependent parts for when syscall auditing is enabled. > > > > > > Please see the github kernel audit issue > > > https://github.com/linux-audit/audit-kernel/issues/111 > > > > > > Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; > > > --- > > > include/linux/audit.h | 6 ++++++ > > > kernel/audit.c | 27 +++++++++++++++++++++++++++ > > > kernel/audit.h | 4 ++-- > > > kernel/auditsc.c | 19 +++---------------- > > > kernel/signal.c | 2 +- > > > 5 files changed, 39 insertions(+), 19 deletions(-) > > > > ... > > > > > diff --git a/include/linux/audit.h b/include/linux/audit.h > > > index 1e69d9fe16da..4a22fc3f824f 100644 > > > --- a/include/linux/audit.h > > > +++ b/include/linux/audit.h > > > @@ -226,6 +229,9 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk) > > > } > > > > > > #define audit_enabled AUDIT_OFF > > > + > > > +#define audit_signal_info(s, t) AUDIT_OFF > > > + > > > > Should this be AUDIT_DISABLED to preserve the current value/behavior? > > Technically they should both have a value of zero right now, but since > > the AUDIT_DISABLED value isn't explicit it seems safer to go with > > AUDIT_DISABLED. > > I did that first, but that symbol was not available when one or both of > CONFIG_AUDITSYSCALL or CONFIG_AUDIT was off, so I had to change it to > AUDIT_OFF. I followed the logic to confirm that is what was intended by > the original code. > > When auidit is off, we want to just return zero so it gets skipped > rather than throwing an error. I understand the desire to return zero in that case, I'm not arguing against that, I'm just not really in love with how these are defined when CONFIG_AUDIT isn't. Part of it is the AUDIT_DISABLED/AUDIT_OFF change, part of it is the function being defined as a cpp macro instead of a dummy function (this of course predates your change). Based on other comments in this thread it looks like you're looking into a few things and will likely be respinning this patch, since that is the case, I would prefer if you changed this to just using simply "0" as opposed to AUDIT_OFF. If you really want to make me happy about this patch, you would also change this to a dummy function instead of a cpp macro. This is a style nit, and isn't strictly necessary, but I would appreciate it :)

> > > diff --git a/kernel/audit.h b/kernel/audit.h > > > index 958d5b8fc1b3..18a8ae812e9f 100644 > > > --- a/kernel/audit.h > > > +++ b/kernel/audit.h > > > @@ -299,7 +299,7 @@ extern bool audit_tree_match(struct audit_chunk *chunk, > > > extern void audit_put_tree(struct audit_tree *tree); > > > extern void audit_kill_trees(struct audit_context *context); > > > > > > -extern int audit_signal_info(int sig, struct task_struct *t); > > > +extern int audit_signal_info_syscall(struct task_struct *t); > > > extern void audit_filter_inodes(struct task_struct *tsk, > > > struct audit_context *ctx); > > > extern struct list_head *audit_killed_trees(void); > > > @@ -330,7 +330,7 @@ extern void audit_filter_inodes(struct task_struct *tsk, > > > #define audit_tree_path(rule) "" /* never called */ > > > #define audit_kill_trees(context) BUG() > > > > > > -#define audit_signal_info(s, t) AUDIT_DISABLED > > > +#define audit_signal_info_syscall(t) AUDIT_OFF > > > > Similar as above. -- paul moore www.paul-moore.com