[PATCH bpf-next v2 0/3] bpf: Fix array bounds error with may_goto and add selftest

[PATCH bpf-next v2 0/3] bpf: Fix array bounds error with may_goto and add selftest

[Jiayuan Chen]

Syzbot caught an array out-of-bounds bug [1]. It turns out that when the
BPF program runs through do_misc_fixups(), it allocates an extra 8 bytes
on the call stack, which eventually causes stack_depth to exceed 512.

I was able to reproduce this issue probabilistically by enabling
CONFIG_UBSAN=y and disabling CONFIG_BPF_JIT_ALWAYS_ON with the selfttest
I provide in second patch(although it doesn't happen every time - I didn't
dig deeper into why UBSAN behaves this way).

Furthermore, if I set /proc/sys/net/core/bpf_jit_enable to 0 to disable
the jit, a panic occurs, and the reason is the same, that bpf_func is
assigned an incorrect address.

[---[ end trace ]---
[Oops: general protection fault, probably for non-canonical address
0x100f0e0e0d090808: 0000 [#1] PREEMPT SMP NOPTI
[Tainted: [W]=WARN, [O]=OOT_MODULE
[RIP: 0010:bpf_test_run+0x1d2/0x360
[RSP: 0018:ffffafc7955178a0 EFLAGS: 00010246
[RAX: 100f0e0e0d090808 RBX: ffff8e9fdb2c4100 RCX: 0000000000000018
[RDX: 00000000002b5b18 RSI: ffffafc780497048 RDI: ffff8ea04d601700
[RBP: ffffafc780497000 R08: ffffafc795517a0c R09: 0000000000000000
[R10: 0000000000000000 R11: fefefefefefefeff R12: ffff8ea04d601700
[R13: ffffafc795517928 R14: ffffafc795517928 R15: 0000000000000000
[CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[CR2: 00007f181c064648 CR3: 00000001aa2be003 CR4: 0000000000770ef0
[DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
[PKRU: 55555554
[Call Trace:
[ <TASK>
[ ? die_addr+0x36/0x90
[ ? exc_general_protection+0x237/0x430
[ ? asm_exc_general_protection+0x26/0x30
[ ? bpf_test_run+0x1d2/0x360
[ ? bpf_test_run+0x10d/0x360
[ ? __link_object+0x12a/0x1e0
[ ? slab_build_skb+0x23/0x130
[ ? kmem_cache_alloc_noprof+0x2ea/0x3f0
[ ? sk_prot_alloc+0xc2/0x120
[ bpf_prog_test_run_skb+0x21b/0x590
[ __sys_bpf+0x340/0xa80
[ __x64_sys_bpf+0x1e/0x30

---
v1 -> v2:
Directly reject loading programs with a stack size greater than 512 when
jit disabled.(Suggested by Alexei Starovoitov)
https://lore.kernel.org/bpf/20250212135251.85487-1-mrpre@163.com/T/#u

---
Jiayuan Chen (3):
  bpf: Fix array bounds error with may_goto
  selftests/bpf: Allow the program to select specific modes for testing
  selftests/bpf: Add selftest for may_goto

 kernel/bpf/core.c                             | 18 +++++--
 kernel/bpf/verifier.c                         |  7 +++
 tools/testing/selftests/bpf/progs/bpf_misc.h  |  2 +
 .../selftests/bpf/progs/verifier_stack_ptr.c  | 50 +++++++++++++++++++
 tools/testing/selftests/bpf/test_loader.c     | 27 ++++++++++
 5 files changed, 100 insertions(+), 4 deletions(-)

-- 
2.47.1

[PATCH bpf-next v2 1/3] bpf: Fix array bounds error with may_goto

[Jiayuan Chen]

may_goto uses an additional 8 bytes on the stack, which causes the
interpreters[] array to go out of bounds when calculating index by
stack_size.

1. If a BPF program is rewritten, re-evaluate the stack size. For non-JIT
cases, reject loading directly.

2. For non-JIT cases, calculating interpreters[idx] may still cause
out-of-bounds array access, and just warn about it.

3. For jit_requested cases, the execution of bpf_func also needs to be
warned. So Move the definition of function __bpf_prog_ret0_warn out of
the macro definition CONFIG_BPF_JIT_ALWAYS_ON

Reported-by: syzbot+d2a2c639d03ac200a4f1@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/bpf/0000000000000f823606139faa5d@google.com/
Fixes: 011832b97b311 ("bpf: Introduce may_goto instruction")
Signed-off-by: Jiayuan Chen <mrpre@163.com>
---
 kernel/bpf/core.c     | 18 ++++++++++++++----
 kernel/bpf/verifier.c |  7 +++++++
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index da729cbbaeb9..59291261f825 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -2269,6 +2269,9 @@ EVAL6(PROG_NAME_LIST, 32, 64, 96, 128, 160, 192)
 EVAL6(PROG_NAME_LIST, 224, 256, 288, 320, 352, 384)
 EVAL4(PROG_NAME_LIST, 416, 448, 480, 512)
 };
+
+#define MAX_INTERPRETERS_CALLBACK (sizeof(interpreters) / sizeof(*interpreters))
+
 #undef PROG_NAME_LIST
 #define PROG_NAME_LIST(stack_size) PROG_NAME_ARGS(stack_size),
 static __maybe_unused
@@ -2290,17 +2293,18 @@ void bpf_patch_call_args(struct bpf_insn *insn, u32 stack_depth)
 	insn->code = BPF_JMP | BPF_CALL_ARGS;
 }
 #endif
-#else
+#endif
+
 static unsigned int __bpf_prog_ret0_warn(const void *ctx,
 					 const struct bpf_insn *insn)
 {
 	/* If this handler ever gets executed, then BPF_JIT_ALWAYS_ON
-	 * is not working properly, so warn about it!
+	 * is not working properly, or interpreter is being used when
+	 * prog->jit_requested is not 0, so warn about it!
 	 */
 	WARN_ON_ONCE(1);
 	return 0;
 }
-#endif
 
 bool bpf_prog_map_compatible(struct bpf_map *map,
 			     const struct bpf_prog *fp)
@@ -2380,8 +2384,14 @@ static void bpf_prog_select_func(struct bpf_prog *fp)
 {
 #ifndef CONFIG_BPF_JIT_ALWAYS_ON
 	u32 stack_depth = max_t(u32, fp->aux->stack_depth, 1);
+	u32 idx = (round_up(stack_depth, 32) / 32) - 1;
 
-	fp->bpf_func = interpreters[(round_up(stack_depth, 32) / 32) - 1];
+	if (!fp->jit_requested) {
+		WARN_ON_ONCE(idx >= MAX_INTERPRETERS_CALLBACK);
+		fp->bpf_func = interpreters[idx];
+	} else {
+		fp->bpf_func = __bpf_prog_ret0_warn;
+	}
 #else
 	fp->bpf_func = __bpf_prog_ret0_warn;
 #endif
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 9971c03adfd5..fcd302904ba0 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -21882,6 +21882,13 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
 		if (subprogs[cur_subprog + 1].start == i + delta + 1) {
 			subprogs[cur_subprog].stack_depth += stack_depth_extra;
 			subprogs[cur_subprog].stack_extra = stack_depth_extra;
+
+			stack_depth = subprogs[cur_subprog].stack_depth;
+			if (stack_depth > MAX_BPF_STACK && !prog->jit_requested) {
+				verbose(env, "stack size %d(extra %d) is too large\n",
+					stack_depth, stack_depth_extra);
+				return -EINVAL;
+			}
 			cur_subprog++;
 			stack_depth = subprogs[cur_subprog].stack_depth;
 			stack_depth_extra = 0;
-- 
2.47.1

[PATCH bpf-next v2 2/3] selftests/bpf: Allow the program to select specific modes for testing

[Jiayuan Chen]

In some cases, the verification logic under the interpreter and JIT
differs, such as may_goto, and the test program behaves differently under
different runtime modes, requiring separate verification logic for each
result.

Signed-off-by: Jiayuan Chen <mrpre@163.com>
---
 tools/testing/selftests/bpf/progs/bpf_misc.h |  2 ++
 tools/testing/selftests/bpf/test_loader.c    | 27 ++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/bpf_misc.h b/tools/testing/selftests/bpf/progs/bpf_misc.h
index f45f4352feeb..dd23822cfb5a 100644
--- a/tools/testing/selftests/bpf/progs/bpf_misc.h
+++ b/tools/testing/selftests/bpf/progs/bpf_misc.h
@@ -135,6 +135,8 @@
 #define __arch_arm64		__arch("ARM64")
 #define __arch_riscv64		__arch("RISCV64")
 #define __caps_unpriv(caps)	__attribute__((btf_decl_tag("comment:test_caps_unpriv=" EXPAND_QUOTE(caps))))
+#define __use_jit()		__attribute__((btf_decl_tag("comment:run_mode=jit")))
+#define __use_interp()		__attribute__((btf_decl_tag("comment:run_mode=interpreter")))
 
 /* Define common capabilities tested using __caps_unpriv */
 #define CAP_NET_ADMIN		12
diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
index 53b06647cf57..2c23178d9a7a 100644
--- a/tools/testing/selftests/bpf/test_loader.c
+++ b/tools/testing/selftests/bpf/test_loader.c
@@ -37,6 +37,7 @@
 #define TEST_TAG_JITED_PFX "comment:test_jited="
 #define TEST_TAG_JITED_PFX_UNPRIV "comment:test_jited_unpriv="
 #define TEST_TAG_CAPS_UNPRIV "comment:test_caps_unpriv="
+#define TEST_TAG_RUN_MODE_PFX "comment:run_mode="
 
 /* Warning: duplicated in bpf_misc.h */
 #define POINTER_VALUE	0xcafe4all
@@ -55,6 +56,11 @@ enum mode {
 	UNPRIV = 2
 };
 
+enum run_mode {
+	JIT = 1 << 0,
+	INTERP = 1 << 1
+};
+
 struct expect_msg {
 	const char *substr; /* substring match */
 	regex_t regex;
@@ -87,6 +93,7 @@ struct test_spec {
 	int prog_flags;
 	int mode_mask;
 	int arch_mask;
+	int run_mode;
 	bool auxiliary;
 	bool valid;
 };
@@ -406,6 +413,7 @@ static int parse_test_spec(struct test_loader *tester,
 	bool collect_jit = false;
 	int func_id, i, err = 0;
 	u32 arch_mask = 0;
+	u32 run_mode = 0;
 	struct btf *btf;
 	enum arch arch;
 
@@ -580,10 +588,22 @@ static int parse_test_spec(struct test_loader *tester,
 			if (err)
 				goto cleanup;
 			spec->mode_mask |= UNPRIV;
+		} else if (str_has_pfx(s, TEST_TAG_RUN_MODE_PFX)) {
+			val = s + sizeof(TEST_TAG_RUN_MODE_PFX) - 1;
+			if (strcmp(val, "jit") == 0) {
+				run_mode = JIT;
+			} else if (strcmp(val, "interpreter") == 0) {
+				run_mode = INTERP;
+			} else {
+				PRINT_FAIL("bad run mode spec: '%s'", val);
+				err = -EINVAL;
+				goto cleanup;
+			}
 		}
 	}
 
 	spec->arch_mask = arch_mask ?: -1;
+	spec->run_mode = run_mode ?: (JIT | INTERP);
 
 	if (spec->mode_mask == 0)
 		spec->mode_mask = PRIV;
@@ -930,6 +950,7 @@ void run_subtest(struct test_loader *tester,
 	struct test_subspec *subspec = unpriv ? &spec->unpriv : &spec->priv;
 	struct bpf_program *tprog = NULL, *tprog_iter;
 	struct bpf_link *link, *links[32] = {};
+	bool jit_enabled = is_jit_enabled();
 	struct test_spec *spec_iter;
 	struct cap_state caps = {};
 	struct bpf_object *tobj;
@@ -946,6 +967,12 @@ void run_subtest(struct test_loader *tester,
 		return;
 	}
 
+	if ((jit_enabled && spec->run_mode & INTERP) ||
+	    (!jit_enabled && spec->run_mode & JIT)) {
+		test__skip();
+		return;
+	}
+
 	if (unpriv) {
 		if (!can_execute_unpriv(tester, spec)) {
 			test__skip();
-- 
2.47.1

[PATCH bpf-next v2 3/3] selftests/bpf: Add selftest for may_goto

[Jiayuan Chen]

Add test cases to ensure the maximum stack size can be properly limited to
512.

Test result:
echo "0" > /proc/sys/net/core/bpf_jit_enable
./test_progs -t verifier_stack_ptr
verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto with jit:SKIP
verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto without jit:OK

echo "1" > /proc/sys/net/core/bpf_jit_enable
verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto with jit:OK
verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto without jit:SKIP

Signed-off-by: Jiayuan Chen <mrpre@163.com>
---
 .../selftests/bpf/progs/verifier_stack_ptr.c  | 50 +++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c b/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c
index 417c61cd4b19..8ffe5a01d140 100644
--- a/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c
+++ b/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c
@@ -481,4 +481,54 @@ l1_%=:	r0 = 42;					\
 	: __clobber_all);
 }
 
+SEC("socket")
+__description("PTR_TO_STACK stack size > 512")
+__failure __msg("invalid write to stack R1 off=-520 size=8")
+__naked void stack_check_size_gt_512(void)
+{
+	asm volatile (
+	"r1 = r10;"
+	"r1 += -520;"
+	"r0 = 42;"
+	"*(u64*)(r1 + 0) = r0;"
+	"exit;"
+	::: __clobber_all);
+}
+
+#ifdef __BPF_FEATURE_MAY_GOTO
+SEC("socket")
+__description("PTR_TO_STACK stack size 512 with may_goto with jit")
+__use_jit()
+__success __retval(42)
+__naked void stack_check_size_512_with_may_goto_jit(void)
+{
+	asm volatile (
+	"r1 = r10;"
+	"r1 += -512;"
+	"r0 = 42;"
+	"*(u32*)(r1 + 0) = r0;"
+	"may_goto l0_%=;"
+	"r2 = 100;"
+"l0_%=:	exit;"
+	::: __clobber_all);
+}
+
+SEC("socket")
+__description("PTR_TO_STACK stack size 512 with may_goto without jit")
+__use_interp()
+__failure __msg("stack size 520(extra 8) is too large")
+__naked void stack_check_size_512_with_may_goto(void)
+{
+	asm volatile (
+	"r1 = r10;"
+	"r1 += -512;"
+	"r0 = 42;"
+	"*(u32*)(r1 + 0) = r0;"
+	"may_goto l0_%=;"
+	"r2 = 100;"
+"l0_%=:	exit;"
+	::: __clobber_all);
+}
+#endif
+
 char _license[] SEC("license") = "GPL";
-- 
2.47.1

Re: [PATCH bpf-next v2 1/3] bpf: Fix array bounds error with may_goto

[Alexei Starovoitov]

On Thu, Feb 13, 2025 at 5:13 AM Jiayuan Chen <mrpre@163.com> wrote:
>
> may_goto uses an additional 8 bytes on the stack, which causes the
> interpreters[] array to go out of bounds when calculating index by
> stack_size.
>
> 1. If a BPF program is rewritten, re-evaluate the stack size. For non-JIT
> cases, reject loading directly.
>
> 2. For non-JIT cases, calculating interpreters[idx] may still cause
> out-of-bounds array access, and just warn about it.
>
> 3. For jit_requested cases, the execution of bpf_func also needs to be
> warned. So Move the definition of function __bpf_prog_ret0_warn out of
> the macro definition CONFIG_BPF_JIT_ALWAYS_ON
>
> Reported-by: syzbot+d2a2c639d03ac200a4f1@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/bpf/0000000000000f823606139faa5d@google.com/
> Fixes: 011832b97b311 ("bpf: Introduce may_goto instruction")
> Signed-off-by: Jiayuan Chen <mrpre@163.com>
> ---
>  kernel/bpf/core.c     | 18 ++++++++++++++----
>  kernel/bpf/verifier.c |  7 +++++++
>  2 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index da729cbbaeb9..59291261f825 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -2269,6 +2269,9 @@ EVAL6(PROG_NAME_LIST, 32, 64, 96, 128, 160, 192)
>  EVAL6(PROG_NAME_LIST, 224, 256, 288, 320, 352, 384)
>  EVAL4(PROG_NAME_LIST, 416, 448, 480, 512)
>  };
> +
> +#define MAX_INTERPRETERS_CALLBACK (sizeof(interpreters) / sizeof(*interpreters))

There is ARRAY_SIZE macro.

>  #undef PROG_NAME_LIST
>  #define PROG_NAME_LIST(stack_size) PROG_NAME_ARGS(stack_size),
>  static __maybe_unused
> @@ -2290,17 +2293,18 @@ void bpf_patch_call_args(struct bpf_insn *insn, u32 stack_depth)
>         insn->code = BPF_JMP | BPF_CALL_ARGS;
>  }
>  #endif
> -#else
> +#endif
> +
>  static unsigned int __bpf_prog_ret0_warn(const void *ctx,
>                                          const struct bpf_insn *insn)
>  {
>         /* If this handler ever gets executed, then BPF_JIT_ALWAYS_ON
> -        * is not working properly, so warn about it!
> +        * is not working properly, or interpreter is being used when
> +        * prog->jit_requested is not 0, so warn about it!
>          */
>         WARN_ON_ONCE(1);
>         return 0;
>  }
> -#endif
>
>  bool bpf_prog_map_compatible(struct bpf_map *map,
>                              const struct bpf_prog *fp)
> @@ -2380,8 +2384,14 @@ static void bpf_prog_select_func(struct bpf_prog *fp)
>  {
>  #ifndef CONFIG_BPF_JIT_ALWAYS_ON
>         u32 stack_depth = max_t(u32, fp->aux->stack_depth, 1);
> +       u32 idx = (round_up(stack_depth, 32) / 32) - 1;
>
> -       fp->bpf_func = interpreters[(round_up(stack_depth, 32) / 32) - 1];
> +       if (!fp->jit_requested) {

I don't think above check is necessary.
Why not just
if (WARN_ON_ONCE(idx >= ARRAY_SIZE(interpreters)))
  fp->bpf_func = __bpf_prog_ret0_warn;
else
  fp->bpf_func = interpreters[idx];

> +               WARN_ON_ONCE(idx >= MAX_INTERPRETERS_CALLBACK);
> +               fp->bpf_func = interpreters[idx];
> +       } else {
> +               fp->bpf_func = __bpf_prog_ret0_warn;
> +       }
>  #else
>         fp->bpf_func = __bpf_prog_ret0_warn;
>  #endif
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 9971c03adfd5..fcd302904ba0 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -21882,6 +21882,13 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
>                 if (subprogs[cur_subprog + 1].start == i + delta + 1) {
>                         subprogs[cur_subprog].stack_depth += stack_depth_extra;
>                         subprogs[cur_subprog].stack_extra = stack_depth_extra;
> +
> +                       stack_depth = subprogs[cur_subprog].stack_depth;
> +                       if (stack_depth > MAX_BPF_STACK && !prog->jit_requested) {
> +                               verbose(env, "stack size %d(extra %d) is too large\n",
> +                                       stack_depth, stack_depth_extra);
> +                               return -EINVAL;
> +                       }
>                         cur_subprog++;
>                         stack_depth = subprogs[cur_subprog].stack_depth;
>                         stack_depth_extra = 0;
> --
> 2.47.1
>

Re: [PATCH bpf-next v2 3/3] selftests/bpf: Add selftest for may_goto

[Alexei Starovoitov]

On Thu, Feb 13, 2025 at 5:13 AM Jiayuan Chen <mrpre@163.com> wrote:
>
> Add test cases to ensure the maximum stack size can be properly limited to
> 512.
>
> Test result:
> echo "0" > /proc/sys/net/core/bpf_jit_enable
> ./test_progs -t verifier_stack_ptr
> verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto with jit:SKIP
> verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto without jit:OK
>
> echo "1" > /proc/sys/net/core/bpf_jit_enable
> verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto with jit:OK
> verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto without jit:SKIP

echo '0|1' is not longer necessary ?
The commit log seems obsolete?

pw-bot: cr

> Signed-off-by: Jiayuan Chen <mrpre@163.com>
> ---
>  .../selftests/bpf/progs/verifier_stack_ptr.c  | 50 +++++++++++++++++++
>  1 file changed, 50 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c b/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c
> index 417c61cd4b19..8ffe5a01d140 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c
> @@ -481,4 +481,54 @@ l1_%=:     r0 = 42;                                        \
>         : __clobber_all);
>  }
>
> +SEC("socket")
> +__description("PTR_TO_STACK stack size > 512")
> +__failure __msg("invalid write to stack R1 off=-520 size=8")
> +__naked void stack_check_size_gt_512(void)
> +{
> +       asm volatile (
> +       "r1 = r10;"
> +       "r1 += -520;"
> +       "r0 = 42;"
> +       "*(u64*)(r1 + 0) = r0;"
> +       "exit;"
> +       ::: __clobber_all);
> +}
> +
> +#ifdef __BPF_FEATURE_MAY_GOTO
> +SEC("socket")
> +__description("PTR_TO_STACK stack size 512 with may_goto with jit")
> +__use_jit()
> +__success __retval(42)
> +__naked void stack_check_size_512_with_may_goto_jit(void)
> +{
> +       asm volatile (
> +       "r1 = r10;"
> +       "r1 += -512;"
> +       "r0 = 42;"
> +       "*(u32*)(r1 + 0) = r0;"
> +       "may_goto l0_%=;"
> +       "r2 = 100;"
> +"l0_%=:        exit;"
> +       ::: __clobber_all);
> +}
> +
> +SEC("socket")
> +__description("PTR_TO_STACK stack size 512 with may_goto without jit")
> +__use_interp()
> +__failure __msg("stack size 520(extra 8) is too large")
> +__naked void stack_check_size_512_with_may_goto(void)
> +{
> +       asm volatile (
> +       "r1 = r10;"
> +       "r1 += -512;"
> +       "r0 = 42;"
> +       "*(u32*)(r1 + 0) = r0;"
> +       "may_goto l0_%=;"
> +       "r2 = 100;"
> +"l0_%=:        exit;"
> +       ::: __clobber_all);
> +}
> +#endif
> +
>  char _license[] SEC("license") = "GPL";
> --
> 2.47.1
>

Re: [PATCH bpf-next v2 3/3] selftests/bpf: Add selftest for may_goto

[Jiayuan Chen]

On Thu, Feb 13, 2025 at 08:04:05AM -0800, Alexei Starovoitov wrote:
> On Thu, Feb 13, 2025 at 5:13 AM Jiayuan Chen <mrpre@163.com> wrote:
> >
> > Add test cases to ensure the maximum stack size can be properly limited to
> > 512.
> >
> > Test result:
> > echo "0" > /proc/sys/net/core/bpf_jit_enable
> > ./test_progs -t verifier_stack_ptr
> > verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto with jit:SKIP
> > verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto without jit:OK
> >
> > echo "1" > /proc/sys/net/core/bpf_jit_enable
> > verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto with jit:OK
> > verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto without jit:SKIP
> 
> echo '0|1' is not longer necessary ?
> The commit log seems obsolete?
> 
> pw-bot: cr

It looks like the problem only arises when CONFIG_BPF_JIT_ALWAYS_ON is
turned off, and we're only restricting the stack size when
prog->jit_requested is false. To test this, I simulated different
scenarios by echoing '0' or '1' to see how the program would behave when
jit_requested is enabled or disabled.

As expected, when I echoed '0', the program failed verification, and when
I echoed '1', it ran smoothly.

Thanks
> 
> > Signed-off-by: Jiayuan Chen <mrpre@163.com>
> > ---
> >  .../selftests/bpf/progs/verifier_stack_ptr.c  | 50 +++++++++++++++++++
> >  1 file changed, 50 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c b/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c
> > index 417c61cd4b19..8ffe5a01d140 100644
> > --- a/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c
> > +++ b/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c
> > @@ -481,4 +481,54 @@ l1_%=:     r0 = 42;                                        \
> >         : __clobber_all);
> >  }
> >
> > +SEC("socket")
> > +__description("PTR_TO_STACK stack size > 512")
> > +__failure __msg("invalid write to stack R1 off=-520 size=8")
> > +__naked void stack_check_size_gt_512(void)
> > +{
> > +       asm volatile (
> > +       "r1 = r10;"
> > +       "r1 += -520;"
> > +       "r0 = 42;"
> > +       "*(u64*)(r1 + 0) = r0;"
> > +       "exit;"
> > +       ::: __clobber_all);
> > +}
> > +
> > +#ifdef __BPF_FEATURE_MAY_GOTO
> > +SEC("socket")
> > +__description("PTR_TO_STACK stack size 512 with may_goto with jit")
> > +__use_jit()
> > +__success __retval(42)
> > +__naked void stack_check_size_512_with_may_goto_jit(void)
> > +{
> > +       asm volatile (
> > +       "r1 = r10;"
> > +       "r1 += -512;"
> > +       "r0 = 42;"
> > +       "*(u32*)(r1 + 0) = r0;"
> > +       "may_goto l0_%=;"
> > +       "r2 = 100;"
> > +"l0_%=:        exit;"
> > +       ::: __clobber_all);
> > +}
> > +
> > +SEC("socket")
> > +__description("PTR_TO_STACK stack size 512 with may_goto without jit")
> > +__use_interp()
> > +__failure __msg("stack size 520(extra 8) is too large")
> > +__naked void stack_check_size_512_with_may_goto(void)
> > +{
> > +       asm volatile (
> > +       "r1 = r10;"
> > +       "r1 += -512;"
> > +       "r0 = 42;"
> > +       "*(u32*)(r1 + 0) = r0;"
> > +       "may_goto l0_%=;"
> > +       "r2 = 100;"
> > +"l0_%=:        exit;"
> > +       ::: __clobber_all);
> > +}
> > +#endif
> > +
> >  char _license[] SEC("license") = "GPL";
> > --
> > 2.47.1
> >

Re: [PATCH bpf-next v2 1/3] bpf: Fix array bounds error with may_goto

[Jiayuan Chen]

On Thu, Feb 13, 2025 at 08:02:55AM -0800, Alexei Starovoitov wrote:
> On Thu, Feb 13, 2025 at 5:13 AM Jiayuan Chen <mrpre@163.com> wrote:
> >
> > may_goto uses an additional 8 bytes on the stack, which causes the
> > interpreters[] array to go out of bounds when calculating index by
> > stack_size.
> >
> > 1. If a BPF program is rewritten, re-evaluate the stack size. For non-JIT
> > cases, reject loading directly.
> >
> > 2. For non-JIT cases, calculating interpreters[idx] may still cause
> > out-of-bounds array access, and just warn about it.
> >
> > 3. For jit_requested cases, the execution of bpf_func also needs to be
> > warned. So Move the definition of function __bpf_prog_ret0_warn out of
> > the macro definition CONFIG_BPF_JIT_ALWAYS_ON
> >
[...]
> > ---
> >  EVAL6(PROG_NAME_LIST, 224, 256, 288, 320, 352, 384)
> >  EVAL4(PROG_NAME_LIST, 416, 448, 480, 512)
> >  };
> > +
> > +#define MAX_INTERPRETERS_CALLBACK (sizeof(interpreters) / sizeof(*interpreters))
> 
> There is ARRAY_SIZE macro.
Thanks, I will use it.
> 
> >  #undef PROG_NAME_LIST
> >  #define PROG_NAME_LIST(stack_size) PROG_NAME_ARGS(stack_size),
> >  static __maybe_unused
> > @@ -2290,17 +2293,18 @@ void bpf_patch_call_args(struct bpf_insn *insn, u32 stack_depth)
> >         insn->code = BPF_JMP | BPF_CALL_ARGS;
> >  }
> >  #endif
> > -#else
> > +#endif
> > +
> >  static unsigned int __bpf_prog_ret0_warn(const void *ctx,
> >                                          const struct bpf_insn *insn)
> >  {
> >         /* If this handler ever gets executed, then BPF_JIT_ALWAYS_ON
> > -        * is not working properly, so warn about it!
> > +        * is not working properly, or interpreter is being used when
> > +        * prog->jit_requested is not 0, so warn about it!
> >          */
> >         WARN_ON_ONCE(1);
> >         return 0;
> >  }
> > -#endif
> >
> >  bool bpf_prog_map_compatible(struct bpf_map *map,
> >                              const struct bpf_prog *fp)
> > @@ -2380,8 +2384,14 @@ static void bpf_prog_select_func(struct bpf_prog *fp)
> >  {
> >  #ifndef CONFIG_BPF_JIT_ALWAYS_ON
> >         u32 stack_depth = max_t(u32, fp->aux->stack_depth, 1);
> > +       u32 idx = (round_up(stack_depth, 32) / 32) - 1;
> >
> > -       fp->bpf_func = interpreters[(round_up(stack_depth, 32) / 32) - 1];
> > +       if (!fp->jit_requested) {
> 
> I don't think above check is necessary.
> Why not just
> if (WARN_ON_ONCE(idx >= ARRAY_SIZE(interpreters)))
>   fp->bpf_func = __bpf_prog_ret0_warn;
> else
>   fp->bpf_func = interpreters[idx];
> 

When jit_requested is set 1, the stack_depth can still go above 512,
and we'd end up executing this function, where the index calculation would
overflow, triggering an array out-of-bounds warning from USCAN or WAR().

> > +               WARN_ON_ONCE(idx >= MAX_INTERPRETERS_CALLBACK);
> > +               fp->bpf_func = interpreters[idx];
> > +       } else {
> > +               fp->bpf_func = __bpf_prog_ret0_warn;
> >                         stack_depth_extra = 0;
> > --
> > 2.47.1
> >

Re: [PATCH bpf-next v2 1/3] bpf: Fix array bounds error with may_goto

[Alexei Starovoitov]

On Thu, Feb 13, 2025 at 9:03 AM Jiayuan Chen <mrpre@163.com> wrote:
>
> On Thu, Feb 13, 2025 at 08:02:55AM -0800, Alexei Starovoitov wrote:
> > On Thu, Feb 13, 2025 at 5:13 AM Jiayuan Chen <mrpre@163.com> wrote:
> > >
> > > may_goto uses an additional 8 bytes on the stack, which causes the
> > > interpreters[] array to go out of bounds when calculating index by
> > > stack_size.
> > >
> > > 1. If a BPF program is rewritten, re-evaluate the stack size. For non-JIT
> > > cases, reject loading directly.
> > >
> > > 2. For non-JIT cases, calculating interpreters[idx] may still cause
> > > out-of-bounds array access, and just warn about it.
> > >
> > > 3. For jit_requested cases, the execution of bpf_func also needs to be
> > > warned. So Move the definition of function __bpf_prog_ret0_warn out of
> > > the macro definition CONFIG_BPF_JIT_ALWAYS_ON
> > >
> [...]
> > > ---
> > >  EVAL6(PROG_NAME_LIST, 224, 256, 288, 320, 352, 384)
> > >  EVAL4(PROG_NAME_LIST, 416, 448, 480, 512)
> > >  };
> > > +
> > > +#define MAX_INTERPRETERS_CALLBACK (sizeof(interpreters) / sizeof(*interpreters))
> >
> > There is ARRAY_SIZE macro.
> Thanks, I will use it.
> >
> > >  #undef PROG_NAME_LIST
> > >  #define PROG_NAME_LIST(stack_size) PROG_NAME_ARGS(stack_size),
> > >  static __maybe_unused
> > > @@ -2290,17 +2293,18 @@ void bpf_patch_call_args(struct bpf_insn *insn, u32 stack_depth)
> > >         insn->code = BPF_JMP | BPF_CALL_ARGS;
> > >  }
> > >  #endif
> > > -#else
> > > +#endif
> > > +
> > >  static unsigned int __bpf_prog_ret0_warn(const void *ctx,
> > >                                          const struct bpf_insn *insn)
> > >  {
> > >         /* If this handler ever gets executed, then BPF_JIT_ALWAYS_ON
> > > -        * is not working properly, so warn about it!
> > > +        * is not working properly, or interpreter is being used when
> > > +        * prog->jit_requested is not 0, so warn about it!
> > >          */
> > >         WARN_ON_ONCE(1);
> > >         return 0;
> > >  }
> > > -#endif
> > >
> > >  bool bpf_prog_map_compatible(struct bpf_map *map,
> > >                              const struct bpf_prog *fp)
> > > @@ -2380,8 +2384,14 @@ static void bpf_prog_select_func(struct bpf_prog *fp)
> > >  {
> > >  #ifndef CONFIG_BPF_JIT_ALWAYS_ON
> > >         u32 stack_depth = max_t(u32, fp->aux->stack_depth, 1);
> > > +       u32 idx = (round_up(stack_depth, 32) / 32) - 1;
> > >
> > > -       fp->bpf_func = interpreters[(round_up(stack_depth, 32) / 32) - 1];
> > > +       if (!fp->jit_requested) {
> >
> > I don't think above check is necessary.
> > Why not just
> > if (WARN_ON_ONCE(idx >= ARRAY_SIZE(interpreters)))
> >   fp->bpf_func = __bpf_prog_ret0_warn;
> > else
> >   fp->bpf_func = interpreters[idx];
> >
>
> When jit_requested is set 1, the stack_depth can still go above 512,
> and we'd end up executing this function, where the index calculation would
> overflow, triggering an array out-of-bounds warning from USCAN or WAR().

Ok, then do:
if (!fp->jit_requested && WARN_ON_ONCE(idx >= ARRAY_SIZE(interpreters)))

> > > +               WARN_ON_ONCE(idx >= MAX_INTERPRETERS_CALLBACK);
> > > +               fp->bpf_func = interpreters[idx];

since warning and anyway proceeding to access the array out of bounds
is just wrong.

Re: [PATCH bpf-next v2 3/3] selftests/bpf: Add selftest for may_goto

[Alexei Starovoitov]

On Thu, Feb 13, 2025 at 8:42 AM Jiayuan Chen <mrpre@163.com> wrote:
>
> On Thu, Feb 13, 2025 at 08:04:05AM -0800, Alexei Starovoitov wrote:
> > On Thu, Feb 13, 2025 at 5:13 AM Jiayuan Chen <mrpre@163.com> wrote:
> > >
> > > Add test cases to ensure the maximum stack size can be properly limited to
> > > 512.
> > >
> > > Test result:
> > > echo "0" > /proc/sys/net/core/bpf_jit_enable
> > > ./test_progs -t verifier_stack_ptr
> > > verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto with jit:SKIP
> > > verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto without jit:OK
> > >
> > > echo "1" > /proc/sys/net/core/bpf_jit_enable
> > > verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto with jit:OK
> > > verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto without jit:SKIP
> >
> > echo '0|1' is not longer necessary ?
> > The commit log seems obsolete?
> >
> > pw-bot: cr
>
> It looks like the problem only arises when CONFIG_BPF_JIT_ALWAYS_ON is
> turned off, and we're only restricting the stack size when
> prog->jit_requested is false. To test this, I simulated different
> scenarios by echoing '0' or '1' to see how the program would behave when
> jit_requested is enabled or disabled.
>
> As expected, when I echoed '0', the program failed verification, and when
> I echoed '1', it ran smoothly.

I misunderstood the tags in patch 2. I thought:

+#define __use_jit() __attribute__((btf_decl_tag("comment:run_mode=jit")))
+#define __use_interp()
__attribute__((btf_decl_tag("comment:run_mode=interpreter")))

"use jit" actually means use jit.

while what it's doing is different:

+ if ((jit_enabled && spec->run_mode & INTERP) ||
+    (!jit_enabled && spec->run_mode & JIT)) {
+     test__skip();
+     return;
+ }
+

The tags should probably be named __load_if_JITed and __load_if_interpreted
or something like that.