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

[Paul Chaignon <paul.chaignon@gmail.com>]

Thanks for the review!

On Wed, Jun 25, 2025 at 01:19:01PM -0700, Eduard Zingerman wrote:
> 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@syzkaller.appspotmail.com
> > Fixes: d6f1c85f2253 ("bpf: Fall back to nospec for Spectre v1")
> > Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
> > ---
> >  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?

In the if condition, "cur_aux(env)" points to the aux data of the next
instruction (#17 here) because we incremented "insn_idx" in
do_check_insn(). In my fix, "insn" points to the previous instruction
because we retrieved it before calling do_check_insn().

Therefore, the processing sequence would look like:
- prev_insn_idx <- 15
- do_check_insn()
  - env->insn_idx <- 17
- state->speculative && cur_aux(env)->nospec_result == true:
  - WARN_ON_ONCE(17 != 15 + 1) // warning

I added a verbose() and recompiled to confirm those numbers.

If that makes sense, I'll send a v2 with:
- A better description, probably with a walkthrough.
- A test case simplified from the syzkaller repro.
- insn_sz renamed to prev_insn_sz for clarity.

> Could you please add a test case?
> 
> >  process_bpf_exit:
> >  			mark_verifier_state_scratched(env);
> >  			err = update_branch_counts(env, env->cur_state);