Re: [PATCH bpf-next] bpf: Fix unwarranted warning on speculative path

[Eduard Zingerman <eddyz87@xxxxxxxxx>]

On Wed, 2025-06-25 at 20:01 +0200, Paul Chaignon wrote:
> Commit d6f1c85f2253 ("bpf: Fall back to nospec for Spectre v1") added a
> WARN_ON_ONCE to check that we're not skipping a nospec due to a jump.
> It however failed to take into account LDIMM64 instructions as below:
>
>     15: (18) r1 = 0x2020200005642020
>     17: (7b) *(u64 *)(r10 -264) = r1
>
> This bytecode snippet generates a warning because the move from the
> LDIMM64 instruction to the next instruction is seen as a jump. This
> patch fixes it.
>
> Reported-by: syzbot+dc27c5fb8388e38d2d37@xxxxxxxxxxxxxxxxxxxxxxxxx
> Fixes: d6f1c85f2253 ("bpf: Fall back to nospec for Spectre v1")
> Signed-off-by: Paul Chaignon <paul.chaignon@xxxxxxxxx>
> ---
>  kernel/bpf/verifier.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 279a64933262..66841ed6dfc0 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -19819,6 +19819,7 @@ static int do_check(struct bpf_verifier_env *env)
>  	int insn_cnt = env->prog->len;
>  	bool do_print_state = false;
>  	int prev_insn_idx = -1;
> +	int insn_sz;
>
>  	for (;;) {
>  		struct bpf_insn *insn;
> @@ -19942,7 +19943,8 @@ static int do_check(struct bpf_verifier_env *env)
>  			 * to document this in case nospec_result is used
>  			 * elsewhere in the future.
>  			 */
> -			WARN_ON_ONCE(env->insn_idx != prev_insn_idx + 1);
> +			insn_sz = bpf_is_ldimm64(insn) ? 2 : 1;
> +			WARN_ON_ONCE(env->insn_idx != prev_insn_idx + insn_sz);

Could you please elaborate a bit?
The code looks as follows:

                 prev_insn_idx = env->insn_idx;
                 ...
                 err = do_check_insn(env, do_print_state: &do_print_state);
                 ...
                 if (state->speculative && cur_aux(env)->nospec_result) {
                         ...
                         insn_sz = bpf_is_ldimm64(insn) ? 2 : 1;
                         WARN_ON_ONCE(env->insn_idx != prev_insn_idx + insn_sz);
                         ...
                 }

The `cur_aux(env)->nospec_result` is set to true only for ST/STX
instructions which are 8-bytes wide. `do_check_insn` moves
env->isns_idx by 1 for these instructions.

So, suppose there is a program:

     15: (18) r1 = 0x2020200005642020
     17: (7b) *(u64 *)(r10 -264) = r1

Insn processing sequence would look like (starting from 15):
- prev_insn_idx <- 15
- do_check_insn()
  - env->insn_idx <- 17
- prev_insn_idx <- 17
- do_check_insn():
  - nospec_result <- true
  - env->insn_idx <- 18
- state->speculative && cur_aux(env)->nospec_result == true:
  - WARN_ON_ONCE(18 != 17 + 1) // no warning

What do I miss?
Could you please add a test case?

>  process_bpf_exit:
>  			mark_verifier_state_scratched(env);
>  			err = update_branch_counts(env, env->cur_state);