lkml.org 
[lkml]   [2012]   [Mar]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v12 07/13] seccomp: add SECCOMP_RET_ERRNO
    Quoting Will Drewry (wad@chromium.org):
    > This change adds the SECCOMP_RET_ERRNO as a valid return value from a
    > seccomp filter. Additionally, it makes the first use of the lower
    > 16-bits for storing a filter-supplied errno. 16-bits is more than
    > enough for the errno-base.h calls.
    >
    > Returning errors instead of immediately terminating processes that
    > violate seccomp policy allow for broader use of this functionality
    > for kernel attack surface reduction. For example, a linux container
    > could maintain a whitelist of pre-existing system calls but drop
    > all new ones with errnos. This would keep a logically static attack
    > surface while providing errnos that may allow for graceful failure
    > without the downside of do_exit() on a bad call.
    >
    > v12: - move to WARN_ON if filter is NULL
    > (oleg@redhat.com, luto@mit.edu, keescook@chromium.org)
    > - return immediately for filter==NULL (keescook@chromium.org)
    > - change evaluation to only compare the ACTION so that layered
    > errnos don't result in the lowest one being returned.
    > (keeschook@chromium.org)
    > v11: - check for NULL filter (keescook@chromium.org)
    > v10: - change loaders to fn
    > v9: - n/a
    > v8: - update Kconfig to note new need for syscall_set_return_value.
    > - reordered such that TRAP behavior follows on later.
    > - made the for loop a little less indent-y
    > v7: - introduced
    >
    > Reviewed-by: Kees Cook <keescook@chromium.org>
    > Signed-off-by: Will Drewry <wad@chromium.org>

    Clever :)

    Thanks, Will.

    For patches 1-7,

    Acked-by: Serge Hallyn <serge.hallyn@canonical.com>

    The -1 return value from __secure_computing_int() seems like it
    could stand #define, like

    #define SECCOMP_DONTRUN -1
    #define SECCOMP_RUN 0

    or something Maybe not, but -1 always scares me and I had to look back
    and forth a few times to make sure it was doing what I would want.

    (I've only quickly looked at the following ones. I had no
    objection, but didn't seriously review them.)

    > ---
    > arch/Kconfig | 6 ++++--
    > include/linux/seccomp.h | 15 +++++++++++----
    > kernel/seccomp.c | 43 ++++++++++++++++++++++++++++++++++---------
    > 3 files changed, 49 insertions(+), 15 deletions(-)
    >
    > diff --git a/arch/Kconfig b/arch/Kconfig
    > index 7a696a9..1350d07 100644
    > --- a/arch/Kconfig
    > +++ b/arch/Kconfig
    > @@ -237,8 +237,10 @@ config HAVE_ARCH_SECCOMP_FILTER
    > bool
    > help
    > This symbol should be selected by an architecure if it provides
    > - asm/syscall.h, specifically syscall_get_arguments() and
    > - syscall_get_arch().
    > + asm/syscall.h, specifically syscall_get_arguments(),
    > + syscall_get_arch(), and syscall_set_return_value(). Additionally,
    > + its system call entry path must respect a return value of -1 from
    > + __secure_computing_int() and/or secure_computing().
    >
    > config SECCOMP_FILTER
    > def_bool y
    > diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
    > index 6ef133c..a81fccd 100644
    > --- a/include/linux/seccomp.h
    > +++ b/include/linux/seccomp.h
    > @@ -12,13 +12,14 @@
    >
    > /*
    > * All BPF programs must return a 32-bit value.
    > - * The bottom 16-bits are reserved for future use.
    > + * The bottom 16-bits are for optional return data.
    > * The upper 16-bits are ordered from least permissive values to most.
    > *
    > * The ordering ensures that a min_t() over composed return values always
    > * selects the least permissive choice.
    > */
    > #define SECCOMP_RET_KILL 0x00000000U /* kill the task immediately */
    > +#define SECCOMP_RET_ERRNO 0x00030000U /* returns an errno */
    > #define SECCOMP_RET_ALLOW 0x7fff0000U /* allow */
    >
    > /* Masks for the return value sections. */
    > @@ -64,11 +65,17 @@ struct seccomp {
    > struct seccomp_filter *filter;
    > };
    >
    > -extern void __secure_computing(int);
    > -static inline void secure_computing(int this_syscall)
    > +/*
    > + * Direct callers to __secure_computing should be updated as
    > + * CONFIG_HAVE_ARCH_SECCOMP_FILTER propagates.
    > + */
    > +extern void __secure_computing(int) __deprecated;
    > +extern int __secure_computing_int(int);
    > +static inline int secure_computing(int this_syscall)
    > {
    > if (unlikely(test_thread_flag(TIF_SECCOMP)))
    > - __secure_computing(this_syscall);
    > + return __secure_computing_int(this_syscall);
    > + return 0;
    > }
    >
    > extern long prctl_get_seccomp(void);
    > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
    > index 71df324..88dd568 100644
    > --- a/kernel/seccomp.c
    > +++ b/kernel/seccomp.c
    > @@ -137,21 +137,25 @@ static void *bpf_load(const void *nr, int off, unsigned int size, void *buf)
    > static u32 seccomp_run_filters(int syscall)
    > {
    > struct seccomp_filter *f;
    > - u32 ret = SECCOMP_RET_KILL;
    > static const struct bpf_load_fn fns = {
    > bpf_load,
    > sizeof(struct seccomp_data),
    > };
    > + u32 ret = SECCOMP_RET_ALLOW;
    > const void *sc_ptr = (const void *)(uintptr_t)syscall;
    >
    > + /* Ensure unexpected behavior doesn't result in failing open. */
    > + if (WARN_ON(current->seccomp.filter == NULL))
    > + return SECCOMP_RET_KILL;
    > +
    > /*
    > * All filters are evaluated in order of youngest to oldest. The lowest
    > - * BPF return value always takes priority.
    > + * BPF return value (ignoring the DATA) always takes priority.
    > */
    > for (f = current->seccomp.filter; f; f = f->prev) {
    > - ret = bpf_run_filter(sc_ptr, f->insns, &fns);
    > - if (ret != SECCOMP_RET_ALLOW)
    > - break;
    > + u32 cur_ret = bpf_run_filter(sc_ptr, f->insns, &fns);
    > + if ((cur_ret & SECCOMP_RET_ACTION) < (ret & SECCOMP_RET_ACTION))
    > + ret = cur_ret;
    > }
    > return ret;
    > }
    > @@ -289,6 +293,13 @@ static int mode1_syscalls_32[] = {
    >
    > void __secure_computing(int this_syscall)
    > {
    > + /* Filter calls should never use this function. */
    > + BUG_ON(current->seccomp.mode == SECCOMP_MODE_FILTER);
    > + __secure_computing_int(this_syscall);
    > +}
    > +
    > +int __secure_computing_int(int this_syscall)
    > +{
    > int mode = current->seccomp.mode;
    > int exit_code = SIGKILL;
    > int *syscall;
    > @@ -302,16 +313,29 @@ void __secure_computing(int this_syscall)
    > #endif
    > do {
    > if (*syscall == this_syscall)
    > - return;
    > + return 0;
    > } while (*++syscall);
    > break;
    > #ifdef CONFIG_SECCOMP_FILTER
    > - case SECCOMP_MODE_FILTER:
    > - if (seccomp_run_filters(this_syscall) == SECCOMP_RET_ALLOW)
    > - return;
    > + case SECCOMP_MODE_FILTER: {
    > + u32 action = seccomp_run_filters(this_syscall);
    > + switch (action & SECCOMP_RET_ACTION) {
    > + case SECCOMP_RET_ERRNO:
    > + /* Set the low-order 16-bits as a errno. */
    > + syscall_set_return_value(current, task_pt_regs(current),
    > + -(action & SECCOMP_RET_DATA),
    > + 0);
    > + return -1;
    > + case SECCOMP_RET_ALLOW:
    > + return 0;
    > + case SECCOMP_RET_KILL:
    > + default:
    > + break;
    > + }
    > seccomp_filter_log_failure(this_syscall);
    > exit_code = SIGSYS;
    > break;
    > + }
    > #endif
    > default:
    > BUG();
    > @@ -322,6 +346,7 @@ void __secure_computing(int this_syscall)
    > #endif
    > audit_seccomp(this_syscall);
    > do_exit(exit_code);
    > + return -1; /* never reached */
    > }
    >
    > long prctl_get_seccomp(void)
    > --
    > 1.7.5.4
    >
    > --
    > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    > the body of a message to majordomo@vger.kernel.org
    > More majordomo info at http://vger.kernel.org/majordomo-info.html
    > Please read the FAQ at http://www.tux.org/lkml/


    \
     
     \ /
      Last update: 2012-03-02 19:27    [W:4.085 / U:0.620 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site