Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CABqD9ha8o3JU0v0ZnKcsjqm8osjSiUfCyvc=vrxhDTHHuG_V4A@mail.gmail.com>
Date: Mon, 5 Mar 2012 15:03:11 -0600
From: Will Drewry <wad@...omium.org>
To: "Serge E. Hallyn" <serge@...lyn.com>
Cc: linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org, 
	linux-doc@...r.kernel.org, kernel-hardening@...ts.openwall.com, 
	netdev@...r.kernel.org, x86@...nel.org, arnd@...db.de, davem@...emloft.net, 
	hpa@...or.com, mingo@...hat.com, oleg@...hat.com, peterz@...radead.org, 
	rdunlap@...otime.net, mcgrathr@...omium.org, tglx@...utronix.de, luto@....edu, 
	eparis@...hat.com, serge.hallyn@...onical.com, djm@...drot.org, 
	scarybeasts@...il.com, indan@....nu, pmoore@...hat.com, 
	akpm@...ux-foundation.org, corbet@....net, eric.dumazet@...il.com, 
	markus@...omium.org, coreyb@...ux.vnet.ibm.com, keescook@...omium.org
Subject: Re: [PATCH v12 07/13] seccomp: add SECCOMP_RET_ERRNO

On Fri, Mar 2, 2012 at 12:24 PM, Serge E. Hallyn <serge@...lyn.com> wrote:
> Quoting Will Drewry (wad@...omium.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@...hat.com, luto@....edu, keescook@...omium.org)
>>      - return immediately for filter==NULL (keescook@...omium.org)
>>      - change evaluation to only compare the ACTION so that layered
>>        errnos don't result in the lowest one being returned.
>>        (keeschook@...omium.org)
>> v11: - check for NULL filter (keescook@...omium.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@...omium.org>
>> Signed-off-by: Will Drewry <wad@...omium.org>
>
> Clever :)
>
> Thanks, Will.
>
> For patches 1-7,
>
> Acked-by: Serge Hallyn <serge.hallyn@...onical.com>

Thanks!

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

Works for me.  The -1 just matches what syscall emulation, etc does on
x86.  I'll add this to the tweaks for v14.

Thanks!

> (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@...r.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.