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

[Alexei Starovoitov]

On Tue, Jul 1, 2014 at 10:05 PM, Namhyung Kim <namhyung@xxxxxxxxx> 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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/