On Tue, Dec 19, 2023 at 9:01 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Mon, 2023-12-18 at 21:54 -0500, Andrei Matei wrote:
> > 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.
>
> 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;
wouldn't variable size access be problematic here without properly
working with size range (instead of a max offset)? Just because max
offset falls into allowed field, doesn't mean that min offset falls
into allowed field. What's even worth, both min and max by themselves
can fall into allowed fields (different ones, though), but between
those two fields there will be a forbidden one?
> 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.
Given the current (seemingly incomplete) checking logic Andrei change
makes sense. But the variable-sized BTF access throws a wrinkle into
this, no? It can't be checked just at min/max offset boundaries, as I
mentioned above.
I don't know if variable-sized BTF access is important (Alexei?,
Martin?), but maybe BTF access has to be checked separately and then
we can keep the check that does pure dump memory access checks simply
and correctly?
>
> [...]
>
> ---
>
> 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
