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@gmail.com/
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);
--
2.49.0On Sat, 2025-06-28 at 16:50 +0200, Luis Gerhorst wrote:
[...]
> @@ -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;
I don't like the prev_aux() call in this position, as one needs to
understand that after do_check_insn() call what was current became
previous. This at-least requires a comment. Implementation with a
temporary variable (as at the bottom of this email), imo, is less
cognitive load.
> /* 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;
[...]
---
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index a136d9b1b25f..a923614b7104 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19953,6 +19953,7 @@ static int do_check(struct bpf_verifier_env *env)
bool pop_log = !(env->log.level & BPF_LOG_LEVEL2);
struct bpf_verifier_state *state = env->cur_state;
struct bpf_insn *insns = env->prog->insnsi;
+ struct bpf_insn_aux_data *insn_aux;
int insn_cnt = env->prog->len;
bool do_print_state = false;
int prev_insn_idx = -1;
@@ -19972,6 +19973,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,
@@ -20048,7 +20050,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);
@@ -20056,11 +20058,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;
@@ -20069,7 +20071,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
Eduard Zingerman <eddyz87@gmail.com> writes:
> On Sat, 2025-06-28 at 16:50 +0200, Luis Gerhorst wrote:
>
> [...]
>
>> @@ -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;
>
> I don't like the prev_aux() call in this position, as one needs to
> understand that after do_check_insn() call what was current became
> previous. This at-least requires a comment. Implementation with a
> temporary variable (as at the bottom of this email), imo, is less
> cognitive load.
I think I agree. I will send a v3 with the variable.
>> /* 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;
>
> [...]
>
> ---
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index a136d9b1b25f..a923614b7104 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -19953,6 +19953,7 @@ static int do_check(struct bpf_verifier_env *env)
> bool pop_log = !(env->log.level & BPF_LOG_LEVEL2);
> struct bpf_verifier_state *state = env->cur_state;
> struct bpf_insn *insns = env->prog->insnsi;
> + struct bpf_insn_aux_data *insn_aux;
> int insn_cnt = env->prog->len;
> bool do_print_state = false;
> int prev_insn_idx = -1;
> @@ -19972,6 +19973,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,
> @@ -20048,7 +20050,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);
> @@ -20056,11 +20058,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;
> @@ -20069,7 +20071,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
© 2016 - 2025 Red Hat, Inc.