* Re: [PATCH RFC v2 net-next 10/16] bpf: add eBPF verifier
From: Alexei Starovoitov @ 2014-07-24 0:48 UTC (permalink / raw)
To: Kees Cook
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Daniel Borkmann, Chema Gonzalez, Eric Dumazet,
Peter Zijlstra, Arnaldo Carvalho de Melo, Jiri Olsa,
Thomas Gleixner, H. Peter Anvin, Andrew Morton, Linux API,
Network Development, LKML
In-Reply-To: <CAGXu5j+HJ-sSkjE_1+UZXZiL19u0oOA8QSrOmkepWPaMBsv9eg@mail.gmail.com>
On Wed, Jul 23, 2014 at 4:38 PM, Kees Cook <keescook@chromium.org> wrote:
>> +Program that doesn't check return value of map_lookup_elem() before accessing
>> +map element:
>> + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
>> + BPF_ALU64_REG(BPF_MOV, BPF_REG_2, BPF_REG_10),
>> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
>> + BPF_ALU64_IMM(BPF_MOV, BPF_REG_1, 1),
>> + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
>
> Is the expectation that these pointers are direct kernel function
> addresses? It looks like they're indexes in the check_call routine
> below. What specifically were the pointer leaks you'd mentioned?
yes, the pointer returned from map_lookup_elem() is a direct pointer
to map element value. If program prints it, that obviously a leak.
Therefore I'm planning to add 'secure' mode to verifier where such
pointer leaks are detected and rejected. This mode will be on for
any non-root syscall.
>> +#define _(OP) ({ int ret = OP; if (ret < 0) return ret; })
>
> This seems overly terse. :) And the meaning tends to be overloaded
> (this obviously isn't a translatable string, etc). Perhaps call it
> "chk" or "ret_fail"? And I think OP in the body should have ()s around
> it to avoid potential macro expansion silliness.
Sure, I'll wrap OP in ().
you've missed the previous thread about my favorite _ macro:
http://www.spinics.net/lists/netdev/msg288070.html
I think I gave a ton of 'pro' arguments already.
Looks like I have to order a bunch of t-shirts with '#define _()' on
them and give it to everyone on the next conference :)
>> +static const char *const bpf_jmp_string[] = {
>> + "jmp", "==", ">", ">=", "&", "!=", "s>", "s>=", "call", "exit"
>> +};
>
> It seems like these string arrays should have literal initializers
> like reg_type_str does.
yeah. good point. will do.
>> +static int check_reg_arg(struct reg_state *regs, int regno, bool is_src)
>> +{
>
> Since regno is always populated with dst_reg/src_reg (u8 :4 sized),
> shouldn't this be u8 instead of int? (And in check_* below too?) More
why? 'int' type is much friendlier to compiler. u8,u16 is a pain to deal with.
unsigned types in general are much harder for optimizer.
> importantly, regno needs bounds checking. MAX_BPF_REG is 10, but
> dst_reg/src_reg could be up to 15, IIUC.
grr. yes. somehow lost this check in this version. good catch.
>> + } else {
>> + if (regno == BPF_REG_FP)
>> + /* frame pointer is read only */
>
> Why no verbose() call here?
no good reason.will add.
>> + slot = &state->stack[MAX_BPF_STACK + off];
>> + slot->stype = STACK_SPILL;
>> + /* save register state */
>> + slot->type = state->regs[value_regno].type;
>> + slot->imm = state->regs[value_regno].imm;
>> + for (i = 1; i < 8; i++) {
>> + slot = &state->stack[MAX_BPF_STACK + off + i];
>
> off and size need bounds checking here and below.
off and size were checked in check_mem_access().
Here size is 1,2,4,8 and off is within [-MAX_BPF_STACK,0)
so no extra checks needed.
>> +/* check read/write into map element returned by bpf_map_lookup_elem() */
>> +static int check_map_access(struct verifier_env *env, int regno, int off,
>> + int size)
>> +{
>> + struct bpf_map *map;
>> + int map_id = env->cur_state.regs[regno].imm;
>> +
>> + _(get_map_info(env, map_id, &map));
>> +
>> + if (off < 0 || off + size > map->value_size) {
>
> This could be tricked with a negative size, or a giant size, wrapping negative.
nope. cannot. check_map_access() is called from check_mem_access()
where off and size were checked.
>> +static int check_mem_access(struct verifier_env *env, int regno, int off,
>> + int bpf_size, enum bpf_access_type t,
>> + int value_regno)
>> +{
>> + struct verifier_state *state = &env->cur_state;
>> + int size;
>> +
>> + _(size = bpf_size_to_bytes(bpf_size));
>> +
>> + if (off % size != 0) {
>> + verbose("misaligned access off %d size %d\n", off, size);
>> + return -EACCES;
>> + }
>
> I think more off and size checking is needed here.
I don't see the problem. Here it's the main entry into other checks.
alignment check above is a common check for all memory accesses.
All other stricter checks are in check_map_access(), check_stack_*(),
check_ctx_access() that are called from this check_mem_access() func.
Why do you think more checking is needed?
>> +/* when register 'regno' is passed into function that will read 'access_size'
>> + * bytes from that pointer, make sure that it's within stack boundary
>> + * and all elements of stack are initialized
>> + */
>> +static int check_stack_boundary(struct verifier_env *env,
>> + int regno, int access_size)
>> +{
>> + struct verifier_state *state = &env->cur_state;
>> + struct reg_state *regs = state->regs;
>> + int off, i;
>> +
>
> regno bounds checking needed.
nope. check_stack_boundary() is called from check_func_arg()
which is called only with constant regnos: 1,2,3,4,5 to check function
arguments.
> Unless I've overlooked something, I think this needs much stricter
> evaluation of register numbers, offsets, and sizes.
sorry to hear that first glance was disappointing :)
I hope my explanation made it more clear.
The only check that I forgot to carry over the last year is in
check_reg_arg(). Around november last year the verifier patches I keep
posting diverged a little bit from the one we keep running in production,
since eBPF got few instruction renamed, so I had to keep tracking the two.
Once this version gets upstreamed we can finally drop the internal one.
check_reg_arg() is indeed incorrect here. Will fix. That was a good catch.
Thank you for review!
^ permalink raw reply
* Re: [PATCH RFC v2 net-next 13/16] tracing: allow eBPF programs to be attached to events
From: Alexei Starovoitov @ 2014-07-24 0:06 UTC (permalink / raw)
To: Kees Cook
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Daniel Borkmann, Chema Gonzalez, Eric Dumazet,
Peter Zijlstra, Arnaldo Carvalho de Melo, Jiri Olsa,
Thomas Gleixner, H. Peter Anvin, Andrew Morton, Linux API,
Network Development, LKML
In-Reply-To: <CAGXu5jJyw19h0+AwZEWSJt7hGnkRHAsf8fYim=XLzA-LH=svjw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wed, Jul 23, 2014 at 4:46 PM, Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>>
>> eBPF programs can call in-kernel helper functions to:
>> - lookup/update/delete elements in maps
>> - memcmp
>> - trace_printk
>> - load_pointer
>> - dump_stack
>
> Ah, this must be the pointer leaking you mentioned. :)
>
>
> Can the existing tracing mechanisms already expose kernel addresses? I
> suspect "yes". So I guess existing limitations on tracing exposure
> should already cover access control here? (I'm trying to figure out if
> a separate CONFIG is needed -- I don't think so: nothing "new" is
> exposed via eBPF, is that right?)
correct. through debugfs/tracing the whole kernel is already exposed.
Idea of eBPF for tracing is to give kernel developers and performance
engineers a tool to analyze what kernel is doing by writing programs
in C and attaching them to kprobe/tracepoint events, so it's definitely
for root only.
^ permalink raw reply
* Re: [PATCH RFC v2 net-next 13/16] tracing: allow eBPF programs to be attached to events
From: Kees Cook @ 2014-07-23 23:46 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Daniel Borkmann, Chema Gonzalez, Eric Dumazet,
Peter Zijlstra, Arnaldo Carvalho de Melo, Jiri Olsa,
Thomas Gleixner, H. Peter Anvin, Andrew Morton, Linux API,
Network Development, LKML
In-Reply-To: <1405657206-12060-14-git-send-email-ast@plumgrid.com>
On Thu, Jul 17, 2014 at 9:20 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
> User interface:
> fd = open("/sys/kernel/debug/tracing/__event__/filter")
>
> write(fd, "bpf_123")
>
> where 123 is process local FD associated with eBPF program previously loaded.
> __event__ is static tracepoint event.
> (kprobe events will be supported in the future patches)
> Once program is successfully attached to tracepoint event, the tracepoint
> will be auto-enabled
>
> close(fd)
> auto-disables tracepoint event and detaches eBPF program from it
>
> eBPF programs can call in-kernel helper functions to:
> - lookup/update/delete elements in maps
> - memcmp
> - trace_printk
> - load_pointer
> - dump_stack
Ah, this must be the pointer leaking you mentioned. :)
>
> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
> ---
> include/linux/ftrace_event.h | 5 +
> include/trace/bpf_trace.h | 29 +++++
> include/trace/ftrace.h | 10 ++
> include/uapi/linux/bpf.h | 5 +
> kernel/trace/Kconfig | 1 +
> kernel/trace/Makefile | 1 +
> kernel/trace/bpf_trace.c | 212 ++++++++++++++++++++++++++++++++++++
> kernel/trace/trace.h | 3 +
> kernel/trace/trace_events.c | 36 +++++-
> kernel/trace/trace_events_filter.c | 72 +++++++++++-
> 10 files changed, 372 insertions(+), 2 deletions(-)
> create mode 100644 include/trace/bpf_trace.h
> create mode 100644 kernel/trace/bpf_trace.c
>
> diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
> index cff3106ffe2c..de313bd9a434 100644
> --- a/include/linux/ftrace_event.h
> +++ b/include/linux/ftrace_event.h
> @@ -237,6 +237,7 @@ enum {
> TRACE_EVENT_FL_WAS_ENABLED_BIT,
> TRACE_EVENT_FL_USE_CALL_FILTER_BIT,
> TRACE_EVENT_FL_TRACEPOINT_BIT,
> + TRACE_EVENT_FL_BPF_BIT,
> };
>
> /*
> @@ -259,6 +260,7 @@ enum {
> TRACE_EVENT_FL_WAS_ENABLED = (1 << TRACE_EVENT_FL_WAS_ENABLED_BIT),
> TRACE_EVENT_FL_USE_CALL_FILTER = (1 << TRACE_EVENT_FL_USE_CALL_FILTER_BIT),
> TRACE_EVENT_FL_TRACEPOINT = (1 << TRACE_EVENT_FL_TRACEPOINT_BIT),
> + TRACE_EVENT_FL_BPF = (1 << TRACE_EVENT_FL_BPF_BIT),
> };
>
> struct ftrace_event_call {
> @@ -536,6 +538,9 @@ event_trigger_unlock_commit_regs(struct ftrace_event_file *file,
> event_triggers_post_call(file, tt);
> }
>
> +struct bpf_context;
> +void trace_filter_call_bpf(struct event_filter *filter, struct bpf_context *ctx);
> +
> enum {
> FILTER_OTHER = 0,
> FILTER_STATIC_STRING,
> diff --git a/include/trace/bpf_trace.h b/include/trace/bpf_trace.h
> new file mode 100644
> index 000000000000..2122437f1317
> --- /dev/null
> +++ b/include/trace/bpf_trace.h
> @@ -0,0 +1,29 @@
> +/* Copyright (c) 2011-2014 PLUMgrid, http://plumgrid.com
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of version 2 of the GNU General Public
> + * License as published by the Free Software Foundation.
> + */
> +#ifndef _LINUX_KERNEL_BPF_TRACE_H
> +#define _LINUX_KERNEL_BPF_TRACE_H
> +
> +/* For tracing filters save first six arguments of tracepoint events.
> + * On 64-bit architectures argN fields will match one to one to arguments passed
> + * to tracepoint events.
> + * On 32-bit architectures u64 arguments to events will be seen into two
> + * consecutive argN, argN+1 fields. Pointers, u32, u16, u8, bool types will
> + * match one to one
> + */
> +struct bpf_context {
> + unsigned long arg1;
> + unsigned long arg2;
> + unsigned long arg3;
> + unsigned long arg4;
> + unsigned long arg5;
> + unsigned long arg6;
> +};
> +
> +/* call from ftrace_raw_event_*() to copy tracepoint arguments into ctx */
> +void populate_bpf_context(struct bpf_context *ctx, ...);
> +
> +#endif /* _LINUX_KERNEL_BPF_TRACE_H */
> diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
> index 26b4f2e13275..ad4987ac68bb 100644
> --- a/include/trace/ftrace.h
> +++ b/include/trace/ftrace.h
> @@ -17,6 +17,7 @@
> */
>
> #include <linux/ftrace_event.h>
> +#include <trace/bpf_trace.h>
>
> /*
> * DECLARE_EVENT_CLASS can be used to add a generic function
> @@ -634,6 +635,15 @@ ftrace_raw_event_##call(void *__data, proto) \
> if (ftrace_trigger_soft_disabled(ftrace_file)) \
> return; \
> \
> + if (unlikely(ftrace_file->flags & FTRACE_EVENT_FL_FILTERED) && \
> + unlikely(ftrace_file->event_call->flags & TRACE_EVENT_FL_BPF)) { \
> + struct bpf_context __ctx; \
> + \
> + populate_bpf_context(&__ctx, args, 0, 0, 0, 0, 0); \
> + trace_filter_call_bpf(ftrace_file->filter, &__ctx); \
> + return; \
> + } \
> + \
> __data_size = ftrace_get_offsets_##call(&__data_offsets, args); \
> \
> entry = ftrace_event_buffer_reserve(&fbuffer, ftrace_file, \
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 06e0f63055fb..cedcf9a0db53 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -370,6 +370,7 @@ enum bpf_prog_attributes {
> enum bpf_prog_type {
> BPF_PROG_TYPE_UNSPEC,
> BPF_PROG_TYPE_SOCKET_FILTER,
> + BPF_PROG_TYPE_TRACING_FILTER,
> };
>
> /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> @@ -380,6 +381,10 @@ enum bpf_func_id {
> BPF_FUNC_map_lookup_elem, /* void *map_lookup_elem(map_id, void *key) */
> BPF_FUNC_map_update_elem, /* int map_update_elem(map_id, void *key, void *value) */
> BPF_FUNC_map_delete_elem, /* int map_delete_elem(map_id, void *key) */
> + BPF_FUNC_load_pointer, /* void *bpf_load_pointer(void *unsafe_ptr) */
> + BPF_FUNC_memcmp, /* int bpf_memcmp(void *unsafe_ptr, void *safe_ptr, int size) */
> + BPF_FUNC_dump_stack, /* void bpf_dump_stack(void) */
> + BPF_FUNC_printk, /* int bpf_printk(const char *fmt, int fmt_size, ...) */
> __BPF_FUNC_MAX_ID,
> };
>
> diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> index d4409356f40d..e36d42876634 100644
> --- a/kernel/trace/Kconfig
> +++ b/kernel/trace/Kconfig
> @@ -80,6 +80,7 @@ config FTRACE_NMI_ENTER
>
> config EVENT_TRACING
> select CONTEXT_SWITCH_TRACER
> + depends on NET
> bool
>
> config CONTEXT_SWITCH_TRACER
> diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile
> index 2611613f14f1..a0fcfd97101d 100644
> --- a/kernel/trace/Makefile
> +++ b/kernel/trace/Makefile
> @@ -52,6 +52,7 @@ obj-$(CONFIG_EVENT_TRACING) += trace_event_perf.o
> endif
> obj-$(CONFIG_EVENT_TRACING) += trace_events_filter.o
> obj-$(CONFIG_EVENT_TRACING) += trace_events_trigger.o
> +obj-$(CONFIG_EVENT_TRACING) += bpf_trace.o
Can the existing tracing mechanisms already expose kernel addresses? I
suspect "yes". So I guess existing limitations on tracing exposure
should already cover access control here? (I'm trying to figure out if
a separate CONFIG is needed -- I don't think so: nothing "new" is
exposed via eBPF, is that right?)
-Kees
> obj-$(CONFIG_KPROBE_EVENT) += trace_kprobe.o
> obj-$(CONFIG_TRACEPOINTS) += power-traces.o
> ifeq ($(CONFIG_PM_RUNTIME),y)
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> new file mode 100644
> index 000000000000..7263491be792
> --- /dev/null
> +++ b/kernel/trace/bpf_trace.c
> @@ -0,0 +1,212 @@
> +/* Copyright (c) 2011-2014 PLUMgrid, http://plumgrid.com
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of version 2 of the GNU General Public
> + * License as published by the Free Software Foundation.
> + */
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/slab.h>
> +#include <linux/bpf.h>
> +#include <linux/filter.h>
> +#include <linux/uaccess.h>
> +#include <trace/bpf_trace.h>
> +#include "trace.h"
> +
> +/* call from ftrace_raw_event_*() to copy tracepoint arguments into ctx */
> +void populate_bpf_context(struct bpf_context *ctx, ...)
> +{
> + va_list args;
> +
> + va_start(args, ctx);
> +
> + ctx->arg1 = va_arg(args, unsigned long);
> + ctx->arg2 = va_arg(args, unsigned long);
> + ctx->arg3 = va_arg(args, unsigned long);
> + ctx->arg4 = va_arg(args, unsigned long);
> + ctx->arg5 = va_arg(args, unsigned long);
> + ctx->arg6 = va_arg(args, unsigned long);
> +
> + va_end(args);
> +}
> +EXPORT_SYMBOL_GPL(populate_bpf_context);
> +
> +/* called from eBPF program with rcu lock held */
> +static u64 bpf_load_ptr(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5)
> +{
> + void *unsafe_ptr = (void *) r1;
> + void *ptr = NULL;
> +
> + probe_kernel_read(&ptr, unsafe_ptr, sizeof(void *));
> + return (u64) (unsigned long) ptr;
> +}
> +
> +static u64 bpf_memcmp(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5)
> +{
> + void *unsafe_ptr = (void *) r1;
> + void *safe_ptr = (void *) r2;
> + u32 size = (u32) r3;
> + char buf[64];
> + int err;
> +
> + if (size < 64) {
> + err = probe_kernel_read(buf, unsafe_ptr, size);
> + if (err)
> + return err;
> + return memcmp(buf, safe_ptr, size);
> + }
> + return -1;
> +}
> +
> +static u64 bpf_dump_stack(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5)
> +{
> + trace_dump_stack(0);
> + return 0;
> +}
> +
> +/* limited printk()
> + * only %d %u %x conversion specifiers allowed
> + */
> +static u64 bpf_printk(u64 r1, u64 fmt_size, u64 r3, u64 r4, u64 r5)
> +{
> + char *fmt = (char *) r1;
> + int fmt_cnt = 0;
> + int i;
> +
> + /* bpf_check() guarantees that fmt points to bpf program stack and
> + * fmt_size bytes of it were initialized by bpf program
> + */
> + if (fmt[fmt_size - 1] != 0)
> + return -EINVAL;
> +
> + /* check format string for allowed specifiers */
> + for (i = 0; i < fmt_size; i++)
> + if (fmt[i] == '%') {
> + if (i + 1 >= fmt_size)
> + return -EINVAL;
> + if (fmt[i + 1] != 'd' && fmt[i + 1] != 'u' &&
> + fmt[i + 1] != 'x')
> + return -EINVAL;
> + fmt_cnt++;
> + }
> +
> + if (fmt_cnt > 3)
> + return -EINVAL;
> +
> + return __trace_printk((unsigned long) __builtin_return_address(3), fmt,
> + (u32) r3, (u32) r4, (u32) r5);
> +}
> +
> +static struct bpf_func_proto tracing_filter_funcs[] = {
> + [BPF_FUNC_load_pointer] = {
> + .func = bpf_load_ptr,
> + .gpl_only = true,
> + .ret_type = RET_INTEGER,
> + },
> + [BPF_FUNC_memcmp] = {
> + .func = bpf_memcmp,
> + .gpl_only = false,
> + .ret_type = RET_INTEGER,
> + .arg1_type = ARG_ANYTHING,
> + .arg2_type = ARG_PTR_TO_STACK,
> + .arg3_type = ARG_CONST_STACK_SIZE,
> + },
> + [BPF_FUNC_dump_stack] = {
> + .func = bpf_dump_stack,
> + .gpl_only = false,
> + .ret_type = RET_VOID,
> + },
> + [BPF_FUNC_printk] = {
> + .func = bpf_printk,
> + .gpl_only = true,
> + .ret_type = RET_INTEGER,
> + .arg1_type = ARG_PTR_TO_STACK,
> + .arg2_type = ARG_CONST_STACK_SIZE,
> + },
> + [BPF_FUNC_map_lookup_elem] = {
> + .func = bpf_map_lookup_elem,
> + .gpl_only = false,
> + .ret_type = RET_PTR_TO_MAP_OR_NULL,
> + .arg1_type = ARG_CONST_MAP_ID,
> + .arg2_type = ARG_PTR_TO_MAP_KEY,
> + },
> + [BPF_FUNC_map_update_elem] = {
> + .func = bpf_map_update_elem,
> + .gpl_only = false,
> + .ret_type = RET_INTEGER,
> + .arg1_type = ARG_CONST_MAP_ID,
> + .arg2_type = ARG_PTR_TO_MAP_KEY,
> + .arg3_type = ARG_PTR_TO_MAP_VALUE,
> + },
> + [BPF_FUNC_map_delete_elem] = {
> + .func = bpf_map_delete_elem,
> + .gpl_only = false,
> + .ret_type = RET_INTEGER,
> + .arg1_type = ARG_CONST_MAP_ID,
> + .arg2_type = ARG_PTR_TO_MAP_KEY,
> + },
> +};
> +
> +static const struct bpf_func_proto *tracing_filter_func_proto(enum bpf_func_id func_id)
> +{
> + if (func_id < 0 || func_id >= ARRAY_SIZE(tracing_filter_funcs))
> + return NULL;
> + return &tracing_filter_funcs[func_id];
> +}
> +
> +static const struct bpf_context_access {
> + int size;
> + enum bpf_access_type type;
> +} tracing_filter_ctx_access[] = {
> + [offsetof(struct bpf_context, arg1)] = {
> + FIELD_SIZEOF(struct bpf_context, arg1),
> + BPF_READ
> + },
> + [offsetof(struct bpf_context, arg2)] = {
> + FIELD_SIZEOF(struct bpf_context, arg2),
> + BPF_READ
> + },
> + [offsetof(struct bpf_context, arg3)] = {
> + FIELD_SIZEOF(struct bpf_context, arg3),
> + BPF_READ
> + },
> + [offsetof(struct bpf_context, arg4)] = {
> + FIELD_SIZEOF(struct bpf_context, arg4),
> + BPF_READ
> + },
> + [offsetof(struct bpf_context, arg5)] = {
> + FIELD_SIZEOF(struct bpf_context, arg5),
> + BPF_READ
> + },
> +};
> +
> +static bool tracing_filter_is_valid_access(int off, int size, enum bpf_access_type type)
> +{
> + const struct bpf_context_access *access;
> +
> + if (off < 0 || off >= ARRAY_SIZE(tracing_filter_ctx_access))
> + return false;
> +
> + access = &tracing_filter_ctx_access[off];
> + if (access->size == size && (access->type & type))
> + return true;
> +
> + return false;
> +}
> +
> +static struct bpf_verifier_ops tracing_filter_ops = {
> + .get_func_proto = tracing_filter_func_proto,
> + .is_valid_access = tracing_filter_is_valid_access,
> +};
> +
> +static struct bpf_prog_type_list tl = {
> + .ops = &tracing_filter_ops,
> + .type = BPF_PROG_TYPE_TRACING_FILTER,
> +};
> +
> +static int __init register_tracing_filter_ops(void)
> +{
> + bpf_register_prog_type(&tl);
> + return 0;
> +}
> +late_initcall(register_tracing_filter_ops);
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 9258f5a815db..bb7c6a19ead5 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -984,12 +984,15 @@ struct ftrace_event_field {
> int is_signed;
> };
>
> +struct sk_filter;
> +
> struct event_filter {
> int n_preds; /* Number assigned */
> int a_preds; /* allocated */
> struct filter_pred *preds;
> struct filter_pred *root;
> char *filter_string;
> + struct sk_filter *prog;
> };
>
> struct event_subsystem {
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index f99e0b3bca8c..de79c27a0a42 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -1048,6 +1048,26 @@ event_filter_read(struct file *filp, char __user *ubuf, size_t cnt,
> return r;
> }
>
> +static int event_filter_release(struct inode *inode, struct file *filp)
> +{
> + struct ftrace_event_file *file;
> + char buf[2] = "0";
> +
> + mutex_lock(&event_mutex);
> + file = event_file_data(filp);
> + if (file) {
> + if (file->event_call->flags & TRACE_EVENT_FL_BPF) {
> + /* auto-disable the filter */
> + ftrace_event_enable_disable(file, 0);
> +
> + /* if BPF filter was used, clear it on fd close */
> + apply_event_filter(file, buf);
> + }
> + }
> + mutex_unlock(&event_mutex);
> + return 0;
> +}
> +
> static ssize_t
> event_filter_write(struct file *filp, const char __user *ubuf, size_t cnt,
> loff_t *ppos)
> @@ -1071,10 +1091,23 @@ event_filter_write(struct file *filp, const char __user *ubuf, size_t cnt,
>
> mutex_lock(&event_mutex);
> file = event_file_data(filp);
> - if (file)
> + if (file) {
> err = apply_event_filter(file, buf);
> + if (!err && file->event_call->flags & TRACE_EVENT_FL_BPF)
> + /* once filter is applied, auto-enable it */
> + ftrace_event_enable_disable(file, 1);
> + }
> +
> mutex_unlock(&event_mutex);
>
> + if (file && file->event_call->flags & TRACE_EVENT_FL_BPF) {
> + /*
> + * allocate per-cpu printk buffers, since eBPF program
> + * might be calling bpf_trace_printk
> + */
> + trace_printk_init_buffers();
> + }
> +
> free_page((unsigned long) buf);
> if (err < 0)
> return err;
> @@ -1325,6 +1358,7 @@ static const struct file_operations ftrace_event_filter_fops = {
> .open = tracing_open_generic,
> .read = event_filter_read,
> .write = event_filter_write,
> + .release = event_filter_release,
> .llseek = default_llseek,
> };
>
> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index 8a8631926a07..a27526fae0fe 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -23,6 +23,9 @@
> #include <linux/mutex.h>
> #include <linux/perf_event.h>
> #include <linux/slab.h>
> +#include <linux/bpf.h>
> +#include <trace/bpf_trace.h>
> +#include <linux/filter.h>
>
> #include "trace.h"
> #include "trace_output.h"
> @@ -535,6 +538,16 @@ static int filter_match_preds_cb(enum move_type move, struct filter_pred *pred,
> return WALK_PRED_DEFAULT;
> }
>
> +void trace_filter_call_bpf(struct event_filter *filter, struct bpf_context *ctx)
> +{
> + BUG_ON(!filter || !filter->prog);
> +
> + rcu_read_lock();
> + SK_RUN_FILTER(filter->prog, (void *) ctx);
> + rcu_read_unlock();
> +}
> +EXPORT_SYMBOL_GPL(trace_filter_call_bpf);
> +
> /* return 1 if event matches, 0 otherwise (discard) */
> int filter_match_preds(struct event_filter *filter, void *rec)
> {
> @@ -794,6 +807,8 @@ static void __free_filter(struct event_filter *filter)
> if (!filter)
> return;
>
> + if (filter->prog)
> + sk_unattached_filter_destroy(filter->prog);
> __free_preds(filter);
> kfree(filter->filter_string);
> kfree(filter);
> @@ -1898,6 +1913,48 @@ static int create_filter_start(char *filter_str, bool set_str,
> return err;
> }
>
> +static int create_filter_bpf(char *filter_str, struct event_filter **filterp)
> +{
> + struct event_filter *filter;
> + struct sk_filter *prog;
> + long ufd;
> + int err = 0;
> +
> + *filterp = NULL;
> +
> + filter = __alloc_filter();
> + if (!filter)
> + return -ENOMEM;
> +
> + err = replace_filter_string(filter, filter_str);
> + if (err)
> + goto free_filter;
> +
> + err = kstrtol(filter_str + 4, 0, &ufd);
> + if (err)
> + goto free_filter;
> +
> + err = -ESRCH;
> + prog = bpf_prog_get(ufd);
> + if (!prog)
> + goto free_filter;
> +
> + filter->prog = prog;
> +
> + err = -EINVAL;
> + if (prog->info->prog_type != BPF_PROG_TYPE_TRACING_FILTER)
> + /* prog_id is valid, but it's not a tracing filter program */
> + goto free_filter;
> +
> + *filterp = filter;
> +
> + return 0;
> +
> +free_filter:
> + __free_filter(filter);
> + return err;
> +}
> +
> static void create_filter_finish(struct filter_parse_state *ps)
> {
> if (ps) {
> @@ -2007,7 +2064,20 @@ int apply_event_filter(struct ftrace_event_file *file, char *filter_string)
> return 0;
> }
>
> - err = create_filter(call, filter_string, true, &filter);
> + /*
> + * 'bpf_123' string is a request to attach eBPF program with id == 123
> + * also accept 'bpf 123', 'bpf.123', 'bpf-123' variants
> + */
> + if (memcmp(filter_string, "bpf", 3) == 0 && filter_string[3] != 0 &&
> + filter_string[4] != 0) {
> + err = create_filter_bpf(filter_string, &filter);
> + if (!err)
> + call->flags |= TRACE_EVENT_FL_BPF;
> + } else {
> + err = create_filter(call, filter_string, true, &filter);
> + if (!err)
> + call->flags &= ~TRACE_EVENT_FL_BPF;
> + }
>
> /*
> * Always swap the call filter with the new filter
> --
> 1.7.9.5
>
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH RFC v2 net-next 10/16] bpf: add eBPF verifier
From: Kees Cook @ 2014-07-23 23:38 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Daniel Borkmann, Chema Gonzalez, Eric Dumazet,
Peter Zijlstra, Arnaldo Carvalho de Melo, Jiri Olsa,
Thomas Gleixner, H. Peter Anvin, Andrew Morton, Linux API,
Network Development, LKML
In-Reply-To: <1405657206-12060-11-git-send-email-ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
On Thu, Jul 17, 2014 at 9:20 PM, Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org> wrote:
> Safety of eBPF programs is statically determined by the verifier, which detects:
> - loops
> - out of range jumps
> - unreachable instructions
> - invalid instructions
> - uninitialized register access
> - uninitialized stack access
> - misaligned stack access
> - out of range stack access
> - invalid calling convention
>
> It checks that
> - R1-R5 registers statisfy function prototype
> - program terminates
> - BPF_LD_ABS|IND instructions are only used in socket filters
>
> It is configured with:
>
> - bool (*is_valid_access)(int off, int size, enum bpf_access_type type);
> that provides information to the verifer which fields of 'ctx'
> are accessible (remember 'ctx' is the first argument to eBPF program)
>
> - const struct bpf_func_proto *(*get_func_proto)(enum bpf_func_id func_id);
> reports argument types of kernel helper functions that eBPF program
> may call, so that verifier can checks that R1-R5 types match prototype
>
> More details in Documentation/networking/filter.txt
>
> Signed-off-by: Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
> ---
> Documentation/networking/filter.txt | 233 ++++++
> include/linux/bpf.h | 49 ++
> include/uapi/linux/bpf.h | 1 +
> kernel/bpf/Makefile | 2 +-
> kernel/bpf/syscall.c | 2 +-
> kernel/bpf/verifier.c | 1520 +++++++++++++++++++++++++++++++++++
> 6 files changed, 1805 insertions(+), 2 deletions(-)
> create mode 100644 kernel/bpf/verifier.c
>
> diff --git a/Documentation/networking/filter.txt b/Documentation/networking/filter.txt
> index e14e486f69cd..778f763fce10 100644
> --- a/Documentation/networking/filter.txt
> +++ b/Documentation/networking/filter.txt
> @@ -995,6 +995,108 @@ BPF_XADD | BPF_DW | BPF_STX: lock xadd *(u64 *)(dst_reg + off16) += src_reg
> Where size is one of: BPF_B or BPF_H or BPF_W or BPF_DW. Note that 1 and
> 2 byte atomic increments are not supported.
>
> +eBPF verifier
> +-------------
> +The safety of the eBPF program is determined in two steps.
> +
> +First step does DAG check to disallow loops and other CFG validation.
> +In particular it will detect programs that have unreachable instructions.
> +(though classic BPF checker allows them)
> +
> +Second step starts from the first insn and descends all possible paths.
> +It simulates execution of every insn and observes the state change of
> +registers and stack.
> +
> +At the start of the program the register R1 contains a pointer to context
> +and has type PTR_TO_CTX.
> +If verifier sees an insn that does R2=R1, then R2 has now type
> +PTR_TO_CTX as well and can be used on the right hand side of expression.
> +If R1=PTR_TO_CTX and insn is R2=R1+R1, then R2=INVALID_PTR,
> +since addition of two valid pointers makes invalid pointer.
> +
> +If register was never written to, it's not readable:
> + bpf_mov R0 = R2
> + bpf_exit
> +will be rejected, since R2 is unreadable at the start of the program.
> +
> +After kernel function call, R1-R5 are reset to unreadable and
> +R0 has a return type of the function.
> +
> +Since R6-R9 are callee saved, their state is preserved across the call.
> + bpf_mov R6 = 1
> + bpf_call foo
> + bpf_mov R0 = R6
> + bpf_exit
> +is a correct program. If there was R1 instead of R6, it would have
> +been rejected.
> +
> +Classic BPF register X is mapped to eBPF register R7 inside sk_convert_filter(),
> +so that its state is preserved across calls.
> +
> +load/store instructions are allowed only with registers of valid types, which
> +are PTR_TO_CTX, PTR_TO_MAP, PTR_TO_STACK. They are bounds and alignment checked.
> +For example:
> + bpf_mov R1 = 1
> + bpf_mov R2 = 2
> + bpf_xadd *(u32 *)(R1 + 3) += R2
> + bpf_exit
> +will be rejected, since R1 doesn't have a valid pointer type at the time of
> +execution of instruction bpf_xadd.
> +
> +At the start R1 contains pointer to ctx and R1 type is PTR_TO_CTX.
> +ctx is generic. verifier is configured to known what context is for particular
> +class of bpf programs. For example, context == skb (for socket filters) and
> +ctx == seccomp_data for seccomp filters.
> +A callback is used to customize verifier to restrict eBPF program access to only
> +certain fields within ctx structure with specified size and alignment.
> +
> +For example, the following insn:
> + bpf_ld R0 = *(u32 *)(R6 + 8)
> +intends to load a word from address R6 + 8 and store it into R0
> +If R6=PTR_TO_CTX, via is_valid_access() callback the verifier will know
> +that offset 8 of size 4 bytes can be accessed for reading, otherwise
> +the verifier will reject the program.
> +If R6=PTR_TO_STACK, then access should be aligned and be within
> +stack bounds, which are [-MAX_BPF_STACK, 0). In this example offset is 8,
> +so it will fail verification, since it's out of bounds.
> +
> +The verifier will allow eBPF program to read data from stack only after
> +it wrote into it.
> +Classic BPF verifier does similar check with M[0-15] memory slots.
> +For example:
> + bpf_ld R0 = *(u32 *)(R10 - 4)
> + bpf_exit
> +is invalid program.
> +Though R10 is correct read-only register and has type PTR_TO_STACK
> +and R10 - 4 is within stack bounds, there were no stores into that location.
> +
> +Pointer register spill/fill is tracked as well, since four (R6-R9)
> +callee saved registers may not be enough for some programs.
> +
> +Allowed function calls are customized with bpf_verifier_ops->get_func_proto()
> +For example, skb_get_nlattr() function has the following definition:
> + struct bpf_func_proto proto = {RET_INTEGER, PTR_TO_CTX};
> +and eBPF verifier will check that this function is always called with first
> +argument being 'ctx'. In other words R1 must have type PTR_TO_CTX
> +at the time of bpf_call insn.
> +After the call register R0 will be set to readable state, so that
> +program can access it.
> +
> +Function calls is a main mechanism to extend functionality of eBPF programs.
> +Socket filters may let programs to call one set of functions, whereas tracing
> +filters may allow completely different set.
> +
> +If a function made accessible to eBPF program, it needs to be thought through
> +from security point of view. The verifier will guarantee that the function is
> +called with valid arguments.
> +
> +seccomp vs socket filters have different security restrictions for classic BPF.
> +Seccomp solves this by two stage verifier: classic BPF verifier is followed
> +by seccomp verifier. In case of eBPF one configurable verifier is shared for
> +all use cases.
> +
> +See details of eBPF verifier in kernel/bpf/verifier.c
> +
> eBPF maps
> ---------
> 'maps' is a generic storage of different types for sharing data between kernel
> @@ -1064,6 +1166,137 @@ size. It will not let programs pass junk values as 'key' and 'value' to
> bpf_map_*_elem() functions, so these functions (implemented in C inside kernel)
> can safely access the pointers in all cases.
>
> +Understanding eBPF verifier messages
> +------------------------------------
> +
> +The following are few examples of invalid eBPF programs and verifier error
> +messages as seen in the log:
> +
> +Program with unreachable instructions:
> +static struct bpf_insn prog[] = {
> + BPF_EXIT_INSN(),
> + BPF_EXIT_INSN(),
> +};
> +Error:
> + unreachable insn 1
> +
> +Program that reads uninitialized register:
> + BPF_ALU64_REG(BPF_MOV, BPF_REG_0, BPF_REG_2),
> + BPF_EXIT_INSN(),
> +Error:
> + 0: (bf) r0 = r2
> + R2 !read_ok
> +
> +Program that doesn't initialize R0 before exiting:
> + BPF_ALU64_REG(BPF_MOV, BPF_REG_2, BPF_REG_1),
> + BPF_EXIT_INSN(),
> +Error:
> + 0: (bf) r2 = r1
> + 1: (95) exit
> + R0 !read_ok
> +
> +Program that accesses stack out of bounds:
> + BPF_ST_MEM(BPF_DW, BPF_REG_10, 8, 0),
> + BPF_EXIT_INSN(),
> +Error:
> + 0: (7a) *(u64 *)(r10 +8) = 0
> + invalid stack off=8 size=8
> +
> +Program that doesn't initialize stack before passing its address into function:
> + BPF_ALU64_REG(BPF_MOV, BPF_REG_2, BPF_REG_10),
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> + BPF_ALU64_IMM(BPF_MOV, BPF_REG_1, 1),
> + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
> + BPF_EXIT_INSN(),
> +Error:
> + 0: (bf) r2 = r10
> + 1: (07) r2 += -8
> + 2: (b7) r1 = 1
> + 3: (85) call 1
> + invalid indirect read from stack off -8+0 size 8
> +
> +Program that uses invalid map_id=2 while calling to map_lookup_elem() function:
> + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> + BPF_ALU64_REG(BPF_MOV, BPF_REG_2, BPF_REG_10),
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> + BPF_ALU64_IMM(BPF_MOV, BPF_REG_1, 2),
> + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
> + BPF_EXIT_INSN(),
> +Error:
> + 0: (7a) *(u64 *)(r10 -8) = 0
> + 1: (bf) r2 = r10
> + 2: (07) r2 += -8
> + 3: (b7) r1 = 2
> + 4: (85) call 1
> + invalid access to map_id=2
> +
> +Program that doesn't check return value of map_lookup_elem() before accessing
> +map element:
> + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> + BPF_ALU64_REG(BPF_MOV, BPF_REG_2, BPF_REG_10),
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> + BPF_ALU64_IMM(BPF_MOV, BPF_REG_1, 1),
> + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
Is the expectation that these pointers are direct kernel function
addresses? It looks like they're indexes in the check_call routine
below. What specifically were the pointer leaks you'd mentioned?
> + BPF_ST_MEM(BPF_DW, BPF_REG_0, 0, 0),
> + BPF_EXIT_INSN(),
> +Error:
> + 0: (7a) *(u64 *)(r10 -8) = 0
> + 1: (bf) r2 = r10
> + 2: (07) r2 += -8
> + 3: (b7) r1 = 1
> + 4: (85) call 1
> + 5: (7a) *(u64 *)(r0 +0) = 0
> + R0 invalid mem access 'map_value_or_null'
> +
> +Program that correctly checks map_lookup_elem() returned value for NULL, but
> +accesses the memory with incorrect alignment:
> + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> + BPF_ALU64_REG(BPF_MOV, BPF_REG_2, BPF_REG_10),
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> + BPF_ALU64_IMM(BPF_MOV, BPF_REG_1, 1),
> + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
> + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 1),
> + BPF_ST_MEM(BPF_DW, BPF_REG_0, 4, 0),
> + BPF_EXIT_INSN(),
> +Error:
> + 0: (7a) *(u64 *)(r10 -8) = 0
> + 1: (bf) r2 = r10
> + 2: (07) r2 += -8
> + 3: (b7) r1 = 1
> + 4: (85) call 1
> + 5: (15) if r0 == 0x0 goto pc+1
> + R0=map_value1 R10=fp
> + 6: (7a) *(u64 *)(r0 +4) = 0
> + misaligned access off 4 size 8
> +
> +Program that correctly checks map_lookup_elem() returned value for NULL and
> +accesses memory with correct alignment in one side of 'if' branch, but fails
> +to do so in the other side of 'if' branch:
> + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> + BPF_ALU64_REG(BPF_MOV, BPF_REG_2, BPF_REG_10),
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> + BPF_ALU64_IMM(BPF_MOV, BPF_REG_1, 1),
> + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
> + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2),
> + BPF_ST_MEM(BPF_DW, BPF_REG_0, 0, 0),
> + BPF_EXIT_INSN(),
> + BPF_ST_MEM(BPF_DW, BPF_REG_0, 0, 1),
> + BPF_EXIT_INSN(),
> +Error:
> + 0: (7a) *(u64 *)(r10 -8) = 0
> + 1: (bf) r2 = r10
> + 2: (07) r2 += -8
> + 3: (b7) r1 = 1
> + 4: (85) call 1
> + 5: (15) if r0 == 0x0 goto pc+2
> + R0=map_value1 R10=fp
> + 6: (7a) *(u64 *)(r0 +0) = 0
> + 7: (95) exit
> +
> + from 5 to 8: R0=imm0 R10=fp
> + 8: (7a) *(u64 *)(r0 +0) = 1
> + R0 invalid mem access 'imm'
> +
> Testing
> -------
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 4967619595cc..b5e90efddfcf 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -46,6 +46,31 @@ struct bpf_map_type_list {
> void bpf_register_map_type(struct bpf_map_type_list *tl);
> struct bpf_map *bpf_map_get(u32 map_id);
>
> +/* function argument constraints */
> +enum bpf_arg_type {
> + ARG_ANYTHING = 0, /* any argument is ok */
> +
> + /* the following constraints used to prototype
> + * bpf_map_lookup/update/delete_elem() functions
> + */
> + ARG_CONST_MAP_ID, /* int const argument used as map_id */
> + ARG_PTR_TO_MAP_KEY, /* pointer to stack used as map key */
> + ARG_PTR_TO_MAP_VALUE, /* pointer to stack used as map value */
> +
> + /* the following constraints used to prototype bpf_memcmp() and other
> + * functions that access data on eBPF program stack
> + */
> + ARG_PTR_TO_STACK, /* any pointer to eBPF program stack */
> + ARG_CONST_STACK_SIZE, /* number of bytes accessed from stack */
> +};
> +
> +/* type of values returned from helper functions */
> +enum bpf_return_type {
> + RET_INTEGER, /* function returns integer */
> + RET_VOID, /* function doesn't return anything */
> + RET_PTR_TO_MAP_OR_NULL, /* function returns a pointer to map elem value or NULL */
> +};
> +
> /* eBPF function prototype used by verifier to allow BPF_CALLs from eBPF programs
> * to in-kernel helper functions and for adjusting imm32 field in BPF_CALL
> * instructions after verifying
> @@ -53,11 +78,33 @@ struct bpf_map *bpf_map_get(u32 map_id);
> struct bpf_func_proto {
> u64 (*func)(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5);
> bool gpl_only;
> + enum bpf_return_type ret_type;
> + enum bpf_arg_type arg1_type;
> + enum bpf_arg_type arg2_type;
> + enum bpf_arg_type arg3_type;
> + enum bpf_arg_type arg4_type;
> + enum bpf_arg_type arg5_type;
> +};
> +
> +/* bpf_context is intentionally undefined structure. Pointer to bpf_context is
> + * the first argument to eBPF programs.
> + * For socket filters: 'struct bpf_context *' == 'struct sk_buff *'
> + */
> +struct bpf_context;
> +
> +enum bpf_access_type {
> + BPF_READ = 1,
> + BPF_WRITE = 2
> };
>
> struct bpf_verifier_ops {
> /* return eBPF function prototype for verification */
> const struct bpf_func_proto *(*get_func_proto)(enum bpf_func_id func_id);
> +
> + /* return true if 'size' wide access at offset 'off' within bpf_context
> + * with 'type' (read or write) is allowed
> + */
> + bool (*is_valid_access)(int off, int size, enum bpf_access_type type);
> };
>
> struct bpf_prog_type_list {
> @@ -78,5 +125,7 @@ struct bpf_prog_info {
>
> void free_bpf_prog_info(struct bpf_prog_info *info);
> struct sk_filter *bpf_prog_get(u32 ufd);
> +/* verify correctness of eBPF program */
> +int bpf_check(struct sk_filter *fp);
>
> #endif /* _LINUX_BPF_H */
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 06ba71b49f64..3f288e1d08f1 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -369,6 +369,7 @@ enum bpf_prog_attributes {
>
> enum bpf_prog_type {
> BPF_PROG_TYPE_UNSPEC,
> + BPF_PROG_TYPE_SOCKET_FILTER,
> };
>
> /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
> index 558e12712ebc..95a9035e0f29 100644
> --- a/kernel/bpf/Makefile
> +++ b/kernel/bpf/Makefile
> @@ -1 +1 @@
> -obj-y := core.o syscall.o hashtab.o
> +obj-y := core.o syscall.o hashtab.o verifier.o
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 9e45ca6b6937..9d441f17548e 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -634,7 +634,7 @@ static int bpf_prog_load(enum bpf_prog_type type, struct nlattr __user *uattr,
> mutex_lock(&bpf_map_lock);
>
> /* run eBPF verifier */
> - /* err = bpf_check(prog); */
> + err = bpf_check(prog);
>
> if (err == 0 && prog->info->used_maps) {
> /* program passed verifier and it's using some maps,
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> new file mode 100644
> index 000000000000..0fce771632b4
> --- /dev/null
> +++ b/kernel/bpf/verifier.c
> @@ -0,0 +1,1520 @@
> +/* Copyright (c) 2011-2014 PLUMgrid, http://plumgrid.com
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of version 2 of the GNU General Public
> + * License as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + */
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/slab.h>
> +#include <linux/bpf.h>
> +#include <linux/filter.h>
> +#include <linux/capability.h>
> +
> +/* bpf_check() is a static code analyzer that walks eBPF program
> + * instruction by instruction and updates register/stack state.
> + * All paths of conditional branches are analyzed until 'bpf_exit' insn.
> + *
> + * At the first pass depth-first-search verifies that the BPF program is a DAG.
> + * It rejects the following programs:
> + * - larger than BPF_MAXINSNS insns
> + * - if loop is present (detected via back-edge)
> + * - unreachable insns exist (shouldn't be a forest. program = one function)
> + * - out of bounds or malformed jumps
> + * The second pass is all possible path descent from the 1st insn.
> + * Conditional branch target insns keep a link list of verifier states.
> + * If the state already visited, this path can be pruned.
> + * If it wasn't a DAG, such state prunning would be incorrect, since it would
> + * skip cycles. Since it's analyzing all pathes through the program,
> + * the length of the analysis is limited to 32k insn, which may be hit even
> + * if insn_cnt < 4K, but there are too many branches that change stack/regs.
> + * Number of 'branches to be analyzed' is limited to 1k
> + *
> + * On entry to each instruction, each register has a type, and the instruction
> + * changes the types of the registers depending on instruction semantics.
> + * If instruction is BPF_MOV64_REG(BPF_REG_1, BPF_REG_5), then type of R5 is
> + * copied to R1.
> + *
> + * All registers are 64-bit (even on 32-bit arch)
> + * R0 - return register
> + * R1-R5 argument passing registers
> + * R6-R9 callee saved registers
> + * R10 - frame pointer read-only
> + *
> + * At the start of BPF program the register R1 contains a pointer to bpf_context
> + * and has type PTR_TO_CTX.
> + *
> + * Most of the time the registers have UNKNOWN_VALUE type, which
> + * means the register has some value, but it's not a valid pointer.
> + * Verifier doesn't attemp to track all arithmetic operations on pointers.
> + * The only special case is the sequence:
> + * BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
> + * BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -20),
> + * 1st insn copies R10 (which has FRAME_PTR) type into R1
> + * and 2nd arithmetic instruction is pattern matched to recognize
> + * that it wants to construct a pointer to some element within stack.
> + * So after 2nd insn, the register R1 has type PTR_TO_STACK
> + * (and -20 constant is saved for further stack bounds checking).
> + * Meaning that this reg is a pointer to stack plus known immediate constant.
> + *
> + * When program is doing load or store insns the type of base register can be:
> + * PTR_TO_MAP, PTR_TO_CTX, FRAME_PTR. These are three pointer types recognized
> + * by check_mem_access() function.
> + *
> + * PTR_TO_MAP means that this register is pointing to 'map element value'
> + * and the range of [ptr, ptr + map's value_size) is accessible.
> + *
> + * registers used to pass pointers to function calls are verified against
> + * function prototypes
> + *
> + * ARG_PTR_TO_MAP_KEY is a function argument constraint.
> + * It means that the register type passed to this function must be
> + * PTR_TO_STACK and it will be used inside the function as
> + * 'pointer to map element key'
> + *
> + * For example the argument constraints for bpf_map_lookup_elem():
> + * .ret_type = RET_PTR_TO_MAP_OR_NULL,
> + * .arg1_type = ARG_CONST_MAP_ID,
> + * .arg2_type = ARG_PTR_TO_MAP_KEY,
> + *
> + * ret_type says that this function returns 'pointer to map elem value or null'
> + * 1st argument is a 'const immediate' value which must be one of valid map_ids.
> + * 2nd argument is a pointer to stack, which will be used inside the function as
> + * a pointer to map element key.
> + *
> + * On the kernel side the helper function looks like:
> + * u64 bpf_map_lookup_elem(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5)
> + * {
> + * struct bpf_map *map;
> + * int map_id = r1;
> + * void *key = (void *) (unsigned long) r2;
> + * void *value;
> + *
> + * here kernel can access 'key' pointer safely, knowing that
> + * [key, key + map->key_size) bytes are valid and were initialized on
> + * the stack of eBPF program.
> + * }
> + *
> + * Corresponding eBPF program looked like:
> + * BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), // after this insn R2 type is FRAME_PTR
> + * BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -4), // after this insn R2 type is PTR_TO_STACK
> + * BPF_MOV64_IMM(BPF_REG_1, MAP_ID), // after this insn R1 type is CONST_ARG
> + * BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
> + * here verifier looks a prototype of map_lookup_elem and sees:
> + * .arg1_type == ARG_CONST_MAP_ID and R1->type == CONST_ARG, which is ok so far,
> + * then it goes and finds a map with map_id equal to R1->imm value.
> + * Now verifier knows that this map has key of key_size bytes
> + *
> + * Then .arg2_type == ARG_PTR_TO_MAP_KEY and R2->type == PTR_TO_STACK, ok so far,
> + * Now verifier checks that [R2, R2 + map's key_size) are within stack limits
> + * and were initialized prior to this call.
> + * If it's ok, then verifier allows this BPF_CALL insn and looks at
> + * .ret_type which is RET_PTR_TO_MAP_OR_NULL, so it sets
> + * R0->type = PTR_TO_MAP_OR_NULL which means bpf_map_lookup_elem() function
> + * returns ether pointer to map value or NULL.
> + *
> + * When type PTR_TO_MAP_OR_NULL passes through 'if (reg != 0) goto +off' insn,
> + * the register holding that pointer in the true branch changes state to
> + * PTR_TO_MAP and the same register changes state to CONST_IMM in the false
> + * branch. See check_cond_jmp_op().
> + *
> + * After the call R0 is set to return type of the function and registers R1-R5
> + * are set to NOT_INIT to indicate that they are no longer readable.
> + *
> + * load/store alignment is checked:
> + * BPF_STX_MEM(BPF_DW, dest_reg, src_reg, 3)
> + * is rejected, because it's misaligned
> + *
> + * load/store to stack are bounds checked and register spill is tracked
> + * BPF_STX_MEM(BPF_B, BPF_REG_10, src_reg, 0)
> + * is rejected, because it's out of bounds
> + *
> + * load/store to map are bounds checked:
> + * BPF_STX_MEM(BPF_H, dest_reg, src_reg, 8)
> + * is ok, if dest_reg->type == PTR_TO_MAP and
> + * 8 + sizeof(u16) <= map_info->value_size
> + *
> + * load/store to bpf_context are checked against known fields
> + */
> +
> +#define _(OP) ({ int ret = OP; if (ret < 0) return ret; })
This seems overly terse. :) And the meaning tends to be overloaded
(this obviously isn't a translatable string, etc). Perhaps call it
"chk" or "ret_fail"? And I think OP in the body should have ()s around
it to avoid potential macro expansion silliness.
> +
> +/* types of values stored in eBPF registers */
> +enum bpf_reg_type {
> + NOT_INIT = 0, /* nothing was written into register */
> + UNKNOWN_VALUE, /* reg doesn't contain a valid pointer */
> + PTR_TO_CTX, /* reg points to bpf_context */
> + PTR_TO_MAP, /* reg points to map element value */
> + PTR_TO_MAP_OR_NULL, /* points to map element value or NULL */
> + FRAME_PTR, /* reg == frame_pointer */
> + PTR_TO_STACK, /* reg == frame_pointer + imm */
> + CONST_IMM, /* constant integer value */
> +};
> +
> +struct reg_state {
> + enum bpf_reg_type type;
> + int imm;
> +};
> +
> +enum bpf_stack_slot_type {
> + STACK_INVALID, /* nothing was stored in this stack slot */
> + STACK_SPILL, /* 1st byte of register spilled into stack */
> + STACK_SPILL_PART, /* other 7 bytes of register spill */
> + STACK_MISC /* BPF program wrote some data into this slot */
> +};
> +
> +struct bpf_stack_slot {
> + enum bpf_stack_slot_type stype;
> + enum bpf_reg_type type;
> + int imm;
> +};
> +
> +/* state of the program:
> + * type of all registers and stack info
> + */
> +struct verifier_state {
> + struct reg_state regs[MAX_BPF_REG];
> + struct bpf_stack_slot stack[MAX_BPF_STACK];
> +};
> +
> +/* linked list of verifier states used to prune search */
> +struct verifier_state_list {
> + struct verifier_state state;
> + struct verifier_state_list *next;
> +};
> +
> +/* verifier_state + insn_idx are pushed to stack when branch is encountered */
> +struct verifier_stack_elem {
> + /* verifer state is 'st'
> + * before processing instruction 'insn_idx'
> + * and after processing instruction 'prev_insn_idx'
> + */
> + struct verifier_state st;
> + int insn_idx;
> + int prev_insn_idx;
> + struct verifier_stack_elem *next;
> +};
> +
> +#define MAX_USED_MAPS 64 /* max number of maps accessed by one eBPF program */
> +
> +/* single container for all structs
> + * one verifier_env per bpf_check() call
> + */
> +struct verifier_env {
> + struct sk_filter *prog; /* eBPF program being verified */
> + struct verifier_stack_elem *head; /* stack of verifier states to be processed */
> + int stack_size; /* number of states to be processed */
> + struct verifier_state cur_state; /* current verifier state */
> + struct verifier_state_list **branch_landing; /* search prunning optimization */
> + u32 used_maps[MAX_USED_MAPS]; /* array of map_id's used by eBPF program */
> + u32 used_map_cnt; /* number of used maps */
> +};
> +
> +/* verbose verifier prints what it's seeing
> + * bpf_check() is called under map lock, so no race to access this global var
> + */
> +static bool verbose_on;
> +
> +/* when verifier rejects eBPF program, it does a second path with verbose on
> + * to dump the verification trace to the log, so the user can figure out what's
> + * wrong with the program
> + */
> +static int verbose(const char *fmt, ...)
> +{
> + va_list args;
> + int ret;
> +
> + if (!verbose_on)
> + return 0;
> +
> + va_start(args, fmt);
> + ret = vprintk(fmt, args);
> + va_end(args);
> + return ret;
> +}
> +
> +/* string representation of 'enum bpf_reg_type' */
> +static const char * const reg_type_str[] = {
> + [NOT_INIT] = "?",
> + [UNKNOWN_VALUE] = "inv",
> + [PTR_TO_CTX] = "ctx",
> + [PTR_TO_MAP] = "map_value",
> + [PTR_TO_MAP_OR_NULL] = "map_value_or_null",
> + [FRAME_PTR] = "fp",
> + [PTR_TO_STACK] = "fp",
> + [CONST_IMM] = "imm",
> +};
> +
> +static void pr_cont_verifier_state(struct verifier_env *env)
> +{
> + enum bpf_reg_type t;
> + int i;
> +
> + for (i = 0; i < MAX_BPF_REG; i++) {
> + t = env->cur_state.regs[i].type;
> + if (t == NOT_INIT)
> + continue;
> + pr_cont(" R%d=%s", i, reg_type_str[t]);
> + if (t == CONST_IMM ||
> + t == PTR_TO_STACK ||
> + t == PTR_TO_MAP_OR_NULL ||
> + t == PTR_TO_MAP)
> + pr_cont("%d", env->cur_state.regs[i].imm);
> + }
> + for (i = 0; i < MAX_BPF_STACK; i++) {
> + if (env->cur_state.stack[i].stype == STACK_SPILL)
> + pr_cont(" fp%d=%s", -MAX_BPF_STACK + i,
> + reg_type_str[env->cur_state.stack[i].type]);
> + }
> + pr_cont("\n");
> +}
> +
> +static const char *const bpf_class_string[] = {
> + "ld", "ldx", "st", "stx", "alu", "jmp", "BUG", "alu64"
> +};
> +
> +static const char *const bpf_alu_string[] = {
> + "+=", "-=", "*=", "/=", "|=", "&=", "<<=", ">>=", "neg",
> + "%=", "^=", "=", "s>>=", "endian", "BUG", "BUG"
> +};
> +
> +static const char *const bpf_ldst_string[] = {
> + "u32", "u16", "u8", "u64"
> +};
> +
> +static const char *const bpf_jmp_string[] = {
> + "jmp", "==", ">", ">=", "&", "!=", "s>", "s>=", "call", "exit"
> +};
It seems like these string arrays should have literal initializers
like reg_type_str does.
> +
> +static void pr_cont_bpf_insn(struct bpf_insn *insn)
> +{
> + u8 class = BPF_CLASS(insn->code);
> +
> + if (class == BPF_ALU || class == BPF_ALU64) {
> + if (BPF_SRC(insn->code) == BPF_X)
> + pr_cont("(%02x) %sr%d %s %sr%d\n",
> + insn->code, class == BPF_ALU ? "(u32) " : "",
> + insn->dst_reg,
> + bpf_alu_string[BPF_OP(insn->code) >> 4],
> + class == BPF_ALU ? "(u32) " : "",
> + insn->src_reg);
> + else
> + pr_cont("(%02x) %sr%d %s %s%d\n",
> + insn->code, class == BPF_ALU ? "(u32) " : "",
> + insn->dst_reg,
> + bpf_alu_string[BPF_OP(insn->code) >> 4],
> + class == BPF_ALU ? "(u32) " : "",
> + insn->imm);
> + } else if (class == BPF_STX) {
> + if (BPF_MODE(insn->code) == BPF_MEM)
> + pr_cont("(%02x) *(%s *)(r%d %+d) = r%d\n",
> + insn->code,
> + bpf_ldst_string[BPF_SIZE(insn->code) >> 3],
> + insn->dst_reg,
> + insn->off, insn->src_reg);
> + else if (BPF_MODE(insn->code) == BPF_XADD)
> + pr_cont("(%02x) lock *(%s *)(r%d %+d) += r%d\n",
> + insn->code,
> + bpf_ldst_string[BPF_SIZE(insn->code) >> 3],
> + insn->dst_reg, insn->off,
> + insn->src_reg);
> + else
> + pr_cont("BUG_%02x\n", insn->code);
As an optimization, would this be more readable by having BPF_SIZE >>
3 and BPF_OP >> 4 pre-loaded in some local variables?
> + } else if (class == BPF_ST) {
> + if (BPF_MODE(insn->code) != BPF_MEM) {
> + pr_cont("BUG_st_%02x\n", insn->code);
> + return;
> + }
> + pr_cont("(%02x) *(%s *)(r%d %+d) = %d\n",
> + insn->code,
> + bpf_ldst_string[BPF_SIZE(insn->code) >> 3],
> + insn->dst_reg,
> + insn->off, insn->imm);
> + } else if (class == BPF_LDX) {
> + if (BPF_MODE(insn->code) != BPF_MEM) {
> + pr_cont("BUG_ldx_%02x\n", insn->code);
> + return;
> + }
> + pr_cont("(%02x) r%d = *(%s *)(r%d %+d)\n",
> + insn->code, insn->dst_reg,
> + bpf_ldst_string[BPF_SIZE(insn->code) >> 3],
> + insn->src_reg, insn->off);
> + } else if (class == BPF_LD) {
> + if (BPF_MODE(insn->code) == BPF_ABS) {
> + pr_cont("(%02x) r0 = *(%s *)skb[%d]\n",
> + insn->code,
> + bpf_ldst_string[BPF_SIZE(insn->code) >> 3],
> + insn->imm);
> + } else if (BPF_MODE(insn->code) == BPF_IND) {
> + pr_cont("(%02x) r0 = *(%s *)skb[r%d + %d]\n",
> + insn->code,
> + bpf_ldst_string[BPF_SIZE(insn->code) >> 3],
> + insn->src_reg, insn->imm);
> + } else {
> + pr_cont("BUG_ld_%02x\n", insn->code);
> + return;
> + }
> + } else if (class == BPF_JMP) {
> + u8 opcode = BPF_OP(insn->code);
> +
> + if (opcode == BPF_CALL) {
> + pr_cont("(%02x) call %d\n", insn->code, insn->imm);
> + } else if (insn->code == (BPF_JMP | BPF_JA)) {
> + pr_cont("(%02x) goto pc%+d\n",
> + insn->code, insn->off);
> + } else if (insn->code == (BPF_JMP | BPF_EXIT)) {
> + pr_cont("(%02x) exit\n", insn->code);
> + } else if (BPF_SRC(insn->code) == BPF_X) {
> + pr_cont("(%02x) if r%d %s r%d goto pc%+d\n",
> + insn->code, insn->dst_reg,
> + bpf_jmp_string[BPF_OP(insn->code) >> 4],
> + insn->src_reg, insn->off);
> + } else {
> + pr_cont("(%02x) if r%d %s 0x%x goto pc%+d\n",
> + insn->code, insn->dst_reg,
> + bpf_jmp_string[BPF_OP(insn->code) >> 4],
> + insn->imm, insn->off);
> + }
> + } else {
> + pr_cont("(%02x) %s\n", insn->code, bpf_class_string[class]);
> + }
> +}
> +
> +static int pop_stack(struct verifier_env *env, int *prev_insn_idx)
> +{
> + struct verifier_stack_elem *elem;
> + int insn_idx;
> +
> + if (env->head == NULL)
> + return -1;
> +
> + memcpy(&env->cur_state, &env->head->st, sizeof(env->cur_state));
> + insn_idx = env->head->insn_idx;
> + if (prev_insn_idx)
> + *prev_insn_idx = env->head->prev_insn_idx;
> + elem = env->head->next;
> + kfree(env->head);
> + env->head = elem;
> + env->stack_size--;
> + return insn_idx;
> +}
> +
> +static struct verifier_state *push_stack(struct verifier_env *env, int insn_idx,
> + int prev_insn_idx)
> +{
> + struct verifier_stack_elem *elem;
> +
> + elem = kmalloc(sizeof(struct verifier_stack_elem), GFP_KERNEL);
> + if (!elem)
> + goto err;
> +
> + memcpy(&elem->st, &env->cur_state, sizeof(env->cur_state));
> + elem->insn_idx = insn_idx;
> + elem->prev_insn_idx = prev_insn_idx;
> + elem->next = env->head;
> + env->head = elem;
> + env->stack_size++;
> + if (env->stack_size > 1024) {
> + verbose("BPF program is too complex\n");
> + goto err;
> + }
> + return &elem->st;
> +err:
> + /* pop all elements and return */
> + while (pop_stack(env, NULL) >= 0);
> + return NULL;
> +}
> +
> +#define CALLER_SAVED_REGS 6
> +static const int caller_saved[CALLER_SAVED_REGS] = {
> + BPF_REG_0, BPF_REG_1, BPF_REG_2, BPF_REG_3, BPF_REG_4, BPF_REG_5
> +};
> +
> +static void init_reg_state(struct reg_state *regs)
> +{
> + int i;
> +
> + for (i = 0; i < MAX_BPF_REG; i++) {
> + regs[i].type = NOT_INIT;
> + regs[i].imm = 0;
> + }
> +
> + /* frame pointer */
> + regs[BPF_REG_FP].type = FRAME_PTR;
> +
> + /* 1st arg to a function */
> + regs[BPF_REG_1].type = PTR_TO_CTX;
> +}
> +
> +static void mark_reg_unknown_value(struct reg_state *regs, int regno)
> +{
> + regs[regno].type = UNKNOWN_VALUE;
> + regs[regno].imm = 0;
> +}
> +
> +static int check_reg_arg(struct reg_state *regs, int regno, bool is_src)
> +{
Since regno is always populated with dst_reg/src_reg (u8 :4 sized),
shouldn't this be u8 instead of int? (And in check_* below too?) More
importantly, regno needs bounds checking. MAX_BPF_REG is 10, but
dst_reg/src_reg could be up to 15, IIUC.
> + if (is_src) {
> + if (regs[regno].type == NOT_INIT) {
> + verbose("R%d !read_ok\n", regno);
> + return -EACCES;
> + }
> + } else {
> + if (regno == BPF_REG_FP)
> + /* frame pointer is read only */
Why no verbose() call here?
> + return -EACCES;
> + mark_reg_unknown_value(regs, regno);
> + }
> + return 0;
> +}
> +
> +static int bpf_size_to_bytes(int bpf_size)
> +{
> + if (bpf_size == BPF_W)
> + return 4;
> + else if (bpf_size == BPF_H)
> + return 2;
> + else if (bpf_size == BPF_B)
> + return 1;
> + else if (bpf_size == BPF_DW)
> + return 8;
> + else
> + return -EACCES;
> +}
> +
> +static int check_stack_write(struct verifier_state *state, int off, int size,
> + int value_regno)
> +{
> + struct bpf_stack_slot *slot;
> + int i;
> +
> + if (value_regno >= 0 &&
> + (state->regs[value_regno].type == PTR_TO_MAP ||
> + state->regs[value_regno].type == PTR_TO_STACK ||
> + state->regs[value_regno].type == PTR_TO_CTX)) {
> +
> + /* register containing pointer is being spilled into stack */
> + if (size != 8) {
> + verbose("invalid size of register spill\n");
> + return -EACCES;
> + }
> +
> + slot = &state->stack[MAX_BPF_STACK + off];
> + slot->stype = STACK_SPILL;
> + /* save register state */
> + slot->type = state->regs[value_regno].type;
> + slot->imm = state->regs[value_regno].imm;
> + for (i = 1; i < 8; i++) {
> + slot = &state->stack[MAX_BPF_STACK + off + i];
off and size need bounds checking here and below.
> + slot->stype = STACK_SPILL_PART;
> + slot->type = UNKNOWN_VALUE;
> + slot->imm = 0;
> + }
> + } else {
> +
> + /* regular write of data into stack */
> + for (i = 0; i < size; i++) {
> + slot = &state->stack[MAX_BPF_STACK + off + i];
> + slot->stype = STACK_MISC;
> + slot->type = UNKNOWN_VALUE;
> + slot->imm = 0;
> + }
> + }
> + return 0;
> +}
> +
> +static int check_stack_read(struct verifier_state *state, int off, int size,
> + int value_regno)
> +{
> + int i;
> + struct bpf_stack_slot *slot;
> +
> + slot = &state->stack[MAX_BPF_STACK + off];
> +
> + if (slot->stype == STACK_SPILL) {
> + if (size != 8) {
> + verbose("invalid size of register spill\n");
> + return -EACCES;
> + }
> + for (i = 1; i < 8; i++) {
> + if (state->stack[MAX_BPF_STACK + off + i].stype !=
> + STACK_SPILL_PART) {
> + verbose("corrupted spill memory\n");
> + return -EACCES;
> + }
> + }
> +
> + /* restore register state from stack */
> + state->regs[value_regno].type = slot->type;
> + state->regs[value_regno].imm = slot->imm;
> + return 0;
> + } else {
> + for (i = 0; i < size; i++) {
> + if (state->stack[MAX_BPF_STACK + off + i].stype !=
> + STACK_MISC) {
> + verbose("invalid read from stack off %d+%d size %d\n",
> + off, i, size);
> + return -EACCES;
> + }
> + }
> + /* have read misc data from the stack */
> + mark_reg_unknown_value(state->regs, value_regno);
> + return 0;
> + }
> +}
> +
> +static int remember_map_id(struct verifier_env *env, u32 map_id)
> +{
> + int i;
> +
> + /* check whether we recorded this map_id already */
> + for (i = 0; i < env->used_map_cnt; i++)
> + if (env->used_maps[i] == map_id)
> + return 0;
> +
> + if (env->used_map_cnt >= MAX_USED_MAPS)
> + return -E2BIG;
> +
> + /* remember this map_id */
> + env->used_maps[env->used_map_cnt++] = map_id;
> + return 0;
> +}
> +
> +static int get_map_info(struct verifier_env *env, u32 map_id,
> + struct bpf_map **map)
> +{
> + /* if BPF program contains bpf_map_lookup_elem(map_id, key)
> + * the incorrect map_id will be caught here
> + */
> + *map = bpf_map_get(map_id);
> + if (!*map) {
> + verbose("invalid access to map_id=%d\n", map_id);
> + return -EACCES;
> + }
> +
> + _(remember_map_id(env, map_id));
> +
> + return 0;
> +}
> +
> +/* check read/write into map element returned by bpf_map_lookup_elem() */
> +static int check_map_access(struct verifier_env *env, int regno, int off,
> + int size)
> +{
> + struct bpf_map *map;
> + int map_id = env->cur_state.regs[regno].imm;
> +
> + _(get_map_info(env, map_id, &map));
> +
> + if (off < 0 || off + size > map->value_size) {
This could be tricked with a negative size, or a giant size, wrapping negative.
> + verbose("invalid access to map_id=%d leaf_size=%d off=%d size=%d\n",
> + map_id, map->value_size, off, size);
> + return -EACCES;
> + }
> + return 0;
> +}
> +
> +/* check access to 'struct bpf_context' fields */
> +static int check_ctx_access(struct verifier_env *env, int off, int size,
> + enum bpf_access_type t)
> +{
> + if (env->prog->info->ops->is_valid_access &&
> + env->prog->info->ops->is_valid_access(off, size, t))
> + return 0;
> +
> + verbose("invalid bpf_context access off=%d size=%d\n", off, size);
> + return -EACCES;
> +}
> +
> +static int check_mem_access(struct verifier_env *env, int regno, int off,
> + int bpf_size, enum bpf_access_type t,
> + int value_regno)
> +{
> + struct verifier_state *state = &env->cur_state;
> + int size;
> +
> + _(size = bpf_size_to_bytes(bpf_size));
> +
> + if (off % size != 0) {
> + verbose("misaligned access off %d size %d\n", off, size);
> + return -EACCES;
> + }
I think more off and size checking is needed here.
> +
> + if (state->regs[regno].type == PTR_TO_MAP) {
> + _(check_map_access(env, regno, off, size));
> + if (t == BPF_READ)
> + mark_reg_unknown_value(state->regs, value_regno);
> + } else if (state->regs[regno].type == PTR_TO_CTX) {
> + _(check_ctx_access(env, off, size, t));
> + if (t == BPF_READ)
> + mark_reg_unknown_value(state->regs, value_regno);
> + } else if (state->regs[regno].type == FRAME_PTR) {
> + if (off >= 0 || off < -MAX_BPF_STACK) {
> + verbose("invalid stack off=%d size=%d\n", off, size);
> + return -EACCES;
> + }
> + if (t == BPF_WRITE)
> + _(check_stack_write(state, off, size, value_regno));
> + else
> + _(check_stack_read(state, off, size, value_regno));
> + } else {
> + verbose("R%d invalid mem access '%s'\n",
> + regno, reg_type_str[state->regs[regno].type]);
> + return -EACCES;
> + }
> + return 0;
> +}
> +
> +/* when register 'regno' is passed into function that will read 'access_size'
> + * bytes from that pointer, make sure that it's within stack boundary
> + * and all elements of stack are initialized
> + */
> +static int check_stack_boundary(struct verifier_env *env,
> + int regno, int access_size)
> +{
> + struct verifier_state *state = &env->cur_state;
> + struct reg_state *regs = state->regs;
> + int off, i;
> +
regno bounds checking needed.
> + if (regs[regno].type != PTR_TO_STACK)
> + return -EACCES;
> +
> + off = regs[regno].imm;
> + if (off >= 0 || off < -MAX_BPF_STACK || off + access_size > 0 ||
> + access_size <= 0) {
> + verbose("invalid stack type R%d off=%d access_size=%d\n",
> + regno, off, access_size);
> + return -EACCES;
> + }
> +
> + for (i = 0; i < access_size; i++) {
> + if (state->stack[MAX_BPF_STACK + off + i].stype != STACK_MISC) {
> + verbose("invalid indirect read from stack off %d+%d size %d\n",
> + off, i, access_size);
> + return -EACCES;
> + }
> + }
> + return 0;
> +}
> +
> +static int check_func_arg(struct verifier_env *env, int regno,
> + enum bpf_arg_type arg_type, int *map_id,
> + struct bpf_map **mapp)
> +{
> + struct reg_state *reg = env->cur_state.regs + regno;
I would use [] instead of + here. (and regno needs bounds checking)
> + enum bpf_reg_type expected_type;
> +
> + if (arg_type == ARG_ANYTHING)
> + return 0;
> +
> + if (reg->type == NOT_INIT) {
> + verbose("R%d !read_ok\n", regno);
> + return -EACCES;
> + }
> +
> + if (arg_type == ARG_PTR_TO_MAP_KEY || arg_type == ARG_PTR_TO_MAP_VALUE) {
> + expected_type = PTR_TO_STACK;
> + } else if (arg_type == ARG_CONST_MAP_ID || arg_type == ARG_CONST_STACK_SIZE) {
> + expected_type = CONST_IMM;
> + } else {
> + verbose("unsupported arg_type %d\n", arg_type);
> + return -EFAULT;
> + }
> +
> + if (reg->type != expected_type) {
> + verbose("R%d type=%s expected=%s\n", regno,
> + reg_type_str[reg->type], reg_type_str[expected_type]);
> + return -EACCES;
> + }
> +
> + if (arg_type == ARG_CONST_MAP_ID) {
> + /* bpf_map_xxx(map_id) call: check that map_id is valid */
> + *map_id = reg->imm;
> + _(get_map_info(env, reg->imm, mapp));
> + } else if (arg_type == ARG_PTR_TO_MAP_KEY) {
> + /*
> + * bpf_map_xxx(..., map_id, ..., key) call:
> + * check that [key, key + map->key_size) are within
> + * stack limits and initialized
> + */
> + if (!*mapp) {
> + /*
> + * in function declaration map_id must come before
> + * map_key or map_elem, so that it's verified
> + * and known before we have to check map_key here
> + */
> + verbose("invalid map_id to access map->key\n");
> + return -EACCES;
> + }
> + _(check_stack_boundary(env, regno, (*mapp)->key_size));
> + } else if (arg_type == ARG_PTR_TO_MAP_VALUE) {
> + /*
> + * bpf_map_xxx(..., map_id, ..., value) call:
> + * check [value, value + map->value_size) validity
> + */
> + if (!*mapp) {
> + verbose("invalid map_id to access map->elem\n");
> + return -EACCES;
> + }
> + _(check_stack_boundary(env, regno, (*mapp)->value_size));
> + } else if (arg_type == ARG_CONST_STACK_SIZE) {
> + /*
> + * bpf_xxx(..., buf, len) call will access 'len' bytes
> + * from stack pointer 'buf'. Check it
> + * note: regno == len, regno - 1 == buf
> + */
> + _(check_stack_boundary(env, regno - 1, reg->imm));
> + }
> +
> + return 0;
> +}
> +
> +static int check_call(struct verifier_env *env, int func_id)
> +{
> + struct verifier_state *state = &env->cur_state;
> + const struct bpf_func_proto *fn = NULL;
> + struct reg_state *regs = state->regs;
> + struct bpf_map *map = NULL;
> + struct reg_state *reg;
> + int map_id = -1;
> + int i;
> +
> + /* find function prototype */
> + if (func_id <= 0 || func_id >= __BPF_FUNC_MAX_ID) {
> + verbose("invalid func %d\n", func_id);
> + return -EINVAL;
> + }
> +
> + if (env->prog->info->ops->get_func_proto)
> + fn = env->prog->info->ops->get_func_proto(func_id);
> +
> + if (!fn) {
> + verbose("unknown func %d\n", func_id);
> + return -EINVAL;
> + }
> +
> + /* eBPF programs must be GPL compatible to use GPL-ed functions */
> + if (!env->prog->info->is_gpl_compatible && fn->gpl_only) {
> + verbose("cannot call GPL only function from proprietary program\n");
> + return -EINVAL;
> + }
> +
> + /* check args */
> + _(check_func_arg(env, BPF_REG_1, fn->arg1_type, &map_id, &map));
> + _(check_func_arg(env, BPF_REG_2, fn->arg2_type, &map_id, &map));
> + _(check_func_arg(env, BPF_REG_3, fn->arg3_type, &map_id, &map));
> + _(check_func_arg(env, BPF_REG_4, fn->arg4_type, &map_id, &map));
> + _(check_func_arg(env, BPF_REG_5, fn->arg5_type, &map_id, &map));
> +
> + /* reset caller saved regs */
> + for (i = 0; i < CALLER_SAVED_REGS; i++) {
> + reg = regs + caller_saved[i];
> + reg->type = NOT_INIT;
> + reg->imm = 0;
> + }
> +
> + /* update return register */
> + if (fn->ret_type == RET_INTEGER) {
> + regs[BPF_REG_0].type = UNKNOWN_VALUE;
> + } else if (fn->ret_type == RET_VOID) {
> + regs[BPF_REG_0].type = NOT_INIT;
> + } else if (fn->ret_type == RET_PTR_TO_MAP_OR_NULL) {
> + regs[BPF_REG_0].type = PTR_TO_MAP_OR_NULL;
> + /*
> + * remember map_id, so that check_map_access()
> + * can check 'value_size' boundary of memory access
> + * to map element returned from bpf_map_lookup_elem()
> + */
> + regs[BPF_REG_0].imm = map_id;
> + } else {
> + verbose("unknown return type %d of func %d\n",
> + fn->ret_type, func_id);
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +/* check validity of 32-bit and 64-bit arithmetic operations */
> +static int check_alu_op(struct reg_state *regs, struct bpf_insn *insn)
> +{
> + u8 opcode = BPF_OP(insn->code);
> +
> + if (opcode == BPF_END || opcode == BPF_NEG) {
> + if (BPF_SRC(insn->code) != BPF_X)
> + return -EINVAL;
> + /* check src operand */
> + _(check_reg_arg(regs, insn->dst_reg, 1));
> +
> + /* check dest operand */
> + _(check_reg_arg(regs, insn->dst_reg, 0));
> +
> + } else if (opcode == BPF_MOV) {
> +
> + if (BPF_SRC(insn->code) == BPF_X)
> + /* check src operand */
> + _(check_reg_arg(regs, insn->src_reg, 1));
> +
> + /* check dest operand */
> + _(check_reg_arg(regs, insn->dst_reg, 0));
> +
> + if (BPF_SRC(insn->code) == BPF_X) {
> + if (BPF_CLASS(insn->code) == BPF_ALU64) {
> + /* case: R1 = R2
> + * copy register state to dest reg
> + */
> + regs[insn->dst_reg].type = regs[insn->src_reg].type;
> + regs[insn->dst_reg].imm = regs[insn->src_reg].imm;
> + } else {
> + regs[insn->dst_reg].type = UNKNOWN_VALUE;
> + regs[insn->dst_reg].imm = 0;
> + }
> + } else {
> + /* case: R = imm
> + * remember the value we stored into this reg
> + */
> + regs[insn->dst_reg].type = CONST_IMM;
> + regs[insn->dst_reg].imm = insn->imm;
> + }
> +
> + } else { /* all other ALU ops: and, sub, xor, add, ... */
> +
> + int stack_relative = 0;
> +
> + if (BPF_SRC(insn->code) == BPF_X)
> + /* check src1 operand */
> + _(check_reg_arg(regs, insn->src_reg, 1));
> +
> + /* check src2 operand */
> + _(check_reg_arg(regs, insn->dst_reg, 1));
> +
> + if ((opcode == BPF_MOD || opcode == BPF_DIV) &&
> + BPF_SRC(insn->code) == BPF_K && insn->imm == 0) {
> + verbose("div by zero\n");
> + return -EINVAL;
> + }
> +
> + if (opcode == BPF_ADD && BPF_CLASS(insn->code) == BPF_ALU64 &&
> + regs[insn->dst_reg].type == FRAME_PTR &&
> + BPF_SRC(insn->code) == BPF_K)
> + stack_relative = 1;
> +
> + /* check dest operand */
> + _(check_reg_arg(regs, insn->dst_reg, 0));
> +
> + if (stack_relative) {
> + regs[insn->dst_reg].type = PTR_TO_STACK;
> + regs[insn->dst_reg].imm = insn->imm;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int check_cond_jmp_op(struct verifier_env *env,
> + struct bpf_insn *insn, int *insn_idx)
> +{
> + struct reg_state *regs = env->cur_state.regs;
> + struct verifier_state *other_branch;
> + u8 opcode = BPF_OP(insn->code);
> +
> + if (BPF_SRC(insn->code) == BPF_X)
> + /* check src1 operand */
> + _(check_reg_arg(regs, insn->src_reg, 1));
> +
> + /* check src2 operand */
> + _(check_reg_arg(regs, insn->dst_reg, 1));
> +
> + /* detect if R == 0 where R was initialized to zero earlier */
> + if (BPF_SRC(insn->code) == BPF_K &&
> + (opcode == BPF_JEQ || opcode == BPF_JNE) &&
> + regs[insn->dst_reg].type == CONST_IMM &&
> + regs[insn->dst_reg].imm == insn->imm) {
> + if (opcode == BPF_JEQ) {
> + /* if (imm == imm) goto pc+off;
> + * only follow the goto, ignore fall-through
> + */
> + *insn_idx += insn->off;
> + return 0;
> + } else {
> + /* if (imm != imm) goto pc+off;
> + * only follow fall-through branch, since
> + * that's where the program will go
> + */
> + return 0;
> + }
> + }
> +
> + other_branch = push_stack(env, *insn_idx + insn->off + 1, *insn_idx);
> + if (!other_branch)
> + return -EFAULT;
> +
> + /* detect if R == 0 where R is returned value from bpf_map_lookup_elem() */
> + if (BPF_SRC(insn->code) == BPF_K &&
> + insn->imm == 0 && (opcode == BPF_JEQ ||
> + opcode == BPF_JNE) &&
> + regs[insn->dst_reg].type == PTR_TO_MAP_OR_NULL) {
> + if (opcode == BPF_JEQ) {
> + /* next fallthrough insn can access memory via
> + * this register
> + */
> + regs[insn->dst_reg].type = PTR_TO_MAP;
> + /* branch targer cannot access it, since reg == 0 */
> + other_branch->regs[insn->dst_reg].type = CONST_IMM;
> + other_branch->regs[insn->dst_reg].imm = 0;
> + } else {
> + other_branch->regs[insn->dst_reg].type = PTR_TO_MAP;
> + regs[insn->dst_reg].type = CONST_IMM;
> + regs[insn->dst_reg].imm = 0;
> + }
> + } else if (BPF_SRC(insn->code) == BPF_K &&
> + (opcode == BPF_JEQ || opcode == BPF_JNE)) {
> +
> + if (opcode == BPF_JEQ) {
> + /* detect if (R == imm) goto
> + * and in the target state recognize that R = imm
> + */
> + other_branch->regs[insn->dst_reg].type = CONST_IMM;
> + other_branch->regs[insn->dst_reg].imm = insn->imm;
> + } else {
> + /* detect if (R != imm) goto
> + * and in the fall-through state recognize that R = imm
> + */
> + regs[insn->dst_reg].type = CONST_IMM;
> + regs[insn->dst_reg].imm = insn->imm;
> + }
> + }
> + if (verbose_on)
> + pr_cont_verifier_state(env);
> + return 0;
> +}
> +
> +/* verify safety of LD_ABS|LD_IND instructions:
> + * - they can only appear in the programs where ctx == skb
> + * - since they are wrappers of function calls, they scratch R1-R5 registers,
> + * preserve R6-R9, and store return value into R0
> + *
> + * Implicit input:
> + * ctx == skb == R6 == CTX
> + *
> + * Explicit input:
> + * SRC == any register
> + * IMM == 32-bit immediate
> + *
> + * Output:
> + * R0 - 8/16/32-bit skb data converted to cpu endianness
> + */
> +
> +static int check_ld_abs(struct verifier_env *env, struct bpf_insn *insn)
> +{
> + struct reg_state *regs = env->cur_state.regs;
> + u8 mode = BPF_MODE(insn->code);
> + struct reg_state *reg;
> + int i;
> +
> + if (mode != BPF_ABS && mode != BPF_IND)
> + return -EINVAL;
> +
> + if (env->prog->info->prog_type != BPF_PROG_TYPE_SOCKET_FILTER) {
> + verbose("BPF_LD_ABS|IND instructions are only allowed in socket filters\n");
> + return -EINVAL;
> + }
> +
> + /* check whether implicit source operand (register R6) is readable */
> + _(check_reg_arg(regs, BPF_REG_6, 1));
> +
> + if (regs[BPF_REG_6].type != PTR_TO_CTX) {
> + verbose("at the time of BPF_LD_ABS|IND R6 != pointer to skb\n");
> + return -EINVAL;
> + }
> +
> + if (mode == BPF_IND)
> + /* check explicit source operand */
> + _(check_reg_arg(regs, insn->src_reg, 1));
> +
> + /* reset caller saved regs to unreadable */
> + for (i = 0; i < CALLER_SAVED_REGS; i++) {
> + reg = regs + caller_saved[i];
> + reg->type = NOT_INIT;
> + reg->imm = 0;
> + }
> +
> + /* mark destination R0 register as readable, since it contains
> + * the value fetched from the packet
> + */
> + regs[BPF_REG_0].type = UNKNOWN_VALUE;
> + return 0;
> +}
> +
> +/* non-recursive DFS pseudo code
> + * 1 procedure DFS-iterative(G,v):
> + * 2 label v as discovered
> + * 3 let S be a stack
> + * 4 S.push(v)
> + * 5 while S is not empty
> + * 6 t <- S.pop()
> + * 7 if t is what we're looking for:
> + * 8 return t
> + * 9 for all edges e in G.adjacentEdges(t) do
> + * 10 if edge e is already labelled
> + * 11 continue with the next edge
> + * 12 w <- G.adjacentVertex(t,e)
> + * 13 if vertex w is not discovered and not explored
> + * 14 label e as tree-edge
> + * 15 label w as discovered
> + * 16 S.push(w)
> + * 17 continue at 5
> + * 18 else if vertex w is discovered
> + * 19 label e as back-edge
> + * 20 else
> + * 21 // vertex w is explored
> + * 22 label e as forward- or cross-edge
> + * 23 label t as explored
> + * 24 S.pop()
> + *
> + * convention:
> + * 1 - discovered
> + * 2 - discovered and 1st branch labelled
> + * 3 - discovered and 1st and 2nd branch labelled
> + * 4 - explored
> + */
> +
> +#define STATE_END ((struct verifier_state_list *)-1)
> +
> +#define PUSH_INT(I) \
> + do { \
> + if (cur_stack >= insn_cnt) { \
> + ret = -E2BIG; \
> + goto free_st; \
> + } \
> + stack[cur_stack++] = I; \
> + } while (0)
> +
> +#define PEEK_INT() \
> + ({ \
> + int _ret; \
> + if (cur_stack == 0) \
> + _ret = -1; \
> + else \
> + _ret = stack[cur_stack - 1]; \
> + _ret; \
> + })
> +
> +#define POP_INT() \
> + ({ \
> + int _ret; \
> + if (cur_stack == 0) \
> + _ret = -1; \
> + else \
> + _ret = stack[--cur_stack]; \
> + _ret; \
> + })
> +
> +#define PUSH_INSN(T, W, E) \
> + do { \
> + int w = W; \
> + if (E == 1 && st[T] >= 2) \
> + break; \
> + if (E == 2 && st[T] >= 3) \
> + break; \
> + if (w >= insn_cnt) { \
> + ret = -EACCES; \
> + goto free_st; \
> + } \
> + if (E == 2) \
> + /* mark branch target for state pruning */ \
> + env->branch_landing[w] = STATE_END; \
> + if (st[w] == 0) { \
> + /* tree-edge */ \
> + st[T] = 1 + E; \
> + st[w] = 1; /* discovered */ \
> + PUSH_INT(w); \
> + goto peak_stack; \
> + } else if (st[w] == 1 || st[w] == 2 || st[w] == 3) { \
> + verbose("back-edge from insn %d to %d\n", t, w); \
> + ret = -EINVAL; \
> + goto free_st; \
> + } else if (st[w] == 4) { \
> + /* forward- or cross-edge */ \
> + st[T] = 1 + E; \
> + } else { \
> + verbose("insn state internal bug\n"); \
> + ret = -EFAULT; \
> + goto free_st; \
> + } \
> + } while (0)
> +
> +/* non-recursive depth-first-search to detect loops in BPF program
> + * loop == back-edge in directed graph
> + */
> +static int check_cfg(struct verifier_env *env)
> +{
> + struct bpf_insn *insns = env->prog->insnsi;
> + int insn_cnt = env->prog->len;
> + int cur_stack = 0;
> + int *stack;
> + int ret = 0;
> + int *st;
> + int i, t;
> +
> + if (insns[insn_cnt - 1].code != (BPF_JMP | BPF_EXIT)) {
> + verbose("last insn is not a 'ret'\n");
> + return -EINVAL;
> + }
> +
> + st = kzalloc(sizeof(int) * insn_cnt, GFP_KERNEL);
> + if (!st)
> + return -ENOMEM;
> +
> + stack = kzalloc(sizeof(int) * insn_cnt, GFP_KERNEL);
> + if (!stack) {
> + kfree(st);
> + return -ENOMEM;
> + }
> +
> + st[0] = 1; /* mark 1st insn as discovered */
> + PUSH_INT(0);
> +
> +peak_stack:
> + while ((t = PEEK_INT()) != -1) {
> + if (insns[t].code == (BPF_JMP | BPF_EXIT))
> + goto mark_explored;
> +
> + if (BPF_CLASS(insns[t].code) == BPF_JMP) {
> + u8 opcode = BPF_OP(insns[t].code);
> +
> + if (opcode == BPF_CALL) {
> + PUSH_INSN(t, t + 1, 1);
> + } else if (opcode == BPF_JA) {
> + if (BPF_SRC(insns[t].code) != BPF_X) {
> + ret = -EINVAL;
> + goto free_st;
> + }
> + PUSH_INSN(t, t + insns[t].off + 1, 1);
> + } else {
> + PUSH_INSN(t, t + 1, 1);
> + PUSH_INSN(t, t + insns[t].off + 1, 2);
> + }
> + /* tell verifier to check for equivalent verifier states
> + * after every call and jump
> + */
> + env->branch_landing[t + 1] = STATE_END;
> + } else {
> + PUSH_INSN(t, t + 1, 1);
> + }
> +
> +mark_explored:
> + st[t] = 4; /* explored */
> + if (POP_INT() == -1) {
> + verbose("pop_int internal bug\n");
> + ret = -EFAULT;
> + goto free_st;
> + }
> + }
> +
> +
> + for (i = 0; i < insn_cnt; i++) {
> + if (st[i] != 4) {
> + verbose("unreachable insn %d\n", i);
> + ret = -EINVAL;
> + goto free_st;
> + }
> + }
> +
> +free_st:
> + kfree(st);
> + kfree(stack);
> + return ret;
> +}
> +
> +/* compare two verifier states
> + *
> + * all states stored in state_list are known to be valid, since
> + * verifier reached 'bpf_exit' instruction through them
> + *
> + * this function is called when verifier exploring different branches of
> + * execution popped from the state stack. If it sees an old state that has
> + * more strict register state and more strict stack state then this execution
> + * branch doesn't need to be explored further, since verifier already
> + * concluded that more strict state leads to valid finish.
> + *
> + * Therefore two states are equivalent if register state is more conservative
> + * and explored stack state is more conservative than the current one.
> + * Example:
> + * explored current
> + * (slot1=INV slot2=MISC) == (slot1=MISC slot2=MISC)
> + * (slot1=MISC slot2=MISC) != (slot1=INV slot2=MISC)
> + *
> + * In other words if current stack state (one being explored) has more
> + * valid slots than old one that already passed validation, it means
> + * the verifier can stop exploring and conclude that current state is valid too
> + *
> + * Similarly with registers. If explored state has register type as invalid
> + * whereas register type in current state is meaningful, it means that
> + * the current state will reach 'bpf_exit' instruction safely
> + */
> +static bool states_equal(struct verifier_state *old, struct verifier_state *cur)
> +{
> + int i;
> +
> + for (i = 0; i < MAX_BPF_REG; i++) {
> + if (memcmp(&old->regs[i], &cur->regs[i],
> + sizeof(old->regs[0])) != 0) {
> + if (old->regs[i].type == NOT_INIT ||
> + old->regs[i].type == UNKNOWN_VALUE)
> + continue;
> + return false;
> + }
> + }
> +
> + for (i = 0; i < MAX_BPF_STACK; i++) {
> + if (memcmp(&old->stack[i], &cur->stack[i],
> + sizeof(old->stack[0])) != 0) {
> + if (old->stack[i].stype == STACK_INVALID)
> + continue;
> + return false;
> + }
> + }
> + return true;
> +}
> +
> +static int is_state_visited(struct verifier_env *env, int insn_idx)
> +{
> + struct verifier_state_list *new_sl;
> + struct verifier_state_list *sl;
> +
> + sl = env->branch_landing[insn_idx];
> + if (!sl)
> + /* no branch jump to this insn, ignore it */
> + return 0;
> +
> + while (sl != STATE_END) {
> + if (states_equal(&sl->state, &env->cur_state))
> + /* reached equivalent register/stack state,
> + * prune the search
> + */
> + return 1;
> + sl = sl->next;
> + }
> + new_sl = kmalloc(sizeof(struct verifier_state_list), GFP_KERNEL);
> +
> + if (!new_sl)
> + /* ignore ENOMEM, it doesn't affect correctness */
> + return 0;
> +
> + /* add new state to the head of linked list */
> + memcpy(&new_sl->state, &env->cur_state, sizeof(env->cur_state));
> + new_sl->next = env->branch_landing[insn_idx];
> + env->branch_landing[insn_idx] = new_sl;
> + return 0;
> +}
> +
> +static int do_check(struct verifier_env *env)
> +{
> + struct verifier_state *state = &env->cur_state;
> + struct bpf_insn *insns = env->prog->insnsi;
> + struct reg_state *regs = state->regs;
> + int insn_cnt = env->prog->len;
> + int insn_idx, prev_insn_idx = 0;
> + int insn_processed = 0;
> + bool do_print_state = false;
> +
> + init_reg_state(regs);
> + insn_idx = 0;
> + for (;;) {
> + struct bpf_insn *insn;
> + u8 class;
> +
> + if (insn_idx >= insn_cnt) {
> + verbose("invalid insn idx %d insn_cnt %d\n",
> + insn_idx, insn_cnt);
> + return -EFAULT;
> + }
> +
> + insn = &insns[insn_idx];
> + class = BPF_CLASS(insn->code);
> +
> + if (++insn_processed > 32768) {
> + verbose("BPF program is too large. Proccessed %d insn\n",
> + insn_processed);
> + return -E2BIG;
> + }
> +
> + if (is_state_visited(env, insn_idx)) {
> + if (verbose_on) {
> + if (do_print_state)
> + pr_cont("\nfrom %d to %d: safe\n",
> + prev_insn_idx, insn_idx);
> + else
> + pr_cont("%d: safe\n", insn_idx);
> + }
> + goto process_bpf_exit;
> + }
> +
> + if (verbose_on && do_print_state) {
> + pr_cont("\nfrom %d to %d:", prev_insn_idx, insn_idx);
> + pr_cont_verifier_state(env);
> + do_print_state = false;
> + }
> +
> + if (verbose_on) {
> + pr_cont("%d: ", insn_idx);
> + pr_cont_bpf_insn(insn);
> + }
> +
> + if (class == BPF_ALU || class == BPF_ALU64) {
> + _(check_alu_op(regs, insn));
> +
> + } else if (class == BPF_LDX) {
> + if (BPF_MODE(insn->code) != BPF_MEM)
> + return -EINVAL;
> +
> + /* check src operand */
> + _(check_reg_arg(regs, insn->src_reg, 1));
> +
> + _(check_mem_access(env, insn->src_reg, insn->off,
> + BPF_SIZE(insn->code), BPF_READ,
> + insn->dst_reg));
> +
> + /* dest reg state will be updated by mem_access */
> +
> + } else if (class == BPF_STX) {
> + /* check src1 operand */
> + _(check_reg_arg(regs, insn->src_reg, 1));
> + /* check src2 operand */
> + _(check_reg_arg(regs, insn->dst_reg, 1));
> + _(check_mem_access(env, insn->dst_reg, insn->off,
> + BPF_SIZE(insn->code), BPF_WRITE,
> + insn->src_reg));
> +
> + } else if (class == BPF_ST) {
> + if (BPF_MODE(insn->code) != BPF_MEM)
> + return -EINVAL;
> + /* check src operand */
> + _(check_reg_arg(regs, insn->dst_reg, 1));
> + _(check_mem_access(env, insn->dst_reg, insn->off,
> + BPF_SIZE(insn->code), BPF_WRITE,
> + -1));
> +
> + } else if (class == BPF_JMP) {
> + u8 opcode = BPF_OP(insn->code);
> +
> + if (opcode == BPF_CALL) {
> + _(check_call(env, insn->imm));
> + } else if (opcode == BPF_JA) {
> + if (BPF_SRC(insn->code) != BPF_X)
> + return -EINVAL;
> + insn_idx += insn->off + 1;
> + continue;
> + } else if (opcode == BPF_EXIT) {
> + /* eBPF calling convetion is such that R0 is used
> + * to return the value from eBPF program.
> + * Make sure that it's readable at this time
> + * of bpf_exit, which means that program wrote
> + * something into it earlier
> + */
> + _(check_reg_arg(regs, BPF_REG_0, 1));
> +process_bpf_exit:
> + insn_idx = pop_stack(env, &prev_insn_idx);
> + if (insn_idx < 0) {
> + break;
> + } else {
> + do_print_state = true;
> + continue;
> + }
> + } else {
> + _(check_cond_jmp_op(env, insn, &insn_idx));
> + }
> + } else if (class == BPF_LD) {
> + _(check_ld_abs(env, insn));
> + } else {
> + verbose("unknown insn class %d\n", class);
> + return -EINVAL;
> + }
> +
> + insn_idx++;
> + }
> +
> + return 0;
> +}
> +
> +static void free_states(struct verifier_env *env, int insn_cnt)
> +{
> + struct verifier_state_list *sl, *sln;
> + int i;
> +
> + for (i = 0; i < insn_cnt; i++) {
> + sl = env->branch_landing[i];
> +
> + if (sl)
> + while (sl != STATE_END) {
> + sln = sl->next;
> + kfree(sl);
> + sl = sln;
> + }
> + }
> +
> + kfree(env->branch_landing);
> +}
> +
> +int bpf_check(struct sk_filter *prog)
> +{
> + struct verifier_env *env;
> + int ret;
> +
> + if (prog->len <= 0 || prog->len > BPF_MAXINSNS)
> + return -E2BIG;
> +
> + env = kzalloc(sizeof(struct verifier_env), GFP_KERNEL);
> + if (!env)
> + return -ENOMEM;
> +
> + verbose_on = false;
> +retry:
> + env->prog = prog;
> + env->branch_landing = kcalloc(prog->len,
> + sizeof(struct verifier_state_list *),
> + GFP_KERNEL);
> +
> + if (!env->branch_landing) {
> + kfree(env);
> + return -ENOMEM;
> + }
> +
> + ret = check_cfg(env);
> + if (ret < 0)
> + goto free_env;
> +
> + ret = do_check(env);
> +
> +free_env:
> + while (pop_stack(env, NULL) >= 0);
> + free_states(env, prog->len);
> +
> + if (ret < 0 && !verbose_on && capable(CAP_SYS_ADMIN)) {
> + /* verification failed, redo it with verbose on */
> + memset(env, 0, sizeof(struct verifier_env));
> + verbose_on = true;
> + goto retry;
> + }
> +
> + if (ret == 0 && env->used_map_cnt) {
> + /* if program passed verifier, update used_maps in bpf_prog_info */
> + prog->info->used_maps = kmalloc_array(env->used_map_cnt,
> + sizeof(u32), GFP_KERNEL);
> + if (!prog->info->used_maps) {
> + kfree(env);
> + return -ENOMEM;
> + }
> + memcpy(prog->info->used_maps, env->used_maps,
> + sizeof(u32) * env->used_map_cnt);
> + prog->info->used_map_cnt = env->used_map_cnt;
> + }
> +
> + kfree(env);
> + return ret;
> +}
> --
> 1.7.9.5
>
Unless I've overlooked something, I think this needs much stricter
evaluation of register numbers, offsets, and sizes.
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH RFC v2 net-next 08/16] bpf: add hashtable type of BPF maps
From: Alexei Starovoitov @ 2014-07-23 21:42 UTC (permalink / raw)
To: Kees Cook
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Daniel Borkmann, Chema Gonzalez, Eric Dumazet,
Peter Zijlstra, Arnaldo Carvalho de Melo, Jiri Olsa,
Thomas Gleixner, H. Peter Anvin, Andrew Morton, Linux API,
Network Development, LKML
In-Reply-To: <CAGXu5jLbcrR76=omiqRNERhjZS8bsR1-jG+Ctra97uCrpHLPUA@mail.gmail.com>
On Wed, Jul 23, 2014 at 1:33 PM, Kees Cook <keescook@chromium.org> wrote:
>>
>>>> + htab->slab_name = kasprintf(GFP_USER, "bpf_htab_%p", htab);
>>>
>>> This leaks a kernel heap memory pointer to userspace. If a unique name
>>> needed, I think map_id should be used instead.
>>
>> it leaks, how? slabinfo is only available to root.
>> The same code exists in conntrack:
>> net/netfilter/nf_conntrack_core.c:1767
>
> Right, in extreme cases, there are system configurations where leaking
> addresses even to root can be considered a bug. There are a lot of
> these situations in the kernel still, that's true. However, if we can
> at all avoid it, I'd really like to avoid adding new ones. Nearly all
> the cases of using a memory pointer is for uniqueness concerns, but I
> think can already get that from the map_id.
ok. fair enough. I think slab name doesn't have to be unique anymore.
It's used to be a requirement in older kernels. If it is ok to reuse now,
I'll just use the same for all hash-type maps.
Advice from slab expert would be great...
^ permalink raw reply
* Re: [PATCH RFC v2 net-next 07/16] bpf: add lookup/update/delete/iterate methods to BPF maps
From: Alexei Starovoitov @ 2014-07-23 21:22 UTC (permalink / raw)
To: Kees Cook
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Daniel Borkmann, Chema Gonzalez, Eric Dumazet,
Peter Zijlstra, Arnaldo Carvalho de Melo, Jiri Olsa,
Thomas Gleixner, H. Peter Anvin, Andrew Morton, Linux API,
Network Development, LKML
In-Reply-To: <CAGXu5jKA9gArKaqODsD0kswmP0Q77w152VXhq9zCS4pgTZf+Jg@mail.gmail.com>
On Wed, Jul 23, 2014 at 1:25 PM, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Jul 23, 2014 at 12:49 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
>> On Wed, Jul 23, 2014 at 11:25 AM, Kees Cook <keescook@chromium.org> wrote:
>>>> +
>>>> + /* lookup key in a given map referenced by map_id
>>>> + * err = bpf_map_lookup_elem(int map_id, void *key, void *value)
>>>
>>> This needs map_id documentation updates too?
>>
>> yes. will grep for it just to make sure.
>>
>>>> +static int get_map_id(struct fd f)
>>>> +{
>>>> + struct bpf_map *map;
>>>> +
>>>> + if (!f.file)
>>>> + return -EBADF;
>>>> +
>>>> + if (f.file->f_op != &bpf_map_fops) {
>>>> + fdput(f);
>>>
>>> It feels weird to me to do the fdput inside this function. Instead,
>>> should map_lookup_elem get a "err_put" label, instead?
>>
>> I don't think it will work, since I'm not sure that fd.flags will be zero
>> when fd.file == NULL. It looks so by analyzing return code path
>> in fs/file.c, but I wasn't sure that I followed all code paths,
>> so I just picked this style from fs/timerfd.c assuming it was
>> done this away on purpose and there can be the case where
>> fd.file == null and fd.flags !=0. In such case we cannot call fdput().
>
> Yeah, hm, looking around, this does seem to be the case. I guess the
> thought is that when get_map_id fails, struct fd has been handled.
correct.
> Maybe add a comment above that function as a reminder?
yes. will do.
^ permalink raw reply
* Re: [PATCH RFC v2 net-next 08/16] bpf: add hashtable type of BPF maps
From: Kees Cook @ 2014-07-23 20:33 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Daniel Borkmann, Chema Gonzalez, Eric Dumazet,
Peter Zijlstra, Arnaldo Carvalho de Melo, Jiri Olsa,
Thomas Gleixner, H. Peter Anvin, Andrew Morton, Linux API,
Network Development, LKML
In-Reply-To: <CAMEtUuyRUG6ZdRvL0JuakD0zPdYo3WoqQepK7_b_MW+r2aVzVA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wed, Jul 23, 2014 at 12:57 PM, Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org> wrote:
> On Wed, Jul 23, 2014 at 11:36 AM, Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>>> +static struct bpf_map *htab_map_alloc(struct nlattr *attr[BPF_MAP_ATTR_MAX + 1])
>>> +{
>>> + struct bpf_htab *htab;
>>> + int err, i;
>>> +
>>> + htab = kmalloc(sizeof(*htab), GFP_USER);
>>
>> I'd prefer kzalloc here.
>
> in this case I agree. will change, since it's not in critical path and we
> can waste few cycles zeroing memory.
>
>>> + err = -ENOMEM;
>>> + htab->buckets = kmalloc(htab->n_buckets * sizeof(struct hlist_head),
>>> + GFP_USER);
>>
>> I'd prefer kcalloc here, even though n_buckets can't currently trigger
>> an integer overflow.
>
> hmm, I would argue that kmalloc_array is a preferred way, but kcalloc ?
> Few lines below the whole array is inited with INIT_HLIST_HEAD...
Ah! I didn't realize kmalloc_array existed! Perfect. Yes, that would
be great to use. The zeroing is not needed, due to the init below, as
you say.
>
>>> + for (i = 0; i < htab->n_buckets; i++)
>>> + INIT_HLIST_HEAD(&htab->buckets[i]);
>
>>> + htab->slab_name = kasprintf(GFP_USER, "bpf_htab_%p", htab);
>>
>> This leaks a kernel heap memory pointer to userspace. If a unique name
>> needed, I think map_id should be used instead.
>
> it leaks, how? slabinfo is only available to root.
> The same code exists in conntrack:
> net/netfilter/nf_conntrack_core.c:1767
Right, in extreme cases, there are system configurations where leaking
addresses even to root can be considered a bug. There are a lot of
these situations in the kernel still, that's true. However, if we can
at all avoid it, I'd really like to avoid adding new ones. Nearly all
the cases of using a memory pointer is for uniqueness concerns, but I
think can already get that from the map_id.
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH RFC v2 net-next 07/16] bpf: add lookup/update/delete/iterate methods to BPF maps
From: Kees Cook @ 2014-07-23 20:25 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Daniel Borkmann, Chema Gonzalez, Eric Dumazet,
Peter Zijlstra, Arnaldo Carvalho de Melo, Jiri Olsa,
Thomas Gleixner, H. Peter Anvin, Andrew Morton, Linux API,
Network Development, LKML
In-Reply-To: <CAMEtUuwE1oLUZb2QM243uKGSAfT_ahozducakpi9uvADzsj5XA@mail.gmail.com>
On Wed, Jul 23, 2014 at 12:49 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
> On Wed, Jul 23, 2014 at 11:25 AM, Kees Cook <keescook@chromium.org> wrote:
>>> +
>>> + /* lookup key in a given map referenced by map_id
>>> + * err = bpf_map_lookup_elem(int map_id, void *key, void *value)
>>
>> This needs map_id documentation updates too?
>
> yes. will grep for it just to make sure.
>
>>> +static int get_map_id(struct fd f)
>>> +{
>>> + struct bpf_map *map;
>>> +
>>> + if (!f.file)
>>> + return -EBADF;
>>> +
>>> + if (f.file->f_op != &bpf_map_fops) {
>>> + fdput(f);
>>
>> It feels weird to me to do the fdput inside this function. Instead,
>> should map_lookup_elem get a "err_put" label, instead?
>
> I don't think it will work, since I'm not sure that fd.flags will be zero
> when fd.file == NULL. It looks so by analyzing return code path
> in fs/file.c, but I wasn't sure that I followed all code paths,
> so I just picked this style from fs/timerfd.c assuming it was
> done this away on purpose and there can be the case where
> fd.file == null and fd.flags !=0. In such case we cannot call fdput().
Yeah, hm, looking around, this does seem to be the case. I guess the
thought is that when get_map_id fails, struct fd has been handled.
Maybe add a comment above that function as a reminder?
>>> + err = -EFAULT;
>>> + if (copy_to_user(uvalue, value, map->value_size) != 0)
>>> + goto free_key;
>>
>> I'm made uncomfortable with memory copying where explicit lengths from
>> userspace aren't being used. It does look like it would be redundant,
>> though. Are there other syscalls where the kernel may stomp on user
>> memory based on internal kernel sizes? I think this is fine as-is, but
>> it makes me want to think harder about it. :)
>
> good question :)
> key_size and value_size are passed initially from user space.
> Kernel only verifies and allocates internal map elements with given
> sizes. Then it copies the value back with the size it remembered.
> If user space said at map creation time that value_size is 100,
> it should be using it consistently in user space program.
Yeah, I think this should be fine as-is.
>
>>> + err = -ENOMEM;
>>> + next_key = kmalloc(map->key_size, GFP_ATOMIC);
>>
>> In the interests of defensiveness, I'd use kzalloc here.
>
> I think it would be an overkill. Map implementation must consume
> all bytes of incoming 'key' and return exactly the same number
> of bytes in 'next_key'. Otherwise the whole iteration over map
> with 'get_next_key' won't work. So if map implementation is
> broken, it will be seen right away. No security leak here :)
Okay, fair enough. I had a few similar suggestions later. I kind of
wish there was a kcalloc that didn't zero memory to handle the case of
multiplied size input, but no need to spend the time clearing.
>
>>> + case BPF_MAP_GET_NEXT_KEY:
>>> + return map_get_next_key((int) arg2, (void __user *) arg3,
>>> + (void __user *) arg4);
>>
>> Same observation as the other syscall cmd: perhaps arg5 == 0 should be
>> checked? Also, since each of these functions looks up the fd and
>
> yes. will do.
>
>> builds the key, maybe those should be added to a common helper instead
>> of copy/pasting into each demuxed function?
>
> well, get_map_id() is a common helper. I didn't move fdget() all
> the way to switch statement, since it looks less readable.
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH RFC v2 net-next 09/16] bpf: expand BPF syscall with program load/unload
From: Alexei Starovoitov @ 2014-07-23 20:22 UTC (permalink / raw)
To: Kees Cook
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Daniel Borkmann, Chema Gonzalez, Eric Dumazet,
Peter Zijlstra, Arnaldo Carvalho de Melo, Jiri Olsa,
Thomas Gleixner, H. Peter Anvin, Andrew Morton, Linux API,
Network Development, LKML
In-Reply-To: <CAGXu5j+gMepR-euMhBjYqMVHsdJM-Do5yc0rLw5dMK-C9BBieA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wed, Jul 23, 2014 at 12:00 PM, Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> On Thu, Jul 17, 2014 at 9:19 PM, Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org> wrote:
>> eBPF programs are safe run-to-completion functions with load/unload
>> methods from userspace similar to kernel modules.
>>
>> User space API:
>>
>> - load eBPF program
>> fd = bpf_prog_load(bpf_prog_type, struct nlattr *prog, int len)
>>
>> where 'prog' is a sequence of sections (TEXT, LICENSE, MAP_ASSOC)
>> TEXT - array of eBPF instructions
>> LICENSE - must be GPL compatible to call helper functions marked gpl_only
>> MAP_FIXUP - array of {insn idx, map fd} used by kernel to adjust
>> imm constants in 'mov' instructions used to access maps
>
> Nit: naming mismatch between MAP_ASSOC vs MAP_FIXUP.
ohh yes, I used map_assoc name initially and forgot to rename it
in commit log. will fix.
>> + */
>> +static int fixup_bpf_map_id(struct sk_filter *prog, struct nlattr *map_fixup)
>> +{
>> + struct {
>> + u32 insn_idx;
>> + u32 ufd;
>> + } *fixup = nla_data(map_fixup);
>> + int fixup_len = nla_len(map_fixup) / sizeof(*fixup);
>> + struct bpf_insn *insn;
>> + struct fd f;
>> + u32 idx;
>> + int i, map_id;
>> +
>> + if (fixup_len <= 0)
>> + return -EINVAL;
>> +
>> + for (i = 0; i < fixup_len; i++) {
>> + idx = fixup[i].insn_idx;
>> + if (idx >= prog->len)
>> + return -EINVAL;
>> +
>> + insn = &prog->insnsi[idx];
>> + if (insn->code != (BPF_ALU64 | BPF_MOV | BPF_K) &&
>> + insn->code != (BPF_ALU | BPF_MOV | BPF_K))
>> + return -EINVAL;
>> +
>> + f = fdget(fixup[i].ufd);
>> +
>> + map_id = get_map_id(f);
>> +
>> + if (map_id < 0)
>> + return map_id;
>> +
>> + insn->imm = map_id;
>> + fdput(f);
>
> It looks like there a potentially race risk of a map_id changing out
> from under a running program? Between the call to fixup_bpf_map_id()
> and the bpf_map_get() calls during bpf_prog_load() below...
Excellent question!
If user space created a bunch of maps and has another thread
that closes fds (and may be creating new maps) while main thread is
doing syscall(prog_load,...) then map_ids stored inside instructions
can become stale by the time bpf_check() is called. In such case
bpf_check() will reject the program. Either it will find unknown map_id
or map will have invalid key/value ranges for the program to access.
So this is not an issue, but I agree the code is not obviously correct.
My bad to allow such subtle races.
I'll increase mutex_lock() range.
>> + if (tb[BPF_PROG_MAP_FIXUP]) {
>> + /* if program is using maps, fixup map_ids */
>> + err = fixup_bpf_map_id(prog, tb[BPF_PROG_MAP_FIXUP]);
>> + if (err < 0)
>> + goto free_prog;
>> + }
>> +
>> + /* allocate eBPF related auxilary data */
>> + prog->info = kzalloc(sizeof(struct bpf_prog_info), GFP_USER);
>> + if (!prog->info)
>> + goto free_prog;
>> + prog->ebpf = 1;
>> + prog->info->is_gpl_compatible = is_gpl;
>> +
>> + /* find program type: socket_filter vs tracing_filter */
>> + err = find_prog_type(type, prog);
>> + if (err < 0)
>> + goto free_prog;
>> +
>> + /* lock maps to prevent any changes to maps, since eBPF program may
>> + * use them. In such case bpf_check() will populate prog->used_maps
>> + */
>> + mutex_lock(&bpf_map_lock);
>> +
>> + /* run eBPF verifier */
>> + /* err = bpf_check(prog); */
>> +
>> + if (err == 0 && prog->info->used_maps) {
>> + /* program passed verifier and it's using some maps,
>> + * hold them
>> + */
>> + for (i = 0; i < prog->info->used_map_cnt; i++) {
>> + map = bpf_map_get(prog->info->used_maps[i]);
>> + BUG_ON(!map);
>> + atomic_inc(&map->refcnt);
>> + }
>> + }
>> + mutex_unlock(&bpf_map_lock);
>
> As mentioned above, I think fixup_bpf_map_id needs to be done under
> the map_lock mutex, unless I'm misunderstanding something in the
> object lifetime.
yes. Excellent point. Will increase the range.
Thank you so much for the review!
^ permalink raw reply
* Re: [PATCH RFC v2 net-next 08/16] bpf: add hashtable type of BPF maps
From: Alexei Starovoitov @ 2014-07-23 19:57 UTC (permalink / raw)
To: Kees Cook
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Daniel Borkmann, Chema Gonzalez, Eric Dumazet,
Peter Zijlstra, Arnaldo Carvalho de Melo, Jiri Olsa,
Thomas Gleixner, H. Peter Anvin, Andrew Morton, Linux API,
Network Development, LKML
In-Reply-To: <CAGXu5jK1hrwvPs7f+mSOer+81J9SBxwMrqb=6fFeZQ7hd-FMzA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wed, Jul 23, 2014 at 11:36 AM, Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>> +static struct bpf_map *htab_map_alloc(struct nlattr *attr[BPF_MAP_ATTR_MAX + 1])
>> +{
>> + struct bpf_htab *htab;
>> + int err, i;
>> +
>> + htab = kmalloc(sizeof(*htab), GFP_USER);
>
> I'd prefer kzalloc here.
in this case I agree. will change, since it's not in critical path and we
can waste few cycles zeroing memory.
>> + err = -ENOMEM;
>> + htab->buckets = kmalloc(htab->n_buckets * sizeof(struct hlist_head),
>> + GFP_USER);
>
> I'd prefer kcalloc here, even though n_buckets can't currently trigger
> an integer overflow.
hmm, I would argue that kmalloc_array is a preferred way, but kcalloc ?
Few lines below the whole array is inited with INIT_HLIST_HEAD...
>> + for (i = 0; i < htab->n_buckets; i++)
>> + INIT_HLIST_HEAD(&htab->buckets[i]);
>> + htab->slab_name = kasprintf(GFP_USER, "bpf_htab_%p", htab);
>
> This leaks a kernel heap memory pointer to userspace. If a unique name
> needed, I think map_id should be used instead.
it leaks, how? slabinfo is only available to root.
The same code exists in conntrack:
net/netfilter/nf_conntrack_core.c:1767
^ permalink raw reply
* Re: [PATCH 5/5] cgroup: introduce cgroup namespaces
From: Aditya Kali @ 2014-07-23 19:52 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Linux API, Linux Containers,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Tejun Heo,
cgroups-u79uwXL29TY76Z2rM5mHXA, Ingo Molnar
In-Reply-To: <CALCETrUhd41LFfF9epbVYJSOwqBq308Z8RZG9tzyPfx+Joe15Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Mon, Jul 21, 2014 at 3:16 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
> On Mon, Jul 21, 2014 at 3:11 PM, Aditya Kali <adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>> On Fri, Jul 18, 2014 at 11:57 AM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>>> On Fri, Jul 18, 2014 at 11:51 AM, Aditya Kali <adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>>>> On Fri, Jul 18, 2014 at 9:51 AM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>>>>> On Jul 17, 2014 1:56 PM, "Aditya Kali" <adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>>>>>>
>>>>>> On Thu, Jul 17, 2014 at 12:57 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>>>>>> > What happens if someone moves a task in a cgroup namespace outside of
>>>>>> > the namespace root cgroup?
>>>>>> >
>>>>>>
>>>>>> Attempt to move a task outside of cgroupns root will fail with EPERM.
>>>>>> This is true irrespective of the privileges of the process attempting
>>>>>> this. Once cgroupns is created, the task will be confined to the
>>>>>> cgroup hierarchy under its cgroupns root until it dies.
>>>>>
>>>>> Can a task in a non-init userns create a cgroupns? If not, that's
>>>>> unusual. If so, is it problematic if they can prevent themselves from
>>>>> being moved?
>>>>>
>>>>
>>>> Currently, only a task with CAP_SYS_ADMIN in the init-userns can
>>>> create cgroupns. It is stricter than for other namespaces, yes.
>>>
>>> I'm slightly hesitant to have unshare(CLONE_NEWUSER |
>>> CLONE_NEWCGROUPNS | ...) start having weird side effects that are
>>> visible outside the namespace, especially when those side effects
>>> don't happen (because the call fails entirely) if
>>> unshare(CLONE_NEWUSER) happens first. I don't see a real problem with
>>> it, but it's weird.
>>>
>>
>> I expect this to be only in the initial version of the patch. We can
>> make this consistent with other namespaces once we figure out how
>> cgroupns can be safely enabled for non-init-userns.
>>
>>>>
>>>>> I hate to say it, but it might be worth requiring explicit permission
>>>>> from the cgroup manager for this. For example, there could be a new
>>>>> cgroup attribute may_unshare, and any attempt to unshare the cgroup ns
>>>>> will fail with -EPERM unless the caller is in a may_share=1 cgroup.
>>>>> may_unshare in a parent cgroup would not give child cgroups the
>>>>> ability to unshare.
>>>>>
>>>>
>>>> What you suggest can be done. The current patch-set punts the problem
>>>> of permission checking by only allowing unshare from a
>>>> capable(CAP_SYS_ADMIN) process. This can be implemented as a follow-up
>>>> improvement to cgroupns feature if we want to open it to non-init
>>>> userns.
>>>>
>>>> Being said that, I would argue that even if we don't have this
>>>> explicit permission and relax the check to non-init userns, it should
>>>> be 'OK' to let ns_capable(current_user_ns(), CAP_SYS_ADMIN) tasks to
>>>> unshare cgroupns (basically, if you can "create" a cgroup hierarchy,
>>>> you should probably be allowed to unshare() it).
>>>
>>> But non-init-userns tasks can't create cgroup hierarchies, unless I
>>> misunderstand the current code. And, if they can, I bet I can find
>>> three or four serious security issues in an hour or two. :)
>>>
>>
>> Task running in non-init userns can create cgroup hierarchies if you
>> chown/chgrp their cgroup root to the task user:
>
> Won't the systemd people hate you forever for this suggestion? (I do
> exactly this myself...)
>
I was actually thinking this feature will really simplify container
management tools (since cgroupns allows you to recursively run them
inside containers without any hacks). I would appreciate any feedback
from them on how we can improve this to help their usecase.
Thanks for your comments!
>
>> This is a powerful feature as it allows non-root tasks to run
>> container-management tools and provision their resources properly. But
>> this makes implementing your suggestion of having 'cgroup.may_unshare'
>> file tricky as the cgroup owner (task) will be able to set it and
>> still unshare cgroupns. Instead, may be we could just check if the
>> task has appropriate (write?) permissions on the cgroup directory
>> before allowing nested cgroupns creation.
>
> I bet that systemd will want to set may_unshare but not give write
> access. Who knows?
>
>> [shudder]
>> I am surprised that this even works correctly.
>>
>> Either way, may be checking cgroup directory permissions will work for
>> you? i.e., if you "chown" a cgroup directory to the user, it should be
>> OK if the user's task unshares cgroupns under that cgroup and you
>> don't care about moving tasks from under that cgroup. Without
>> ownership of the cgroup directory, creation of cgroupns will be
>> disallowed. What do you think?
>
> I think this is *safe* but may not useful for eventual systemd stuff.
> Not really sure.
>
> --Andy
--
Aditya
^ permalink raw reply
* Re: [PATCH RFC v2 net-next 07/16] bpf: add lookup/update/delete/iterate methods to BPF maps
From: Alexei Starovoitov @ 2014-07-23 19:49 UTC (permalink / raw)
To: Kees Cook
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Daniel Borkmann, Chema Gonzalez, Eric Dumazet,
Peter Zijlstra, Arnaldo Carvalho de Melo, Jiri Olsa,
Thomas Gleixner, H. Peter Anvin, Andrew Morton, Linux API,
Network Development, LKML
In-Reply-To: <CAGXu5j+cvG5BhNSA=UrcbYs3LPSZUZyj4_ey8=9O0KgbUYUd9g@mail.gmail.com>
On Wed, Jul 23, 2014 at 11:25 AM, Kees Cook <keescook@chromium.org> wrote:
>> +
>> + /* lookup key in a given map referenced by map_id
>> + * err = bpf_map_lookup_elem(int map_id, void *key, void *value)
>
> This needs map_id documentation updates too?
yes. will grep for it just to make sure.
>> +static int get_map_id(struct fd f)
>> +{
>> + struct bpf_map *map;
>> +
>> + if (!f.file)
>> + return -EBADF;
>> +
>> + if (f.file->f_op != &bpf_map_fops) {
>> + fdput(f);
>
> It feels weird to me to do the fdput inside this function. Instead,
> should map_lookup_elem get a "err_put" label, instead?
I don't think it will work, since I'm not sure that fd.flags will be zero
when fd.file == NULL. It looks so by analyzing return code path
in fs/file.c, but I wasn't sure that I followed all code paths,
so I just picked this style from fs/timerfd.c assuming it was
done this away on purpose and there can be the case where
fd.file == null and fd.flags !=0. In such case we cannot call fdput().
>> + err = -EFAULT;
>> + if (copy_to_user(uvalue, value, map->value_size) != 0)
>> + goto free_key;
>
> I'm made uncomfortable with memory copying where explicit lengths from
> userspace aren't being used. It does look like it would be redundant,
> though. Are there other syscalls where the kernel may stomp on user
> memory based on internal kernel sizes? I think this is fine as-is, but
> it makes me want to think harder about it. :)
good question :)
key_size and value_size are passed initially from user space.
Kernel only verifies and allocates internal map elements with given
sizes. Then it copies the value back with the size it remembered.
If user space said at map creation time that value_size is 100,
it should be using it consistently in user space program.
>> + err = -ENOMEM;
>> + next_key = kmalloc(map->key_size, GFP_ATOMIC);
>
> In the interests of defensiveness, I'd use kzalloc here.
I think it would be an overkill. Map implementation must consume
all bytes of incoming 'key' and return exactly the same number
of bytes in 'next_key'. Otherwise the whole iteration over map
with 'get_next_key' won't work. So if map implementation is
broken, it will be seen right away. No security leak here :)
>> + case BPF_MAP_GET_NEXT_KEY:
>> + return map_get_next_key((int) arg2, (void __user *) arg3,
>> + (void __user *) arg4);
>
> Same observation as the other syscall cmd: perhaps arg5 == 0 should be
> checked? Also, since each of these functions looks up the fd and
yes. will do.
> builds the key, maybe those should be added to a common helper instead
> of copy/pasting into each demuxed function?
well, get_map_id() is a common helper. I didn't move fdget() all
the way to switch statement, since it looks less readable.
^ permalink raw reply
* Re: [PATCH RFC v2 net-next 05/16] bpf: introduce syscall(BPF, ...) and BPF maps
From: Alexei Starovoitov @ 2014-07-23 19:30 UTC (permalink / raw)
To: Kees Cook
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Daniel Borkmann, Chema Gonzalez, Eric Dumazet,
Peter Zijlstra, Arnaldo Carvalho de Melo, Jiri Olsa,
Thomas Gleixner, H. Peter Anvin, Andrew Morton, Linux API,
Network Development, LKML
In-Reply-To: <CAGXu5jJAOyA=UB-k4fmu84RTB_ygHAs2UqtbBfUPs2JCaxo0Eg@mail.gmail.com>
On Wed, Jul 23, 2014 at 11:02 AM, Kees Cook <keescook@chromium.org> wrote:
>> --- a/Documentation/networking/filter.txt
>> +++ b/Documentation/networking/filter.txt
>>
>> +eBPF maps
>> +---------
>> +'maps' is a generic storage of different types for sharing data between kernel
>> +and userspace.
>> +
>> +The maps are accessed from user space via BPF syscall, which has commands:
>> +- create a map with given id, type and attributes
>> + map_id = bpf_map_create(int map_id, map_type, struct nlattr *attr, int len)
>> + returns positive map id or negative error
>
> Looks like these docs need updating for the fd-based approach instead
> of the map_id approach?
ohh, yes. updated it in srcs and in commit log, but forgot in docs.
>> +SYSCALL_DEFINE5(bpf, int, cmd, unsigned long, arg2, unsigned long, arg3,
>> + unsigned long, arg4, unsigned long, arg5)
>> +{
>> + if (!capable(CAP_SYS_ADMIN))
>> + return -EPERM;
>
> It might be valuable to have a comment here describing why this is
> currently limited to CAP_SYS_ADMIN.
makes sense.
There are several reasons it should be limited to root initially:
- to phase changes in gradually
- verifier is not detecting pointer leaks yet
- full security audit wasn't performed
- tracing and network analytics are root only anyway
Currently eBPF is safe (non-crashing), since safety is relatively easy
to enforce by static analysis. For somebody with compiler background
it's natural to think about bounds, alignments, uninitialized access, etc.
So I'm confident that I didn't miss anything big in 'safety' aspect.
'Non-root security' is harder. I'll add pointer leak detection first
and will ask for more suggestions.
>> + switch (cmd) {
>> + case BPF_MAP_CREATE:
>> + return map_create((enum bpf_map_type) arg2,
>> + (struct nlattr __user *) arg3, (int) arg4);
>
> I'd recommend requiring arg5 == 0 here, just for future flexibility.
Though I expect all extensions to go through nlattr attributes,
it's indeed cleaner to enforce arg5==0 here and for all other cmds.
^ permalink raw reply
* Re: [PATCH RFC v2 net-next 09/16] bpf: expand BPF syscall with program load/unload
From: Kees Cook @ 2014-07-23 19:00 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Daniel Borkmann, Chema Gonzalez, Eric Dumazet,
Peter Zijlstra, Arnaldo Carvalho de Melo, Jiri Olsa,
Thomas Gleixner, H. Peter Anvin, Andrew Morton, Linux API,
Network Development, LKML
In-Reply-To: <1405657206-12060-10-git-send-email-ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
On Thu, Jul 17, 2014 at 9:19 PM, Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org> wrote:
> eBPF programs are safe run-to-completion functions with load/unload
> methods from userspace similar to kernel modules.
>
> User space API:
>
> - load eBPF program
> fd = bpf_prog_load(bpf_prog_type, struct nlattr *prog, int len)
>
> where 'prog' is a sequence of sections (TEXT, LICENSE, MAP_ASSOC)
> TEXT - array of eBPF instructions
> LICENSE - must be GPL compatible to call helper functions marked gpl_only
> MAP_FIXUP - array of {insn idx, map fd} used by kernel to adjust
> imm constants in 'mov' instructions used to access maps
Nit: naming mismatch between MAP_ASSOC vs MAP_FIXUP.
>
> - unload eBPF program
> close(fd)
>
> User space example of syscall(__NR_bpf, BPF_PROG_LOAD, prog_type, ...)
> follows in later patches
>
> Signed-off-by: Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
> ---
> include/linux/bpf.h | 33 +++++
> include/linux/filter.h | 9 +-
> include/uapi/linux/bpf.h | 29 +++++
> kernel/bpf/core.c | 5 +-
> kernel/bpf/syscall.c | 309 ++++++++++++++++++++++++++++++++++++++++++++++
> net/core/filter.c | 9 +-
> 6 files changed, 388 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 91e2caf8edf9..4967619595cc 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -46,4 +46,37 @@ struct bpf_map_type_list {
> void bpf_register_map_type(struct bpf_map_type_list *tl);
> struct bpf_map *bpf_map_get(u32 map_id);
>
> +/* eBPF function prototype used by verifier to allow BPF_CALLs from eBPF programs
> + * to in-kernel helper functions and for adjusting imm32 field in BPF_CALL
> + * instructions after verifying
> + */
> +struct bpf_func_proto {
> + u64 (*func)(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5);
> + bool gpl_only;
> +};
> +
> +struct bpf_verifier_ops {
> + /* return eBPF function prototype for verification */
> + const struct bpf_func_proto *(*get_func_proto)(enum bpf_func_id func_id);
> +};
> +
> +struct bpf_prog_type_list {
> + struct list_head list_node;
> + struct bpf_verifier_ops *ops;
> + enum bpf_prog_type type;
> +};
> +
> +void bpf_register_prog_type(struct bpf_prog_type_list *tl);
> +
> +struct bpf_prog_info {
> + bool is_gpl_compatible;
> + enum bpf_prog_type prog_type;
> + struct bpf_verifier_ops *ops;
> + u32 *used_maps;
> + u32 used_map_cnt;
> +};
> +
> +void free_bpf_prog_info(struct bpf_prog_info *info);
> +struct sk_filter *bpf_prog_get(u32 ufd);
> +
> #endif /* _LINUX_BPF_H */
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index b43ad6a2b3cf..822b310e75e1 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -30,12 +30,17 @@ struct sock_fprog_kern {
> struct sk_buff;
> struct sock;
> struct seccomp_data;
> +struct bpf_prog_info;
>
> struct sk_filter {
> atomic_t refcnt;
> u32 jited:1, /* Is our filter JIT'ed? */
> - len:31; /* Number of filter blocks */
> - struct sock_fprog_kern *orig_prog; /* Original BPF program */
> + ebpf:1, /* Is it eBPF program ? */
> + len:30; /* Number of filter blocks */
> + union {
> + struct sock_fprog_kern *orig_prog; /* Original BPF program */
> + struct bpf_prog_info *info;
> + };
> struct rcu_head rcu;
> unsigned int (*bpf_func)(const struct sk_buff *skb,
> const struct bpf_insn *filter);
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 3ea11ba053a8..06ba71b49f64 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -333,6 +333,13 @@ enum bpf_cmd {
> * returns zero and stores next key or negative error
> */
> BPF_MAP_GET_NEXT_KEY,
> +
> + /* verify and load eBPF program
> + * prog_id = bpf_prog_load(bpf_prog_type, struct nlattr *prog, int len)
> + * prog is a sequence of sections
> + * returns fd or negative error
> + */
> + BPF_PROG_LOAD,
> };
>
> enum bpf_map_attributes {
> @@ -350,4 +357,26 @@ enum bpf_map_type {
> BPF_MAP_TYPE_HASH,
> };
>
> +enum bpf_prog_attributes {
> + BPF_PROG_UNSPEC,
> + BPF_PROG_TEXT, /* array of eBPF instructions */
> + BPF_PROG_LICENSE, /* license string */
> + BPF_PROG_MAP_FIXUP, /* array of {insn idx, map fd} to fixup insns */
> + __BPF_PROG_ATTR_MAX,
> +};
> +#define BPF_PROG_ATTR_MAX (__BPF_PROG_ATTR_MAX - 1)
> +#define BPF_PROG_MAX_ATTR_SIZE 65535
> +
> +enum bpf_prog_type {
> + BPF_PROG_TYPE_UNSPEC,
> +};
> +
> +/* integer value in 'imm' field of BPF_CALL instruction selects which helper
> + * function eBPF program intends to call
> + */
> +enum bpf_func_id {
> + BPF_FUNC_unspec,
> + __BPF_FUNC_MAX_ID,
> +};
> +
> #endif /* _UAPI__LINUX_BPF_H__ */
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 265a02cc822d..e65ecdc36358 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -23,6 +23,7 @@
> #include <linux/filter.h>
> #include <linux/skbuff.h>
> #include <asm/unaligned.h>
> +#include <linux/bpf.h>
>
> /* Registers */
> #define BPF_R0 regs[BPF_REG_0]
> @@ -528,9 +529,11 @@ void sk_filter_select_runtime(struct sk_filter *fp)
> }
> EXPORT_SYMBOL_GPL(sk_filter_select_runtime);
>
> -/* free internal BPF program */
> +/* free internal BPF program, called after RCU grace period */
> void sk_filter_free(struct sk_filter *fp)
> {
> + if (fp->ebpf)
> + free_bpf_prog_info(fp->info);
> bpf_jit_free(fp);
> }
> EXPORT_SYMBOL_GPL(sk_filter_free);
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index ca2be66845b3..9e45ca6b6937 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -14,6 +14,8 @@
> #include <net/netlink.h>
> #include <linux/anon_inodes.h>
> #include <linux/file.h>
> +#include <linux/license.h>
> +#include <linux/filter.h>
>
> /* mutex to protect insertion/deletion of map_id in IDR */
> static DEFINE_MUTEX(bpf_map_lock);
> @@ -406,6 +408,310 @@ err_unlock:
> return err;
> }
>
> +static LIST_HEAD(bpf_prog_types);
> +
> +static int find_prog_type(enum bpf_prog_type type, struct sk_filter *prog)
> +{
> + struct bpf_prog_type_list *tl;
> +
> + list_for_each_entry(tl, &bpf_prog_types, list_node) {
> + if (tl->type == type) {
> + prog->info->ops = tl->ops;
> + prog->info->prog_type = type;
> + return 0;
> + }
> + }
> + return -EINVAL;
> +}
> +
> +void bpf_register_prog_type(struct bpf_prog_type_list *tl)
> +{
> + list_add(&tl->list_node, &bpf_prog_types);
> +}
> +
> +/* fixup insn->imm field of bpf_call instructions:
> + * if (insn->imm == BPF_FUNC_map_lookup_elem)
> + * insn->imm = bpf_map_lookup_elem - __bpf_call_base;
> + * else if (insn->imm == BPF_FUNC_map_update_elem)
> + * insn->imm = bpf_map_update_elem - __bpf_call_base;
> + * else ...
> + *
> + * this function is called after eBPF program passed verification
> + */
> +static void fixup_bpf_calls(struct sk_filter *prog)
> +{
> + const struct bpf_func_proto *fn;
> + int i;
> +
> + for (i = 0; i < prog->len; i++) {
> + struct bpf_insn *insn = &prog->insnsi[i];
> +
> + if (insn->code == (BPF_JMP | BPF_CALL)) {
> + /* we reach here when program has bpf_call instructions
> + * and it passed bpf_check(), means that
> + * ops->get_func_proto must have been supplied, check it
> + */
> + BUG_ON(!prog->info->ops->get_func_proto);
> +
> + fn = prog->info->ops->get_func_proto(insn->imm);
> + /* all functions that have prototype and verifier allowed
> + * programs to call them, must be real in-kernel functions
> + */
> + BUG_ON(!fn->func);
> + insn->imm = fn->func - __bpf_call_base;
> + }
> + }
> +}
> +
> +/* fixup instructions that are using map_ids:
> + *
> + * BPF_MOV64_IMM(BPF_REG_1, MAP_ID), // r1 = MAP_ID
> + * BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
> + *
> + * in the 1st insn kernel replaces MAP_ID with global map_id,
> + * since programs are executing out of different contexts and must use
> + * globally visible ids to access maps
> + *
> + * map_fixup is an array of pairs {insn idx, map ufd}
> + *
> + * kernel resolves ufd -> global map_id and adjusts eBPF instructions
> + */
> +static int fixup_bpf_map_id(struct sk_filter *prog, struct nlattr *map_fixup)
> +{
> + struct {
> + u32 insn_idx;
> + u32 ufd;
> + } *fixup = nla_data(map_fixup);
> + int fixup_len = nla_len(map_fixup) / sizeof(*fixup);
> + struct bpf_insn *insn;
> + struct fd f;
> + u32 idx;
> + int i, map_id;
> +
> + if (fixup_len <= 0)
> + return -EINVAL;
> +
> + for (i = 0; i < fixup_len; i++) {
> + idx = fixup[i].insn_idx;
> + if (idx >= prog->len)
> + return -EINVAL;
> +
> + insn = &prog->insnsi[idx];
> + if (insn->code != (BPF_ALU64 | BPF_MOV | BPF_K) &&
> + insn->code != (BPF_ALU | BPF_MOV | BPF_K))
> + return -EINVAL;
> +
> + f = fdget(fixup[i].ufd);
> +
> + map_id = get_map_id(f);
> +
> + if (map_id < 0)
> + return map_id;
> +
> + insn->imm = map_id;
> + fdput(f);
It looks like there a potentially race risk of a map_id changing out
from under a running program? Between the call to fixup_bpf_map_id()
and the bpf_map_get() calls during bpf_prog_load() below...
> + }
> + return 0;
> +}
> +
> +/* free eBPF program auxilary data, called after rcu grace period,
> + * so it's safe to drop refcnt on maps used by this program
> + *
> + * called from sk_filter_release()->sk_filter_release_rcu()->sk_filter_free()
> + */
> +void free_bpf_prog_info(struct bpf_prog_info *info)
> +{
> + bool found;
> + int i;
> +
> + for (i = 0; i < info->used_map_cnt; i++) {
> + found = bpf_map_put(info->used_maps[i]);
> + /* all maps that this program was using should obviously still
> + * be there
> + */
> + BUG_ON(!found);
> + }
> + kfree(info);
> +}
> +
> +static int bpf_prog_release(struct inode *inode, struct file *filp)
> +{
> + struct sk_filter *prog = filp->private_data;
> +
> + sk_unattached_filter_destroy(prog);
> + return 0;
> +}
> +
> +static const struct file_operations bpf_prog_fops = {
> + .release = bpf_prog_release,
> +};
> +
> +static const struct nla_policy prog_policy[BPF_PROG_ATTR_MAX + 1] = {
> + [BPF_PROG_TEXT] = { .type = NLA_BINARY },
> + [BPF_PROG_LICENSE] = { .type = NLA_NUL_STRING },
> + [BPF_PROG_MAP_FIXUP] = { .type = NLA_BINARY },
> +};
> +
> +static int bpf_prog_load(enum bpf_prog_type type, struct nlattr __user *uattr,
> + int len)
> +{
> + struct nlattr *tb[BPF_PROG_ATTR_MAX + 1];
> + struct sk_filter *prog;
> + struct bpf_map *map;
> + struct nlattr *attr;
> + size_t insn_len;
> + int err, i;
> + bool is_gpl;
> +
> + if (len <= 0 || len > BPF_PROG_MAX_ATTR_SIZE)
> + return -EINVAL;
> +
> + attr = kmalloc(len, GFP_USER);
> + if (!attr)
> + return -ENOMEM;
> +
> + /* copy eBPF program from user space */
> + err = -EFAULT;
> + if (copy_from_user(attr, uattr, len) != 0)
> + goto free_attr;
> +
> + /* perform basic validation */
> + err = nla_parse(tb, BPF_PROG_ATTR_MAX, attr, len, prog_policy);
> + if (err < 0)
> + goto free_attr;
> +
> + err = -EINVAL;
> + /* look for mandatory license string */
> + if (!tb[BPF_PROG_LICENSE])
> + goto free_attr;
> +
> + /* eBPF programs must be GPL compatible to use GPL-ed functions */
> + is_gpl = license_is_gpl_compatible(nla_data(tb[BPF_PROG_LICENSE]));
> +
> + /* look for mandatory array of eBPF instructions */
> + if (!tb[BPF_PROG_TEXT])
> + goto free_attr;
> +
> + insn_len = nla_len(tb[BPF_PROG_TEXT]);
> + if (insn_len % sizeof(struct bpf_insn) != 0 || insn_len <= 0)
> + goto free_attr;
> +
> + /* plain sk_filter allocation */
> + err = -ENOMEM;
> + prog = kmalloc(sk_filter_size(insn_len), GFP_USER);
> + if (!prog)
> + goto free_attr;
> +
> + prog->len = insn_len / sizeof(struct bpf_insn);
> + memcpy(prog->insns, nla_data(tb[BPF_PROG_TEXT]), insn_len);
> + prog->orig_prog = NULL;
> + prog->jited = 0;
> + prog->ebpf = 0;
> + atomic_set(&prog->refcnt, 1);
> +
> + if (tb[BPF_PROG_MAP_FIXUP]) {
> + /* if program is using maps, fixup map_ids */
> + err = fixup_bpf_map_id(prog, tb[BPF_PROG_MAP_FIXUP]);
> + if (err < 0)
> + goto free_prog;
> + }
> +
> + /* allocate eBPF related auxilary data */
> + prog->info = kzalloc(sizeof(struct bpf_prog_info), GFP_USER);
> + if (!prog->info)
> + goto free_prog;
> + prog->ebpf = 1;
> + prog->info->is_gpl_compatible = is_gpl;
> +
> + /* find program type: socket_filter vs tracing_filter */
> + err = find_prog_type(type, prog);
> + if (err < 0)
> + goto free_prog;
> +
> + /* lock maps to prevent any changes to maps, since eBPF program may
> + * use them. In such case bpf_check() will populate prog->used_maps
> + */
> + mutex_lock(&bpf_map_lock);
> +
> + /* run eBPF verifier */
> + /* err = bpf_check(prog); */
> +
> + if (err == 0 && prog->info->used_maps) {
> + /* program passed verifier and it's using some maps,
> + * hold them
> + */
> + for (i = 0; i < prog->info->used_map_cnt; i++) {
> + map = bpf_map_get(prog->info->used_maps[i]);
> + BUG_ON(!map);
> + atomic_inc(&map->refcnt);
> + }
> + }
> + mutex_unlock(&bpf_map_lock);
As mentioned above, I think fixup_bpf_map_id needs to be done under
the map_lock mutex, unless I'm misunderstanding something in the
object lifetime.
> +
> + if (err < 0)
> + goto free_prog;
> +
> + /* fixup BPF_CALL->imm field */
> + fixup_bpf_calls(prog);
> +
> + /* eBPF program is ready to be JITed */
> + sk_filter_select_runtime(prog);
> +
> + err = anon_inode_getfd("bpf-prog", &bpf_prog_fops, prog, O_RDWR | O_CLOEXEC);
> +
> + if (err < 0)
> + /* failed to allocate fd */
> + goto free_prog;
> +
> + /* user supplied eBPF prog attributes are no longer needed */
> + kfree(attr);
> +
> + return err;
> +free_prog:
> + sk_filter_free(prog);
> +free_attr:
> + kfree(attr);
> + return err;
> +}
> +
> +static struct sk_filter *get_prog(struct fd f)
> +{
> + struct sk_filter *prog;
> +
> + if (!f.file)
> + return ERR_PTR(-EBADF);
> +
> + if (f.file->f_op != &bpf_prog_fops) {
> + fdput(f);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + prog = f.file->private_data;
> +
> + return prog;
> +}
> +
> +/* called from sk_attach_filter_ebpf() or from tracing filter attach
> + * pairs with
> + * sk_detach_filter()->sk_filter_uncharge()->sk_filter_release()
> + * or with
> + * sk_unattached_filter_destroy()->sk_filter_release()
> + */
> +struct sk_filter *bpf_prog_get(u32 ufd)
> +{
> + struct fd f = fdget(ufd);
> + struct sk_filter *prog;
> +
> + prog = get_prog(f);
> +
> + if (IS_ERR(prog))
> + return prog;
> +
> + atomic_inc(&prog->refcnt);
> + fdput(f);
> + return prog;
> +}
> +
> SYSCALL_DEFINE5(bpf, int, cmd, unsigned long, arg2, unsigned long, arg3,
> unsigned long, arg4, unsigned long, arg5)
> {
> @@ -428,6 +734,9 @@ SYSCALL_DEFINE5(bpf, int, cmd, unsigned long, arg2, unsigned long, arg3,
> case BPF_MAP_GET_NEXT_KEY:
> return map_get_next_key((int) arg2, (void __user *) arg3,
> (void __user *) arg4);
> + case BPF_PROG_LOAD:
> + return bpf_prog_load((enum bpf_prog_type) arg2,
> + (struct nlattr __user *) arg3, (int) arg4);
> default:
> return -EINVAL;
> }
> diff --git a/net/core/filter.c b/net/core/filter.c
> index f3b2d5e9fe5f..255dba1bb678 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -835,7 +835,7 @@ static void sk_release_orig_filter(struct sk_filter *fp)
> {
> struct sock_fprog_kern *fprog = fp->orig_prog;
>
> - if (fprog) {
> + if (!fp->ebpf && fprog) {
> kfree(fprog->filter);
> kfree(fprog);
> }
> @@ -867,14 +867,16 @@ static void sk_filter_release(struct sk_filter *fp)
>
> void sk_filter_uncharge(struct sock *sk, struct sk_filter *fp)
> {
> - atomic_sub(sk_filter_size(fp->len), &sk->sk_omem_alloc);
> + if (!fp->ebpf)
> + atomic_sub(sk_filter_size(fp->len), &sk->sk_omem_alloc);
> sk_filter_release(fp);
> }
>
> void sk_filter_charge(struct sock *sk, struct sk_filter *fp)
> {
> atomic_inc(&fp->refcnt);
> - atomic_add(sk_filter_size(fp->len), &sk->sk_omem_alloc);
> + if (!fp->ebpf)
> + atomic_add(sk_filter_size(fp->len), &sk->sk_omem_alloc);
> }
>
> static struct sk_filter *__sk_migrate_realloc(struct sk_filter *fp,
> @@ -978,6 +980,7 @@ static struct sk_filter *__sk_prepare_filter(struct sk_filter *fp,
>
> fp->bpf_func = NULL;
> fp->jited = 0;
> + fp->ebpf = 0;
>
> err = sk_chk_filter(fp->insns, fp->len);
> if (err) {
> --
> 1.7.9.5
>
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH RFC v2 net-next 02/16] bpf: update MAINTAINERS entry
From: Kees Cook @ 2014-07-23 18:39 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Daniel Borkmann, Chema Gonzalez, Eric Dumazet,
Peter Zijlstra, Arnaldo Carvalho de Melo, Jiri Olsa,
Thomas Gleixner, H. Peter Anvin, Andrew Morton, Linux API,
Network Development, LKML
In-Reply-To: <CAMEtUuzcMcYnRryykqb9LQWjnVmYi3ErMtGQ+9nyY5uS+OpwpQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wed, Jul 23, 2014 at 10:48 AM, Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org> wrote:
> On Wed, Jul 23, 2014 at 10:37 AM, Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>> On Thu, Jul 17, 2014 at 9:19 PM, Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org> wrote:
>>> Signed-off-by: Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
>>> ---
>>> MAINTAINERS | 7 +++++++
>>> 1 file changed, 7 insertions(+)
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index ae8cd00215b2..32e24ff46da3 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -1912,6 +1912,13 @@ S: Supported
>>> F: drivers/net/bonding/
>>> F: include/uapi/linux/if_bonding.h
>>>
>>> +BPF (Safe dynamic programs and tools)
>>
>> bikeshed: I feel like this shouldn't be an acronym. Maybe instead:
>>
>> BERKELEY PACKET FILTER (BPF: Safe dynamic programs and tools)
>
> pile on :)
>
> I think eBPF is no longer acronym. 'e' stands for 'extended',
> but BPF is no longer 'packet filter' only and definitely not 'berkeley'.
> So I'd rather keep BPF as a magic abbreviation without spelling it out,
> since full name is historic and no longer meaningful.
> I've considered coming up with brand new abbreviation and full name
> for this instruction set, but none looked good and all lose in comparison
> to 'eBPF' name, which is concise and carries enough historical
> references to explain the idea behind new ISA.
Yeah, that's a fair point. No sense in using "BEE PEE EFF" :)
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH RFC v2 net-next 08/16] bpf: add hashtable type of BPF maps
From: Kees Cook @ 2014-07-23 18:36 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Daniel Borkmann, Chema Gonzalez, Eric Dumazet,
Peter Zijlstra, Arnaldo Carvalho de Melo, Jiri Olsa,
Thomas Gleixner, H. Peter Anvin, Andrew Morton, Linux API,
Network Development, LKML
In-Reply-To: <1405657206-12060-9-git-send-email-ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
On Thu, Jul 17, 2014 at 9:19 PM, Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org> wrote:
> add new map type: BPF_MAP_TYPE_HASH
> and its simple (not auto resizeable) hash table implementation
>
> Signed-off-by: Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
> ---
> include/uapi/linux/bpf.h | 1 +
> kernel/bpf/Makefile | 2 +-
> kernel/bpf/hashtab.c | 371 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 373 insertions(+), 1 deletion(-)
> create mode 100644 kernel/bpf/hashtab.c
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 5e1bfbc9cdc7..3ea11ba053a8 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -347,6 +347,7 @@ enum bpf_map_attributes {
>
> enum bpf_map_type {
> BPF_MAP_TYPE_UNSPEC,
> + BPF_MAP_TYPE_HASH,
> };
>
> #endif /* _UAPI__LINUX_BPF_H__ */
> diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
> index e9f7334ed07a..558e12712ebc 100644
> --- a/kernel/bpf/Makefile
> +++ b/kernel/bpf/Makefile
> @@ -1 +1 @@
> -obj-y := core.o syscall.o
> +obj-y := core.o syscall.o hashtab.o
> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> new file mode 100644
> index 000000000000..6e481cacbba3
> --- /dev/null
> +++ b/kernel/bpf/hashtab.c
> @@ -0,0 +1,371 @@
> +/* Copyright (c) 2011-2014 PLUMgrid, http://plumgrid.com
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of version 2 of the GNU General Public
> + * License as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + */
> +#include <linux/bpf.h>
> +#include <net/netlink.h>
> +#include <linux/jhash.h>
> +
> +struct bpf_htab {
> + struct bpf_map map;
> + struct hlist_head *buckets;
> + struct kmem_cache *elem_cache;
> + char *slab_name;
> + spinlock_t lock;
> + u32 count; /* number of elements in this hashtable */
> + u32 n_buckets; /* number of hash buckets */
> + u32 elem_size; /* size of each element in bytes */
> +};
> +
> +/* each htab element is struct htab_elem + key + value */
> +struct htab_elem {
> + struct hlist_node hash_node;
> + struct rcu_head rcu;
> + struct bpf_htab *htab;
> + u32 hash;
> + u32 pad;
> + char key[0];
> +};
> +
> +#define HASH_MAX_BUCKETS 1024
> +#define BPF_MAP_MAX_KEY_SIZE 256
> +static struct bpf_map *htab_map_alloc(struct nlattr *attr[BPF_MAP_ATTR_MAX + 1])
> +{
> + struct bpf_htab *htab;
> + int err, i;
> +
> + htab = kmalloc(sizeof(*htab), GFP_USER);
I'd prefer kzalloc here.
> + if (!htab)
> + return ERR_PTR(-ENOMEM);
> +
> + /* look for mandatory map attributes */
> + err = -EINVAL;
> + if (!attr[BPF_MAP_KEY_SIZE])
> + goto free_htab;
> + htab->map.key_size = nla_get_u32(attr[BPF_MAP_KEY_SIZE]);
> +
> + if (!attr[BPF_MAP_VALUE_SIZE])
> + goto free_htab;
> + htab->map.value_size = nla_get_u32(attr[BPF_MAP_VALUE_SIZE]);
> +
> + if (!attr[BPF_MAP_MAX_ENTRIES])
> + goto free_htab;
> + htab->map.max_entries = nla_get_u32(attr[BPF_MAP_MAX_ENTRIES]);
> +
> + htab->n_buckets = (htab->map.max_entries <= HASH_MAX_BUCKETS) ?
> + htab->map.max_entries : HASH_MAX_BUCKETS;
> +
> + /* hash table size must be power of 2 */
> + if ((htab->n_buckets & (htab->n_buckets - 1)) != 0)
> + goto free_htab;
> +
> + err = -E2BIG;
> + if (htab->map.key_size > BPF_MAP_MAX_KEY_SIZE)
> + goto free_htab;
> +
> + err = -ENOMEM;
> + htab->buckets = kmalloc(htab->n_buckets * sizeof(struct hlist_head),
> + GFP_USER);
I'd prefer kcalloc here, even though n_buckets can't currently trigger
an integer overflow.
> +
> + if (!htab->buckets)
> + goto free_htab;
> +
> + for (i = 0; i < htab->n_buckets; i++)
> + INIT_HLIST_HEAD(&htab->buckets[i]);
> +
> + spin_lock_init(&htab->lock);
> + htab->count = 0;
> +
> + htab->elem_size = sizeof(struct htab_elem) +
> + round_up(htab->map.key_size, 8) +
> + htab->map.value_size;
> +
> + htab->slab_name = kasprintf(GFP_USER, "bpf_htab_%p", htab);
This leaks a kernel heap memory pointer to userspace. If a unique name
needed, I think map_id should be used instead.
> + if (!htab->slab_name)
> + goto free_buckets;
> +
> + htab->elem_cache = kmem_cache_create(htab->slab_name,
> + htab->elem_size, 0, 0, NULL);
> + if (!htab->elem_cache)
> + goto free_slab_name;
> +
> + return &htab->map;
> +
> +free_slab_name:
> + kfree(htab->slab_name);
> +free_buckets:
> + kfree(htab->buckets);
> +free_htab:
> + kfree(htab);
> + return ERR_PTR(err);
> +}
> +
> +static inline u32 htab_map_hash(const void *key, u32 key_len)
> +{
> + return jhash(key, key_len, 0);
> +}
> +
> +static inline struct hlist_head *select_bucket(struct bpf_htab *htab, u32 hash)
> +{
> + return &htab->buckets[hash & (htab->n_buckets - 1)];
> +}
> +
> +static struct htab_elem *lookup_elem_raw(struct hlist_head *head, u32 hash,
> + void *key, u32 key_size)
> +{
> + struct htab_elem *l;
> +
> + hlist_for_each_entry_rcu(l, head, hash_node) {
> + if (l->hash == hash && !memcmp(&l->key, key, key_size))
> + return l;
> + }
> + return NULL;
> +}
> +
> +/* Must be called with rcu_read_lock. */
> +static void *htab_map_lookup_elem(struct bpf_map *map, void *key)
> +{
> + struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
> + struct hlist_head *head;
> + struct htab_elem *l;
> + u32 hash, key_size;
> +
> + WARN_ON_ONCE(!rcu_read_lock_held());
> +
> + key_size = map->key_size;
> +
> + hash = htab_map_hash(key, key_size);
> +
> + head = select_bucket(htab, hash);
> +
> + l = lookup_elem_raw(head, hash, key, key_size);
> +
> + if (l)
> + return l->key + round_up(map->key_size, 8);
> + else
> + return NULL;
> +}
> +
> +/* Must be called with rcu_read_lock. */
> +static int htab_map_get_next_key(struct bpf_map *map, void *key, void *next_key)
> +{
> + struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
> + struct hlist_head *head;
> + struct htab_elem *l, *next_l;
> + u32 hash, key_size;
> + int i;
> +
> + WARN_ON_ONCE(!rcu_read_lock_held());
> +
> + key_size = map->key_size;
> +
> + hash = htab_map_hash(key, key_size);
> +
> + head = select_bucket(htab, hash);
> +
> + /* lookup the key */
> + l = lookup_elem_raw(head, hash, key, key_size);
> +
> + if (!l) {
> + i = 0;
> + goto find_first_elem;
> + }
> +
> + /* key was found, get next key in the same bucket */
> + next_l = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu(&l->hash_node)),
> + struct htab_elem, hash_node);
> +
> + if (next_l) {
> + /* if next elem in this hash list is non-zero, just return it */
> + memcpy(next_key, next_l->key, key_size);
> + return 0;
> + } else {
> + /* no more elements in this hash list, go to the next bucket */
> + i = hash & (htab->n_buckets - 1);
> + i++;
> + }
> +
> +find_first_elem:
> + /* iterate over buckets */
> + for (; i < htab->n_buckets; i++) {
> + head = select_bucket(htab, i);
> +
> + /* pick first element in the bucket */
> + next_l = hlist_entry_safe(rcu_dereference_raw(hlist_first_rcu(head)),
> + struct htab_elem, hash_node);
> + if (next_l) {
> + /* if it's not empty, just return it */
> + memcpy(next_key, next_l->key, key_size);
> + return 0;
> + }
> + }
> +
> + /* itereated over all buckets and all elements */
> + return -ENOENT;
> +}
> +
> +static struct htab_elem *htab_alloc_elem(struct bpf_htab *htab)
> +{
> + void *l;
> +
> + l = kmem_cache_alloc(htab->elem_cache, GFP_ATOMIC);
> + if (!l)
> + return ERR_PTR(-ENOMEM);
> + return l;
> +}
> +
> +static void free_htab_elem_rcu(struct rcu_head *rcu)
> +{
> + struct htab_elem *l = container_of(rcu, struct htab_elem, rcu);
> +
> + kmem_cache_free(l->htab->elem_cache, l);
> +}
> +
> +static void release_htab_elem(struct bpf_htab *htab, struct htab_elem *l)
> +{
> + l->htab = htab;
> + call_rcu(&l->rcu, free_htab_elem_rcu);
> +}
> +
> +/* Must be called with rcu_read_lock. */
> +static int htab_map_update_elem(struct bpf_map *map, void *key, void *value)
> +{
> + struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
> + struct htab_elem *l_new, *l_old;
> + struct hlist_head *head;
> + u32 key_size;
> +
> + WARN_ON_ONCE(!rcu_read_lock_held());
> +
> + l_new = htab_alloc_elem(htab);
> + if (IS_ERR(l_new))
> + return -ENOMEM;
> +
> + key_size = map->key_size;
> +
> + memcpy(l_new->key, key, key_size);
> + memcpy(l_new->key + round_up(key_size, 8), value, map->value_size);
> +
> + l_new->hash = htab_map_hash(l_new->key, key_size);
> +
> + head = select_bucket(htab, l_new->hash);
> +
> + l_old = lookup_elem_raw(head, l_new->hash, key, key_size);
> +
> + spin_lock_bh(&htab->lock);
> + if (!l_old && unlikely(htab->count >= map->max_entries)) {
> + /* if elem with this 'key' doesn't exist and we've reached
> + * max_entries limit, fail insertion of new elem
> + */
> + spin_unlock_bh(&htab->lock);
> + kmem_cache_free(htab->elem_cache, l_new);
> + return -EFBIG;
> + }
> +
> + /* add new element to the head of the list, so that concurrent
> + * search will find it before old elem
> + */
> + hlist_add_head_rcu(&l_new->hash_node, head);
> + if (l_old) {
> + hlist_del_rcu(&l_old->hash_node);
> + release_htab_elem(htab, l_old);
> + } else {
> + htab->count++;
> + }
> + spin_unlock_bh(&htab->lock);
> +
> + return 0;
> +}
> +
> +/* Must be called with rcu_read_lock. */
> +static int htab_map_delete_elem(struct bpf_map *map, void *key)
> +{
> + struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
> + struct htab_elem *l;
> + struct hlist_head *head;
> + u32 hash, key_size;
> +
> + WARN_ON_ONCE(!rcu_read_lock_held());
> +
> + key_size = map->key_size;
> +
> + hash = htab_map_hash(key, key_size);
> +
> + head = select_bucket(htab, hash);
> +
> + l = lookup_elem_raw(head, hash, key, key_size);
> +
> + if (l) {
> + spin_lock_bh(&htab->lock);
> + hlist_del_rcu(&l->hash_node);
> + htab->count--;
> + release_htab_elem(htab, l);
> + spin_unlock_bh(&htab->lock);
> + return 0;
> + }
> + return -ESRCH;
> +}
> +
> +static void delete_all_elements(struct bpf_htab *htab)
> +{
> + int i;
> +
> + for (i = 0; i < htab->n_buckets; i++) {
> + struct hlist_head *head = select_bucket(htab, i);
> + struct hlist_node *n;
> + struct htab_elem *l;
> +
> + hlist_for_each_entry_safe(l, n, head, hash_node) {
> + hlist_del_rcu(&l->hash_node);
> + htab->count--;
> + kmem_cache_free(htab->elem_cache, l);
> + }
> + }
> +}
> +
> +/* called when map->refcnt goes to zero */
> +static void htab_map_free(struct bpf_map *map)
> +{
> + struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
> +
> + /* wait for all outstanding updates to complete */
> + synchronize_rcu();
> +
> + /* kmem_cache_free all htab elements */
> + delete_all_elements(htab);
> +
> + /* and destroy cache, which might sleep */
> + kmem_cache_destroy(htab->elem_cache);
> +
> + kfree(htab->buckets);
> + kfree(htab->slab_name);
> + kfree(htab);
> +}
> +
> +static struct bpf_map_ops htab_ops = {
> + .map_alloc = htab_map_alloc,
> + .map_free = htab_map_free,
> + .map_get_next_key = htab_map_get_next_key,
> + .map_lookup_elem = htab_map_lookup_elem,
> + .map_update_elem = htab_map_update_elem,
> + .map_delete_elem = htab_map_delete_elem,
> +};
> +
> +static struct bpf_map_type_list tl = {
> + .ops = &htab_ops,
> + .type = BPF_MAP_TYPE_HASH,
> +};
> +
> +static int __init register_htab_map(void)
> +{
> + bpf_register_map_type(&tl);
> + return 0;
> +}
> +late_initcall(register_htab_map);
> --
> 1.7.9.5
>
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH RFC v2 net-next 07/16] bpf: add lookup/update/delete/iterate methods to BPF maps
From: Kees Cook @ 2014-07-23 18:25 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Daniel Borkmann, Chema Gonzalez, Eric Dumazet,
Peter Zijlstra, Arnaldo Carvalho de Melo, Jiri Olsa,
Thomas Gleixner, H. Peter Anvin, Andrew Morton, Linux API,
Network Development, LKML
In-Reply-To: <1405657206-12060-8-git-send-email-ast@plumgrid.com>
On Thu, Jul 17, 2014 at 9:19 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
> 'maps' is a generic storage of different types for sharing data between kernel
> and userspace.
>
> The maps are accessed from user space via BPF syscall, which has commands:
>
> - create a map with given type and attributes
> fd = bpf_map_create(map_type, struct nlattr *attr, int len)
> returns fd or negative error
>
> - lookup key in a given map referenced by fd
> err = bpf_map_lookup_elem(int fd, void *key, void *value)
> returns zero and stores found elem into value or negative error
>
> - create or update key/value pair in a given map
> err = bpf_map_update_elem(int fd, void *key, void *value)
> returns zero or negative error
>
> - find and delete element by key in a given map
> err = bpf_map_delete_elem(int fd, void *key)
>
> - iterate map elements (based on input key return next_key)
> err = bpf_map_get_next_key(int fd, void *key, void *next_key)
>
> - close(fd) deletes the map
>
> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
> ---
> include/linux/bpf.h | 6 ++
> include/uapi/linux/bpf.h | 25 ++++++
> kernel/bpf/syscall.c | 209 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 240 insertions(+)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 57af236a0eb4..91e2caf8edf9 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -18,6 +18,12 @@ struct bpf_map_ops {
> /* funcs callable from userspace (via syscall) */
> struct bpf_map *(*map_alloc)(struct nlattr *attrs[BPF_MAP_ATTR_MAX + 1]);
> void (*map_free)(struct bpf_map *);
> + int (*map_get_next_key)(struct bpf_map *map, void *key, void *next_key);
> +
> + /* funcs callable from userspace and from eBPF programs */
> + void *(*map_lookup_elem)(struct bpf_map *map, void *key);
> + int (*map_update_elem)(struct bpf_map *map, void *key, void *value);
> + int (*map_delete_elem)(struct bpf_map *map, void *key);
> };
>
> struct bpf_map {
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index dcc7eb97a64a..5e1bfbc9cdc7 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -308,6 +308,31 @@ enum bpf_cmd {
> * map is deleted when fd is closed
> */
> BPF_MAP_CREATE,
> +
> + /* lookup key in a given map referenced by map_id
> + * err = bpf_map_lookup_elem(int map_id, void *key, void *value)
This needs map_id documentation updates too?
> + * returns zero and stores found elem into value
> + * or negative error
> + */
> + BPF_MAP_LOOKUP_ELEM,
> +
> + /* create or update key/value pair in a given map
> + * err = bpf_map_update_elem(int map_id, void *key, void *value)
> + * returns zero or negative error
> + */
> + BPF_MAP_UPDATE_ELEM,
> +
> + /* find and delete elem by key in a given map
> + * err = bpf_map_delete_elem(int map_id, void *key)
> + * returns zero or negative error
> + */
> + BPF_MAP_DELETE_ELEM,
> +
> + /* lookup key in a given map and return next key
> + * err = bpf_map_get_elem(int map_id, void *key, void *next_key)
> + * returns zero and stores next key or negative error
> + */
> + BPF_MAP_GET_NEXT_KEY,
> };
>
> enum bpf_map_attributes {
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index c4a330642653..ca2be66845b3 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -13,6 +13,7 @@
> #include <linux/syscalls.h>
> #include <net/netlink.h>
> #include <linux/anon_inodes.h>
> +#include <linux/file.h>
>
> /* mutex to protect insertion/deletion of map_id in IDR */
> static DEFINE_MUTEX(bpf_map_lock);
> @@ -209,6 +210,202 @@ free_attr:
> return err;
> }
>
> +static int get_map_id(struct fd f)
> +{
> + struct bpf_map *map;
> +
> + if (!f.file)
> + return -EBADF;
> +
> + if (f.file->f_op != &bpf_map_fops) {
> + fdput(f);
It feels weird to me to do the fdput inside this function. Instead,
should map_lookup_elem get a "err_put" label, instead?
> + return -EINVAL;
> + }
> +
> + map = f.file->private_data;
> +
> + return map->map_id;
> +}
> +
> +static int map_lookup_elem(int ufd, void __user *ukey, void __user *uvalue)
> +{
> + struct fd f = fdget(ufd);
> + struct bpf_map *map;
> + void *key, *value;
> + int err;
> +
> + err = get_map_id(f);
> + if (err < 0)
> + return err;
for example:
if (err < 0)
goto fail_put;
> +
> + rcu_read_lock();
> + map = idr_find(&bpf_map_id_idr, err);
> + err = -EINVAL;
> + if (!map)
> + goto err_unlock;
> +
> + err = -ENOMEM;
> + key = kmalloc(map->key_size, GFP_ATOMIC);
> + if (!key)
> + goto err_unlock;
> +
> + err = -EFAULT;
> + if (copy_from_user(key, ukey, map->key_size) != 0)
> + goto free_key;
> +
> + err = -ESRCH;
> + value = map->ops->map_lookup_elem(map, key);
> + if (!value)
> + goto free_key;
> +
> + err = -EFAULT;
> + if (copy_to_user(uvalue, value, map->value_size) != 0)
> + goto free_key;
I'm made uncomfortable with memory copying where explicit lengths from
userspace aren't being used. It does look like it would be redundant,
though. Are there other syscalls where the kernel may stomp on user
memory based on internal kernel sizes? I think this is fine as-is, but
it makes me want to think harder about it. :)
> +
> + err = 0;
> +
> +free_key:
> + kfree(key);
> +err_unlock:
> + rcu_read_unlock();
fail_put:
> + fdput(f);
> + return err;
> +}
> +
> +static int map_update_elem(int ufd, void __user *ukey, void __user *uvalue)
> +{
> + struct fd f = fdget(ufd);
> + struct bpf_map *map;
> + void *key, *value;
> + int err;
> +
> + err = get_map_id(f);
> + if (err < 0)
> + return err;
Same thing?
> +
> + rcu_read_lock();
> + map = idr_find(&bpf_map_id_idr, err);
> + err = -EINVAL;
> + if (!map)
> + goto err_unlock;
> +
> + err = -ENOMEM;
> + key = kmalloc(map->key_size, GFP_ATOMIC);
> + if (!key)
> + goto err_unlock;
> +
> + err = -EFAULT;
> + if (copy_from_user(key, ukey, map->key_size) != 0)
> + goto free_key;
> +
> + err = -ENOMEM;
> + value = kmalloc(map->value_size, GFP_ATOMIC);
> + if (!value)
> + goto free_key;
> +
> + err = -EFAULT;
> + if (copy_from_user(value, uvalue, map->value_size) != 0)
> + goto free_value;
> +
> + err = map->ops->map_update_elem(map, key, value);
> +
> +free_value:
> + kfree(value);
> +free_key:
> + kfree(key);
> +err_unlock:
> + rcu_read_unlock();
> + fdput(f);
> + return err;
> +}
> +
> +static int map_delete_elem(int ufd, void __user *ukey)
> +{
> + struct fd f = fdget(ufd);
> + struct bpf_map *map;
> + void *key;
> + int err;
> +
> + err = get_map_id(f);
> + if (err < 0)
> + return err;
> +
> + rcu_read_lock();
> + map = idr_find(&bpf_map_id_idr, err);
> + err = -EINVAL;
> + if (!map)
> + goto err_unlock;
> +
> + err = -ENOMEM;
> + key = kmalloc(map->key_size, GFP_ATOMIC);
> + if (!key)
> + goto err_unlock;
> +
> + err = -EFAULT;
> + if (copy_from_user(key, ukey, map->key_size) != 0)
> + goto free_key;
> +
> + err = map->ops->map_delete_elem(map, key);
> +
> +free_key:
> + kfree(key);
> +err_unlock:
> + rcu_read_unlock();
> + fdput(f);
> + return err;
> +}
> +
> +static int map_get_next_key(int ufd, void __user *ukey, void __user *unext_key)
> +{
> + struct fd f = fdget(ufd);
> + struct bpf_map *map;
> + void *key, *next_key;
> + int err;
> +
> + err = get_map_id(f);
> + if (err < 0)
> + return err;
> +
> + rcu_read_lock();
> + map = idr_find(&bpf_map_id_idr, err);
> + err = -EINVAL;
> + if (!map)
> + goto err_unlock;
> +
> + err = -ENOMEM;
> + key = kmalloc(map->key_size, GFP_ATOMIC);
> + if (!key)
> + goto err_unlock;
> +
> + err = -EFAULT;
> + if (copy_from_user(key, ukey, map->key_size) != 0)
> + goto free_key;
> +
> + err = -ENOMEM;
> + next_key = kmalloc(map->key_size, GFP_ATOMIC);
In the interests of defensiveness, I'd use kzalloc here.
> + if (!next_key)
> + goto free_key;
> +
> + err = map->ops->map_get_next_key(map, key, next_key);
> + if (err)
> + goto free_next_key;
> +
> + err = -EFAULT;
> + if (copy_to_user(unext_key, next_key, map->key_size) != 0)
> + goto free_next_key;
> +
> + err = 0;
> +
> +free_next_key:
> + kfree(next_key);
> +free_key:
> + kfree(key);
> +err_unlock:
> + rcu_read_unlock();
> + fdput(f);
> + return err;
> +}
> +
> SYSCALL_DEFINE5(bpf, int, cmd, unsigned long, arg2, unsigned long, arg3,
> unsigned long, arg4, unsigned long, arg5)
> {
> @@ -219,6 +416,18 @@ SYSCALL_DEFINE5(bpf, int, cmd, unsigned long, arg2, unsigned long, arg3,
> case BPF_MAP_CREATE:
> return map_create((enum bpf_map_type) arg2,
> (struct nlattr __user *) arg3, (int) arg4);
> + case BPF_MAP_LOOKUP_ELEM:
> + return map_lookup_elem((int) arg2, (void __user *) arg3,
> + (void __user *) arg4);
> + case BPF_MAP_UPDATE_ELEM:
> + return map_update_elem((int) arg2, (void __user *) arg3,
> + (void __user *) arg4);
> + case BPF_MAP_DELETE_ELEM:
> + return map_delete_elem((int) arg2, (void __user *) arg3);
> +
> + case BPF_MAP_GET_NEXT_KEY:
> + return map_get_next_key((int) arg2, (void __user *) arg3,
> + (void __user *) arg4);
Same observation as the other syscall cmd: perhaps arg5 == 0 should be
checked? Also, since each of these functions looks up the fd and
builds the key, maybe those should be added to a common helper instead
of copy/pasting into each demuxed function?
-Kees
> default:
> return -EINVAL;
> }
> --
> 1.7.9.5
>
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH RFC v2 net-next 05/16] bpf: introduce syscall(BPF, ...) and BPF maps
From: Kees Cook @ 2014-07-23 18:02 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Daniel Borkmann, Chema Gonzalez, Eric Dumazet,
Peter Zijlstra, Arnaldo Carvalho de Melo, Jiri Olsa,
Thomas Gleixner, H. Peter Anvin, Andrew Morton, Linux API,
Network Development, LKML
In-Reply-To: <1405657206-12060-6-git-send-email-ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
On Thu, Jul 17, 2014 at 9:19 PM, Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org> wrote:
> BPF syscall is a demux for different BPF releated commands.
>
> 'maps' is a generic storage of different types for sharing data between kernel
> and userspace.
>
> The maps can be created from user space via BPF syscall:
> - create a map with given type and attributes
> fd = bpf_map_create(map_type, struct nlattr *attr, int len)
> returns fd or negative error
>
> - close(fd) deletes the map
>
> Next patch allows userspace programs to populate/read maps that eBPF programs
> are concurrently updating.
>
> maps can have different types: hash, bloom filter, radix-tree, etc.
>
> The map is defined by:
> . type
> . max number of elements
> . key size in bytes
> . value size in bytes
>
> Next patches allow eBPF programs to access maps via API:
> void * bpf_map_lookup_elem(u32 fd, void *key);
> int bpf_map_update_elem(u32 fd, void *key, void *value);
> int bpf_map_delete_elem(u32 fd, void *key);
>
> This patch establishes core infrastructure for BPF maps.
> Next patches implement lookup/update and hashtable type.
> More map types can be added in the future.
>
> syscall is using type-length-value style of passing arguments to be backwards
> compatible with future extensions to map attributes. Different map types may
> use different attributes as well.
> The concept of type-lenght-value is borrowed from netlink, but netlink itself
> is not applicable here, since BPF programs and maps can be used in NET-less
> configurations.
>
> Signed-off-by: Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
> ---
> Documentation/networking/filter.txt | 69 +++++++++++
> include/linux/bpf.h | 43 +++++++
> include/uapi/linux/bpf.h | 24 ++++
> kernel/bpf/Makefile | 2 +-
> kernel/bpf/syscall.c | 225 +++++++++++++++++++++++++++++++++++
> 5 files changed, 362 insertions(+), 1 deletion(-)
> create mode 100644 include/linux/bpf.h
> create mode 100644 kernel/bpf/syscall.c
>
> diff --git a/Documentation/networking/filter.txt b/Documentation/networking/filter.txt
> index ee78eba78a9d..e14e486f69cd 100644
> --- a/Documentation/networking/filter.txt
> +++ b/Documentation/networking/filter.txt
> @@ -995,6 +995,75 @@ BPF_XADD | BPF_DW | BPF_STX: lock xadd *(u64 *)(dst_reg + off16) += src_reg
> Where size is one of: BPF_B or BPF_H or BPF_W or BPF_DW. Note that 1 and
> 2 byte atomic increments are not supported.
>
> +eBPF maps
> +---------
> +'maps' is a generic storage of different types for sharing data between kernel
> +and userspace.
> +
> +The maps are accessed from user space via BPF syscall, which has commands:
> +- create a map with given id, type and attributes
> + map_id = bpf_map_create(int map_id, map_type, struct nlattr *attr, int len)
> + returns positive map id or negative error
Looks like these docs need updating for the fd-based approach instead
of the map_id approach?
> +
> +- delete map with given map id
> + err = bpf_map_delete(int map_id)
> + returns zero or negative error
> +
> +- lookup key in a given map referenced by map_id
> + err = bpf_map_lookup_elem(int map_id, void *key, void *value)
> + returns zero and stores found elem into value or negative error
> +
> +- create or update key/value pair in a given map
> + err = bpf_map_update_elem(int map_id, void *key, void *value)
> + returns zero or negative error
> +
> +- find and delete element by key in a given map
> + err = bpf_map_delete_elem(int map_id, void *key)
> +
> +userspace programs uses this API to create/populate/read maps that eBPF programs
> +are concurrently updating.
> +
> +maps can have different types: hash, bloom filter, radix-tree, etc.
> +
> +The map is defined by:
> + . id
> + . type
> + . max number of elements
> + . key size in bytes
> + . value size in bytes
> +
> +The maps are accesible from eBPF program with API:
> + void * bpf_map_lookup_elem(u32 map_id, void *key);
> + int bpf_map_update_elem(u32 map_id, void *key, void *value);
> + int bpf_map_delete_elem(u32 map_id, void *key);
> +
> +If eBPF verifier is configured to recognize extra calls in the program
> +bpf_map_lookup_elem() and bpf_map_update_elem() then access to maps looks like:
> + ...
> + ptr_to_value = map_lookup_elem(const_int_map_id, key)
> + access memory [ptr_to_value, ptr_to_value + value_size_in_bytes]
> + ...
> + prepare key2 and value2 on stack of key_size and value_size
> + err = map_update_elem(const_int_map_id2, key2, value2)
> + ...
> +
> +eBPF program cannot create or delete maps
> +(such calls will be unknown to verifier)
> +
> +During program loading the refcnt of used maps is incremented, so they don't get
> +deleted while program is running
> +
> +bpf_map_update_elem() can fail if maximum number of elements reached.
> +if key2 already exists, bpf_map_update_elem() replaces it with value2 atomically
> +
> +bpf_map_lookup_elem() can return null or ptr_to_value
> +ptr_to_value is read/write from the program point of view.
> +
> +The verifier will check that the program accesses map elements within specified
> +size. It will not let programs pass junk values as 'key' and 'value' to
> +bpf_map_*_elem() functions, so these functions (implemented in C inside kernel)
> +can safely access the pointers in all cases.
> +
> Testing
> -------
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> new file mode 100644
> index 000000000000..57af236a0eb4
> --- /dev/null
> +++ b/include/linux/bpf.h
> @@ -0,0 +1,43 @@
> +/* Copyright (c) 2011-2014 PLUMgrid, http://plumgrid.com
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of version 2 of the GNU General Public
> + * License as published by the Free Software Foundation.
> + */
> +#ifndef _LINUX_BPF_H
> +#define _LINUX_BPF_H 1
> +
> +#include <uapi/linux/bpf.h>
> +#include <linux/workqueue.h>
> +
> +struct bpf_map;
> +struct nlattr;
> +
> +/* map is generic key/value storage optionally accesible by eBPF programs */
> +struct bpf_map_ops {
> + /* funcs callable from userspace (via syscall) */
> + struct bpf_map *(*map_alloc)(struct nlattr *attrs[BPF_MAP_ATTR_MAX + 1]);
> + void (*map_free)(struct bpf_map *);
> +};
> +
> +struct bpf_map {
> + atomic_t refcnt;
> + int map_id;
> + enum bpf_map_type map_type;
> + u32 key_size;
> + u32 value_size;
> + u32 max_entries;
> + struct bpf_map_ops *ops;
> + struct work_struct work;
> +};
> +
> +struct bpf_map_type_list {
> + struct list_head list_node;
> + struct bpf_map_ops *ops;
> + enum bpf_map_type type;
> +};
> +
> +void bpf_register_map_type(struct bpf_map_type_list *tl);
> +struct bpf_map *bpf_map_get(u32 map_id);
> +
> +#endif /* _LINUX_BPF_H */
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 3ff5bf5045a7..dcc7eb97a64a 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -300,4 +300,28 @@ struct bpf_insn {
> __s32 imm; /* signed immediate constant */
> };
>
> +/* BPF syscall commands */
> +enum bpf_cmd {
> + /* create a map with given type and attributes
> + * fd = bpf_map_create(bpf_map_type, struct nlattr *attr, int len)
> + * returns fd or negative error
> + * map is deleted when fd is closed
> + */
> + BPF_MAP_CREATE,
> +};
> +
> +enum bpf_map_attributes {
> + BPF_MAP_UNSPEC,
> + BPF_MAP_KEY_SIZE, /* size of key in bytes */
> + BPF_MAP_VALUE_SIZE, /* size of value in bytes */
> + BPF_MAP_MAX_ENTRIES, /* maximum number of entries in a map */
> + __BPF_MAP_ATTR_MAX,
> +};
> +#define BPF_MAP_ATTR_MAX (__BPF_MAP_ATTR_MAX - 1)
> +#define BPF_MAP_MAX_ATTR_SIZE 65535
> +
> +enum bpf_map_type {
> + BPF_MAP_TYPE_UNSPEC,
> +};
> +
> #endif /* _UAPI__LINUX_BPF_H__ */
> diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
> index 6a71145e2769..e9f7334ed07a 100644
> --- a/kernel/bpf/Makefile
> +++ b/kernel/bpf/Makefile
> @@ -1 +1 @@
> -obj-y := core.o
> +obj-y := core.o syscall.o
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> new file mode 100644
> index 000000000000..c4a330642653
> --- /dev/null
> +++ b/kernel/bpf/syscall.c
> @@ -0,0 +1,225 @@
> +/* Copyright (c) 2011-2014 PLUMgrid, http://plumgrid.com
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of version 2 of the GNU General Public
> + * License as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + */
> +#include <linux/bpf.h>
> +#include <linux/syscalls.h>
> +#include <net/netlink.h>
> +#include <linux/anon_inodes.h>
> +
> +/* mutex to protect insertion/deletion of map_id in IDR */
> +static DEFINE_MUTEX(bpf_map_lock);
> +static DEFINE_IDR(bpf_map_id_idr);
> +
> +/* maximum number of outstanding maps */
> +#define MAX_BPF_MAP_CNT 1024
> +static u32 bpf_map_cnt;
> +
> +static LIST_HEAD(bpf_map_types);
> +
> +static struct bpf_map *find_and_alloc_map(enum bpf_map_type type,
> + struct nlattr *tb[BPF_MAP_ATTR_MAX + 1])
> +{
> + struct bpf_map_type_list *tl;
> + struct bpf_map *map;
> +
> + list_for_each_entry(tl, &bpf_map_types, list_node) {
> + if (tl->type == type) {
> + map = tl->ops->map_alloc(tb);
> + if (IS_ERR(map))
> + return map;
> + map->ops = tl->ops;
> + map->map_type = type;
> + return map;
> + }
> + }
> + return ERR_PTR(-EINVAL);
> +}
> +
> +/* boot time registration of different map implementations */
> +void bpf_register_map_type(struct bpf_map_type_list *tl)
> +{
> + list_add(&tl->list_node, &bpf_map_types);
> +}
> +
> +/* called from workqueue */
> +static void bpf_map_free_deferred(struct work_struct *work)
> +{
> + struct bpf_map *map = container_of(work, struct bpf_map, work);
> +
> + /* grab the mutex and free the map */
> + mutex_lock(&bpf_map_lock);
> +
> + bpf_map_cnt--;
> + idr_remove(&bpf_map_id_idr, map->map_id);
> +
> + mutex_unlock(&bpf_map_lock);
> +
> + /* implementation dependent freeing */
> + map->ops->map_free(map);
> +}
> +
> +/* decrement map refcnt and schedule it for freeing via workqueue
> + * (unrelying map implementation ops->map_free() might sleep)
> + */
> +static void __bpf_map_put(struct bpf_map *map)
> +{
> + if (atomic_dec_and_test(&map->refcnt)) {
> + INIT_WORK(&map->work, bpf_map_free_deferred);
> + schedule_work(&map->work);
> + }
> +}
> +
> +/* find map by id and decrement its refcnt
> + *
> + * can be called without any locks held
> + *
> + * returns true if map was found
> + */
> +static bool bpf_map_put(u32 map_id)
> +{
> + struct bpf_map *map;
> +
> + rcu_read_lock();
> + map = idr_find(&bpf_map_id_idr, map_id);
> +
> + if (!map) {
> + rcu_read_unlock();
> + return false;
> + }
> +
> + __bpf_map_put(map);
> + rcu_read_unlock();
> +
> + return true;
> +}
> +
> +/* called with bpf_map_lock held */
> +struct bpf_map *bpf_map_get(u32 map_id)
> +{
> + BUG_ON(!mutex_is_locked(&bpf_map_lock));
> +
> + return idr_find(&bpf_map_id_idr, map_id);
> +}
> +
> +static int bpf_map_release(struct inode *inode, struct file *filp)
> +{
> + struct bpf_map *map = filp->private_data;
> +
> + __bpf_map_put(map);
> + return 0;
> +}
> +
> +static const struct file_operations bpf_map_fops = {
> + .release = bpf_map_release,
> +};
> +
> +static const struct nla_policy map_policy[BPF_MAP_ATTR_MAX + 1] = {
> + [BPF_MAP_KEY_SIZE] = { .type = NLA_U32 },
> + [BPF_MAP_VALUE_SIZE] = { .type = NLA_U32 },
> + [BPF_MAP_MAX_ENTRIES] = { .type = NLA_U32 },
> +};
> +
> +/* called via syscall */
> +static int map_create(enum bpf_map_type type, struct nlattr __user *uattr, int len)
> +{
> + struct nlattr *tb[BPF_MAP_ATTR_MAX + 1];
> + struct bpf_map *map;
> + struct nlattr *attr;
> + int err;
> +
> + if (len <= 0 || len > BPF_MAP_MAX_ATTR_SIZE)
> + return -EINVAL;
> +
> + attr = kmalloc(len, GFP_USER);
> + if (!attr)
> + return -ENOMEM;
> +
> + /* copy map attributes from user space */
> + err = -EFAULT;
> + if (copy_from_user(attr, uattr, len) != 0)
> + goto free_attr;
> +
> + /* perform basic validation */
> + err = nla_parse(tb, BPF_MAP_ATTR_MAX, attr, len, map_policy);
> + if (err < 0)
> + goto free_attr;
> +
> + /* find map type and init map: hashtable vs rbtree vs bloom vs ... */
> + map = find_and_alloc_map(type, tb);
> + if (IS_ERR(map)) {
> + err = PTR_ERR(map);
> + goto free_attr;
> + }
> +
> + atomic_set(&map->refcnt, 1);
> +
> + mutex_lock(&bpf_map_lock);
> +
> + if (bpf_map_cnt >= MAX_BPF_MAP_CNT) {
> + mutex_unlock(&bpf_map_lock);
> + err = -ENOSPC;
> + goto free_map;
> + }
> +
> + /* allocate map id */
> + err = idr_alloc(&bpf_map_id_idr, map, 1 /* min map_id */, 0, GFP_USER);
> +
> + if (err > 0)
> + bpf_map_cnt++;
> +
> + map->map_id = err;
> +
> + mutex_unlock(&bpf_map_lock);
> +
> + if (err < 0)
> + /* failed to allocate map id */
> + goto free_map;
> +
> + err = anon_inode_getfd("bpf-map", &bpf_map_fops, map, O_RDWR | O_CLOEXEC);
> +
> + if (err < 0)
> + /* failed to allocate fd */
> + goto free_map_id;
> +
> + /* user supplied array of map attributes is no longer needed */
> + kfree(attr);
> +
> + return err;
> +
> +free_map_id:
> + /* grab the mutex and free the map */
> + mutex_lock(&bpf_map_lock);
> +
> + bpf_map_cnt--;
> + idr_remove(&bpf_map_id_idr, map->map_id);
> +
> + mutex_unlock(&bpf_map_lock);
> +free_map:
> + map->ops->map_free(map);
> +free_attr:
> + kfree(attr);
> + return err;
> +}
> +
> +SYSCALL_DEFINE5(bpf, int, cmd, unsigned long, arg2, unsigned long, arg3,
> + unsigned long, arg4, unsigned long, arg5)
> +{
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
It might be valuable to have a comment here describing why this is
currently limited to CAP_SYS_ADMIN.
> +
> + switch (cmd) {
> + case BPF_MAP_CREATE:
> + return map_create((enum bpf_map_type) arg2,
> + (struct nlattr __user *) arg3, (int) arg4);
I'd recommend requiring arg5 == 0 here, just for future flexibility.
-Kees
> + default:
> + return -EINVAL;
> + }
> +}
> --
> 1.7.9.5
>
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH RFC v2 net-next 02/16] bpf: update MAINTAINERS entry
From: Alexei Starovoitov @ 2014-07-23 17:48 UTC (permalink / raw)
To: Kees Cook
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Daniel Borkmann, Chema Gonzalez, Eric Dumazet,
Peter Zijlstra, Arnaldo Carvalho de Melo, Jiri Olsa,
Thomas Gleixner, H. Peter Anvin, Andrew Morton, Linux API,
Network Development, LKML
In-Reply-To: <CAGXu5j+mGDEPx5rCdVXzAdxR9SwQQ4ERM-Brxt4TmmncdPjzzg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wed, Jul 23, 2014 at 10:37 AM, Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> On Thu, Jul 17, 2014 at 9:19 PM, Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org> wrote:
>> Signed-off-by: Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
>> ---
>> MAINTAINERS | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index ae8cd00215b2..32e24ff46da3 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1912,6 +1912,13 @@ S: Supported
>> F: drivers/net/bonding/
>> F: include/uapi/linux/if_bonding.h
>>
>> +BPF (Safe dynamic programs and tools)
>
> bikeshed: I feel like this shouldn't be an acronym. Maybe instead:
>
> BERKELEY PACKET FILTER (BPF: Safe dynamic programs and tools)
pile on :)
I think eBPF is no longer acronym. 'e' stands for 'extended',
but BPF is no longer 'packet filter' only and definitely not 'berkeley'.
So I'd rather keep BPF as a magic abbreviation without spelling it out,
since full name is historic and no longer meaningful.
I've considered coming up with brand new abbreviation and full name
for this instruction set, but none looked good and all lose in comparison
to 'eBPF' name, which is concise and carries enough historical
references to explain the idea behind new ISA.
^ permalink raw reply
* Re: [PATCH RFC v2 net-next 02/16] bpf: update MAINTAINERS entry
From: Kees Cook @ 2014-07-23 17:37 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Daniel Borkmann, Chema Gonzalez, Eric Dumazet,
Peter Zijlstra, Arnaldo Carvalho de Melo, Jiri Olsa,
Thomas Gleixner, H. Peter Anvin, Andrew Morton, Linux API,
Network Development, LKML
In-Reply-To: <1405657206-12060-3-git-send-email-ast@plumgrid.com>
On Thu, Jul 17, 2014 at 9:19 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
> ---
> MAINTAINERS | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ae8cd00215b2..32e24ff46da3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1912,6 +1912,13 @@ S: Supported
> F: drivers/net/bonding/
> F: include/uapi/linux/if_bonding.h
>
> +BPF (Safe dynamic programs and tools)
bikeshed: I feel like this shouldn't be an acronym. Maybe instead:
BERKELEY PACKET FILTER (BPF: Safe dynamic programs and tools)
-Kees
> +M: Alexei Starovoitov <ast@kernel.org>
> +L: netdev@vger.kernel.org
> +L: linux-kernel@vger.kernel.org
> +S: Supported
> +F: kernel/bpf/
> +
> BROADCOM B44 10/100 ETHERNET DRIVER
> M: Gary Zambrano <zambrano@broadcom.com>
> L: netdev@vger.kernel.org
> --
> 1.7.9.5
>
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [RFC PATCH 0/5] futex: introduce an optimistic spinning futex
From: Peter Zijlstra @ 2014-07-23 7:52 UTC (permalink / raw)
To: Mike Galbraith
Cc: Andi Kleen, Waiman Long, Thomas Gleixner, Ingo Molnar,
Darren Hart, Davidlohr Bueso, Heiko Carstens, linux-kernel,
linux-api, linux-doc, Jason Low, Scott J Norton
In-Reply-To: <1406101143.5076.100.camel@marge.simpson.net>
On Wed, Jul 23, 2014 at 09:39:03AM +0200, Mike Galbraith wrote:
> On Wed, 2014-07-23 at 09:35 +0200, Peter Zijlstra wrote:
> > On Wed, Jul 23, 2014 at 09:25:46AM +0200, Mike Galbraith wrote:
> > > I also resurrect mwait_idle(), as while you may consider it obsolete, I
> > > still love my lovely little Q6600 box (power sucking pig) dearly :)
> >
> > Yeah, I keep forgetting about that one.. we should get that fixed.
>
> (it is an obsolete pig, and nobody else seems to mind, so I don't mind)
Hey, I fixed early P4 topology setup yesterday, core2 is like brand
spanking new compared :-)
^ permalink raw reply
* Re: [RFC PATCH 0/5] futex: introduce an optimistic spinning futex
From: Mike Galbraith @ 2014-07-23 7:39 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Andi Kleen, Waiman Long, Thomas Gleixner, Ingo Molnar,
Darren Hart, Davidlohr Bueso, Heiko Carstens,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA, Jason Low, Scott J Norton
In-Reply-To: <20140723073518.GP3935@laptop>
On Wed, 2014-07-23 at 09:35 +0200, Peter Zijlstra wrote:
> On Wed, Jul 23, 2014 at 09:25:46AM +0200, Mike Galbraith wrote:
> > I also resurrect mwait_idle(), as while you may consider it obsolete, I
> > still love my lovely little Q6600 box (power sucking pig) dearly :)
>
> Yeah, I keep forgetting about that one.. we should get that fixed.
(it is an obsolete pig, and nobody else seems to mind, so I don't mind)
^ permalink raw reply
* Re: [RFC PATCH 0/5] futex: introduce an optimistic spinning futex
From: Peter Zijlstra @ 2014-07-23 7:35 UTC (permalink / raw)
To: Mike Galbraith
Cc: Andi Kleen, Waiman Long, Thomas Gleixner, Ingo Molnar,
Darren Hart, Davidlohr Bueso, Heiko Carstens, linux-kernel,
linux-api, linux-doc, Jason Low, Scott J Norton
In-Reply-To: <1406100346.5076.98.camel@marge.simpson.net>
On Wed, Jul 23, 2014 at 09:25:46AM +0200, Mike Galbraith wrote:
> I also resurrect mwait_idle(), as while you may consider it obsolete, I
> still love my lovely little Q6600 box (power sucking pig) dearly :)
Yeah, I keep forgetting about that one.. we should get that fixed.
^ permalink raw reply
* Re: [RFC PATCH 0/5] futex: introduce an optimistic spinning futex
From: Mike Galbraith @ 2014-07-23 7:25 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Andi Kleen, Waiman Long, Thomas Gleixner, Ingo Molnar,
Darren Hart, Davidlohr Bueso, Heiko Carstens,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA, Jason Low, Scott J Norton
In-Reply-To: <20140723065736.GM3935@laptop>
On Wed, 2014-07-23 at 08:57 +0200, Peter Zijlstra wrote:
> On Wed, Jul 23, 2014 at 06:55:03AM +0200, Mike Galbraith wrote:
> > On Mon, 2014-07-21 at 09:42 -0700, Andi Kleen wrote:
> >
> > > FWIW the main problem is currently that switch-through-idle is so
> > > slow. I think improving that would give a boost to far more
> > > situations.
> >
> > Two high frequency idle enter/exit suckage spots:
> >
> > 1) nohz (tick) - it's expensive to start/stop tick on every micro-idle,
> > throttle it or something.
>
> Yeah, so the idea was to use the cpuidle idle guestimator to control
> this, and now that we've moved it somewhat closer to the scheduler that
> might become possible.
>
> > 2) ondemand governor - tweak silly default settings to reflect the
> > reality that we routinely schedule communicating threads cross core.
>
> Yeah, so the plan is to shoot cpufreq in the head and base the
> replacement on smp aware metrics ;-) Its on a todo list somewhere..
It never ceases to amaze me that people aren't screaming bloody murder
about those two spots. Watching performance of lightly loaded boxen is
enough to make a grown man cry.
SUSE (and I in all of my many regression testing trees) puts tourniquets
on both of these blood spurting gashes, laptops be damned.
I also resurrect mwait_idle(), as while you may consider it obsolete, I
still love my lovely little Q6600 box (power sucking pig) dearly :)
-Mike
^ permalink raw reply
* Re: [RFC PATCH 0/5] futex: introduce an optimistic spinning futex
From: Peter Zijlstra @ 2014-07-23 6:57 UTC (permalink / raw)
To: Mike Galbraith
Cc: Andi Kleen, Waiman Long, Thomas Gleixner, Ingo Molnar,
Darren Hart, Davidlohr Bueso, Heiko Carstens, linux-kernel,
linux-api, linux-doc, Jason Low, Scott J Norton
In-Reply-To: <1406091303.5076.33.camel@marge.simpson.net>
On Wed, Jul 23, 2014 at 06:55:03AM +0200, Mike Galbraith wrote:
> On Mon, 2014-07-21 at 09:42 -0700, Andi Kleen wrote:
>
> > FWIW the main problem is currently that switch-through-idle is so
> > slow. I think improving that would give a boost to far more
> > situations.
>
> Two high frequency idle enter/exit suckage spots:
>
> 1) nohz (tick) - it's expensive to start/stop tick on every micro-idle,
> throttle it or something.
Yeah, so the idea was to use the cpuidle idle guestimator to control
this, and now that we've moved it somewhat closer to the scheduler that
might become possible.
> 2) ondemand governor - tweak silly default settings to reflect the
> reality that we routinely schedule communicating threads cross core.
Yeah, so the plan is to shoot cpufreq in the head and base the
replacement on smp aware metrics ;-) Its on a todo list somewhere..
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox