Eduard Zingerman <eddyz87@xxxxxxxxx> writes:
> 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?
Thanks for looking into it.
Yes, ldimm64 should not require a nospec_result as it can not be subject
to SSB. Should be as in
https://github.com/kernel-patches/bpf/pull/9193/files (ignore the arm
failures, these are because of the BUG in the test)
I will continue looking into it tomorrow if you don't want to.
