[bpf-next,v1,1/2] bpf: Fix array bounds error with may_goto

Commit Message

Jiayuan Chen Feb. 12, 2025, 1:52 p.m. UTC

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.

Reported-by: [e-mail]
Closes: https://lore.kernel.org/bpf/[e-mail]/
Fixes: 011832b97b311 ("bpf: Introduce may_goto instruction")
Signed-off-by: Jiayuan Chen <[e-mail]>
---
 kernel/bpf/core.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Comments

Alexei Starovoitov Feb. 12, 2025, 3:52 p.m. UTC | #1

On Wed, Feb 12, 2025 at 5:53 AM Jiayuan Chen <[e-mail]> 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.
>
> Reported-by: [e-mail]
> Closes: https://lore.kernel.org/bpf/[e-mail]/
> Fixes: 011832b97b311 ("bpf: Introduce may_goto instruction")
> Signed-off-by: Jiayuan Chen <[e-mail]>
> ---
>  kernel/bpf/core.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index da729cbbaeb9..498b35284f81 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -2255,7 +2255,7 @@ static u64 PROG_NAME_ARGS(stack_size)(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5, \
>
>  EVAL6(DEFINE_BPF_PROG_RUN, 32, 64, 96, 128, 160, 192);
>  EVAL6(DEFINE_BPF_PROG_RUN, 224, 256, 288, 320, 352, 384);
> -EVAL4(DEFINE_BPF_PROG_RUN, 416, 448, 480, 512);
> +EVAL5(DEFINE_BPF_PROG_RUN, 416, 448, 480, 512, 544);
>
>  EVAL6(DEFINE_BPF_PROG_RUN_ARGS, 32, 64, 96, 128, 160, 192);
>  EVAL6(DEFINE_BPF_PROG_RUN_ARGS, 224, 256, 288, 320, 352, 384);
> @@ -2267,8 +2267,11 @@ static unsigned int (*interpreters[])(const void *ctx,
>                                       const struct bpf_insn *insn) = {
>  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)
> +EVAL5(PROG_NAME_LIST, 416, 448, 480, 512, 544)
>  };

That's two extra functions for a rare corner case.
Let's do something like the following instead:
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 9971c03adfd5..028de7a6edfc 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -21884,6 +21884,10 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
                        subprogs[cur_subprog].stack_extra = stack_depth_extra;
                        cur_subprog++;
                        stack_depth = subprogs[cur_subprog].stack_depth;
+                       if (stack_depth > MAX_BPF_STACK &&
!prog->jit_requested) {
+                               verbose(...);
+                               return -EINVAL;
+                       }


pw-bot: cr

Patch

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index da729cbbaeb9..498b35284f81 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -2255,7 +2255,7 @@  static u64 PROG_NAME_ARGS(stack_size)(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5, \
 
 EVAL6(DEFINE_BPF_PROG_RUN, 32, 64, 96, 128, 160, 192);
 EVAL6(DEFINE_BPF_PROG_RUN, 224, 256, 288, 320, 352, 384);
-EVAL4(DEFINE_BPF_PROG_RUN, 416, 448, 480, 512);
+EVAL5(DEFINE_BPF_PROG_RUN, 416, 448, 480, 512, 544);
 
 EVAL6(DEFINE_BPF_PROG_RUN_ARGS, 32, 64, 96, 128, 160, 192);
 EVAL6(DEFINE_BPF_PROG_RUN_ARGS, 224, 256, 288, 320, 352, 384);
@@ -2267,8 +2267,11 @@  static unsigned int (*interpreters[])(const void *ctx,
 				      const struct bpf_insn *insn) = {
 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)
+EVAL5(PROG_NAME_LIST, 416, 448, 480, 512, 544)
 };
+
+#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
@@ -2380,8 +2383,10 @@  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];
+	WARN_ON_ONCE(idx >= MAX_INTERPRETERS_CALLBACK);
+	fp->bpf_func = interpreters[idx];
 #else
 	fp->bpf_func = __bpf_prog_ret0_warn;
 #endif