* [PATCH bpf-next v2 1/3] bpf: Fix array bounds error with may_goto
2025-02-13 13:12 [PATCH bpf-next v2 0/3] bpf: Fix array bounds error with may_goto and add selftest Jiayuan Chen
@ 2025-02-13 13:12 ` Jiayuan Chen
2025-02-13 16:02 ` Alexei Starovoitov
2025-02-13 13:12 ` [PATCH bpf-next v2 2/3] selftests/bpf: Allow the program to select specific modes for testing Jiayuan Chen
2025-02-13 13:12 ` [PATCH bpf-next v2 3/3] selftests/bpf: Add selftest for may_goto Jiayuan Chen
2 siblings, 1 reply; 10+ messages in thread
From: Jiayuan Chen @ 2025-02-13 13:12 UTC (permalink / raw)
To: bpf, ast
Cc: linux-kselftest, daniel, john.fastabend, andrii, martin.lau,
eddyz87, song, yonghong.song, kpsingh, sdf, haoluo, jolsa,
mykolal, shuah, Jiayuan Chen, syzbot+d2a2c639d03ac200a4f1
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH bpf-next v2 1/3] bpf: Fix array bounds error with may_goto
2025-02-13 13:12 ` [PATCH bpf-next v2 1/3] bpf: Fix array bounds error with may_goto Jiayuan Chen
@ 2025-02-13 16:02 ` Alexei Starovoitov
2025-02-13 17:02 ` Jiayuan Chen
0 siblings, 1 reply; 10+ messages in thread
From: Alexei Starovoitov @ 2025-02-13 16:02 UTC (permalink / raw)
To: Jiayuan Chen
Cc: bpf, Alexei Starovoitov, open list:KERNEL SELFTEST FRAMEWORK,
Daniel Borkmann, John Fastabend, Andrii Nakryiko,
Martin KaFai Lau, Eddy Z, Song Liu, Yonghong Song, KP Singh,
Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko,
Shuah Khan, syzbot+d2a2c639d03ac200a4f1
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH bpf-next v2 1/3] bpf: Fix array bounds error with may_goto
2025-02-13 16:02 ` Alexei Starovoitov
@ 2025-02-13 17:02 ` Jiayuan Chen
2025-02-14 0:51 ` Alexei Starovoitov
0 siblings, 1 reply; 10+ messages in thread
From: Jiayuan Chen @ 2025-02-13 17:02 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: bpf, Alexei Starovoitov, open list:KERNEL SELFTEST FRAMEWORK,
Daniel Borkmann, John Fastabend, Andrii Nakryiko,
Martin KaFai Lau, Eddy Z, Song Liu, Yonghong Song, KP Singh,
Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko,
Shuah Khan, syzbot+d2a2c639d03ac200a4f1
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
> >
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH bpf-next v2 1/3] bpf: Fix array bounds error with may_goto
2025-02-13 17:02 ` Jiayuan Chen
@ 2025-02-14 0:51 ` Alexei Starovoitov
0 siblings, 0 replies; 10+ messages in thread
From: Alexei Starovoitov @ 2025-02-14 0:51 UTC (permalink / raw)
To: Jiayuan Chen
Cc: bpf, Alexei Starovoitov, open list:KERNEL SELFTEST FRAMEWORK,
Daniel Borkmann, John Fastabend, Andrii Nakryiko,
Martin KaFai Lau, Eddy Z, Song Liu, Yonghong Song, KP Singh,
Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko,
Shuah Khan, syzbot+d2a2c639d03ac200a4f1
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH bpf-next v2 2/3] selftests/bpf: Allow the program to select specific modes for testing
2025-02-13 13:12 [PATCH bpf-next v2 0/3] bpf: Fix array bounds error with may_goto and add selftest Jiayuan Chen
2025-02-13 13:12 ` [PATCH bpf-next v2 1/3] bpf: Fix array bounds error with may_goto Jiayuan Chen
@ 2025-02-13 13:12 ` Jiayuan Chen
2025-02-13 13:12 ` [PATCH bpf-next v2 3/3] selftests/bpf: Add selftest for may_goto Jiayuan Chen
2 siblings, 0 replies; 10+ messages in thread
From: Jiayuan Chen @ 2025-02-13 13:12 UTC (permalink / raw)
To: bpf, ast
Cc: linux-kselftest, daniel, john.fastabend, andrii, martin.lau,
eddyz87, song, yonghong.song, kpsingh, sdf, haoluo, jolsa,
mykolal, shuah, 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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH bpf-next v2 3/3] selftests/bpf: Add selftest for may_goto
2025-02-13 13:12 [PATCH bpf-next v2 0/3] bpf: Fix array bounds error with may_goto and add selftest Jiayuan Chen
2025-02-13 13:12 ` [PATCH bpf-next v2 1/3] bpf: Fix array bounds error with may_goto Jiayuan Chen
2025-02-13 13:12 ` [PATCH bpf-next v2 2/3] selftests/bpf: Allow the program to select specific modes for testing Jiayuan Chen
@ 2025-02-13 13:12 ` Jiayuan Chen
2025-02-13 16:04 ` Alexei Starovoitov
2 siblings, 1 reply; 10+ messages in thread
From: Jiayuan Chen @ 2025-02-13 13:12 UTC (permalink / raw)
To: bpf, ast
Cc: linux-kselftest, daniel, john.fastabend, andrii, martin.lau,
eddyz87, song, yonghong.song, kpsingh, sdf, haoluo, jolsa,
mykolal, shuah, 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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH bpf-next v2 3/3] selftests/bpf: Add selftest for may_goto
2025-02-13 13:12 ` [PATCH bpf-next v2 3/3] selftests/bpf: Add selftest for may_goto Jiayuan Chen
@ 2025-02-13 16:04 ` Alexei Starovoitov
2025-02-13 16:41 ` Jiayuan Chen
0 siblings, 1 reply; 10+ messages in thread
From: Alexei Starovoitov @ 2025-02-13 16:04 UTC (permalink / raw)
To: Jiayuan Chen
Cc: bpf, Alexei Starovoitov, open list:KERNEL SELFTEST FRAMEWORK,
Daniel Borkmann, John Fastabend, Andrii Nakryiko,
Martin KaFai Lau, Eddy Z, Song Liu, Yonghong Song, KP Singh,
Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko,
Shuah Khan
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH bpf-next v2 3/3] selftests/bpf: Add selftest for may_goto
2025-02-13 16:04 ` Alexei Starovoitov
@ 2025-02-13 16:41 ` Jiayuan Chen
2025-02-14 0:58 ` Alexei Starovoitov
0 siblings, 1 reply; 10+ messages in thread
From: Jiayuan Chen @ 2025-02-13 16:41 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: bpf, Alexei Starovoitov, open list:KERNEL SELFTEST FRAMEWORK,
Daniel Borkmann, John Fastabend, Andrii Nakryiko,
Martin KaFai Lau, Eddy Z, Song Liu, Yonghong Song, KP Singh,
Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko,
Shuah Khan
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
> >
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH bpf-next v2 3/3] selftests/bpf: Add selftest for may_goto
2025-02-13 16:41 ` Jiayuan Chen
@ 2025-02-14 0:58 ` Alexei Starovoitov
0 siblings, 0 replies; 10+ messages in thread
From: Alexei Starovoitov @ 2025-02-14 0:58 UTC (permalink / raw)
To: Jiayuan Chen
Cc: bpf, Alexei Starovoitov, open list:KERNEL SELFTEST FRAMEWORK,
Daniel Borkmann, John Fastabend, Andrii Nakryiko,
Martin KaFai Lau, Eddy Z, Song Liu, Yonghong Song, KP Singh,
Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko,
Shuah Khan
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.
^ permalink raw reply [flat|nested] 10+ messages in thread