On Fri, 6 Dec 2024 at 01:04, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Tue, Dec 3, 2024 at 1:42 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
> >
> > On Tue, 2024-12-03 at 12:19 -0800, Eduard Zingerman wrote:
> > > On Tue, 2024-12-03 at 17:26 +0100, Nick Zavaritsky wrote:
> > > > Hi,
> > > >
> > > > Calls to helpers such as bpf_skb_pull_data, are supposed to invalidate
> > > > all prior checks on packet pointers.
> > > >
> > > > I noticed that if I wrap a call to bpf_skb_pull_data in a function with
> > > > global linkage, pointers checked prior to the call are still considered
> > > > valid after the call. The program is accepted on 6.8 and 6.13-rc1.
> > > >
> > > > I'm curious if it is by design and if not, if it is a known issue.
> > > > Please find the program below.
> > > >
> > > > #include <linux/bpf.h>
> > > > #include <bpf/bpf_helpers.h>
> > > >
> > > > __attribute__((__noinline__))
> > > > long skb_pull_data(struct __sk_buff *sk, __u32 len)
> > > > {
> > > > return bpf_skb_pull_data(sk, len);
> > > > }
> > > >
> > > > SEC("tc")
> > > > int test_invalidate_checks(struct __sk_buff *sk)
> > > > {
> > > > int *p = (void *)(long)sk->data;
> > > > if ((void *)(p + 1) > (void *)(long)sk->data_end) return TCX_DROP;
> > > > skb_pull_data(sk, 0);
> > > > *p = 42;
> > > > return TCX_PASS;
> > > > }
> > > >
> > > > If I remove noinline or add static, the program is rejected as expected.
> > > >
> > >
> > > Hi Nick,
> > >
> > > Thank you for the report. This is a bug. Technically, packet pointers
> > > are invalidated by clear_all_pkt_pointers() called from check_helper_callf().
> > > This functions looks through all packets in current verifier state.
> > > However, global functions are verified independent of call sites,
> > > so pointer 'p' does not exist in verifier state when 'skb_pull_data'
> > > is verified, and thus is not invalidated.
> > >
> >
> > There are several ways to fix this:
> > - The "dumb" way:
> > - forbid calling helpers that bpf_helper_changes_pkt_data()
> > from global functions.
> > - The "simple" way:
> > - at some early stage:
> > - scan all global functions, to see if there are any calls to
> > helpers that bpf_helper_changes_pkt_data(). If there are,
> > remember this as an "effect" of the function;
> > - build a call-graph of global functions and propagate computed
> > effects over this call-graph (if A calls B and B does
> > clear_all_pkt_pointers(), then A also does it).
> > - during main verification phase, if a call to a global function is
> > verified, check it's effects and update state accordingly
> > (e.g. call clear_all_pkt_pointers()).
> > - The "correct" way:
> > - build a call-graph of global functions;
> > - verify these functions in a post-order;
> > - while verifying, collect "effects" information
> > (so far, the single effect is whether or not
> > clear_all_pkt_pointers() had been ever called for the function);
>
> So this is the only "side effect" we have right now? Are there any
> others we have already or can reasonably anticipate? I'm just trying
> to decide if we need to generalize this concept.
It would be good to generalize, we can use such "effects" or
"summarization" to also know whether a global func is sleepable or
not.
This would allow lifting the current restriction of calling them from
atomic contexts seen in verifier state, right that's the only thing
that's preventing the call. So it would be a nice side benefit.
>
> > - if a call to global function is verified, check it's effects and
> > update state accordingly (e.g. call clear_all_pkt_pointers()).
> >
> > "dumb" is probably a no-go as it is too restrictive.
> > The only advantage of "simple" over "correct" that I see is
> > that the logic for clear_all_pkt_pointers() remains confined
> > to check_helper_call() and is not duplicated in a separate pass.
>
> "simple" doesn't take into account dead code elimination, undermining
> BPF CO-RE-based feature detection, so I think this is also a no-go
>
> > In theory, this also allows to compute more complex function effects
> > on the main verification pass.
> >
> > I think "simple" is a way to go at the moment.
>
> I think neither of the above are fully valid, tbh. "correct" will do
> eager subprog validation, even if due to dead code elimination that
> global function might not have been called.
>
> From the outset, I think the "right" way to solve this would be to
> start verification from the main program. When we encounter global
> subprog verification, we pause verification for the main program,
> create a new isolated verifier state, proceed with global subprog
> verification, and so on until we check everything. So basically a
> stack of subprogs to validate.
>
> This is PITA, of course, just for this (which is also the question
> about the generalization of the "side effects" concept). So I don't
> know, maybe for now the "dumb" way is the way?
>
> >
> > Alexei, Andrii, what do you think?
> >
>
