Skip to content

Commit c7a8978

Browse files
borkmannAlexei Starovoitov
authored and
Alexei Starovoitov
committed
bpf: don't leave partial mangled prog in jit_subprogs error path
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: 1c2a088 ("bpf: x64: add JIT support for multi-function programs") Reported-by: [email protected] Signed-off-by: Daniel Borkmann <[email protected]> Signed-off-by: Alexei Starovoitov <[email protected]>
1 parent 6e6fddc commit c7a8978

File tree

1 file changed

+9
-2
lines changed

1 file changed

+9
-2
lines changed

kernel/bpf/verifier.c

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5430,6 +5430,10 @@ static int jit_subprogs(struct bpf_verifier_env *env)
54305430
if (insn->code != (BPF_JMP | BPF_CALL) ||
54315431
insn->src_reg != BPF_PSEUDO_CALL)
54325432
continue;
5433+
/* Upon error here we cannot fall back to interpreter but
5434+
* need a hard reject of the program. Thus -EFAULT is
5435+
* propagated in any case.
5436+
*/
54335437
subprog = find_subprog(env, i + insn->imm + 1);
54345438
if (subprog < 0) {
54355439
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)
54505454

54515455
func = kcalloc(env->subprog_cnt, sizeof(prog), GFP_KERNEL);
54525456
if (!func)
5453-
return -ENOMEM;
5457+
goto out_undo_insn;
54545458

54555459
for (i = 0; i < env->subprog_cnt; i++) {
54565460
subprog_start = subprog_end;
@@ -5515,7 +5519,7 @@ static int jit_subprogs(struct bpf_verifier_env *env)
55155519
tmp = bpf_int_jit_compile(func[i]);
55165520
if (tmp != func[i] || func[i]->bpf_func != old_bpf_func) {
55175521
verbose(env, "JIT doesn't support bpf-to-bpf calls\n");
5518-
err = -EFAULT;
5522+
err = -ENOTSUPP;
55195523
goto out_free;
55205524
}
55215525
cond_resched();
@@ -5552,6 +5556,7 @@ static int jit_subprogs(struct bpf_verifier_env *env)
55525556
if (func[i])
55535557
bpf_jit_free(func[i]);
55545558
kfree(func);
5559+
out_undo_insn:
55555560
/* cleanup main prog to be interpreted */
55565561
prog->jit_requested = 0;
55575562
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)
55785583
err = jit_subprogs(env);
55795584
if (err == 0)
55805585
return 0;
5586+
if (err == -EFAULT)
5587+
return err;
55815588
}
55825589
#ifndef CONFIG_BPF_JIT_ALWAYS_ON
55835590
for (i = 0; i < prog->len; i++, insn++) {

0 commit comments

Comments
 (0)