Re: [PATCH v4 2/4] seccomp: Add sysctl to configure actions that should be logged

On Mon, Feb 13, 2017 at 7:55 PM, Tyler Hicks <tyhicks(a)canonical.com&gt; wrote: > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > index e36dfe9..270a227 100644 > --- a/kernel/seccomp.c > +++ b/kernel/seccomp.c > @@ -509,6 +509,22 @@ static void seccomp_send_sigsys(int syscall, int reason) > } > #endif /* CONFIG_SECCOMP_FILTER */ > > +static u32 seccomp_log_max_action = SECCOMP_RET_KILL; > + > +static inline void seccomp_log(unsigned long syscall, long signr, u32 action) > +{ > + /* Force an audit message to be emitted when the action is not greater > + * than the configured maximum action. > + */ > + if (action <= seccomp_log_max_action) > + 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. > + */ Nitpick on comment style, please use: /* * line 1 * line 2... */

> + return audit_seccomp(syscall, signr, action); > +} > + > /* > * Secure computing mode 1 allows only read/write/exit/sigreturn. > * To be fully secure this must be combined with rlimit > @@ -534,7 +550,7 @@ static void __secure_computing_strict(int this_syscall) > #ifdef SECCOMP_DEBUG > dump_stack(); > #endif > - audit_seccomp(this_syscall, SIGKILL, SECCOMP_RET_KILL); > + seccomp_log(this_syscall, SIGKILL, SECCOMP_RET_KILL); > do_exit(SIGKILL); > } > > @@ -633,18 +649,30 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd, > return 0; > > case SECCOMP_RET_ALLOW: > + /* Open-coded seccomp_log(), optimized for the RET_ALLOW hot > + * path. > + * > + * We only want to log RET_ALLOW actions when the admin has > + * configured them to be logged via the log_max_action sysctl. > + * Therefore, call __audit_seccomp() directly so that RET_ALLOW > + * actions are not audited simply because the task is being > + * audited. > + */ > + if (unlikely(seccomp_log_max_action == SECCOMP_RET_ALLOW)) > + __audit_seccomp(this_syscall, 0, action); > + > return 0; > > case SECCOMP_RET_KILL: > default: > - audit_seccomp(this_syscall, SIGSYS, action); > + seccomp_log(this_syscall, SIGSYS, action); > do_exit(SIGSYS); > } > > unreachable(); > > skip: > - audit_seccomp(this_syscall, 0, action); > + seccomp_log(this_syscall, 0, action); > return -1; > } > #else > @@ -917,12 +945,96 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off, > #define SECCOMP_RET_TRACE_NAME "trace" > #define SECCOMP_RET_ALLOW_NAME "allow" > > +/* Largest strlen() of all action names */ > +#define SECCOMP_RET_MAX_NAME_LEN 5 > + > static char seccomp_actions_avail[] = SECCOMP_RET_KILL_NAME " " > SECCOMP_RET_TRAP_NAME " " > SECCOMP_RET_ERRNO_NAME " " > SECCOMP_RET_TRACE_NAME " " > SECCOMP_RET_ALLOW_NAME; > > +struct seccomp_action_name { > + u32 action; > + const char *name; > +}; > + > +static struct seccomp_action_name seccomp_action_names[] = { As long as I'm nit-picking, this can be const too. :)

-Kees