Re: Limiting SECCOMP audit events

[Kees Cook <keescook@chromium.org>]

On Thu, Dec 14, 2017 at 3:06 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
> The reason is because I didn't get clear direction from the audit
> folks about to do when audit is enabled and the process is being audited
> and, therefore, I didn't feel comfortable rocking the boat. In that
> situation, the decision to log is the same as it was in earlier kernels.
> Specifically, you're hitting the last "else if" conditional in the
> pseudocode above.

Yeah, same for me: it's been entirely unclear what the desired
combination of audit vs seccomp should be. It seems like it should be
reporting everything when auditing a specific process, and then ...
something else? ... in the global context.

> If you're happy with having the actions_logged sysctl control whether or
> not to log seccomp actions taken for processes that are being audited,
> then I think the following (untested) patch should do exactly what you
> want. I imagine that you'd also want seccomp to emit audit events
> whenever the value of the actions_logged sysctl is changed, which should
> be pretty easy to do.
>
> I hope this helps!
>
> Tyler
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index af410d9..095b5dd 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -304,12 +304,6 @@ static inline void audit_inode_child(struct inode *parent,
>  }
>  void audit_core_dumps(long signr);
>
> -static inline void audit_seccomp(unsigned long syscall, long signr, int code)
> -{
> -       if (audit_enabled && unlikely(!audit_dummy_context()))
> -               __audit_seccomp(syscall, signr, code);
> -}
> -
>  static inline void audit_ptrace(struct task_struct *t)
>  {
>         if (unlikely(!audit_dummy_context()))
> @@ -502,8 +496,6 @@ static inline void audit_core_dumps(long signr)
>  { }
>  static inline void __audit_seccomp(unsigned long syscall, long signr, int code)
>  { }
> -static inline void audit_seccomp(unsigned long syscall, long signr, int code)
> -{ }
>  static inline int auditsc_get_stamp(struct audit_context *ctx,
>                               struct timespec64 *t, unsigned int *serial)
>  {
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 5f0dfb2ab..914a707 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -590,12 +590,6 @@ static inline void seccomp_log(unsigned long syscall, long signr, u32 action,
>          */
>         if (log)
>                 return __audit_seccomp(syscall, signr, action);
> -
> -       /*
> -        * Let the audit subsystem decide if the action should be audited based
> -        * on whether the current task itself is being audited.
> -        */
> -       return audit_seccomp(syscall, signr, action);
>  }
>
>  /*

If audit folks are happy with this, I am too. :)

-Kees

-- 
Kees Cook
Pixel Security