Re: [PATCH v10 05/11] seccomp: add system call filtering using BPF

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Feb 22, 2012 at 2:19 AM, Indan Zupancic <indan@xxxxxx> wrote:
> Hello,
>
> On Tue, February 21, 2012 18:30, Will Drewry wrote:
>> [This patch depends on luto@xxxxxxx's no_new_privs patch:
>> https://lkml.org/lkml/2012/1/30/264
>> ]
>>
>> This patch adds support for seccomp mode 2.  Mode 2 introduces the
>> ability for unprivileged processes to install system call filtering
>> policy expressed in terms of a Berkeley Packet Filter (BPF) program.
>> This program will be evaluated in the kernel for each system call
>> the task makes and computes a result based on data in the format
>> of struct seccomp_data.
>>
>> A filter program may be installed by calling:
>>  struct sock_fprog fprog = { ... };
>>  ...
>>  prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &fprog);
>>
>> The return value of the filter program determines if the system call is
>> allowed to proceed or denied.  If the first filter program installed
>> allows prctl(2) calls, then the above call may be made repeatedly
>> by a task to further reduce its access to the kernel.  All attached
>> programs must be evaluated before a system call will be allowed to
>> proceed.
>>
>> Filter programs will be inherited across fork/clone and execve.
>> However, if the task attaching the filter is unprivileged
>> (!CAP_SYS_ADMIN) the no_new_privs bit will be set on the task.  This
>> ensures that unprivileged tasks cannot attach filters that affect
>> privileged tasks (e.g., setuid binary).
>>
>> There are a number of benefits to this approach. A few of which are
>> as follows:
>> - BPF has been exposed to userland for a long time
>> - BPF optimization (and JIT'ing) are well understood
>> - Userland already knows its ABI: system call numbers and desired
>>  arguments
>> - No time-of-check-time-of-use vulnerable data accesses are possible.
>> - system call arguments are loaded on access only to minimize copying
>>  required for system call policy decisions.
>>
>> Mode 2 support is restricted to architectures that enable
>> HAVE_ARCH_SECCOMP_FILTER.  In this patch, the primary dependency is on
>> syscall_get_arguments().  The full desired scope of this feature will
>> add a few minor additional requirements expressed later in this series.
>> Based on discussion, SECCOMP_RET_ERRNO and SECCOMP_RET_TRACE seem to be
>> the desired additional functionality.
>>
>> No architectures are enabled in this patch.
>>
>> v10: - seccomp_data has changed again to be more aesthetically pleasing
>>       (hpa@xxxxxxxxx)
>>     - calling convention is noted in a new u32 field using syscall_get_arch.
>>       This allows for cross-calling convention tasks to use seccomp filters.
>>       (hpa@xxxxxxxxx)
>
> I highly disagree with every filter having to check the mode: Filters that
> don't check the arch on e.g. x86 are buggy, so they have to check it, even
> if it's a 32-bit or 64-bit only system, the filters can't know that and
> needs to check the arch at every syscall entry. All other info in the data
> depends on the arch, because of this there isn't much code to share between
> the two archs, so you can as well have one filter for each arch.
>
> Alternative approach: Tell the arch at filter install time and only run the
> filters with the same arch as the current system call. If no filters are run,
> deny the systemcall.

This was roughly how I first implemented compat and non-compat
support.  It causes some implicit behavior across inheritance that is
not nice though.

> Advantages:
>
> - Filters don't have to check the arch every syscall entry.

This I like.

> - Secure by default. Filters don't have to do anything arch specific to
>  be secure, no surprises possible.

This is partially true, but it is exactly why I hid compat before.

> - If a new arch comes into existence, there is no chance of old filters
>  becoming buggy and insecure. This is especially true for archs that
>  had only one mode, but added another one later on: Old filters had no
>  need to check the mode at all.

Perhaps.  A buggy filter that works on x86-64 might be exposed on a
new x32 ABI.  It's hard to predict how audit_arch and the syscall abi
will develop with new platforms.

> - For kernels supporting only one arch, the check can be optimised away,
>  by not installing unsupported arch filters at all.

Somewhat.  Without having a dedicated arch helper, you'd have to guess
that arches only support one or two arches (if compat is supported),
but I don't know if that is a safe assumption to make.

> It's more secure, faster and simpler for the filters.
>
> If something like this is implemented it's fine to expose the arch info
> in the syscall data too, and have a way to install filters for all archs,
> for the few cases where that might be useful, although I can't think of
> any reason why people would like to do unnecessary work in the filters.

It seems to just add complexity to support both. I think we'll
probably end up with it in the filters for better or worse.  Possibly
JITing will be useful since at least a 32-bit load and je is pretty
cheap in native instructions.

> All that's needed is an extra argument to the prctl() call. I propose
> 0 for the current arch, -1 for all archs and anything else to specify
> the arch. Installing a filter for an unsupported arch could return
> ENOEXEC.

Without adding a per-arch call, there is no way to know all the
supported arches at install time.  Current arch, at least, can be
determined with a call to syscall_get_arch().

As is, I'm not sure it makes sense to try to reserve two extra input
types: 0 and -1.  0 would be sane to treat as either a wildcard or
current because it is unlikely to be used by AUDIT_ARCH_* ever since
EM_NONE is assigned to 0.  However, I have no such insight into
whether it will ever be possible to compose 0xffffffff as an
AUDIT_ARCH_.

> As far as the implementation goes, either have a list per supported arch
> or store the arch per filter and check that before running the filter.

You can't do it per arch without adding even more per-arch
dependencies.  Keeping them annotated in the same list is the clearest
way I've seen so far, but it comes with its own burdens.

>>     - lots of clean up (thanks, Indan!)
>> v9: - n/a
>> v8: - use bpf_chk_filter, bpf_run_filter. update load_fns
>>     - Lots of fixes courtesy of indan@xxxxxx:
>>     -- fix up load behavior, compat fixups, and merge alloc code,
>>     -- renamed pc and dropped __packed, use bool compat.
>>     -- Added a hidden CONFIG_SECCOMP_FILTER to synthesize non-arch
>>        dependencies
>> v7:  (massive overhaul thanks to Indan, others)
>>     - added CONFIG_HAVE_ARCH_SECCOMP_FILTER
>>     - merged into seccomp.c
>>     - minimal seccomp_filter.h
>>     - no config option (part of seccomp)
>>     - no new prctl
>>     - doesn't break seccomp on systems without asm/syscall.h
>>       (works but arg access always fails)
>>     - dropped seccomp_init_task, extra free functions, ...
>>     - dropped the no-asm/syscall.h code paths
>>     - merges with network sk_run_filter and sk_chk_filter
>> v6: - fix memory leak on attach compat check failure
>>     - require no_new_privs || CAP_SYS_ADMIN prior to filter
>>       installation. (luto@xxxxxxx)
>>     - s/seccomp_struct_/seccomp_/ for macros/functions (amwang@xxxxxxxxxx)
>>     - cleaned up Kconfig (amwang@xxxxxxxxxx)
>>     - on block, note if the call was compat (so the # means something)
>> v5: - uses syscall_get_arguments
>>       (indan@xxxxxx,oleg@xxxxxxxxxx, mcgrathr@xxxxxxxxxxxx)
>>      - uses union-based arg storage with hi/lo struct to
>>        handle endianness.  Compromises between the two alternate
>>        proposals to minimize extra arg shuffling and account for
>>        endianness assuming userspace uses offsetof().
>>        (mcgrathr@xxxxxxxxxxxx, indan@xxxxxx)
>>      - update Kconfig description
>>      - add include/seccomp_filter.h and add its installation
>>      - (naive) on-demand syscall argument loading
>>      - drop seccomp_t (eparis@xxxxxxxxxx)
>> v4:  - adjusted prctl to make room for PR_[SG]ET_NO_NEW_PRIVS
>>      - now uses current->no_new_privs
>>        (luto@xxxxxxx,torvalds@xxxxxxxxxxxxxxxxxxxx)
>>      - assign names to seccomp modes (rdunlap@xxxxxxxxxxxx)
>>      - fix style issues (rdunlap@xxxxxxxxxxxx)
>>      - reworded Kconfig entry (rdunlap@xxxxxxxxxxxx)
>> v3:  - macros to inline (oleg@xxxxxxxxxx)
>>      - init_task behavior fixed (oleg@xxxxxxxxxx)
>>      - drop creator entry and extra NULL check (oleg@xxxxxxxxxx)
>>      - alloc returns -EINVAL on bad sizing (serge.hallyn@xxxxxxxxxxxxx)
>>      - adds tentative use of "always_unprivileged" as per
>>        torvalds@xxxxxxxxxxxxxxxxxxxx and luto@xxxxxxx
>> v2:  - (patch 2 only)
>>
>> Signed-off-by: Will Drewry <wad@xxxxxxxxxxxx>
>> ---
>> arch/Kconfig            |   18 +++
>> include/linux/Kbuild    |    1 +
>> include/linux/seccomp.h |   76 +++++++++++-
>> kernel/fork.c           |    3 +
>> kernel/seccomp.c        |  321 ++++++++++++++++++++++++++++++++++++++++++++---
>> kernel/sys.c            |    2 +-
>> 6 files changed, 399 insertions(+), 22 deletions(-)
>>
>> diff --git a/arch/Kconfig b/arch/Kconfig
>> index 4f55c73..8150fa2 100644
>> --- a/arch/Kconfig
>> +++ b/arch/Kconfig
>> @@ -199,4 +199,22 @@ config HAVE_CMPXCHG_LOCAL
>> config HAVE_CMPXCHG_DOUBLE
>>       bool
>>
>> +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().
>> +
>> +config SECCOMP_FILTER
>> +     def_bool y
>> +     depends on HAVE_ARCH_SECCOMP_FILTER && SECCOMP && NET
>> +     help
>> +       Enable tasks to build secure computing environments defined
>> +       in terms of Berkeley Packet Filter programs which implement
>> +       task-defined system call filtering polices.
>> +
>> +       See Documentation/prctl/seccomp_filter.txt for more
>> +       information on the topic of seccomp filtering.
>
> The last part is redundant, the topic is clear.

I'll drop it.

>> +
>> source "kernel/gcov/Kconfig"
>> diff --git a/include/linux/Kbuild b/include/linux/Kbuild
>> index c94e717..d41ba12 100644
>> --- a/include/linux/Kbuild
>> +++ b/include/linux/Kbuild
>> @@ -330,6 +330,7 @@ header-y += scc.h
>> header-y += sched.h
>> header-y += screen_info.h
>> header-y += sdla.h
>> +header-y += seccomp.h
>> header-y += securebits.h
>> header-y += selinux_netlink.h
>> header-y += sem.h
>> diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
>> index d61f27f..001f883 100644
>> --- a/include/linux/seccomp.h
>> +++ b/include/linux/seccomp.h
>> @@ -1,14 +1,67 @@
>> #ifndef _LINUX_SECCOMP_H
>> #define _LINUX_SECCOMP_H
>>
>> +#include <linux/compiler.h>
>> +#include <linux/types.h>
>> +
>> +
>> +/* Valid values for seccomp.mode and prctl(PR_SET_SECCOMP, <mode>) */
>> +#define SECCOMP_MODE_DISABLED        0 /* seccomp is not in use. */
>> +#define SECCOMP_MODE_STRICT  1 /* uses hard-coded filter. */
>> +#define SECCOMP_MODE_FILTER  2 /* uses user-supplied filter. */
>> +
>> +/*
>> + * BPF programs may return a 32-bit value.
>
> They have to return a 32-bit value, no "may" about it.

True.

>> + * The bottom 16-bits are reserved for future use.
>> + * 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_ALLOW    0x7fff0000U /* allow */
>> +
>> +/* Masks for the return value sections. */
>> +#define SECCOMP_RET_ACTION   0xffff0000U
>> +#define SECCOMP_RET_DATA     0x0000ffffU
>> +
>> +/**
>> + * struct seccomp_data - the format the BPF program executes over.
>> + * @args: up to 6 system call arguments.  When the calling convention is
>> + *        32-bit, the arguments will still be at each args[X] offset.
>
> What does this mean? Do you mean the data layout will always be "LE" for
> 32-bit archs? I hope not, because that would make it incompatible with
> the 64-bit code for BE archs, so it will be confusing. Except if the data
> layout is always LE, but then you should document that. If neither is the
> case, then the comment is just confusing. Just say that the data layout
> depends on the arch's endianness.

I'll rephrase.  I just wanted to call out that the argument values
will always be treated as a 64-bit value even if the calling
convention is 32-bit.  This doesn't matter for LE systems, except to
acknowledge the padding, but for BE systems they might load the wrong
half.

>> + * @instruction_pointer: at the time of the system call.
>> + * @arch: indicates system call convention as an AUDIT_ARCH_* value
>> + *        as defined in <linux/audit.h>.
>> + * @nr: the system call number
>> + */
>> +struct seccomp_data {
>> +     __u64 args[6];
>> +     __u64 instruction_pointer;
>> +     __u32 arch;
>> +     int nr;
>> +};
>
> I agree this looks a hell of a lot nicer. I just hope it's worth it.
> Oh well, a bit more ugliness in userspace to make the kernel code a
> bit nicer isn't too bad. Just document the endianness issue properly.
>
> What use is the instruction pointer considering it tells nothing about
> the call path?
>
>>
>> +#ifdef __KERNEL__
>> #ifdef CONFIG_SECCOMP
>>
>> #include <linux/thread_info.h>
>> #include <asm/seccomp.h>
>>
>> +struct seccomp_filter;
>> +/**
>> + * struct seccomp - the state of a seccomp'ed process
>> + *
>> + * @mode:  indicates one of the valid values above for controlled
>> + *         system calls available to a process.
>> + * @filter: The metadata and ruleset for determining what system calls
>> + *          are allowed for a task.
>> + *
>> + *          @filter must only be accessed from the context of current as there
>> + *          is no locking.
>> + */
>> struct seccomp {
>>       int mode;
>> +     struct seccomp_filter *filter;
>> };
>>
>> extern void __secure_computing(int);
>> @@ -19,7 +72,7 @@ static inline void secure_computing(int this_syscall)
>> }
>>
>> extern long prctl_get_seccomp(void);
>> -extern long prctl_set_seccomp(unsigned long);
>> +extern long prctl_set_seccomp(unsigned long, char __user *);
>>
>> static inline int seccomp_mode(struct seccomp *s)
>> {
>> @@ -31,15 +84,16 @@ static inline int seccomp_mode(struct seccomp *s)
>> #include <linux/errno.h>
>>
>> struct seccomp { };
>> +struct seccomp_filter { };
>>
>> -#define secure_computing(x) do { } while (0)
>> +#define secure_computing(x) 0
>>
>> static inline long prctl_get_seccomp(void)
>> {
>>       return -EINVAL;
>> }
>>
>> -static inline long prctl_set_seccomp(unsigned long arg2)
>> +static inline long prctl_set_seccomp(unsigned long arg2, char __user *arg3)
>> {
>>       return -EINVAL;
>> }
>> @@ -48,7 +102,21 @@ static inline int seccomp_mode(struct seccomp *s)
>> {
>>       return 0;
>> }
>> -
>> #endif /* CONFIG_SECCOMP */
>>
>> +#ifdef CONFIG_SECCOMP_FILTER
>> +extern void put_seccomp_filter(struct seccomp_filter *);
>> +extern void copy_seccomp(struct seccomp *child,
>> +                      const struct seccomp *parent);
>
> This is 80 chars long, why break it up? Please, stop your bad habit of
> breaking up (slightly too) long lines.
>
>> +#else  /* CONFIG_SECCOMP_FILTER */
>> +/* The macro consumes the ->filter reference. */
>> +#define put_seccomp_filter(_s) do { } while (0)
>> +
>> +static inline void copy_seccomp(struct seccomp *child,
>> +                             const struct seccomp *prev)
>> +{
>> +     return;
>> +}
>> +#endif /* CONFIG_SECCOMP_FILTER */
>> +#endif /* __KERNEL__ */
>> #endif /* _LINUX_SECCOMP_H */
>> diff --git a/kernel/fork.c b/kernel/fork.c
>> index b77fd55..a5187b7 100644
>> --- a/kernel/fork.c
>> +++ b/kernel/fork.c
>> @@ -34,6 +34,7 @@
>> #include <linux/cgroup.h>
>> #include <linux/security.h>
>> #include <linux/hugetlb.h>
>> +#include <linux/seccomp.h>
>> #include <linux/swap.h>
>> #include <linux/syscalls.h>
>> #include <linux/jiffies.h>
>> @@ -169,6 +170,7 @@ void free_task(struct task_struct *tsk)
>>       free_thread_info(tsk->stack);
>>       rt_mutex_debug_task_free(tsk);
>>       ftrace_graph_exit_task(tsk);
>> +     put_seccomp_filter(tsk->seccomp.filter);
>
> So that's why you use macro's sometimes, to make it compile with
> CONFIG_SECCOMP disabled where there is no seccomp.filter.

Exactly!

>>       free_task_struct(tsk);
>> }
>> EXPORT_SYMBOL(free_task);
>> @@ -1113,6 +1115,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
>>               goto fork_out;
>>
>>       ftrace_graph_init_task(p);
>> +     copy_seccomp(&p->seccomp, &current->seccomp);
>>
>>       rt_mutex_init_task(p);
>>
>> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
>> index e8d76c5..0043b7e 100644
>> --- a/kernel/seccomp.c
>> +++ b/kernel/seccomp.c
>> @@ -3,16 +3,287 @@
>>  *
>>  * Copyright 2004-2005  Andrea Arcangeli <andrea@xxxxxxxxxxxx>
>>  *
>> - * This defines a simple but solid secure-computing mode.
>> + * Copyright (C) 2012 Google, Inc.
>> + * Will Drewry <wad@xxxxxxxxxxxx>
>> + *
>> + * This defines a simple but solid secure-computing facility.
>> + *
>> + * Mode 1 uses a fixed list of allowed system calls.
>> + * Mode 2 allows user-defined system call filters in the form
>> + *        of Berkeley Packet Filters/Linux Socket Filters.
>>  */
>>
>> +#include <linux/atomic.h>
>> #include <linux/audit.h>
>> -#include <linux/seccomp.h>
>> -#include <linux/sched.h>
>> #include <linux/compat.h>
>> +#include <linux/filter.h>
>> +#include <linux/sched.h>
>> +#include <linux/seccomp.h>
>> +#include <linux/security.h>
>> +#include <linux/slab.h>
>> +#include <linux/uaccess.h>
>> +
>> +#include <linux/tracehook.h>
>> +#include <asm/syscall.h>
>>
>> /* #define SECCOMP_DEBUG 1 */
>> -#define NR_SECCOMP_MODES 1
>> +
>> +#ifdef CONFIG_SECCOMP_FILTER
>> +/**
>> + * struct seccomp_filter - container for seccomp BPF programs
>> + *
>> + * @usage: reference count to manage the object liftime.
>> + *         get/put helpers should be used when accessing an instance
>> + *         outside of a lifetime-guarded section.  In general, this
>> + *         is only needed for handling filters shared across tasks.
>> + * @prev: points to a previously installed, or inherited, filter
>> + * @compat: indicates the value of is_compat_task() at creation time
>
> You're not really using 'compat', except for logging.

Oops - I should've dropped it altogether!

> But you could use it to run only the filters with the right arch.

Well an int arch would do the trick.

>> + * @insns: the BPF program instructions to evaluate
>> + * @len: the number of instructions in the program
>> + *
>> + * seccomp_filter objects are organized in a tree linked via the @prev
>> + * pointer.  For any task, it appears to be a singly-linked list starting
>> + * with current->seccomp.filter, the most recently attached or inherited filter.
>> + * However, multiple filters may share a @prev node, by way of fork(), which
>> + * results in a unidirectional tree existing in memory.  This is similar to
>> + * how namespaces work.
>> + *
>> + * seccomp_filter objects should never be modified after being attached
>> + * to a task_struct (other than @usage).
>> + */
>> +struct seccomp_filter {
>> +     atomic_t usage;
>> +     struct seccomp_filter *prev;
>> +     bool compat;
>> +     unsigned short len;  /* Instruction count */
>> +     struct sock_filter insns[];
>> +};
>> +
>> +static void seccomp_filter_log_failure(int syscall)
>> +{
>> +     int compat = 0;
>> +#ifdef CONFIG_COMPAT
>> +     compat = is_compat_task();
>> +#endif
>> +     pr_info("%s[%d]: %ssystem call %d blocked at 0x%lx\n",
>> +             current->comm, task_pid_nr(current),
>> +             (compat ? "compat " : ""),
>> +             syscall, KSTK_EIP(current));
>> +}
>> +
>> +/**
>> + * get_u32 - returns a u32 offset into data
>> + * @data: a unsigned 64 bit value
>> + * @index: 0 or 1 to return the first or second 32-bits
>> + *
>> + * This inline exists to hide the length of unsigned long.
>> + * If a 32-bit unsigned long is passed in, it will be extended
>> + * and the top 32-bits will be 0. If it is a 64-bit unsigned
>> + * long, then whatever data is resident will be properly returned.
>> + */
>> +static inline u32 get_u32(u64 data, int index)
>> +{
>> +     return ((u32 *)&data)[index];
>> +}
>> +
>> +/* Helper for bpf_load below. */
>> +#define BPF_DATA(_name) offsetof(struct seccomp_data, _name)
>> +/**
>> + * bpf_load: checks and returns a pointer to the requested offset
>> + * @nr: int syscall passed as a void * to bpf_run_filter
>> + * @off: index into struct seccomp_data to load from
>> + * @size: load width requested
>> + * @buffer: temporary storage supplied by bpf_run_filter
>> + *
>> + * Returns a pointer to @buffer where the value was stored.
>> + * On failure, returns NULL.
>> + */
>> +static void *bpf_load(const void *nr, int off, unsigned int size, void *buf)
>> +{
>> +     unsigned long value;
>> +     u32 *A = buf;
>> +
>> +     if (size != sizeof(u32))
>> +             return NULL;
>> +
>> +     if (off >= BPF_DATA(args[0]) && off < BPF_DATA(args[6])) {
>> +             struct pt_regs *regs = task_pt_regs(current);
>> +             int arg = off >> 3;  /* args[0] is at offset 0. */
>
> Probably clearer if you just do off / 8, you can count on compilers to
> get that right and turn it into a shift.
>
>> +             int index = (off % sizeof(u64)) ? 1 : 0;
>
> Considering the previous line I expected to see (off & 4).
>
> Anyway, this code mostly ignores the lowest three bits and instead of
> either returning an error or the requested data, it returns the aligned
> value instead. Not good.

Hrm true - I got sloppy. I'll fix that up!

>> +             syscall_get_arguments(current, regs, arg, 1, &value);
>> +             *A = get_u32(value, index);
>> +     } else if (off == BPF_DATA(nr)) {
>> +             *A = (u32)(uintptr_t)nr;
>> +     } else if (off == BPF_DATA(arch)) {
>> +             struct pt_regs *regs = task_pt_regs(current);
>> +             *A = syscall_get_arch(current, regs);
>> +     } else if (off == BPF_DATA(instruction_pointer)) {
>> +             *A = get_u32(KSTK_EIP(current), 0);
>> +     } else if (off == BPF_DATA(instruction_pointer) + sizeof(u32)) {
>> +             *A = get_u32(KSTK_EIP(current), 1);
>> +     } else {
>> +             return NULL;
>> +     }
>> +     return buf;
>> +}
>> +
>> +/**
>> + * seccomp_run_filters - evaluates all seccomp filters against @syscall
>> + * @syscall: number of the current system call
>> + *
>> + * Returns valid seccomp BPF response codes.
>> + */
>> +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),
>
> I suppose this could be used to check for new fields if struct seccomp_data
> ever gets extended in the future.

Yeah since the only other indicator might be @arch.

>> +     };
>> +     const void *sc_ptr = (const void *)(uintptr_t)syscall;
>> +
>> +     /*
>> +      * All filters are evaluated in order of youngest to oldest. The lowest
>> +      * BPF return value 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;
>> +     }
>> +     return ret;
>> +}
>> +
>> +/**
>> + * seccomp_attach_filter: Attaches a seccomp filter to current.
>> + * @fprog: BPF program to install
>> + *
>> + * Returns 0 on success or an errno on failure.
>> + */
>> +static long seccomp_attach_filter(struct sock_fprog *fprog)
>> +{
>> +     struct seccomp_filter *filter;
>> +     unsigned long fp_size = fprog->len * sizeof(struct sock_filter);
>> +     long ret;
>> +
>> +     if (fprog->len == 0 || fprog->len > BPF_MAXINSNS)
>> +             return -EINVAL;
>> +
>> +     /* Allocate a new seccomp_filter */
>> +     filter = kzalloc(sizeof(struct seccomp_filter) + fp_size, GFP_KERNEL);
>> +     if (!filter)
>> +             return -ENOMEM;
>> +     atomic_set(&filter->usage, 1);
>> +     filter->len = fprog->len;
>> +
>> +     /* Copy the instructions from fprog. */
>> +     ret = -EFAULT;
>> +     if (copy_from_user(filter->insns, fprog->filter, fp_size))
>> +             goto out;
>> +
>> +     /* Check the fprog */
>> +     ret = bpf_chk_filter(filter->insns, filter->len, BPF_CHK_FLAGS_NO_SKB);
>> +     if (ret)
>> +             goto out;
>> +
>> +     /*
>> +      * Installing a seccomp filter requires that the task
>> +      * have CAP_SYS_ADMIN in its namespace or be running with
>> +      * no_new_privs.  This avoids scenarios where unprivileged
>> +      * tasks can affect the behavior of privileged children.
>> +      */
>> +     ret = -EACCES;
>> +     if (!current->no_new_privs &&
>> +         security_capable_noaudit(current_cred(), current_user_ns(),
>> +                                  CAP_SYS_ADMIN) != 0)
>> +             goto out;
>> +
>> +     /*
>> +      * If there is an existing filter, make it the prev
>> +      * and don't drop its task reference.
>> +      */
>> +     filter->prev = current->seccomp.filter;
>> +     current->seccomp.filter = filter;
>> +     return 0;
>> +out:
>> +     put_seccomp_filter(filter);  /* for get or task, on err */
>> +     return ret;
>> +}
>> +
>> +/**
>> + * seccomp_attach_user_filter - attaches a user-supplied sock_fprog
>> + * @user_filter: pointer to the user data containing a sock_fprog.
>> + *
>> + * This function may be called repeatedly to install additional filters.
>> + * Every filter successfully installed will be evaluated (in reverse order)
>> + * for each system call the task makes.
>> + *
>> + * Returns 0 on success and non-zero otherwise.
>> + */
>> +long seccomp_attach_user_filter(char __user *user_filter)
>> +{
>> +     struct sock_fprog fprog;
>> +     long ret = -EFAULT;
>> +
>> +     if (!user_filter)
>> +             goto out;
>> +#ifdef CONFIG_COMPAT
>> +     if (is_compat_task()) {
>> +             /* XXX: Share with net/compat.c (compat_sock_fprog) */
>
> Then do so as part of your BPF sharing patch.

Makes sense. Queuing it up.

>> +             struct {
>> +                     u16 len;
>> +                     compat_uptr_t filter;   /* struct sock_filter */
>> +             } fprog32;
>> +             if (copy_from_user(&fprog32, user_filter, sizeof(fprog32)))
>> +                     goto out;
>> +             fprog.len = fprog32.len;
>> +             fprog.filter = compat_ptr(fprog32.filter);
>> +     } else /* falls through to the if below. */
>> +#endif
>> +     if (copy_from_user(&fprog, user_filter, sizeof(fprog)))
>> +             goto out;
>> +     ret = seccomp_attach_filter(&fprog);
>> +out:
>> +     return ret;
>> +}
>> +
>> +/* get_seccomp_filter - increments the reference count of @orig. */
>> +static struct seccomp_filter *get_seccomp_filter(struct seccomp_filter *orig)
>> +{
>> +     if (!orig)
>> +             return NULL;
>> +     /* Reference count is bounded by the number of total processes. */
>> +     atomic_inc(&orig->usage);
>> +     return orig;
>> +}
>> +
>> +/* put_seccomp_filter - decrements the ref count of @orig and may free. */
>> +void put_seccomp_filter(struct seccomp_filter *orig)
>> +{
>> +     /* Clean up single-reference branches iteratively. */
>> +     while (orig && atomic_dec_and_test(&orig->usage)) {
>> +             struct seccomp_filter *freeme = orig;
>> +             orig = orig->prev;
>> +             kfree(freeme);
>> +     }
>> +}
>> +
>> +/**
>> + * copy_seccomp: manages inheritance on fork
>> + * @child: forkee's seccomp
>> + * @prev: forker's seccomp
>> + *
>> + * Ensures that @child inherits seccomp mode and state if
>> + * seccomp filtering is in use.
>> + */
>> +void copy_seccomp(struct seccomp *child,
>> +               const struct seccomp *prev)
>
> One line please.

Alright :)

>> +{
>> +     child->mode = prev->mode;
>> +     child->filter = get_seccomp_filter(prev->filter);
>> +}
>> +#endif       /* CONFIG_SECCOMP_FILTER */
>>
>> /*
>>  * Secure computing mode 1 allows only read/write/exit/sigreturn.
>> @@ -34,10 +305,10 @@ static int mode1_syscalls_32[] = {
>> void __secure_computing(int this_syscall)
>> {
>>       int mode = current->seccomp.mode;
>> -     int * syscall;
>> +     int *syscall;
>>
>>       switch (mode) {
>> -     case 1:
>> +     case SECCOMP_MODE_STRICT:
>>               syscall = mode1_syscalls;
>> #ifdef CONFIG_COMPAT
>>               if (is_compat_task())
>> @@ -48,6 +319,13 @@ void __secure_computing(int this_syscall)
>>                               return;
>>               } while (*++syscall);
>>               break;
>> +#ifdef CONFIG_SECCOMP_FILTER
>> +     case SECCOMP_MODE_FILTER:
>> +             if (seccomp_run_filters(this_syscall) == SECCOMP_RET_ALLOW)
>> +                     return;
>> +             seccomp_filter_log_failure(this_syscall);
>> +             break;
>> +#endif
>>       default:
>>               BUG();
>>       }
>> @@ -64,25 +342,34 @@ long prctl_get_seccomp(void)
>>       return current->seccomp.mode;
>> }
>>
>> -long prctl_set_seccomp(unsigned long seccomp_mode)
>> +long prctl_set_seccomp(unsigned long seccomp_mode, char __user *filter)
>> {
>> -     long ret;
>> +     long ret = -EINVAL;
>>
>> -     /* can set it only once to be even more secure */
>> -     ret = -EPERM;
>> -     if (unlikely(current->seccomp.mode))
>> +     if (current->seccomp.mode &&
>> +         current->seccomp.mode != seccomp_mode)
>
> Wouldn't it make sense to allow going from mode 2 to 1?
> After all, the filter could have blocked it if it didn't
> want to permit it, and mode 1 is more restrictive than
> mode 2.

Nope - that might allow a downgrade that bypasses write/read
restrictions.  E.g., a filter could only allow a read to a certain buf
or of a certain size.  Allowing a downgrade would allow bypassing
those filters, whether they are the most sane things or not :)

>>               goto out;
>>
>> -     ret = -EINVAL;
>> -     if (seccomp_mode && seccomp_mode <= NR_SECCOMP_MODES) {
>> -             current->seccomp.mode = seccomp_mode;
>> -             set_thread_flag(TIF_SECCOMP);
>> +     switch (seccomp_mode) {
>> +     case SECCOMP_MODE_STRICT:
>> +             ret = 0;
>> #ifdef TIF_NOTSC
>>               disable_TSC();
>> #endif
>> -             ret = 0;
>> +             break;
>> +#ifdef CONFIG_SECCOMP_FILTER
>> +     case SECCOMP_MODE_FILTER:
>> +             ret = seccomp_attach_user_filter(filter);
>> +             if (ret)
>> +                     goto out;
>> +             break;
>> +#endif
>> +     default:
>> +             goto out;
>>       }
>>
>> - out:
>> +     current->seccomp.mode = seccomp_mode;
>> +     set_thread_flag(TIF_SECCOMP);
>> +out:
>>       return ret;
>> }
>> diff --git a/kernel/sys.c b/kernel/sys.c
>> index 4070153..905031e 100644
>> --- a/kernel/sys.c
>> +++ b/kernel/sys.c
>> @@ -1899,7 +1899,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2,
> unsigned long, arg3,
>>                       error = prctl_get_seccomp();
>>                       break;
>>               case PR_SET_SECCOMP:
>> -                     error = prctl_set_seccomp(arg2);
>> +                     error = prctl_set_seccomp(arg2, (char __user *)arg3);
>>                       break;
>>               case PR_GET_TSC:
>>                       error = GET_TSC_CTL(arg2);
>
> Out of curiosity, did you measure the kernel size differences before and
> after these patches? Would be sad if sharing it with the networking code
> didn't reduce the actual kernel size.

Oh yeah - it was a serious reduction.  Initially, seccomp_filter.o
added 8kb by itself.  With the merged seccomp.o, continued code
trimming (as suggested), and all the SECCOMP_RET_* variations, the
total kernel growth is 2972 bytes for the same kernel config.  This is
shared across ~2000 bytes in seccomp.o and ~800 bytes in filter.o.

Thanks!
will
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux