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