The bpf verifier checks whether packet data is modified within a helper function, and if so invalidates the pointer to that data. Currently the verifier always invalidates if the helper function called is a tail call, as it cannot tell whether the called function does or does not modify the packet data. However, in this case, the fact that the packet might be modified is irrelevant in the code following the helper call, as the tail call only returns if there is nothing to execute, otherwise the calling (sub)program will return directly after the tail call finished. So it is this (sub)program for which the pointer to packet data needs to be invalidated. Fortunately, there are already two distinct points in the code for invalidating packet pointers directly after a helper call, and for entire (sub)programs. This commit assures that the pointer is only invalidated in the relevant case. Note that this is a regression bug: taking care of tail calls only became necessary when subprograms were introduced, before commit 1a4607ffba35 using a packet pointer after a tail call was working fine, as it should. Fixes: 1a4607ffba35 ("bpf: consider that tail calls invalidate packet pointers") Signed-off-by: Martin Teichmann --- kernel/bpf/verifier.c | 3 ++- net/core/filter.c | 2 -- .../selftests/bpf/progs/verifier_sock.c | 18 ++++++++++++++++-- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 6d175849e57a..4bc36a1efe93 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -17879,7 +17879,8 @@ static int visit_insn(int t, struct bpf_verifier_env *env) */ if (ret == 0 && fp->might_sleep) mark_subprog_might_sleep(env, t); - if (bpf_helper_changes_pkt_data(insn->imm)) + if (bpf_helper_changes_pkt_data(insn->imm) + || insn->imm == BPF_FUNC_tail_call) mark_subprog_changes_pkt_data(env, t); } else if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) { struct bpf_kfunc_call_arg_meta meta; diff --git a/net/core/filter.c b/net/core/filter.c index 9d67a34a6650..71a1cfed49f1 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -8038,8 +8038,6 @@ bool bpf_helper_changes_pkt_data(enum bpf_func_id func_id) case BPF_FUNC_xdp_adjust_head: case BPF_FUNC_xdp_adjust_meta: case BPF_FUNC_xdp_adjust_tail: - /* tail-called program could call any of the above */ - case BPF_FUNC_tail_call: return true; default: return false; diff --git a/tools/testing/selftests/bpf/progs/verifier_sock.c b/tools/testing/selftests/bpf/progs/verifier_sock.c index 2b4610b53382..3e22b4f8aec4 100644 --- a/tools/testing/selftests/bpf/progs/verifier_sock.c +++ b/tools/testing/selftests/bpf/progs/verifier_sock.c @@ -1117,10 +1117,10 @@ int tail_call(struct __sk_buff *sk) return 0; } -/* Tail calls invalidate packet pointers. */ +/* Tail calls in sub-programs invalidate packet pointers. */ SEC("tc") __failure __msg("invalid mem access") -int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk) +int invalidate_pkt_pointers_by_indirect_tail_call(struct __sk_buff *sk) { int *p = (void *)(long)sk->data; @@ -1131,4 +1131,18 @@ int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk) return TCX_PASS; } +/* Direct tail calls do not invalidate packet pointers. */ +SEC("tc") +__success +int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk) +{ + int *p = (void *)(long)sk->data; + + if ((void *)(p + 1) > (void *)(long)sk->data_end) + return TCX_DROP; + bpf_tail_call_static(sk, &jmp_table, 0); + *p = 42; /* this is NOT unsafe: tail calls don't return */ + return TCX_PASS; +} + char _license[] SEC("license") = "GPL"; -- 2.43.0