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);
- 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.
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.
Alexei, Andrii, what do you think?
