netdev - Re: [PATCH RFC net-next 08/14] bpf: add eBPF verifier

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 1 Jul 2014 22:57:49 -0700
From:	Alexei Starovoitov <ast@...mgrid.com>
To:	Namhyung Kim <namhyung@...il.com>
Cc:	"David S. Miller" <davem@...emloft.net>,
	Ingo Molnar <mingo@...nel.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Daniel Borkmann <dborkman@...hat.com>,
	Chema Gonzalez <chema@...gle.com>,
	Eric Dumazet <edumazet@...gle.com>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Arnaldo Carvalho de Melo <acme@...radead.org>,
	Jiri Olsa <jolsa@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Kees Cook <keescook@...omium.org>,
	Linux API <linux-api@...r.kernel.org>,
	Network Development <netdev@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC net-next 08/14] bpf: add eBPF verifier

On Tue, Jul 1, 2014 at 10:05 PM, Namhyung Kim <namhyung@...il.com> wrote:
> Mostly questions and few nitpicks.. :)

great questions. Thank you for review! Answers below:

> On Fri, 27 Jun 2014 17:06:00 -0700, Alexei Starovoitov wrote:
>> +/* types of values:
>> + * - stored in an eBPF register
>> + * - passed into helper functions as an argument
>> + * - returned from helper functions
>> + */
>> +enum bpf_reg_type {
>> +     INVALID_PTR,                    /* reg doesn't contain a valid pointer */
>
> I don't think it's a good name.  The INVALID_PTR can be read as it
> contains a "pointer" which is invalid.  Maybe INTEGER, NUMBER or
> something different can be used.  And I think the struct reg_state->ptr
> should be renamed also.

ok. I agree that 'invalid' part of the name is too negative.
May be 'unknown_value' ?

>> +     PTR_TO_CTX,                     /* reg points to bpf_context */
>> +     PTR_TO_MAP,                     /* reg points to map element value */
>> +     PTR_TO_MAP_CONDITIONAL,         /* points to map element value or NULL */
>> +     PTR_TO_STACK,                   /* reg == frame_pointer */
>> +     PTR_TO_STACK_IMM,               /* reg == frame_pointer + imm */
>> +     PTR_TO_STACK_IMM_MAP_KEY,       /* pointer to stack used as map key */
>> +     PTR_TO_STACK_IMM_MAP_VALUE,     /* pointer to stack used as map elem */
>
> So, these PTR_TO_STACK_IMM[_*] types are only for function argument,
> right?  I guessed it could be used to access memory in general too, but
> then I thought it'd make verification complicated..
>
> And I also agree that it'd better splitting reg types and function
> argument constraints.

Ok. Will split this enum into three.

>> +
>> +/* check read/write into map element returned by bpf_table_lookup() */
>> +static int check_table_access(struct verifier_env *env, int regno, int off,
>> +                           int size)
>
> I guess the "table" is an old name of the "map"?

oops :) Yes. I've been calling them 'bpf tables' initially, but it created too
strong of a correlation to 'hash table', so I've changed the name to 'map'
to stress that this is a generic key/value and not just hash table.

>> +     } else if (state->regs[regno].ptr == PTR_TO_STACK) {
>> +             if (off >= 0 || off < -MAX_BPF_STACK) {
>> +                     verbose("invalid stack off=%d size=%d\n", off, size);
>> +                     return -EACCES;
>> +             }
>
> So memory (stack) access is only allowed for a stack base regsiter and a
> constant offset, right?

Correct.
In other words it allows instructions:
BPF_STX_MEM(BPF_W, BPF_REG_10, BPF_REG_xx, -stack_offset);

Verifier makes no attempt to track pointer arithmetic and just marks
the result as 'invalid_ptr'.
For non-root programs it will reject programs that are trying to do
arithmetic on pointers (it's not part of this patch yet).

>> +     /* 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));
>
> Missing BPF_REG_5?

yes. good catch.
I guess this shows that we didn't have a use case for function with 5 args :)
Will fix this.

>> +#define PEAK_INT() \
>
> s/PEAK/PEEK/ ?

aren't these the same? ;))
Will fix. Thanks!
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html