On Thu, 2025-06-26 at 14:49 +0200, Luis Gerhorst wrote:
> 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().
>
> Reported-by: Paul Chaignon <paul.chaignon@xxxxxxxxx>
> Reported-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> Reported-by: syzbot+dc27c5fb8388e38d2d37@xxxxxxxxxxxxxxxxxxxxxxxxx
> Fixes: d6f1c85f2253 ("bpf: Fall back to nospec for Spectre v1")
> Signed-off-by: Luis Gerhorst <luis.gerhorst@xxxxxx>
> ---
The fix makes sense to me, please remove RFC and submit.
Is d6f1c85f2253 a part of a current kernel release?
If so, looks like this fix has to be routed through bpf tree.
> kernel/bpf/verifier.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index f403524bd215..88613fb71b16 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -19955,11 +19955,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;
> + env->insn_aux_data[prev_insn_idx].nospec = true;
I'd say it would be more convenient to have a temporary variable of
type `struct bpf_insn_aux_data` here. Otherwise `prev_insn_idx`
indexing would always be something to stop and think for a moment.
> /* If it was an ADD/SUB insn, potentially remove any
> * markings for alu sanitization.
> */
> - cur_aux(env)->alu_state = 0;
> + env->insn_aux_data[prev_insn_idx].alu_state = 0;
> goto process_bpf_exit;
> } else if (err < 0) {
> return err;
> @@ -19968,7 +19968,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 && env->insn_aux_data[prev_insn_idx].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,6 +19977,8 @@ 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.
> + *
> + * Therefore, no special case for ldimm64/call required.
> */
> WARN_ON_ONCE(env->insn_idx != prev_insn_idx + 1);
> process_bpf_exit:
Maybe change this to simply check that nospec is not set for
instruction of class BPF_JMP?
