Re: [PATCH bpf-next v2 1/1] bpf: Simplify checking size of helper accesses

[Eduard Zingerman <eddyz87@gmail.com>]

On Mon, 2023-12-18 at 21:54 -0500, Andrei Matei wrote:
> On Mon, Dec 18, 2023 at 7:04 PM Eduard Zingerman <eddyz87@gmail.com> 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.

Not checking consistently for all possible offsets is a good argument, thank you.

> 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 agree with you, that disallowing variable size access in BTF case
might make sense. check_ptr_to_btf_access() calls either:
a. env->ops->btf_struct_access(), which is one of the following:
   1. _tc_cls_act_btf_struct_access() (through a function pointer),
      which allows accessing exactly one field: struct nf_conn->mark;
   2. bpf_tcp_ca_btf_struct_access, which allows accessing several
      fields in sock, tcp_sock and inet_connection_sock structures.
b. btf_struct_access(), which checks the following:
   1. part with btf_find_struct_meta() checks that access does not reach
      to some forbidden field;
   2. btf_struct_walk() checks that offset and size of the access match
      offset and size of some field in the target BTF structure;

Technically, checks a.1, a.2 and b.1 are ok with variable size access,
but b.2 is not and it does not seem to be verified.

I tried a patch below and test_progs seem to pass locally
(but I have some troubles with my local setup at the moment,
 so it should be double-checked).

> 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.

Would be good if others could comment.

[...]

---

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index cf2a09408bdc..946415d11338 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7328,6 +7328,7 @@ static int check_mem_size_reg(struct bpf_verifier_env *env,
 {
        int err;
        const bool size_is_const = tnum_is_const(reg->var_off);
+       struct bpf_reg_state *ptr_reg = &cur_regs(env)[regno - 1];
 
        /* This is used to refine r0 return value bounds for helpers
         * that enforce this value as an upper bound on return values.
@@ -7373,6 +7374,13 @@ static int check_mem_size_reg(struct bpf_verifier_env *env,
                verbose(env, "verifier bug: !zero_size_allowed should have been handled already\n");
                return -EFAULT;
        }
+
+       if (base_type(ptr_reg->type) == PTR_TO_BTF_ID && !size_is_const) {
+               verbose(env, "variable length access to r%d %s is not allowed",
+                       regno - 1, reg_type_str(env, ptr_reg->type));
+               return -EACCES;
+       }
+
        err = check_helper_mem_access(env, regno - 1,
                                      reg->umax_value,
                                      /* zero_size_allowed: we asserted above that umax_value is