On Mon, Dec 18, 2023 at 7:04 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Sat, 2023-12-16 at 20:06 -0500, Andrei Matei wrote:
> [...]
>
> > (*) Besides standing to reason that the checks for a bigger size access
> > are a super-set of the checks for a smaller size access, I have also
> > mechanically verified this by reading the code for all types of
> > pointers. I could convince myself that it's true for all but
> > PTR_TO_BTF_ID (check_ptr_to_btf_access). There, simply looking
> > line-by-line does not immediately prove what we want. If anyone has any
> > qualms, let me know.
>
> check_help_mem_access() is a bit obfuscated :)
> After staring at it for a bit I have a question regarding
> check_ptr_to_btf_access():
> - it can call btf_struct_access(),
> which in can call btf_struct_walk(),
> which has the following check:
>
> if (btf_type_is_ptr(mtype)) {
> const struct btf_type *stype, *t;
> enum bpf_type_flag tmp_flag = 0;
> u32 id;
>
> if (msize != size || off != moff) {
> bpf_log(log,
> "cannot access ptr member %s with moff %u in struct %s with off %u size %u\n",
> mname, moff, tname, off, size);
> return -EACCES;
> }
>
> - previously this code was executed twice, for size 0 and for size
> umax_value of the size register;
> - now this code is executed only for umax_value of the size register;
> - is it possible that with size 0 this code could have reported error
> -EACCESS error, which would be missed now?
I don't have a good answer. I too have looked at check_ptr_to_btf_access() and
ended up confused -- but then again, I don't know what's supposed to be allowed
and what's supposed to not be allowed. I will say, though, that I don't think
the code as it stands make sense, and I don't think any interaction between the
zero-size check and btf access is intentional. Around [1] we've looked a bit at
the history of this zero-size check, and it's been there forever, predating
most of the code around it. What convinces me personally that the zero-size
check was not load-bearing is the fact that we were only performing
the check iff
umin == 0 -- we were not consistently performing a check for the umin value.
Also, obviously, we were not performing a check for every possible value in
between umin and umax. So I can't really imagine positive benefits of the
inconsistent check we were doing. But then again, I cannot actually speak with
confidence about it.
As a btw, I'll say that we don't allow variable-offset accesses to btf ptr [2].
I don't know if this should influence how we treat the access size... but
maybe? Like, should we disallow variable-sized accesses on the same argument as
disallowing variable-offset ones (whatever that argument may be)? I don't know
what I'm talking about (generally BTF is foreign to me), but I imagine this all
means that currently the verifier allows one to read from an array field by
starting at a compile-time constant offset, and extending to a variable size.
However, you cannot start from an arbitrary offset, though. Does this
combination of being strict about the offset but permissive about the size make
sense?
I'll take guidance. If people prefer we don't touch this code at all, that's
fine. Although it doesn't feel good to be driven simply by fear.
[1] https://lore.kernel.org/bpf/CAP01T77M=SyNviMYCO-koxizvD6eGm=5KQ1Wv=ahbRU5XQB4bA@xxxxxxxxxxxxxx/
[2] https://github.com/torvalds/linux/blob/ee5cc0363ea0d587f62349ff3b3e2dfa751832e4/kernel/bpf/verifier.c#L3318-L3326
>
> Except for the question above I don't see any issues,
> but check_help_mem_access() has many sub-cases,
> so I might have missed something.
>
> Also a few nits below.
>
> [...]
>
> > @@ -7256,6 +7256,65 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, int regno,
> > }
> > }
> >
> > +/* Helper function for logging an error about an invalid attempt to perform a
> > + * (possibly) zero-sized memory access. The pointer being dereferenced is in
> > + * register @ptr_regno, and the size of the access is in register @size_regno.
> > + * The size register is assumed to either be a constant zero or have a zero lower
> > + * bound.
> > + *
> > + * Logs a message like:
> > + * invalid zero-size read. Size comes from R2=0. Attempting to dereference *map_value R1: off=[0,4] value_size=48
> > + */
> > +static void log_zero_size_access_err(struct bpf_verifier_env *env,
> > + int ptr_regno,
> > + int size_regno)
> > +{
> > + struct bpf_reg_state *ptr_reg = &cur_regs(env)[ptr_regno];
> > + struct bpf_reg_state *size_reg = &cur_regs(env)[size_regno];
> > + const bool size_is_const = tnum_is_const(size_reg->var_off);
> > + const char *ptr_type_str = reg_type_str(env, ptr_reg->type);
> > + /* allocate a few buffers to be used as parts of the error message */
> > + char size_range_buf[64] = {0}, max_size_buf[64] = {0}, off_buf[64] = {0};
> > + s64 min_off, max_off;
>
> Nit: empty is needed here
>
> [...]
>
> > /* verify arguments to helpers or kfuncs consisting of a pointer and an access
> > * size.
> > *
> > @@ -7268,6 +7327,7 @@ static int check_mem_size_reg(struct bpf_verifier_env *env,
> > struct bpf_call_arg_meta *meta)
> > {
> > int err;
> > + const bool size_is_const = tnum_is_const(reg->var_off);
>
> Nit: please swap definitions to get the "reverse Christmas tree":
>
> const bool size_is_const = tnum_is_const(reg->var_off);
> int err;
>
> >
> > /* This is used to refine r0 return value bounds for helpers
> > * that enforce this value as an upper bound on return values.
> > @@ -7282,7 +7342,7 @@ static int check_mem_size_reg(struct bpf_verifier_env *env,
> > /* The register is SCALAR_VALUE; the access check
> > * happens using its boundaries.
> > */
> > - if (!tnum_is_const(reg->var_off))
> > + if (!size_is_const)
> > /* For unprivileged variable accesses, disable raw
> > * mode so that the program is required to
> > * initialize all the memory that the helper could
> > @@ -7296,12 +7356,9 @@ static int check_mem_size_reg(struct bpf_verifier_env *env,
> > return -EACCES;
> > }
> >
> > - if (reg->umin_value == 0) {
> > - err = check_helper_mem_access(env, regno - 1, 0,
> > - zero_size_allowed,
> > - meta);
> > - if (err)
> > - return err;
> > + if (reg->umin_value == 0 && !zero_size_allowed) {
> > + log_zero_size_access_err(env, regno-1, regno);
> > + return -EACCES;
> > }
> >
> > if (reg->umax_value >= BPF_MAX_VAR_SIZ) {
> > @@ -7309,9 +7366,21 @@ static int check_mem_size_reg(struct bpf_verifier_env *env,
> > regno);
> > return -EACCES;
> > }
> > + /* If !zero_size_allowed, we already checked that umin_value > 0, so
> > + * umax_value should also be > 0.
> > + */
> > + if (reg->umax_value == 0 && !zero_size_allowed) {
> > + verbose(env, "verifier bug: !zero_size_allowed should have been handled already\n");
> > + return -EFAULT;
> > + }
> > err = check_helper_mem_access(env, regno - 1,
> > reg->umax_value,
> > - zero_size_allowed, meta);
> > + /* zero_size_allowed: we asserted above that umax_value is
> > + * not zero if !zero_size_allowed, so we don't need any
> > + * further checks.
> > + */
> > + true ,
> ^
> Nit: extra space ---------'
>
> > + meta);
> > if (!err)
> > err = mark_chain_precision(env, regno);
> > return err;
>
> [...]
