* [PATCH bpf-next v3 1/2] bpf: Fix aux usage after do_check_insn()
2025-07-05 19:09 [PATCH bpf-next v3 0/2] bpf: Fix and test aux usage after do_check_insn() Luis Gerhorst
@ 2025-07-05 19:09 ` Luis Gerhorst
2025-07-07 2:14 ` Eduard Zingerman
2025-07-05 19:09 ` [PATCH bpf-next v3 2/2] selftests/bpf: Add Spectre v4 tests Luis Gerhorst
2025-07-07 15:50 ` [PATCH bpf-next v3 0/2] bpf: Fix and test aux usage after do_check_insn() patchwork-bot+netdevbpf
2 siblings, 1 reply; 5+ messages in thread
From: Luis Gerhorst @ 2025-07-05 19:09 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan, Kumar Kartikeya Dwivedi, Peilin Ye,
Luis Gerhorst, Saket Kumar Bhaskar, Viktor Malik, Ihor Solodrai,
Daniel Xu, bpf, linux-kselftest, linux-kernel, Paul Chaignon
Cc: syzbot+dc27c5fb8388e38d2d37
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(). Therefore, introduce and use
insn_aux variable.
Also change cur_aux(env)->nospec in case do_check_insn() ever manages to
increment insn_idx but still fail.
Change the warning to check the insn class (which prevents it from
triggering for ldimm64, for which nospec_result would not be
problematic) and use verifier_bug_if().
In line with Eduard's suggestion, do not introduce prev_aux() because
that requires one to understand that after do_check_insn() call what was
current became previous. This would at-least require a comment.
Fixes: d6f1c85f2253 ("bpf: Fall back to nospec for Spectre v1")
Reported-by: Paul Chaignon <paul.chaignon@gmail.com>
Reported-by: Eduard Zingerman <eddyz87@gmail.com>
Reported-by: syzbot+dc27c5fb8388e38d2d37@syzkaller.appspotmail.com
Link: https://lore.kernel.org/bpf/685b3c1b.050a0220.2303ee.0010.GAE@google.com/
Link: https://lore.kernel.org/bpf/4266fd5de04092aa4971cbef14f1b4b96961f432.camel@gmail.com/
Suggested-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Luis Gerhorst <luis.gerhorst@fau.de>
---
kernel/bpf/verifier.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 0f6cc2275695..96c737b41c3f 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19923,6 +19923,7 @@ static int do_check(struct bpf_verifier_env *env)
for (;;) {
struct bpf_insn *insn;
+ struct bpf_insn_aux_data *insn_aux;
int err;
/* reset current history entry on each new instruction */
@@ -19936,6 +19937,7 @@ static int do_check(struct bpf_verifier_env *env)
}
insn = &insns[env->insn_idx];
+ insn_aux = &env->insn_aux_data[env->insn_idx];
if (++env->insn_processed > BPF_COMPLEXITY_LIMIT_INSNS) {
verbose(env,
@@ -20012,7 +20014,7 @@ static int do_check(struct bpf_verifier_env *env)
/* Reduce verification complexity by stopping speculative path
* verification when a nospec is encountered.
*/
- if (state->speculative && cur_aux(env)->nospec)
+ if (state->speculative && insn_aux->nospec)
goto process_bpf_exit;
err = do_check_insn(env, &do_print_state);
@@ -20020,11 +20022,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;
+ insn_aux->nospec = true;
/* If it was an ADD/SUB insn, potentially remove any
* markings for alu sanitization.
*/
- cur_aux(env)->alu_state = 0;
+ insn_aux->alu_state = 0;
goto process_bpf_exit;
} else if (err < 0) {
return err;
@@ -20033,7 +20035,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 && insn_aux->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
@@ -20042,8 +20044,15 @@ 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.
+ *
+ * All non-branch instructions have a single
+ * fall-through edge. For these, nospec_result should
+ * already work.
*/
- WARN_ON_ONCE(env->insn_idx != prev_insn_idx + 1);
+ if (verifier_bug_if(BPF_CLASS(insn->code) == BPF_JMP ||
+ BPF_CLASS(insn->code) == BPF_JMP32, env,
+ "speculation barrier after jump instruction may not have the desired effect"))
+ return -EFAULT;
process_bpf_exit:
mark_verifier_state_scratched(env);
err = update_branch_counts(env, env->cur_state);
--
2.49.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH bpf-next v3 1/2] bpf: Fix aux usage after do_check_insn()
2025-07-05 19:09 ` [PATCH bpf-next v3 1/2] bpf: Fix " Luis Gerhorst
@ 2025-07-07 2:14 ` Eduard Zingerman
0 siblings, 0 replies; 5+ messages in thread
From: Eduard Zingerman @ 2025-07-07 2:14 UTC (permalink / raw)
To: Luis Gerhorst, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan, Kumar Kartikeya Dwivedi, Peilin Ye,
Saket Kumar Bhaskar, Viktor Malik, Ihor Solodrai, Daniel Xu, bpf,
linux-kselftest, linux-kernel, Paul Chaignon
Cc: syzbot+dc27c5fb8388e38d2d37
On Sat, 2025-07-05 at 21:09 +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(). Therefore, introduce and use
> insn_aux variable.
>
> Also change cur_aux(env)->nospec in case do_check_insn() ever manages to
> increment insn_idx but still fail.
>
> Change the warning to check the insn class (which prevents it from
> triggering for ldimm64, for which nospec_result would not be
> problematic) and use verifier_bug_if().
>
> In line with Eduard's suggestion, do not introduce prev_aux() because
> that requires one to understand that after do_check_insn() call what was
> current became previous. This would at-least require a comment.
>
> Fixes: d6f1c85f2253 ("bpf: Fall back to nospec for Spectre v1")
> Reported-by: Paul Chaignon <paul.chaignon@gmail.com>
> Reported-by: Eduard Zingerman <eddyz87@gmail.com>
> Reported-by: syzbot+dc27c5fb8388e38d2d37@syzkaller.appspotmail.com
> Link: https://lore.kernel.org/bpf/685b3c1b.050a0220.2303ee.0010.GAE@google.com/
> Link: https://lore.kernel.org/bpf/4266fd5de04092aa4971cbef14f1b4b96961f432.camel@gmail.com/
> Suggested-by: Eduard Zingerman <eddyz87@gmail.com>
> Signed-off-by: Luis Gerhorst <luis.gerhorst@fau.de>
> ---
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
[...]
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH bpf-next v3 2/2] selftests/bpf: Add Spectre v4 tests
2025-07-05 19:09 [PATCH bpf-next v3 0/2] bpf: Fix and test aux usage after do_check_insn() Luis Gerhorst
2025-07-05 19:09 ` [PATCH bpf-next v3 1/2] bpf: Fix " Luis Gerhorst
@ 2025-07-05 19:09 ` Luis Gerhorst
2025-07-07 15:50 ` [PATCH bpf-next v3 0/2] bpf: Fix and test aux usage after do_check_insn() patchwork-bot+netdevbpf
2 siblings, 0 replies; 5+ messages in thread
From: Luis Gerhorst @ 2025-07-05 19:09 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan, Kumar Kartikeya Dwivedi, Peilin Ye,
Luis Gerhorst, Saket Kumar Bhaskar, Viktor Malik, Ihor Solodrai,
Daniel Xu, bpf, linux-kselftest, linux-kernel, Paul Chaignon
Add the following tests:
1. A test with an (unimportant) ldimm64 (16 byte insn) and a
Spectre-v4--induced nospec that clarifies and serves as a basic
Spectre v4 test.
2. Make sure a Spectre v4 nospec_result does not prevent a Spectre v1
nospec from being added before the dangerous instruction (tests that
[1] is fixed).
3. Combine the two, which is the combination that triggers the warning
in [2]. This is because the unanalyzed stack write has nospec_result
set, but the ldimm64 (which was just analyzed) had incremented
insn_idx by 2. That violates the assertion that nospec_result is only
used after insns that increment insn_idx by 1 (i.e., stack writes).
[1] https://lore.kernel.org/bpf/4266fd5de04092aa4971cbef14f1b4b96961f432.camel@gmail.com/
[2] https://lore.kernel.org/bpf/685b3c1b.050a0220.2303ee.0010.GAE@google.com/
Signed-off-by: Luis Gerhorst <luis.gerhorst@fau.de>
---
tools/testing/selftests/bpf/progs/bpf_misc.h | 4 +
.../selftests/bpf/progs/verifier_unpriv.c | 149 ++++++++++++++++++
2 files changed, 153 insertions(+)
diff --git a/tools/testing/selftests/bpf/progs/bpf_misc.h b/tools/testing/selftests/bpf/progs/bpf_misc.h
index 20dce508d8e0..530752ddde8e 100644
--- a/tools/testing/selftests/bpf/progs/bpf_misc.h
+++ b/tools/testing/selftests/bpf/progs/bpf_misc.h
@@ -237,4 +237,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..28b4f7035ceb 100644
--- a/tools/testing/selftests/bpf/progs/verifier_unpriv.c
+++ b/tools/testing/selftests/bpf/progs/verifier_unpriv.c
@@ -801,4 +801,153 @@ l2_%=: \
: __clobber_all);
}
+SEC("socket")
+__description("unpriv: ldimm64 before Spectre v4 barrier")
+__success __success_unpriv
+__retval(0)
+#ifdef SPEC_V4
+__xlated_unpriv("r1 = 0x2020200005642020") /* should not matter */
+__xlated_unpriv("*(u64 *)(r10 -8) = r1")
+__xlated_unpriv("nospec")
+#endif
+__naked void unpriv_ldimm64_spectre_v4(void)
+{
+ asm volatile (" \
+ r1 = 0x2020200005642020 ll; \
+ *(u64 *)(r10 -8) = r1; \
+ r0 = 0; \
+ exit; \
+" ::: __clobber_all);
+}
+
+SEC("socket")
+__description("unpriv: Spectre v1 and v4 barrier")
+__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 dangerous `*(u64 *)(NOT_FP -64)
+ * = r1` iff `if r9 == 0 goto pc+4` was mispredicted because of Spectre v1. The
+ * test therefore ensures the Spectre-v4--induced nospec does not prevent the
+ * Spectre-v1--induced speculative path from being fully analyzed.
+ */
+__xlated_unpriv("nospec") /* Spectre v1 */
+__xlated_unpriv("*(u64 *)(r2 -64) = r1") /* could be used to leak r2 */
+__xlated_unpriv("nospec") /* Spectre v4 */
+#endif
+#endif
+__naked void unpriv_spectre_v1_and_v4(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("unpriv: Spectre v1 and v4 barrier (simple)")
+__success __success_unpriv
+__retval(0)
+#ifdef SPEC_V1
+#ifdef SPEC_V4
+__xlated_unpriv("if r8 != 0x0 goto pc+1")
+__xlated_unpriv("goto pc+2")
+__xlated_unpriv("goto pc-1") /* if r9 == 0 goto l3_%= */
+__xlated_unpriv("goto pc-1") /* r2 = r0 */
+__xlated_unpriv("nospec")
+__xlated_unpriv("*(u64 *)(r2 -64) = r1")
+__xlated_unpriv("nospec")
+#endif
+#endif
+__naked void unpriv_spectre_v1_and_v4_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);
+}
+
+SEC("socket")
+__description("unpriv: ldimm64 before Spectre v1 and v4 barrier (simple)")
+__success __success_unpriv
+__retval(0)
+#ifdef SPEC_V1
+#ifdef SPEC_V4
+__xlated_unpriv("if r8 != 0x0 goto pc+1")
+__xlated_unpriv("goto pc+4")
+__xlated_unpriv("goto pc-1") /* if r9 == 0 goto l3_%= */
+__xlated_unpriv("goto pc-1") /* r2 = r0 */
+__xlated_unpriv("goto pc-1") /* r1 = 0x2020200005642020 ll */
+__xlated_unpriv("goto pc-1") /* second part of ldimm64 */
+__xlated_unpriv("nospec")
+__xlated_unpriv("*(u64 *)(r2 -64) = r1")
+__xlated_unpriv("nospec")
+#endif
+#endif
+__naked void unpriv_ldimm64_spectre_v1_and_v4_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; \
+ r1 = 0x2020200005642020 ll; \
+l2_%=: *(u64 *)(r2 -64) = r1; \
+l3_%=: r0 = 0; \
+ exit; \
+" ::: __clobber_all);
+}
+
char _license[] SEC("license") = "GPL";
--
2.49.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH bpf-next v3 0/2] bpf: Fix and test aux usage after do_check_insn()
2025-07-05 19:09 [PATCH bpf-next v3 0/2] bpf: Fix and test aux usage after do_check_insn() Luis Gerhorst
2025-07-05 19:09 ` [PATCH bpf-next v3 1/2] bpf: Fix " Luis Gerhorst
2025-07-05 19:09 ` [PATCH bpf-next v3 2/2] selftests/bpf: Add Spectre v4 tests Luis Gerhorst
@ 2025-07-07 15:50 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 5+ messages in thread
From: patchwork-bot+netdevbpf @ 2025-07-07 15:50 UTC (permalink / raw)
To: Luis Gerhorst
Cc: ast, daniel, andrii, martin.lau, eddyz87, song, yonghong.song,
john.fastabend, kpsingh, sdf, haoluo, jolsa, mykolal, shuah,
memxor, yepeilin, skb99, vmalik, isolodrai, dxu, bpf,
linux-kselftest, linux-kernel, paul.chaignon
Hello:
This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:
On Sat, 5 Jul 2025 21:09:06 +0200 you wrote:
> Fix cur_aux()->nospec_result test after do_check_insn() referring to the
> to-be-analyzed (potentially unsafe) instruction, not the
> already-analyzed (safe) instruction. This might allow a unsafe insn to
> slip through on a speculative path. Create some tests from the
> reproducer [1].
>
> Commit d6f1c85f2253 ("bpf: Fall back to nospec for Spectre v1") should
> not be in any stable kernel yet, therefore bpf-next should suffice.
>
> [...]
Here is the summary with links:
- [bpf-next,v3,1/2] bpf: Fix aux usage after do_check_insn()
https://git.kernel.org/bpf/bpf-next/c/dadb59104c64
- [bpf-next,v3,2/2] selftests/bpf: Add Spectre v4 tests
https://git.kernel.org/bpf/bpf-next/c/92974cef83b5
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 5+ messages in thread