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

[Eduard Zingerman <eddyz87@gmail.com>]

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.

[...]