[PATCH bpf 1/2] bpf: Resolve to prog->aux->dst_prog->type only for BPF_PROG_TYPE_EXT

[PATCH bpf 1/2] bpf: Resolve to prog->aux->dst_prog->type only for BPF_PROG_TYPE_EXT

[Martin KaFai Lau]

The commit 7e40781cc8b7 ("bpf: verifier: Use target program's type for access verifications")
fixes the verifier checking for BPF_PROG_TYPE_EXT (extension)
prog such that the verifier looks for things based
on the target prog type that it is extending instead of
the BPF_PROG_TYPE_EXT itself.

The current resolve_prog_type() returns the target prog type.
It checks for nullness on prog->aux->dst_prog.  However,
when loading a BPF_PROG_TYPE_TRACING prog and it is tracing another
bpf prog instead of a kernel function, prog->aux->dst_prog is not
NULL also.  In this case, the verifier should still verify as the
BPF_PROG_TYPE_TRACING type instead of the traced prog type in
prog->aux->dst_prog->type.

An oops has been reported when tracing a struct_ops prog.  A NULL
dereference happened in check_return_code() when accessing the
prog->aux->attach_func_proto->type and prog->aux->attach_func_proto
is NULL here because the traced struct_ops prog has the "unreliable" set.

This patch is to change the resolve_prog_type() to only
return the target prog type if the prog being verified is
BPF_PROG_TYPE_EXT.

Fixes: 7e40781cc8b7 ("bpf: verifier: Use target program's type for access verifications")
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
 include/linux/bpf_verifier.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index c1fc4af47f69..3a9d2d7cc6b7 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -570,9 +570,11 @@ static inline u32 type_flag(u32 type)
 	return type & ~BPF_BASE_TYPE_MASK;
 }
 
+/* only use after check_attach_btf_id() */
 static inline enum bpf_prog_type resolve_prog_type(struct bpf_prog *prog)
 {
-	return prog->aux->dst_prog ? prog->aux->dst_prog->type : prog->type;
+	return prog->type == BPF_PROG_TYPE_EXT ?
+		prog->aux->dst_prog->type : prog->type;
 }
 
 #endif /* _LINUX_BPF_VERIFIER_H */
-- 
2.30.2

[PATCH bpf 2/2] bpf: selftests: Test fentry tracing a struct_ops program

[Martin KaFai Lau]

This patch tests attaching an fentry prog to a struct_ops prog.

Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
 .../selftests/bpf/prog_tests/dummy_st_ops.c   | 23 +++++++++++++++++++
 .../selftests/bpf/progs/trace_dummy_st_ops.c  | 21 +++++++++++++++++
 2 files changed, 44 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/trace_dummy_st_ops.c

diff --git a/tools/testing/selftests/bpf/prog_tests/dummy_st_ops.c b/tools/testing/selftests/bpf/prog_tests/dummy_st_ops.c
index 5aa52cc31dc2..c11832657d2b 100644
--- a/tools/testing/selftests/bpf/prog_tests/dummy_st_ops.c
+++ b/tools/testing/selftests/bpf/prog_tests/dummy_st_ops.c
@@ -2,6 +2,7 @@
 /* Copyright (C) 2021. Huawei Technologies Co., Ltd */
 #include <test_progs.h>
 #include "dummy_st_ops.skel.h"
+#include "trace_dummy_st_ops.skel.h"
 
 /* Need to keep consistent with definition in include/linux/bpf.h */
 struct bpf_dummy_ops_state {
@@ -56,6 +57,7 @@ static void test_dummy_init_ptr_arg(void)
 		.ctx_in = args,
 		.ctx_size_in = sizeof(args),
 	);
+	struct trace_dummy_st_ops *trace_skel;
 	struct dummy_st_ops *skel;
 	int fd, err;
 
@@ -64,12 +66,33 @@ static void test_dummy_init_ptr_arg(void)
 		return;
 
 	fd = bpf_program__fd(skel->progs.test_1);
+
+	trace_skel = trace_dummy_st_ops__open();
+	if (!ASSERT_OK_PTR(trace_skel, "trace_dummy_st_ops__open"))
+		goto done;
+
+	err = bpf_program__set_attach_target(trace_skel->progs.fentry_test_1,
+					     fd, "test_1");
+	if (!ASSERT_OK(err, "set_attach_target(fentry_test_1)"))
+		goto done;
+
+	err = trace_dummy_st_ops__load(trace_skel);
+	if (!ASSERT_OK(err, "load(trace_skel)"))
+		goto done;
+
+	err = trace_dummy_st_ops__attach(trace_skel);
+	if (!ASSERT_OK(err, "attach(trace_skel)"))
+		goto done;
+
 	err = bpf_prog_test_run_opts(fd, &attr);
 	ASSERT_OK(err, "test_run");
 	ASSERT_EQ(in_state.val, 0x5a, "test_ptr_ret");
 	ASSERT_EQ(attr.retval, exp_retval, "test_ret");
+	ASSERT_EQ(trace_skel->bss->val, exp_retval, "fentry_val");
 
+done:
 	dummy_st_ops__destroy(skel);
+	trace_dummy_st_ops__destroy(trace_skel);
 }
 
 static void test_dummy_multiple_args(void)
diff --git a/tools/testing/selftests/bpf/progs/trace_dummy_st_ops.c b/tools/testing/selftests/bpf/progs/trace_dummy_st_ops.c
new file mode 100644
index 000000000000..00a4be9d3074
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/trace_dummy_st_ops.c
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+int val = 0;
+
+SEC("fentry/test_1")
+int BPF_PROG(fentry_test_1, __u64 *st_ops_ctx)
+{
+	__u64 state;
+
+	/* Read the traced st_ops arg1 which is a pointer */
+	bpf_probe_read_kernel(&state, sizeof(__u64), (void *)st_ops_ctx);
+	/* Read state->val */
+	bpf_probe_read_kernel(&val, sizeof(__u32), (void *)state);
+
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.30.2

Re: [PATCH bpf 1/2] bpf: Resolve to prog->aux->dst_prog->type only for BPF_PROG_TYPE_EXT

[Yonghong Song]

On 3/29/22 6:14 PM, Martin KaFai Lau wrote:
> The commit 7e40781cc8b7 ("bpf: verifier: Use target program's type for access verifications")
> fixes the verifier checking for BPF_PROG_TYPE_EXT (extension)
> prog such that the verifier looks for things based
> on the target prog type that it is extending instead of
> the BPF_PROG_TYPE_EXT itself.
> 
> The current resolve_prog_type() returns the target prog type.
> It checks for nullness on prog->aux->dst_prog.  However,
> when loading a BPF_PROG_TYPE_TRACING prog and it is tracing another
> bpf prog instead of a kernel function, prog->aux->dst_prog is not
> NULL also.  In this case, the verifier should still verify as the
> BPF_PROG_TYPE_TRACING type instead of the traced prog type in
> prog->aux->dst_prog->type.
> 
> An oops has been reported when tracing a struct_ops prog.  A NULL
> dereference happened in check_return_code() when accessing the
> prog->aux->attach_func_proto->type and prog->aux->attach_func_proto
> is NULL here because the traced struct_ops prog has the "unreliable" set.
> 
> This patch is to change the resolve_prog_type() to only
> return the target prog type if the prog being verified is
> BPF_PROG_TYPE_EXT.
> 
> Fixes: 7e40781cc8b7 ("bpf: verifier: Use target program's type for access verifications")
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>

Acked-by: Yonghong Song <yhs@fb.com>

Re: [PATCH bpf 2/2] bpf: selftests: Test fentry tracing a struct_ops program

[Yonghong Song]

On 3/29/22 6:15 PM, Martin KaFai Lau wrote:
> This patch tests attaching an fentry prog to a struct_ops prog.
> 
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>

Acked-by: Yonghong Song <yhs@fb.com>

Re: [PATCH bpf 1/2] bpf: Resolve to prog->aux->dst_prog->type only for BPF_PROG_TYPE_EXT

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Tue, 29 Mar 2022 18:14:56 -0700 you wrote:
> The commit 7e40781cc8b7 ("bpf: verifier: Use target program's type for access verifications")
> fixes the verifier checking for BPF_PROG_TYPE_EXT (extension)
> prog such that the verifier looks for things based
> on the target prog type that it is extending instead of
> the BPF_PROG_TYPE_EXT itself.
> 
> The current resolve_prog_type() returns the target prog type.
> It checks for nullness on prog->aux->dst_prog.  However,
> when loading a BPF_PROG_TYPE_TRACING prog and it is tracing another
> bpf prog instead of a kernel function, prog->aux->dst_prog is not
> NULL also.  In this case, the verifier should still verify as the
> BPF_PROG_TYPE_TRACING type instead of the traced prog type in
> prog->aux->dst_prog->type.
> 
> [...]

Here is the summary with links:
  - [bpf,1/2] bpf: Resolve to prog->aux->dst_prog->type only for BPF_PROG_TYPE_EXT
    https://git.kernel.org/bpf/bpf/c/4a9c7bbe2ed4
  - [bpf,2/2] bpf: selftests: Test fentry tracing a struct_ops program
    https://git.kernel.org/bpf/bpf/c/0a210af6d0a0

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html