[PATCH bpf-next 1/2] bpf: Reject bpf_skb_output() from return-side tracing

[PATCH bpf-next 1/2] bpf: Reject bpf_skb_output() from return-side tracing

[Feng Yang]

From: Feng Yang <yangfeng@kylinos.cn>

BPF fexit programs run after the traced function returns, while their
context still contains the original function argument values. A traced
function is free to consume an skb argument before returning, so the
pointer seen by fexit can already be stale.

The verifier checks that the first argument to bpf_skb_output() has the
BTF type of struct sk_buff, but that does not establish its lifetime.
bpf_skb_event_output() then dereferences skb->len and can trigger a
use-after-free.

Do not expose bpf_skb_output() to tracing programs which can run after
the target: fexit, fexit.multi, fsession and fsession.multi. Keep it
available to fentry and other tracing attach types where it is already
supported. fsession must be rejected because the same program runs on
both entry and return and the verifier cannot prove that a helper call
is entry-only.

Fixes: fec56f5890d9 ("bpf: Introduce BPF trampoline")
Reported-by: Quan Sun <2022090917019@std.uestc.edu.cn>
Reported-by: Yinhao Hu <dddddd@hust.edu.cn>
Reported-by: Kaiyan Mei <M202472210@hust.edu.cn>
Closes: https://lore.kernel.org/all/9d61b891-2d52-42b9-bc1a-ad963ccb675d@std.uestc.edu.cn/
Signed-off-by: Yun Lu <luyun_611@163.com>
Signed-off-by: Feng Yang <yangfeng@kylinos.cn>
---
 kernel/trace/bpf_trace.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 29260951aa87..cdede3926e5d 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1339,6 +1339,20 @@ static inline bool is_trace_fsession(const struct bpf_prog *prog)
 		prog->expected_attach_type == BPF_TRACE_FSESSION_MULTI);
 }
 
+static bool tracing_prog_may_run_after_target(const struct bpf_prog *prog)
+{
+	/* The target may consume pointer arguments before these programs run. */
+	switch (prog->expected_attach_type) {
+	case BPF_TRACE_FEXIT:
+	case BPF_TRACE_FEXIT_MULTI:
+	case BPF_TRACE_FSESSION:
+	case BPF_TRACE_FSESSION_MULTI:
+		return true;
+	default:
+		return false;
+	}
+}
+
 static const struct bpf_func_proto *
 kprobe_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {
@@ -1730,6 +1744,8 @@ tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 	switch (func_id) {
 #ifdef CONFIG_NET
 	case BPF_FUNC_skb_output:
+		if (tracing_prog_may_run_after_target(prog))
+			return NULL;
 		return &bpf_skb_output_proto;
 	case BPF_FUNC_xdp_output:
 		return &bpf_xdp_output_proto;
-- 
2.43.0

[PATCH bpf-next 2/2] selftests/bpf: Check bpf_skb_output() tracing restrictions

[Feng Yang]

From: Feng Yang <yangfeng@kylinos.cn>

Add verifier coverage for the bpf_skb_output() tracing policy. Verify
that an fentry program can still use the helper, while the equivalent
fexit program is rejected at load time.

Signed-off-by: Yun Lu <luyun_611@163.com>
Signed-off-by: Feng Yang<yangfeng@kylinos.cn>
---
 .../bpf/progs/verifier_helper_restricted.c    | 32 +++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/verifier_helper_restricted.c b/tools/testing/selftests/bpf/progs/verifier_helper_restricted.c
index 889c9b78b912..058e71927cd5 100644
--- a/tools/testing/selftests/bpf/progs/verifier_helper_restricted.c
+++ b/tools/testing/selftests/bpf/progs/verifier_helper_restricted.c
@@ -3,6 +3,7 @@
 
 #include <linux/bpf.h>
 #include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
 #include "bpf_misc.h"
 
 struct val {
@@ -17,6 +18,13 @@ struct {
 	__type(value, struct val);
 } map_spin_lock SEC(".maps");
 
+struct {
+	__uint(type, BPF_MAP_TYPE_PERF_EVENT_ARRAY);
+	__uint(max_entries, 1);
+	__type(key, __u32);
+	__type(value, __u32);
+} perf_event_map SEC(".maps");
+
 SEC("kprobe")
 __description("bpf_ktime_get_coarse_ns is forbidden in BPF_PROG_TYPE_KPROBE")
 __failure __msg("program of this type cannot use helper bpf_ktime_get_coarse_ns")
@@ -165,4 +173,28 @@ l0_%=:	exit;						\
 	: __clobber_all);
 }
 
+SEC("fentry/skb_tx_error")
+__description("bpf_skb_output is allowed in BPF_TRACE_FENTRY")
+__success
+int BPF_PROG(skb_output_fentry, void *skb)
+{
+	__u64 meta = 0;
+
+	bpf_skb_output(skb, &perf_event_map, BPF_F_CURRENT_CPU,
+		       &meta, sizeof(meta));
+	return 0;
+}
+
+SEC("fexit/skb_tx_error")
+__description("bpf_skb_output is forbidden in BPF_TRACE_FEXIT")
+__failure __msg("program of this type cannot use helper bpf_skb_output")
+int BPF_PROG(skb_output_fexit, void *skb)
+{
+	__u64 meta = 0;
+
+	bpf_skb_output(skb, &perf_event_map, BPF_F_CURRENT_CPU,
+		       &meta, sizeof(meta));
+	return 0;
+}
+
 char _license[] SEC("license") = "GPL";
-- 
2.43.0

Re: [PATCH bpf-next 1/2] bpf: Reject bpf_skb_output() from return-side tracing

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 2740 bytes --]

> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 29260951aa871..cdede3926e5d6 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1339,6 +1339,20 @@ static inline bool is_trace_fsession(const struct bpf_prog *prog)
>  		prog->expected_attach_type == BPF_TRACE_FSESSION_MULTI);
>  }
>
> +static bool tracing_prog_may_run_after_target(const struct bpf_prog *prog)
> +{
> +	/* The target may consume pointer arguments before these programs run. */
> +	switch (prog->expected_attach_type) {
> +	case BPF_TRACE_FEXIT:
> +	case BPF_TRACE_FEXIT_MULTI:
> +	case BPF_TRACE_FSESSION:
> +	case BPF_TRACE_FSESSION_MULTI:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +

Does this new helper need to be static inline? The function is defined
unconditionally here but only referenced inside the CONFIG_NET block of
tracing_prog_func_proto():

kernel/trace/bpf_trace.c:tracing_prog_func_proto() {
    ...
    switch (func_id) {
#ifdef CONFIG_NET
    case BPF_FUNC_skb_output:
        if (tracing_prog_may_run_after_target(prog))
            return NULL;
        return &bpf_skb_output_proto;
    ...
#endif
    ...
}

bpf_trace.c is built from CONFIG_BPF_EVENTS (kernel/trace/Makefile) which
depends on BPF_SYSCALL and (KPROBE_EVENTS || UPROBE_EVENTS) and PERF_EVENTS
(kernel/trace/Kconfig) -- none of which imply CONFIG_NET.

CONFIG_BPF_EVENTS=y with CONFIG_NET=n is valid, and in that configuration
tracing_prog_may_run_after_target() becomes a defined-but-unreferenced
non-inline static:

  kernel/trace/bpf_trace.c:1342:13: warning: 'tracing_prog_may_run_after_target' defined but not used [-Wunused-function]

With CONFIG_WERROR=y (init/Kconfig) this becomes a build failure.

The pre-existing predicates immediately above (is_kprobe_session(),
is_uprobe_multi(), is_uprobe_session(), is_trace_fsession()) are all
written as static inline, which is why they don't trigger this warning.

Would declaring tracing_prog_may_run_after_target() as static inline fix
this?

>  static const struct bpf_func_proto *
>  kprobe_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>  {
> @@ -1730,6 +1744,8 @@ tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>  	switch (func_id) {
>  #ifdef CONFIG_NET
>  	case BPF_FUNC_skb_output:
> +		if (tracing_prog_may_run_after_target(prog))
> +			return NULL;
>  		return &bpf_skb_output_proto;
>  	case BPF_FUNC_xdp_output:
>  		return &bpf_xdp_output_proto;


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/35495259029