[bpf,v3,2/2] selftests/bpf: Test r0 and ref lifetime after BPF-BPF call with abnormal return

Commit Message

Arthur Fabre Jan. 6, 2025, 5:15 p.m. UTC

In all three cases where a callee can abnormally return (tail_call(),
LD_ABS, and LD_IND), test the verifier doesn't know the bounds of:

- r0 / what the callee returned.
- References to the caller's stack passed to the callee.

Additionally, ensure the tail_call fallthrough case can't access r0, as
bpf_tail_call() returns nothing on failure.

Signed-off-by: Arthur Fabre <[e-mail]>
---
 .../selftests/bpf/prog_tests/verifier.c       |   2 +
 .../bpf/progs/verifier_abnormal_ret.c         | 115 ++++++++++++++++++
 2 files changed, 117 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_abnormal_ret.c

Comments

Eduard Zingerman Jan. 6, 2025, 8:34 p.m. UTC | #1

On Mon, 2025-01-06 at 18:15 +0100, Arthur Fabre wrote:
> In all three cases where a callee can abnormally return (tail_call(),
> LD_ABS, and LD_IND), test the verifier doesn't know the bounds of:
> 
> - r0 / what the callee returned.
> - References to the caller's stack passed to the callee.
> 
> Additionally, ensure the tail_call fallthrough case can't access r0, as
> bpf_tail_call() returns nothing on failure.
> 
> Signed-off-by: Arthur Fabre <[e-mail]>
> ---

Acked-by: Eduard Zingerman <[e-mail]>

[...]

> +#define TEST(NAME, CALLEE) \
> +	SEC("socket")					\
> +	__description("r0: " #NAME)	\
> +	__failure __msg("math between ctx pointer and register with unbounded min value") \
> +	__naked int check_abnormal_ret_r0_##NAME(void)	\
> +	{						\
> +		asm volatile("				\
> +		r6 = r1;				\
> +		r2 = r10;				\
> +		r2 += -8;				\
> +		call " #CALLEE ";			\
> +		r6 += r0;				\
> +		r0 = 0;					\
> +		exit;					\
> +	"	:					\
> +		:					\
> +		: __clobber_all);			\
> +	}						\
> +							\
> +	SEC("socket")					\
> +	__description("ref: " #NAME)	\
> +	__failure __msg("math between ctx pointer and register with unbounded min value") \
> +	__naked int check_abnormal_ret_ref_##NAME(void)	\
> +	{						\
> +		asm volatile("				\
> +		r6 = r1;				\
> +		r7 = r10;				\
> +		r7 += -8;				\
> +		r2 = r7;				\
> +		call " #CALLEE ";			\
> +		r0 = *(u64*)(r7 + 0);			\
> +		r6 += r0;				\
> +		exit;					\
> +	"	:					\
> +		:					\
> +		: __clobber_all);			\
> +	}

Nit: I think having both cases is an overkill, as both effectively
     test if branching occur.

[...]

> +struct {
> +	__uint(type, BPF_MAP_TYPE_PROG_ARRAY);
> +	__uint(max_entries, 1);
> +	__uint(key_size, sizeof(int));
> +	__array(values, void(void));
> +} map_prog SEC(".maps") = {
> +	.values = {
> +		[0] = (void *)&dummy_prog,
> +	},
> +};
> +
> +static __noinline __used
> +int callee_tail_call(struct __sk_buff *skb, __u64 *foo)
> +{
> +	bpf_tail_call(skb, &map_prog, 0);
> +	*foo = 1;
> +	return 0;
> +}

Nit: I'd also add a test where invalid action is taken
     after bpf_tail_call inside the callee,
     just to make sure that both branches are explored.

> +
> +SEC("socket")
> +__description("r0 not set by tail_call")
> +__failure __msg("R0 !read_ok")
> +int check_abnormal_ret_tail_call_fail(struct __sk_buff *skb)
> +{
> +	return bpf_tail_call(skb, &map_prog, 0);
> +}
> +
> +char _license[] SEC("license") = "GPL";

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
index 3ee40ee9413a..6bed606544e3 100644
--- a/tools/testing/selftests/bpf/prog_tests/verifier.c
+++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
@@ -3,6 +3,7 @@ 
 #include <test_progs.h>
 
 #include "cap_helpers.h"
+#include "verifier_abnormal_ret.skel.h"
 #include "verifier_and.skel.h"
 #include "verifier_arena.skel.h"
 #include "verifier_arena_large.skel.h"
@@ -133,6 +134,7 @@  static void run_tests_aux(const char *skel_name,
 
 #define RUN(skel) run_tests_aux(#skel, skel##__elf_bytes, NULL)
 
+void test_verifier_abnormal_ret(void)         { RUN(verifier_abnormal_ret); }
 void test_verifier_and(void)                  { RUN(verifier_and); }
 void test_verifier_arena(void)                { RUN(verifier_arena); }
 void test_verifier_arena_large(void)          { RUN(verifier_arena_large); }
diff --git a/tools/testing/selftests/bpf/progs/verifier_abnormal_ret.c b/tools/testing/selftests/bpf/progs/verifier_abnormal_ret.c
new file mode 100644
index 000000000000..185c19ba3329
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/verifier_abnormal_ret.c
@@ -0,0 +1,115 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include "../../../include/linux/filter.h"
+#include "bpf_misc.h"
+
+#define TEST(NAME, CALLEE) \
+	SEC("socket")					\
+	__description("r0: " #NAME)	\
+	__failure __msg("math between ctx pointer and register with unbounded min value") \
+	__naked int check_abnormal_ret_r0_##NAME(void)	\
+	{						\
+		asm volatile("				\
+		r6 = r1;				\
+		r2 = r10;				\
+		r2 += -8;				\
+		call " #CALLEE ";			\
+		r6 += r0;				\
+		r0 = 0;					\
+		exit;					\
+	"	:					\
+		:					\
+		: __clobber_all);			\
+	}						\
+							\
+	SEC("socket")					\
+	__description("ref: " #NAME)	\
+	__failure __msg("math between ctx pointer and register with unbounded min value") \
+	__naked int check_abnormal_ret_ref_##NAME(void)	\
+	{						\
+		asm volatile("				\
+		r6 = r1;				\
+		r7 = r10;				\
+		r7 += -8;				\
+		r2 = r7;				\
+		call " #CALLEE ";			\
+		r0 = *(u64*)(r7 + 0);			\
+		r6 += r0;				\
+		exit;					\
+	"	:					\
+		:					\
+		: __clobber_all);			\
+	}
+
+TEST(ld_abs, callee_ld_abs);
+TEST(ld_ind, callee_ld_ind);
+TEST(tail_call, callee_tail_call);
+
+static __naked __noinline __used
+int callee_ld_abs(void)
+{
+	asm volatile("					\
+	r6 = r1;					\
+	r9 = r2;					\
+	.8byte %[ld_abs];				\
+	*(u64*)(r9 + 0) = 1;				\
+	r0 = 0;						\
+	exit;						\
+"	:
+	: __imm_insn(ld_abs, BPF_LD_ABS(BPF_W, 0))
+	: __clobber_all);
+}
+
+static __naked __noinline __used
+int callee_ld_ind(void)
+{
+	asm volatile("					\
+	r6 = r1;					\
+	r7 = 1;						\
+	r9 = r2;					\
+	.8byte %[ld_ind];				\
+	*(u64*)(r9 + 0) = 1;				\
+	r0 = 0;						\
+	exit;						\
+"	:
+	: __imm_insn(ld_ind, BPF_LD_IND(BPF_W, BPF_REG_7, 0))
+	: __clobber_all);
+}
+
+SEC("socket")
+__auxiliary __naked
+int dummy_prog(void)
+{
+	asm volatile("r0 = 1; exit;");
+}
+
+struct {
+	__uint(type, BPF_MAP_TYPE_PROG_ARRAY);
+	__uint(max_entries, 1);
+	__uint(key_size, sizeof(int));
+	__array(values, void(void));
+} map_prog SEC(".maps") = {
+	.values = {
+		[0] = (void *)&dummy_prog,
+	},
+};
+
+static __noinline __used
+int callee_tail_call(struct __sk_buff *skb, __u64 *foo)
+{
+	bpf_tail_call(skb, &map_prog, 0);
+	*foo = 1;
+	return 0;
+}
+
+SEC("socket")
+__description("r0 not set by tail_call")
+__failure __msg("R0 !read_ok")
+int check_abnormal_ret_tail_call_fail(struct __sk_buff *skb)
+{
+	return bpf_tail_call(skb, &map_prog, 0);
+}
+
+char _license[] SEC("license") = "GPL";