[PATCH bpf 1/2] bpf: fix REG INVARIANTS VIOLATION on speculative pointer arithmetic

[PATCH bpf 1/2] bpf: fix REG INVARIANTS VIOLATION on speculative pointer arithmetic

[Jiayuan Chen]

Take the following unprivileged program as an example:

	r0 = bpf_map_lookup_elem(...)	/* PTR_TO_MAP_VALUE, offset 0 */
	...
	14: r0 += r1			/* r1 is a bounded scalar */
	15: r9 = r0

Loading it triggers a verifier warning from reg_bounds_sanity_check():

	verifier bug: REG INVARIANTS VIOLATION (alu): const subreg tnum out
	of sync with range bounds r64={.base=0x0, .size=0x0}
	r32={.base=0x0, .size=0xffffffff} var_off=(0x0, 0x0)

What happens:

1. Processing insn 14 (r0 += r1) in adjust_ptr_min_max_vals(), the new
   offset is computed into dst_reg's var_off and 32/64-bit ranges.

2. Because pointer registers do not track 32-bit subregister bounds,
   __mark_reg32_unbounded() first sets r32 to the full range; r32 is
   re-derived from the offset at the end of the function by
   reg_bounds_sync().

3. On the unprivileged path, sanitize_ptr_alu() is called and, via
   sanitize_speculative_path() -> push_stack(), snapshots the current
   register state and schedules the next instruction (insn 15) to be
   verified directly as a speculative path.

4. That snapshot is taken between step 2 and the final reg_bounds_sync():
   at this point dst_reg's var_off still holds the (const) original
   offset while r32 has just been blanked to the full range, i.e. the two
   are out of sync. When the speculative path later verifies insn 15
   (r9 = r0), the inconsistent state reaches reg_bounds_sanity_check() and
   trips the warning.

var_off and the 32-bit range must always be consistent. There are two
ways to keep the snapshot consistent:

  1. sync var_off and r32 before the snapshot so they match, or
  2. leave r32 at its original (already consistent) value and blank it
     only after the snapshot.

The whole point of sanitize_ptr_alu() is to insert a harmless masking
sequence that keeps the access in bounds under speculation, so the state
it snapshots should faithfully represent that. Take approach 2: move
__mark_reg32_unbounded() to after sanitize_ptr_alu(), so the speculative
snapshot keeps the pointer's original, consistent r32. The non-speculative
path is unchanged: r32 is still blanked before the offset is applied and
re-derived by reg_bounds_sync().

Fixes: 5f99f312bd3b ("bpf: add register bounds sanity checks and sanitization")
Reported-by: Hiker Cl <clhiker365@gmail.com>
Closes: https://lore.kernel.org/bpf/CAGM=xGB1fJ9kT8XTitVo74B0WGqgjkoUHdLwzytwV0AyqeVApw@mail.gmail.com/
Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 kernel/bpf/verifier.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index d17f14b35b79..d79038a8da10 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -14558,9 +14558,6 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env, struct bpf_insn
 		return -EINVAL;
 	}
 
-	/* pointer types do not carry 32-bit bounds at the moment. */
-	__mark_reg32_unbounded(dst_reg);
-
 	if (sanitize_needed(opcode)) {
 		ret = sanitize_ptr_alu(env, insn, ptr_reg, off_reg, dst_reg,
 				       &info, false);
@@ -14568,6 +14565,14 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env, struct bpf_insn
 			return sanitize_err(env, insn, ret);
 	}
 
+	/* Pointer types do not carry 32-bit bounds at the moment. Blank r32
+	 * only after sanitize_ptr_alu() may have snapshotted dst_reg into a
+	 * speculative path: otherwise that snapshot freezes a const offset
+	 * with an unbounded r32, which later trips reg_bounds_sanity_check().
+	 * reg_bounds_sync() below re-derives r32 from the updated offset.
+	 */
+	__mark_reg32_unbounded(dst_reg);
+
 	switch (opcode) {
 	case BPF_ADD:
 		/*
-- 
2.43.0

[PATCH bpf 2/2] selftests/bpf: Add reg-invariants test for speculative pointer arithmetic

[Jiayuan Chen]

An unprivileged socket filter does variable pointer arithmetic on a
PTR_TO_MAP_VALUE whose offset collapses to a constant. The Spectre-v1
speculative path used to snapshot the pointer with a const offset and an
unbounded r32, which tripped reg_bounds_sanity_check() on the following
register move.

Mark the test __success_unpriv (the speculative path only runs
unprivileged) and flag it BPF_F_TEST_REG_INVARIANTS so the invariant
violation becomes a hard load failure. The unprivileged run fails without
the verifier fix and passes with it:

  verifier_bounds/spec_ptr_alu_const_offset @unpriv:FAIL   # without fix
  verifier_bounds/spec_ptr_alu_const_offset @unpriv:OK     # with fix

Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 .../selftests/bpf/progs/verifier_bounds.c     | 41 +++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/verifier_bounds.c b/tools/testing/selftests/bpf/progs/verifier_bounds.c
index 1a273e416fed..df8d5309657e 100644
--- a/tools/testing/selftests/bpf/progs/verifier_bounds.c
+++ b/tools/testing/selftests/bpf/progs/verifier_bounds.c
@@ -2267,6 +2267,47 @@ __naked void deduce64_from_32_wrapping_32bit(void)
 	: __clobber_all);
 }
 
+/*
+ * Unprivileged variable pointer arithmetic on a PTR_TO_MAP_VALUE whose
+ * offset collapses to a constant. The Spectre-v1 speculative path snapshots
+ * the pointer while its r32 has just been blanked but its offset not yet
+ * synced; the following register move used to trip reg_bounds_sanity_check()
+ * ("const subreg tnum out of sync with range bounds"). With
+ * BPF_F_TEST_REG_INVARIANTS that violation turns into a load failure, so the
+ * unprivileged program must still load.
+ */
+SEC("socket")
+__success __success_unpriv
+__flag(BPF_F_TEST_REG_INVARIANTS)
+__naked void spec_ptr_alu_const_offset(void)
+{
+	asm volatile ("							\
+	call %[bpf_ktime_get_ns];					\
+	*(u64*)(r10 - 16) = r0;						\
+	r1 = 0;								\
+	*(u64*)(r10 - 8) = r1;						\
+	r2 = r10;							\
+	r2 += -8;							\
+	r1 = %[map_hash_8b] ll;						\
+	call %[bpf_map_lookup_elem];					\
+	if r0 == 0 goto l0_%=;						\
+	r1 = *(u64*)(r10 - 16);						\
+	r2 = 0x40000000;						\
+	if r1 > r2 goto l0_%=;						\
+	if r1 s> 1 goto l0_%=;		/* r1 in [0, 1] */		\
+	r0 += r1;			/* ptr += bounded scalar */	\
+	r9 = r0;			/* used to trip the warning */	\
+	*(u8*)(r0 + 0) = r1;						\
+l0_%=:	r0 = 0;								\
+	exit;								\
+	"
+	:
+	: __imm(bpf_ktime_get_ns),
+	  __imm(bpf_map_lookup_elem),
+	  __imm_addr(map_hash_8b)
+	: __clobber_all);
+}
+
 /* Check that range_within() compares cnum ranges, not min/max projections. */
 SEC("socket")
 __failure __msg("div by zero")
-- 
2.43.0

Re: [PATCH bpf 2/2] selftests/bpf: Add reg-invariants test for speculative pointer arithmetic

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 2469 bytes --]

> diff --git a/tools/testing/selftests/bpf/progs/verifier_bounds.c b/tools/testing/selftests/bpf/progs/verifier_bounds.c
> index bc038ac2df98..91d4600aa677 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_bounds.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_bounds.c
> @@ -2267,6 +2267,47 @@ __naked void deduce64_from_32_wrapping_32bit(void)
>  	: __clobber_all);
>  }
>
> +/*
> + * Unprivileged variable pointer arithmetic on a PTR_TO_MAP_VALUE whose
> + * offset collapses to a constant. The Spectre-v1 speculative path snapshots
> + * the pointer while its r32 has just been blanked but its offset not yet
> + * synced; the following register move used to trip reg_bounds_sanity_check()
> + * ("const subreg tnum out of sync with range bounds"). With
> + * BPF_F_TEST_REG_INVARIANTS that violation turns into a load failure, so the
> + * unprivileged program must still load.
> + */

This isn't a bug, but would it read more clearly to say the base
PTR_TO_MAP_VALUE has a constant offset that the speculative snapshot
captures, rather than that the offset "collapses to a constant"? The
addend itself has to stay variable for sanitize_ptr_alu() to simulate the
path at all.

> +SEC("socket")
> +__success __success_unpriv
> +__flag(BPF_F_TEST_REG_INVARIANTS)
> +__naked void spec_ptr_alu_const_offset(void)
> +{
> +	asm volatile ("							\
> +	call %[bpf_ktime_get_ns];					\
> +	*(u64*)(r10 - 16) = r0;						\
> +	r1 = 0;								\
> +	*(u64*)(r10 - 8) = r1;						\
> +	r2 = r10;							\
> +	r2 += -8;							\
> +	r1 = %[map_hash_8b] ll;						\
> +	call %[bpf_map_lookup_elem];					\
> +	if r0 == 0 goto l0_%=;						\
> +	r1 = *(u64*)(r10 - 16);						\
> +	r2 = 0x40000000;						\
> +	if r1 > r2 goto l0_%=;						\
> +	if r1 s> 1 goto l0_%=;		/* r1 in [0, 1] */		\
> +	r0 += r1;			/* ptr += bounded scalar */	\
> +	r9 = r0;			/* used to trip the warning */	\
> +	*(u8*)(r0 + 0) = r1;						\
> +l0_%=:	r0 = 0;								\
> +	exit;								\
> +	"
> +	:
> +	: __imm(bpf_ktime_get_ns),
> +	  __imm(bpf_map_lookup_elem),
> +	  __imm_addr(map_hash_8b)
> +	: __clobber_all);
> +}
> +
>  /* Check that range_within() compares cnum ranges, not min/max projections. */
>  SEC("socket")
>  __failure __msg("div by zero")


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32257333410