From 6ebc5030e0c5a698f1dd9a6684cddf6ccaed64a0 Mon Sep 17 00:00:00 2001 From: Jiayuan Chen Date: Fri, 14 Feb 2025 17:18:21 +0800 Subject: [PATCH 1/3] bpf: Fix array bounds error with may_goto 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 Link: https://lore.kernel.org/r/20250214091823.46042-2-mrpre@163.com Signed-off-by: Alexei Starovoitov --- kernel/bpf/core.c | 19 +++++++++++++++---- kernel/bpf/verifier.c | 7 +++++++ 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c index da729cbbaeb90..a0200fbbace99 100644 --- a/kernel/bpf/core.c +++ b/kernel/bpf/core.c @@ -2290,17 +2290,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 +2381,18 @@ 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]; + /* may_goto may cause stack size > 512, leading to idx out-of-bounds. + * But for non-JITed programs, we don't need bpf_func, so no bounds + * check needed. + */ + if (!fp->jit_requested && + !WARN_ON_ONCE(idx >= ARRAY_SIZE(interpreters))) { + 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 04d1d75d9ff95..3c286ec9e8413 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -21903,6 +21903,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; From b38c72ab808c5657b5411a47cc6eb4912e8b4824 Mon Sep 17 00:00:00 2001 From: Jiayuan Chen Date: Fri, 14 Feb 2025 17:18:22 +0800 Subject: [PATCH 2/3] selftests/bpf: Introduce __load_if_JITed annotation for tests 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. Introduce __load_if_JITed and __load_if_no_JITed annotation for tests. Signed-off-by: Jiayuan Chen Link: https://lore.kernel.org/r/20250214091823.46042-3-mrpre@163.com Signed-off-by: Alexei Starovoitov --- tools/testing/selftests/bpf/progs/bpf_misc.h | 2 ++ tools/testing/selftests/bpf/test_loader.c | 26 ++++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/tools/testing/selftests/bpf/progs/bpf_misc.h b/tools/testing/selftests/bpf/progs/bpf_misc.h index 02c9f7964e563..34f555da546fb 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 __load_if_JITed() __attribute__((btf_decl_tag("comment:load_mode=jited"))) +#define __load_if_no_JITed() __attribute__((btf_decl_tag("comment:load_mode=no_jited"))) /* 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 53b06647cf57d..4d23a9c463ee2 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_LOAD_MODE_PFX "comment:load_mode=" /* Warning: duplicated in bpf_misc.h */ #define POINTER_VALUE 0xcafe4all @@ -55,6 +56,11 @@ enum mode { UNPRIV = 2 }; +enum load_mode { + JITED = 1 << 0, + NO_JITED = 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 load_mask; 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 load_mask = 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_LOAD_MODE_PFX)) { + val = s + sizeof(TEST_TAG_LOAD_MODE_PFX) - 1; + if (strcmp(val, "jited") == 0) { + load_mask = JITED; + } else if (strcmp(val, "no_jited") == 0) { + load_mask = NO_JITED; + } else { + PRINT_FAIL("bad load spec: '%s'", val); + err = -EINVAL; + goto cleanup; + } } } spec->arch_mask = arch_mask ?: -1; + spec->load_mask = load_mask ?: (JITED | NO_JITED); if (spec->mode_mask == 0) spec->mode_mask = PRIV; @@ -928,6 +948,7 @@ void run_subtest(struct test_loader *tester, bool unpriv) { struct test_subspec *subspec = unpriv ? &spec->unpriv : &spec->priv; + int current_runtime = is_jit_enabled() ? JITED : NO_JITED; struct bpf_program *tprog = NULL, *tprog_iter; struct bpf_link *link, *links[32] = {}; struct test_spec *spec_iter; @@ -946,6 +967,11 @@ void run_subtest(struct test_loader *tester, return; } + if ((current_runtime & spec->load_mask) == 0) { + test__skip(); + return; + } + if (unpriv) { if (!can_execute_unpriv(tester, spec)) { test__skip(); From 72266ee83fed45ca8f0642bdb3c82424b9ed89a4 Mon Sep 17 00:00:00 2001 From: Jiayuan Chen Date: Fri, 14 Feb 2025 17:18:23 +0800 Subject: [PATCH 3/3] selftests/bpf: Add selftest for may_goto Added test cases to ensure that programs with stack sizes exceeding 512 bytes are restricted in non-JITed mode, and can be executed normally in JITed mode, even with stack sizes exceeding 512 bytes due to the presence of may_goto instructions. Test result: echo "0" > /proc/sys/net/core/bpf_jit_enable ./test_progs -t verifier_stack_ptr ... stack size 512 with may_goto with jit:SKIP stack size 512 with may_goto without jit:OK ... Summary: 1/27 PASSED, 25 SKIPPED, 0 FAILED echo "1" > /proc/sys/net/core/bpf_jit_enable ./test_progs -t verifier_stack_ptr ... stack size 512 with may_goto with jit:OK stack size 512 with may_goto without jit:SKIP ... Summary: 1/27 PASSED, 25 SKIPPED, 0 FAILED Signed-off-by: Jiayuan Chen Link: https://lore.kernel.org/r/20250214091823.46042-4-mrpre@163.com Signed-off-by: Alexei Starovoitov --- .../selftests/bpf/progs/verifier_stack_ptr.c | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c b/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c index 417c61cd4b190..24aabc6083fd9 100644 --- a/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c +++ b/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c @@ -481,4 +481,56 @@ 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") +__load_if_JITed() +__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") +__load_if_no_JITed() +__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";