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 prev_aux().
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().
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... Signed-off-by: Luis Gerhorst luis.gerhorst@fau.de --- kernel/bpf/verifier.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index b33bc37d5372..9d066e4b8248 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -11216,6 +11216,11 @@ static struct bpf_insn_aux_data *cur_aux(const struct bpf_verifier_env *env) return &env->insn_aux_data[env->insn_idx]; }
+static struct bpf_insn_aux_data *prev_aux(const struct bpf_verifier_env *env) +{ + return &env->insn_aux_data[env->prev_insn_idx]; +} + static bool loop_flag_is_zero(struct bpf_verifier_env *env) { struct bpf_reg_state *regs = cur_regs(env); @@ -19955,11 +19960,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; + prev_aux(env)->nospec = true; /* If it was an ADD/SUB insn, potentially remove any * markings for alu sanitization. */ - cur_aux(env)->alu_state = 0; + prev_aux(env)->alu_state = 0; goto process_bpf_exit; } else if (err < 0) { return err; @@ -19968,7 +19973,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 && prev_aux(env)->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 @@ -19977,8 +19982,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 != env->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);