We must terminate the speculative analysis if the just-analyzed insn had nospec_result set. Using cur_aux() here is wrong because insn_idx might have been incremented by do_check_insn(). Therefore, introduce and use insn_aux variable.
Also change cur_aux(env)->nospec in case do_check_insn() ever manages to increment insn_idx but still fail.
Change the warning to check the insn class (which prevents it from triggering for ldimm64, for which nospec_result would not be problematic) and use verifier_bug_if().
In line with Eduard's suggestion, do not introduce prev_aux() because that requires one to understand that after do_check_insn() call what was current became previous. This would at-least require a comment.
Fixes: d6f1c85f2253 ("bpf: Fall back to nospec for Spectre v1") Reported-by: Paul Chaignon paul.chaignon@gmail.com Reported-by: Eduard Zingerman eddyz87@gmail.com Reported-by: syzbot+dc27c5fb8388e38d2d37@syzkaller.appspotmail.com Link: https://lore.kernel.org/bpf/685b3c1b.050a0220.2303ee.0010.GAE@google.com/ Link: https://lore.kernel.org/bpf/4266fd5de04092aa4971cbef14f1b4b96961f432.camel@g... Suggested-by: Eduard Zingerman eddyz87@gmail.com Signed-off-by: Luis Gerhorst luis.gerhorst@fau.de --- kernel/bpf/verifier.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 0f6cc2275695..96c737b41c3f 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -19923,6 +19923,7 @@ static int do_check(struct bpf_verifier_env *env)
for (;;) { struct bpf_insn *insn; + struct bpf_insn_aux_data *insn_aux; int err;
/* reset current history entry on each new instruction */ @@ -19936,6 +19937,7 @@ static int do_check(struct bpf_verifier_env *env) }
insn = &insns[env->insn_idx]; + insn_aux = &env->insn_aux_data[env->insn_idx];
if (++env->insn_processed > BPF_COMPLEXITY_LIMIT_INSNS) { verbose(env, @@ -20012,7 +20014,7 @@ static int do_check(struct bpf_verifier_env *env) /* Reduce verification complexity by stopping speculative path * verification when a nospec is encountered. */ - if (state->speculative && cur_aux(env)->nospec) + if (state->speculative && insn_aux->nospec) goto process_bpf_exit;
err = do_check_insn(env, &do_print_state); @@ -20020,11 +20022,11 @@ static int do_check(struct bpf_verifier_env *env) /* Prevent this speculative path from ever reaching the * insn that would have been unsafe to execute. */ - cur_aux(env)->nospec = true; + insn_aux->nospec = true; /* If it was an ADD/SUB insn, potentially remove any * markings for alu sanitization. */ - cur_aux(env)->alu_state = 0; + insn_aux->alu_state = 0; goto process_bpf_exit; } else if (err < 0) { return err; @@ -20033,7 +20035,7 @@ static int do_check(struct bpf_verifier_env *env) } WARN_ON_ONCE(err);
- if (state->speculative && cur_aux(env)->nospec_result) { + if (state->speculative && insn_aux->nospec_result) { /* If we are on a path that performed a jump-op, this * may skip a nospec patched-in after the jump. This can * currently never happen because nospec_result is only @@ -20042,8 +20044,15 @@ static int do_check(struct bpf_verifier_env *env) * never skip the following insn. Still, add a warning * to document this in case nospec_result is used * elsewhere in the future. + * + * All non-branch instructions have a single + * fall-through edge. For these, nospec_result should + * already work. */ - WARN_ON_ONCE(env->insn_idx != prev_insn_idx + 1); + if (verifier_bug_if(BPF_CLASS(insn->code) == BPF_JMP || + BPF_CLASS(insn->code) == BPF_JMP32, env, + "speculation barrier after jump instruction may not have the desired effect")) + return -EFAULT; process_bpf_exit: mark_verifier_state_scratched(env); err = update_branch_counts(env, env->cur_state);