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