On Tue, Dec 19, 2023 at 11:24 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > 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]. Ah, that's great. I probably should go and read that code before asking questions and making suggestions :) > > > > 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. +1, I'd just factor out error message changes, they are separate, IMO > > [...] > > > 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. > agreed > [...]
