[PATCH bpf v2] bpf: don't leave partial mangled prog in jit_subprogs error path

[PATCH bpf v2] bpf: don't leave partial mangled prog in jit_subprogs error path

[Daniel Borkmann]

syzkaller managed to trigger the following bug through fault injection:

  [...]
  [  141.043668] verifier bug. No program starts at insn 3
  [  141.044648] WARNING: CPU: 3 PID: 4072 at kernel/bpf/verifier.c:1613
                 get_callee_stack_depth kernel/bpf/verifier.c:1612 [inline]
  [  141.044648] WARNING: CPU: 3 PID: 4072 at kernel/bpf/verifier.c:1613
                 fixup_call_args kernel/bpf/verifier.c:5587 [inline]
  [  141.044648] WARNING: CPU: 3 PID: 4072 at kernel/bpf/verifier.c:1613
                 bpf_check+0x525e/0x5e60 kernel/bpf/verifier.c:5952
  [  141.047355] CPU: 3 PID: 4072 Comm: a.out Not tainted 4.18.0-rc4+ #51
  [  141.048446] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),BIOS 1.10.2-1 04/01/2014
  [  141.049877] Call Trace:
  [  141.050324]  __dump_stack lib/dump_stack.c:77 [inline]
  [  141.050324]  dump_stack+0x1c9/0x2b4 lib/dump_stack.c:113
  [  141.050950]  ? dump_stack_print_info.cold.2+0x52/0x52 lib/dump_stack.c:60
  [  141.051837]  panic+0x238/0x4e7 kernel/panic.c:184
  [  141.052386]  ? add_taint.cold.5+0x16/0x16 kernel/panic.c:385
  [  141.053101]  ? __warn.cold.8+0x148/0x1ba kernel/panic.c:537
  [  141.053814]  ? __warn.cold.8+0x117/0x1ba kernel/panic.c:530
  [  141.054506]  ? get_callee_stack_depth kernel/bpf/verifier.c:1612 [inline]
  [  141.054506]  ? fixup_call_args kernel/bpf/verifier.c:5587 [inline]
  [  141.054506]  ? bpf_check+0x525e/0x5e60 kernel/bpf/verifier.c:5952
  [  141.055163]  __warn.cold.8+0x163/0x1ba kernel/panic.c:538
  [  141.055820]  ? get_callee_stack_depth kernel/bpf/verifier.c:1612 [inline]
  [  141.055820]  ? fixup_call_args kernel/bpf/verifier.c:5587 [inline]
  [  141.055820]  ? bpf_check+0x525e/0x5e60 kernel/bpf/verifier.c:5952
  [...]

What happens in jit_subprogs() is that kcalloc() for the subprog func
buffer is failing with NULL where we then bail out. Latter is a plain
return -ENOMEM, and this is definitely not okay since earlier in the
loop we are walking all subprogs and temporarily rewrite insn->off to
remember the subprog id as well as insn->imm to temporarily point the
call to __bpf_call_base + 1 for the initial JIT pass. Thus, bailing
out in such state and handing this over to the interpreter is troublesome
since later/subsequent e.g. find_subprog() lookups are based on wrong
insn->imm.

Therefore, once we hit this point, we need to jump to out_free path
where we undo all changes from earlier loop, so that interpreter can
work on unmodified insn->{off,imm}.

Another point is that should find_subprog() fail in jit_subprogs() due
to a verifier bug, then we also should not simply defer the program to
the interpreter since also here we did partial modifications. Instead
we should just bail out entirely and return an error to the user who is
trying to load the program.

Fixes: 1c2a088a6626 ("bpf: x64: add JIT support for multi-function programs")
Reported-by: syzbot+7d427828b2ea6e592804@syzkaller.appspotmail.com
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 v1 -> v2:
   - used label instead of if condition, bit cleaner and shorter

 kernel/bpf/verifier.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 9e2bf83..63aaac5 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5430,6 +5430,10 @@ static int jit_subprogs(struct bpf_verifier_env *env)
 		if (insn->code != (BPF_JMP | BPF_CALL) ||
 		    insn->src_reg != BPF_PSEUDO_CALL)
 			continue;
+		/* Upon error here we cannot fall back to interpreter but
+		 * need a hard reject of the program. Thus -EFAULT is
+		 * propagated in any case.
+		 */
 		subprog = find_subprog(env, i + insn->imm + 1);
 		if (subprog < 0) {
 			WARN_ONCE(1, "verifier bug. No program starts at insn %d\n",
@@ -5450,7 +5454,7 @@ static int jit_subprogs(struct bpf_verifier_env *env)
 
 	func = kcalloc(env->subprog_cnt, sizeof(prog), GFP_KERNEL);
 	if (!func)
-		return -ENOMEM;
+		goto out_undo_insn;
 
 	for (i = 0; i < env->subprog_cnt; i++) {
 		subprog_start = subprog_end;
@@ -5515,7 +5519,7 @@ static int jit_subprogs(struct bpf_verifier_env *env)
 		tmp = bpf_int_jit_compile(func[i]);
 		if (tmp != func[i] || func[i]->bpf_func != old_bpf_func) {
 			verbose(env, "JIT doesn't support bpf-to-bpf calls\n");
-			err = -EFAULT;
+			err = -ENOTSUPP;
 			goto out_free;
 		}
 		cond_resched();
@@ -5552,6 +5556,7 @@ static int jit_subprogs(struct bpf_verifier_env *env)
 		if (func[i])
 			bpf_jit_free(func[i]);
 	kfree(func);
+out_undo_insn:
 	/* cleanup main prog to be interpreted */
 	prog->jit_requested = 0;
 	for (i = 0, insn = prog->insnsi; i < prog->len; i++, insn++) {
@@ -5578,6 +5583,8 @@ static int fixup_call_args(struct bpf_verifier_env *env)
 		err = jit_subprogs(env);
 		if (err == 0)
 			return 0;
+		if (err == -EFAULT)
+			return err;
 	}
 #ifndef CONFIG_BPF_JIT_ALWAYS_ON
 	for (i = 0; i < prog->len; i++, insn++) {
-- 
2.9.5

Re: [PATCH bpf v2] bpf: don't leave partial mangled prog in jit_subprogs error path

[Alexei Starovoitov]

On Thu, Jul 12, 2018 at 09:44:28PM +0200, Daniel Borkmann wrote:
> syzkaller managed to trigger the following bug through fault injection:
> 
>   [...]
>   [  141.043668] verifier bug. No program starts at insn 3
>   [  141.044648] WARNING: CPU: 3 PID: 4072 at kernel/bpf/verifier.c:1613
>                  get_callee_stack_depth kernel/bpf/verifier.c:1612 [inline]
>   [  141.044648] WARNING: CPU: 3 PID: 4072 at kernel/bpf/verifier.c:1613
>                  fixup_call_args kernel/bpf/verifier.c:5587 [inline]
>   [  141.044648] WARNING: CPU: 3 PID: 4072 at kernel/bpf/verifier.c:1613
>                  bpf_check+0x525e/0x5e60 kernel/bpf/verifier.c:5952
>   [  141.047355] CPU: 3 PID: 4072 Comm: a.out Not tainted 4.18.0-rc4+ #51
>   [  141.048446] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),BIOS 1.10.2-1 04/01/2014
>   [  141.049877] Call Trace:
>   [  141.050324]  __dump_stack lib/dump_stack.c:77 [inline]
>   [  141.050324]  dump_stack+0x1c9/0x2b4 lib/dump_stack.c:113
>   [  141.050950]  ? dump_stack_print_info.cold.2+0x52/0x52 lib/dump_stack.c:60
>   [  141.051837]  panic+0x238/0x4e7 kernel/panic.c:184
>   [  141.052386]  ? add_taint.cold.5+0x16/0x16 kernel/panic.c:385
>   [  141.053101]  ? __warn.cold.8+0x148/0x1ba kernel/panic.c:537
>   [  141.053814]  ? __warn.cold.8+0x117/0x1ba kernel/panic.c:530
>   [  141.054506]  ? get_callee_stack_depth kernel/bpf/verifier.c:1612 [inline]
>   [  141.054506]  ? fixup_call_args kernel/bpf/verifier.c:5587 [inline]
>   [  141.054506]  ? bpf_check+0x525e/0x5e60 kernel/bpf/verifier.c:5952
>   [  141.055163]  __warn.cold.8+0x163/0x1ba kernel/panic.c:538
>   [  141.055820]  ? get_callee_stack_depth kernel/bpf/verifier.c:1612 [inline]
>   [  141.055820]  ? fixup_call_args kernel/bpf/verifier.c:5587 [inline]
>   [  141.055820]  ? bpf_check+0x525e/0x5e60 kernel/bpf/verifier.c:5952
>   [...]
> 
> What happens in jit_subprogs() is that kcalloc() for the subprog func
> buffer is failing with NULL where we then bail out. Latter is a plain
> return -ENOMEM, and this is definitely not okay since earlier in the
> loop we are walking all subprogs and temporarily rewrite insn->off to
> remember the subprog id as well as insn->imm to temporarily point the
> call to __bpf_call_base + 1 for the initial JIT pass. Thus, bailing
> out in such state and handing this over to the interpreter is troublesome
> since later/subsequent e.g. find_subprog() lookups are based on wrong
> insn->imm.
> 
> Therefore, once we hit this point, we need to jump to out_free path
> where we undo all changes from earlier loop, so that interpreter can
> work on unmodified insn->{off,imm}.
> 
> Another point is that should find_subprog() fail in jit_subprogs() due
> to a verifier bug, then we also should not simply defer the program to
> the interpreter since also here we did partial modifications. Instead
> we should just bail out entirely and return an error to the user who is
> trying to load the program.
> 
> Fixes: 1c2a088a6626 ("bpf: x64: add JIT support for multi-function programs")
> Reported-by: syzbot+7d427828b2ea6e592804@syzkaller.appspotmail.com
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>

Applied, Thanks