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

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

[Paul Chaignon]

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);
 process_bpf_exit:
 			mark_verifier_state_scratched(env);
 			err = update_branch_counts(env, env->cur_state);
-- 
2.43.0

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

[Eduard Zingerman]

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?
Could you please add a test case?

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

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

[Luis Gerhorst]

Eduard Zingerman <eddyz87@gmail.com> 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@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?
> 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.

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

[Paul Chaignon]

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);

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

[Eduard Zingerman]

On Wed, 2025-06-25 at 23:43 +0200, Paul Chaignon wrote:

[...]

> > 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'm sorry, I'm a bit slow today (well, maybe not only today).
Isn't it a slightly different bug in the original check?
Suppose current insn is ST/STX that do_check_insn() marked as
nospec_result. I think the intent was to stop branch processing right
at that point, as nospec instruction would be inserted after this
store => no need to speculate further.
In other words, cur_aux(env)->nospec_result pointing to instruction
after ST/STX was not anticipated. (Luis, wdyt?)

> 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.

[...]

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

[Luis Gerhorst]

Eduard Zingerman <eddyz87@gmail.com> writes:

> On Wed, 2025-06-25 at 23:43 +0200, Paul Chaignon wrote:
>
> [...]
>
>> > 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'm sorry, I'm a bit slow today (well, maybe not only today).
> Isn't it a slightly different bug in the original check?
> Suppose current insn is ST/STX that do_check_insn() marked as
> nospec_result. I think the intent was to stop branch processing right
> at that point, as nospec instruction would be inserted after this
> store => no need to speculate further.
> In other words, cur_aux(env)->nospec_result pointing to instruction
> after ST/STX was not anticipated. (Luis, wdyt?)

That's a very good point, nospec_result should only stop the path
analysis after the insn that has it set was analyzed. Otherwise, a
nospec required before the insn may not be added.

In reply to this you find a RFC fix and test that shows a nospec might
be missing. If this makes sense I will send a polished version.

The tests fail without the fix [1] (the offset no longer matches because
the nospec before the stack-write is missing, which is the main point),
but succeed otherwise [2].

I manually verified the fix resolves the warning in the reproducer, but
I can add a test for the polished version.

[1] https://github.com/kernel-patches/bpf/actions/runs/15901586938/job/44846011518?pr=9199#step:5:11308
[2] https://github.com/kernel-patches/bpf/pull/9198

[RFC PATCH 1/3] bpf: Fix aux usage after do_check_insn()

[Luis Gerhorst]

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@gmail.com>
Reported-by: Eduard Zingerman <eddyz87@gmail.com>
Reported-by: syzbot+dc27c5fb8388e38d2d37@syzkaller.appspotmail.com
Fixes: d6f1c85f2253 ("bpf: Fall back to nospec for Spectre v1")
Signed-off-by: Luis Gerhorst <luis.gerhorst@fau.de>
---
 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;
 			/* 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:
-- 
2.49.0

Re: [RFC PATCH 1/3] bpf: Fix aux usage after do_check_insn()

[Eduard Zingerman]

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@gmail.com>
> Reported-by: Eduard Zingerman <eddyz87@gmail.com>
> Reported-by: syzbot+dc27c5fb8388e38d2d37@syzkaller.appspotmail.com
> Fixes: d6f1c85f2253 ("bpf: Fall back to nospec for Spectre v1")
> Signed-off-by: Luis Gerhorst <luis.gerhorst@fau.de>
> ---

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?

Re: [RFC PATCH 1/3] bpf: Fix aux usage after do_check_insn()

[Alexei Starovoitov]

On Thu, Jun 26, 2025 at 5:50 AM Luis Gerhorst <luis.gerhorst@fau.de> 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@gmail.com>
> Reported-by: Eduard Zingerman <eddyz87@gmail.com>
> Reported-by: syzbot+dc27c5fb8388e38d2d37@syzkaller.appspotmail.com
> Fixes: d6f1c85f2253 ("bpf: Fall back to nospec for Spectre v1")
> Signed-off-by: Luis Gerhorst <luis.gerhorst@fau.de>
> ---
>  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;

May be introduce prev_aux() similar to cur_aux() ?

In the future don't bother with RFC tag, since CI won't test them.

>                         /* 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);

While at it replace with verifier_bug_if().

[RFC PATCH 2/3] selftests/bpf: Add ldimm64 nospec test

[Luis Gerhorst]

Signed-off-by: Luis Gerhorst <luis.gerhorst@fau.de>
---
 tools/testing/selftests/bpf/progs/bpf_misc.h  |  4 ++++
 .../selftests/bpf/progs/verifier_unpriv.c     | 19 +++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/bpf_misc.h b/tools/testing/selftests/bpf/progs/bpf_misc.h
index a678463e972c..be7d9bfa8390 100644
--- a/tools/testing/selftests/bpf/progs/bpf_misc.h
+++ b/tools/testing/selftests/bpf/progs/bpf_misc.h
@@ -235,4 +235,8 @@
 #define SPEC_V1
 #endif
 
+#if defined(__TARGET_ARCH_x86)
+#define SPEC_V4
+#endif
+
 #endif
diff --git a/tools/testing/selftests/bpf/progs/verifier_unpriv.c b/tools/testing/selftests/bpf/progs/verifier_unpriv.c
index 4470541b5e71..35d2625e97b8 100644
--- a/tools/testing/selftests/bpf/progs/verifier_unpriv.c
+++ b/tools/testing/selftests/bpf/progs/verifier_unpriv.c
@@ -801,4 +801,23 @@ l2_%=:							\
 	: __clobber_all);
 }
 
+SEC("socket")
+__description("ldimm64 nospec")
+__success __success_unpriv
+__retval(0)
+#ifdef SPEC_V4
+__xlated_unpriv("r1 = 0x2020200005642020")
+__xlated_unpriv("*(u64 *)(r10 -8) = r1")
+__xlated_unpriv("nospec")
+#endif
+__naked void ldimm64_nospec(void)
+{
+	asm volatile ("					\
+	r1 = 0x2020200005642020 ll;			\
+	*(u64 *)(r10 -8) = r1;				\
+	r0 = 0;						\
+	exit;						\
+"	::: __clobber_all);
+}
+
 char _license[] SEC("license") = "GPL";
-- 
2.49.0

[RFC PATCH 3/3] selftests/bpf: Add nospec_result test

[Luis Gerhorst]

Make sure nospec_result does not prevent a nospec from being added
before the dangerous insn.

Signed-off-by: Luis Gerhorst <luis.gerhorst@fau.de>
---
 .../selftests/bpf/progs/verifier_unpriv.c     | 90 +++++++++++++++++++
 1 file changed, 90 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/verifier_unpriv.c b/tools/testing/selftests/bpf/progs/verifier_unpriv.c
index 35d2625e97b8..7684b7824f4a 100644
--- a/tools/testing/selftests/bpf/progs/verifier_unpriv.c
+++ b/tools/testing/selftests/bpf/progs/verifier_unpriv.c
@@ -820,4 +820,94 @@ __naked void ldimm64_nospec(void)
 "	::: __clobber_all);
 }
 
+SEC("socket")
+__description("spec_v4-induced path termination only after insn check")
+__success __success_unpriv
+__retval(0)
+#ifdef SPEC_V1
+#ifdef SPEC_V4
+/* starts with r0 == r8 == r9 == 0 */
+__xlated_unpriv("if r8 != 0x0 goto pc+1")
+__xlated_unpriv("goto pc+2")
+__xlated_unpriv("if r9 == 0x0 goto pc+4")
+__xlated_unpriv("r2 = r0")
+/* Following nospec required to prevent following `*(u64 *)(NULL -64) = r1` iff
+ * `if r9 == 0 goto pc+4` was mispredicted because of Spectre v1
+ */
+__xlated_unpriv("nospec") /* Spectre v1 */
+__xlated_unpriv("*(u64 *)(r2 -64) = r1")
+__xlated_unpriv("nospec") /* Spectre v4 (nospec_result) */
+#endif
+#endif
+__naked void v4_nospec_result_terminates_v1_path(void)
+{
+	asm volatile ("					\
+	r1 = 0;						\
+	*(u64*)(r10 - 8) = r1;				\
+	r2 = r10;					\
+	r2 += -8;					\
+	r1 = %[map_hash_8b] ll;				\
+	call %[bpf_map_lookup_elem];			\
+	r8 = r0;					\
+	r2 = r10;					\
+	r2 += -8;					\
+	r1 = %[map_hash_8b] ll;				\
+	call %[bpf_map_lookup_elem];			\
+	r9 = r0;					\
+	r0 = r10;					\
+	r1 = 0;						\
+	r2 = r10;					\
+	if r8 != 0 goto l0_%=;				\
+	if r9 != 0 goto l0_%=;				\
+	r0 = 0;						\
+l0_%=:	if r8 != 0 goto l1_%=;				\
+	goto l2_%=;					\
+l1_%=:	if r9 == 0 goto l3_%=;				\
+	r2 = r0;					\
+l2_%=:	*(u64 *)(r2 -64) = r1;				\
+l3_%=:	r0 = 0;						\
+	exit;						\
+"	:
+	: __imm(bpf_map_lookup_elem),
+	  __imm_addr(map_hash_8b)
+	: __clobber_all);
+}
+
+SEC("socket")
+__description("spec_v4-induced path termination only after insn check (simplified, with dead code)")
+__success __success_unpriv
+__retval(0)
+#ifdef SPEC_V1
+#ifdef SPEC_V4
+/* starts with r0 == r8 == r9 == 0 */
+__xlated_unpriv("if r8 != 0x0 goto pc+1") /* if r9 == 0 goto l3_%= */
+__xlated_unpriv("goto pc+2") /* goto l2_%= */
+__xlated_unpriv("goto pc-1") /* if r9 == 0 goto l3_%= */
+__xlated_unpriv("goto pc-1") /* r2 = r0 */
+__xlated_unpriv("nospec") /* Spectre v1 */
+__xlated_unpriv("*(u64 *)(r2 -64) = r1")
+__xlated_unpriv("nospec") /* Spectre v4 (nospec_result) */
+#endif
+#endif
+__naked void v4_nospec_result_terminates_v1_path_simple(void)
+{
+	asm volatile ("					\
+	r8 = 0;						\
+	r9 = 0;						\
+	r0 = r10;					\
+	r1 = 0;						\
+	r2 = r10;					\
+	if r8 != 0 goto l0_%=;				\
+	if r9 != 0 goto l0_%=;				\
+	r0 = 0;						\
+l0_%=:	if r8 != 0 goto l1_%=;				\
+	goto l2_%=;					\
+l1_%=:	if r9 == 0 goto l3_%=;				\
+	r2 = r0;					\
+l2_%=:	*(u64 *)(r2 -64) = r1;				\
+l3_%=:	r0 = 0;						\
+	exit;						\
+"	::: __clobber_all);
+}
+
 char _license[] SEC("license") = "GPL";
-- 
2.49.0