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