* [PATCH bpf-next v2 0/3] bpf: Fix and test aux usage after do_check_insn()
@ 2025-06-28 14:50 Luis Gerhorst
2025-06-28 14:50 ` [PATCH bpf-next v2 1/3] bpf: Update env->prev_insn_idx " Luis Gerhorst
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Luis Gerhorst @ 2025-06-28 14:50 UTC (permalink / raw)
To: Andrii Nakryiko, Eduard Zingerman, Mykola Lysenko,
Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Shuah Khan, Kumar Kartikeya Dwivedi,
Luis Gerhorst, Peilin Ye, Jiayuan Chen, Saket Kumar Bhaskar,
Ihor Solodrai, Daniel Xu, bpf, linux-kselftest, linux-kernel,
Paul Chaignon
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.
[1] https://lore.kernel.org/bpf/685b3c1b.050a0220.2303ee.0010.GAE@google.com/
Changes since v1:
- Fix compiler error due to missed rename of prev_insn_idx in first
patch
- v1: https://lore.kernel.org/bpf/20250628125927.763088-1-luis.gerhorst@fau.de/
Changes since RFC:
- Introduce prev_aux() as suggested by Alexei. For this, we must move
the env->prev_insn_idx assignment to happen directly after
do_check_insn(), for which I have created a separate commit. This
patch could be simplified by using a local prev_aux variable as
sugested by Eduard, but I figured one might find the new
assignment-strategy easier to understand (before, prev_insn_idx and
env->prev_insn_idx were out-of-sync for the latter part of the loop).
Also, like this we do not have an additional prev_* variable that must
be kept in-sync and the local variable's usage (old prev_insn_idx, new
tmp) is much more local. If you think it would be better to not take
the risk and keep the fix simple by just introducing the prev_aux
variable, let me know.
- Change WARN_ON_ONCE() to verifier_bug_if() as suggested by Alexei
- Change assertion to check instruction is BPF_JMP[32] as suggested by
Eduard
- RFC: https://lore.kernel.org/bpf/8734bmoemx.fsf@fau.de/
Luis Gerhorst (3):
bpf: Update env->prev_insn_idx after do_check_insn()
bpf: Fix aux usage after do_check_insn()
selftests/bpf: Add Spectre v4 tests
kernel/bpf/verifier.c | 30 ++--
tools/testing/selftests/bpf/progs/bpf_misc.h | 4 +
.../selftests/bpf/progs/verifier_unpriv.c | 149 ++++++++++++++++++
3 files changed, 174 insertions(+), 9 deletions(-)
base-commit: d69bafe6ee2b5eff6099fa26626ecc2963f0f363
--
2.49.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH bpf-next v2 1/3] bpf: Update env->prev_insn_idx after do_check_insn()
2025-06-28 14:50 [PATCH bpf-next v2 0/3] bpf: Fix and test aux usage after do_check_insn() Luis Gerhorst
@ 2025-06-28 14:50 ` Luis Gerhorst
2025-06-28 14:50 ` [PATCH bpf-next v2 2/3] bpf: Fix aux usage " Luis Gerhorst
2025-06-28 14:50 ` [PATCH bpf-next v2 3/3] selftests/bpf: Add Spectre v4 tests Luis Gerhorst
2 siblings, 0 replies; 6+ messages in thread
From: Luis Gerhorst @ 2025-06-28 14:50 UTC (permalink / raw)
To: Andrii Nakryiko, Eduard Zingerman, Mykola Lysenko,
Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Shuah Khan, Kumar Kartikeya Dwivedi,
Luis Gerhorst, Peilin Ye, Jiayuan Chen, Saket Kumar Bhaskar,
Ihor Solodrai, Daniel Xu, bpf, linux-kselftest, linux-kernel,
Paul Chaignon
To introduce prev_aux(env), env->prev_insn_idx must be up-to-date
directly after do_check_insn(). To achieve this, replace prev_insn_idx
with a tmp variable (to discourage use) and update env->prev_insn_idx
directly after do_check_insn().
A concern would be that some code relied on env->prev_insn_idx still
having the old value between do_check_insn() and the old
update-assignment. However, outside the do_check() function it is only
used through push_jmp_history()/do_check_insn()/is_state_visisted()
which are not called in-between the old and new assignment location.
This can also be seen from the -O0 call graph for push_jmp_history()
[1].
[1] https://sys.cs.fau.de/extern/person/gerhorst/25-06_d69baf_push_jmp_history_O0_callgraph.png
Signed-off-by: Luis Gerhorst <luis.gerhorst@fau.de>
---
kernel/bpf/verifier.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index f403524bd215..b33bc37d5372 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19854,16 +19854,15 @@ static int do_check(struct bpf_verifier_env *env)
struct bpf_insn *insns = env->prog->insnsi;
int insn_cnt = env->prog->len;
bool do_print_state = false;
- int prev_insn_idx = -1;
+ env->prev_insn_idx = -1;
for (;;) {
struct bpf_insn *insn;
- int err;
+ int err, tmp;
/* reset current history entry on each new instruction */
env->cur_hist_ent = NULL;
- env->prev_insn_idx = prev_insn_idx;
if (env->insn_idx >= insn_cnt) {
verbose(env, "invalid insn idx %d insn_cnt %d\n",
env->insn_idx, insn_cnt);
@@ -19942,7 +19941,6 @@ static int do_check(struct bpf_verifier_env *env)
}
sanitize_mark_insn_seen(env);
- prev_insn_idx = env->insn_idx;
/* Reduce verification complexity by stopping speculative path
* verification when a nospec is encountered.
@@ -19950,7 +19948,9 @@ static int do_check(struct bpf_verifier_env *env)
if (state->speculative && cur_aux(env)->nospec)
goto process_bpf_exit;
+ tmp = env->insn_idx;
err = do_check_insn(env, &do_print_state);
+ env->prev_insn_idx = tmp;
if (error_recoverable_with_nospec(err) && state->speculative) {
/* Prevent this speculative path from ever reaching the
* insn that would have been unsafe to execute.
@@ -19978,13 +19978,13 @@ 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);
+ WARN_ON_ONCE(env->insn_idx != env->prev_insn_idx + 1);
process_bpf_exit:
mark_verifier_state_scratched(env);
err = update_branch_counts(env, env->cur_state);
if (err)
return err;
- err = pop_stack(env, &prev_insn_idx, &env->insn_idx,
+ err = pop_stack(env, &env->prev_insn_idx, &env->insn_idx,
pop_log);
if (err < 0) {
if (err != -ENOENT)
--
2.49.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH bpf-next v2 2/3] bpf: Fix aux usage after do_check_insn()
2025-06-28 14:50 [PATCH bpf-next v2 0/3] bpf: Fix and test aux usage after do_check_insn() Luis Gerhorst
2025-06-28 14:50 ` [PATCH bpf-next v2 1/3] bpf: Update env->prev_insn_idx " Luis Gerhorst
@ 2025-06-28 14:50 ` Luis Gerhorst
2025-06-30 1:05 ` Eduard Zingerman
2025-06-28 14:50 ` [PATCH bpf-next v2 3/3] selftests/bpf: Add Spectre v4 tests Luis Gerhorst
2 siblings, 1 reply; 6+ messages in thread
From: Luis Gerhorst @ 2025-06-28 14:50 UTC (permalink / raw)
To: Andrii Nakryiko, Eduard Zingerman, Mykola Lysenko,
Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Shuah Khan, Kumar Kartikeya Dwivedi,
Luis Gerhorst, Peilin Ye, Jiayuan Chen, Saket Kumar Bhaskar,
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
prev_aux().
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().
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/
Signed-off-by: Luis Gerhorst <luis.gerhorst@fau.de>
---
kernel/bpf/verifier.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index b33bc37d5372..9d066e4b8248 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -11216,6 +11216,11 @@ static struct bpf_insn_aux_data *cur_aux(const struct bpf_verifier_env *env)
return &env->insn_aux_data[env->insn_idx];
}
+static struct bpf_insn_aux_data *prev_aux(const struct bpf_verifier_env *env)
+{
+ return &env->insn_aux_data[env->prev_insn_idx];
+}
+
static bool loop_flag_is_zero(struct bpf_verifier_env *env)
{
struct bpf_reg_state *regs = cur_regs(env);
@@ -19955,11 +19960,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;
+ prev_aux(env)->nospec = true;
/* If it was an ADD/SUB insn, potentially remove any
* markings for alu sanitization.
*/
- cur_aux(env)->alu_state = 0;
+ prev_aux(env)->alu_state = 0;
goto process_bpf_exit;
} else if (err < 0) {
return err;
@@ -19968,7 +19973,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 && prev_aux(env)->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,8 +19982,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 != env->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] 6+ messages in thread
* [PATCH bpf-next v2 3/3] selftests/bpf: Add Spectre v4 tests
2025-06-28 14:50 [PATCH bpf-next v2 0/3] bpf: Fix and test aux usage after do_check_insn() Luis Gerhorst
2025-06-28 14:50 ` [PATCH bpf-next v2 1/3] bpf: Update env->prev_insn_idx " Luis Gerhorst
2025-06-28 14:50 ` [PATCH bpf-next v2 2/3] bpf: Fix aux usage " Luis Gerhorst
@ 2025-06-28 14:50 ` Luis Gerhorst
2 siblings, 0 replies; 6+ messages in thread
From: Luis Gerhorst @ 2025-06-28 14:50 UTC (permalink / raw)
To: Andrii Nakryiko, Eduard Zingerman, Mykola Lysenko,
Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Shuah Khan, Kumar Kartikeya Dwivedi,
Luis Gerhorst, Peilin Ye, Jiayuan Chen, Saket Kumar Bhaskar,
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 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..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] 6+ messages in thread
* Re: [PATCH bpf-next v2 2/3] bpf: Fix aux usage after do_check_insn()
2025-06-28 14:50 ` [PATCH bpf-next v2 2/3] bpf: Fix aux usage " Luis Gerhorst
@ 2025-06-30 1:05 ` Eduard Zingerman
2025-07-02 8:56 ` Luis Gerhorst
0 siblings, 1 reply; 6+ messages in thread
From: Eduard Zingerman @ 2025-06-30 1:05 UTC (permalink / raw)
To: Luis Gerhorst, Andrii Nakryiko, Mykola Lysenko,
Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Shuah Khan, Kumar Kartikeya Dwivedi,
Peilin Ye, Jiayuan Chen, Saket Kumar Bhaskar, Ihor Solodrai,
Daniel Xu, bpf, linux-kselftest, linux-kernel, Paul Chaignon
Cc: syzbot+dc27c5fb8388e38d2d37
On Sat, 2025-06-28 at 16:50 +0200, Luis Gerhorst wrote:
[...]
> @@ -19955,11 +19960,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;
> + prev_aux(env)->nospec = true;
I don't like the prev_aux() call in this position, as one needs to
understand that after do_check_insn() call what was current became
previous. This at-least requires a comment. Implementation with a
temporary variable (as at the bottom of this email), imo, is less
cognitive load.
> /* IF it was an ADD/SUB insn, potentially remove any
> * markings for alu sanitization.
> */
> - cur_aux(env)->alu_state = 0;
> + prev_aux(env)->alu_state = 0;
> goto process_bpf_exit;
> } else if (err < 0) {
> return err;
[...]
---
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index a136d9b1b25f..a923614b7104 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19953,6 +19953,7 @@ static int do_check(struct bpf_verifier_env *env)
bool pop_log = !(env->log.level & BPF_LOG_LEVEL2);
struct bpf_verifier_state *state = env->cur_state;
struct bpf_insn *insns = env->prog->insnsi;
+ struct bpf_insn_aux_data *insn_aux;
int insn_cnt = env->prog->len;
bool do_print_state = false;
int prev_insn_idx = -1;
@@ -19972,6 +19973,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,
@@ -20048,7 +20050,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);
@@ -20056,11 +20058,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;
@@ -20069,7 +20071,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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next v2 2/3] bpf: Fix aux usage after do_check_insn()
2025-06-30 1:05 ` Eduard Zingerman
@ 2025-07-02 8:56 ` Luis Gerhorst
0 siblings, 0 replies; 6+ messages in thread
From: Luis Gerhorst @ 2025-07-02 8:56 UTC (permalink / raw)
To: Eduard Zingerman
Cc: Andrii Nakryiko, Mykola Lysenko, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Shuah Khan, Kumar Kartikeya Dwivedi, Peilin Ye, Jiayuan Chen,
Saket Kumar Bhaskar, Ihor Solodrai, Daniel Xu, bpf,
linux-kselftest, linux-kernel, Paul Chaignon,
syzbot+dc27c5fb8388e38d2d37
Eduard Zingerman <eddyz87@gmail.com> writes:
> On Sat, 2025-06-28 at 16:50 +0200, Luis Gerhorst wrote:
>
> [...]
>
>> @@ -19955,11 +19960,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;
>> + prev_aux(env)->nospec = true;
>
> I don't like the prev_aux() call in this position, as one needs to
> understand that after do_check_insn() call what was current became
> previous. This at-least requires a comment. Implementation with a
> temporary variable (as at the bottom of this email), imo, is less
> cognitive load.
I think I agree. I will send a v3 with the variable.
>> /* IF it was an ADD/SUB insn, potentially remove any
>> * markings for alu sanitization.
>> */
>> - cur_aux(env)->alu_state = 0;
>> + prev_aux(env)->alu_state = 0;
>> goto process_bpf_exit;
>> } else if (err < 0) {
>> return err;
>
> [...]
>
> ---
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index a136d9b1b25f..a923614b7104 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -19953,6 +19953,7 @@ static int do_check(struct bpf_verifier_env *env)
> bool pop_log = !(env->log.level & BPF_LOG_LEVEL2);
> struct bpf_verifier_state *state = env->cur_state;
> struct bpf_insn *insns = env->prog->insnsi;
> + struct bpf_insn_aux_data *insn_aux;
> int insn_cnt = env->prog->len;
> bool do_print_state = false;
> int prev_insn_idx = -1;
> @@ -19972,6 +19973,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,
> @@ -20048,7 +20050,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);
> @@ -20056,11 +20058,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;
> @@ -20069,7 +20071,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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-07-02 8:56 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-28 14:50 [PATCH bpf-next v2 0/3] bpf: Fix and test aux usage after do_check_insn() Luis Gerhorst
2025-06-28 14:50 ` [PATCH bpf-next v2 1/3] bpf: Update env->prev_insn_idx " Luis Gerhorst
2025-06-28 14:50 ` [PATCH bpf-next v2 2/3] bpf: Fix aux usage " Luis Gerhorst
2025-06-30 1:05 ` Eduard Zingerman
2025-07-02 8:56 ` Luis Gerhorst
2025-06-28 14:50 ` [PATCH bpf-next v2 3/3] selftests/bpf: Add Spectre v4 tests Luis Gerhorst
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).