On Tue, 2023-12-19 at 11:08 -0800, Andrii Nakryiko wrote: [...] > > > 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? As far as I understand that part, it checks for each forbidden field that it does not intersect with full range [off, off + max_size]. > > 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. Yes, probably this patch makes sense as-is, as a logic is already not consistent. [...] > 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? check_helper_mem_access() is called form many places, so BTF handling should probably remain there. What it lacks is a notion of variable size access. [...]
