bpf: tail calls do not modify packet data

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
1a4607f using a packet pointer after a tail call was working fine,
as it should.

Fixes: 1a4607f ("bpf: consider that tail calls invalidate packet pointers")
Signed-off-by: Martin Teichmann <[email protected]>

Author: tecki

kernel/bpf/verifier.c

@@ -17827,7 +17827,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;

net/core/filter.c

@@ -8032,8 +8032,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;

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";