[PATCH bpf-next v3 0/3] bpf: Preserve pointer state for commuted arithmetic

[PATCH bpf-next v3 0/3] bpf: Preserve pointer state for commuted arithmetic

[Yiyang Chen]

This series fixes pointer-state propagation for commuted scalar += pointer
arithmetic in the verifier.

Patch 1 keeps the full pointer register state when the pointer operand is
the source of the add, which preserves fields such as the stack frame
number and parent id instead of copying only type and id.

Patch 2 builds on that state propagation and moves the untrusted PTR_TO_MEM
early return after it, so scalar += untrusted_pointer is modeled as
PTR_TO_MEM and remains usable through the probe-read path.

Patch 3 adds verifier selftests for stack frame number preservation,
readonly-untrusted memory access, and dynptr data-slice invalidation.

Changes in v3:
- Preserve the complete pointer register state with verifier-env scratch
  storage, addressing Eduard's comment that copying selected fields is
  fragile and avoiding a temporary bpf_reg_state on the verifier stack.
- Keep the existing RUN(verifier_basic_stack) dispatch unchanged and add the
  stack regression directly to the existing verifier_basic_stack program.
- Keep the original operand direction inside adjust_ptr_min_max_vals() by
  saving the scalar operand in env->fake_reg[0].
- Move untrusted PTR_TO_MEM handling after the unified pointer-state copy so
  the commuted form remains PTR_TO_MEM before the early return.
- Add readonly-untrusted and dynptr selftest coverage, responding to the
  bpf-ci/static review finding that the untrusted pointer case needs a
  regression test.
- Clear the original dynptr data-slice register after deriving the commuted
  alias so the regression test isolates parent-id propagation.
- Make the readonly-untrusted return value endian-neutral by loading an int.
- Rebase to bpf-next base a23a71823352.

v2: https://lore.kernel.org/bpf/cover.1784563950.git.chenyy23@mails.tsinghua.edu.cn/
v1: https://lore.kernel.org/bpf/cover.1784563939.git.chenyy23@mails.tsinghua.edu.cn/

Yiyang Chen (3):
  bpf: Preserve pointer state for commuted arithmetic
  bpf: Propagate untrusted pointer state in commuted arithmetic
  selftests/bpf: Cover commuted pointer state propagation

 kernel/bpf/verifier.c                         | 35 +++++++++-------
 .../testing/selftests/bpf/progs/dynptr_fail.c | 31 ++++++++++++++
 .../bpf/progs/mem_rdonly_untrusted.c          | 17 ++++++++
 .../bpf/progs/verifier_basic_stack.c          | 41 +++++++++++++++++++
 4 files changed, 110 insertions(+), 14 deletions(-)


base-commit: a23a71823352e2d792dcaae25f1ebb744acbfc0b
-- 
2.34.1

[PATCH bpf-next v3 1/3] bpf: Preserve pointer state for commuted arithmetic

[Yiyang Chen]

When scalar += pointer is handled in adjust_ptr_min_max_vals(), the
destination register inherits the pointer state from the source pointer.
Copying only selected fields is fragile because pointer provenance is
tracked by several bpf_reg_state fields.

Use verifier-env scratch storage to preserve the scalar operand while
replacing the destination with the full pointer state. This preserves the
frame number for PTR_TO_STACK registers and keeps parent identity fields
consistent.

Fixes: f1174f77b50c ("bpf/verifier: rework value tracking")
Signed-off-by: Yiyang Chen <chenyy23@mails.tsinghua.edu.cn>
---
 kernel/bpf/verifier.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 52be0a118cce0..085cbd5222737 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -13726,11 +13726,14 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
 	s64 smin_val = reg_smin(off_reg), smax_val = reg_smax(off_reg);
 	u64 umin_val = reg_umin(off_reg), umax_val = reg_umax(off_reg);
 	struct bpf_sanitize_info info = {};
+	const struct bpf_reg_state *orig_off_reg = off_reg;
+	bool ptr_is_dst_reg;
 	u8 opcode = BPF_OP(insn->code);
 	u32 dst = insn->dst_reg;
 	int ret, bounds_ret;
 
 	dst_reg = &regs[dst];
+	ptr_is_dst_reg = ptr_reg == dst_reg;
 
 	if ((known && (smin_val != smax_val || umin_val != umax_val)) ||
 	    smin_val > smax_val || umin_val > umax_val) {
@@ -13796,11 +13799,15 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
 		return -EACCES;
 	}
 
-	/* In case of 'scalar += pointer', dst_reg inherits pointer type and id.
-	 * The id may be overwritten later if we create a new variable offset.
+	/* For 'scalar += pointer', dst_reg inherits the complete pointer
+	 * register state. Individual fields may be adjusted later by pointer
+	 * arithmetic.
 	 */
-	dst_reg->type = ptr_reg->type;
-	dst_reg->id = ptr_reg->id;
+	if (!ptr_is_dst_reg) {
+		env->fake_reg[0] = *off_reg;
+		off_reg = &env->fake_reg[0];
+		*dst_reg = *ptr_reg;
+	}
 
 	if (!check_reg_sane_offset_scalar(env, off_reg, ptr_reg->type) ||
 	    !check_reg_sane_offset_ptr(env, ptr_reg, ptr_reg->type))
@@ -13813,7 +13820,7 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
 		ret = sanitize_ptr_alu(env, insn, ptr_reg, off_reg, dst_reg,
 				       &info, false);
 		if (ret < 0)
-			return sanitize_err(env, insn, ret, off_reg, dst_reg);
+			return sanitize_err(env, insn, ret, orig_off_reg, dst_reg);
 	}
 
 	switch (opcode) {
@@ -13843,7 +13850,7 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
 		}
 		break;
 	case BPF_SUB:
-		if (dst_reg == off_reg) {
+		if (!ptr_is_dst_reg) {
 			/* scalar -= pointer.  Creates an unknown scalar */
 			verbose(env, "R%d tried to subtract pointer from scalar\n",
 				dst);
@@ -13906,7 +13913,7 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
 			return -EFAULT;
 		}
 		if (ret < 0)
-			return sanitize_err(env, insn, ret, off_reg, dst_reg);
+			return sanitize_err(env, insn, ret, orig_off_reg, dst_reg);
 	}
 
 	return 0;
-- 
2.34.1

[PATCH bpf-next v3 2/3] bpf: Propagate untrusted pointer state in commuted arithmetic

[Yiyang Chen]

The untrusted PTR_TO_MEM early return skips pointer offset tracking
because accesses go through probe-read handling. Moving it after full
pointer-state propagation ensures scalar += untrusted_pointer leaves the
destination as PTR_TO_MEM instead of an unrelated scalar.

Fixes: f2362a57aeff ("bpf: allow void* cast using bpf_rdonly_cast()")
Signed-off-by: Yiyang Chen <chenyy23@mails.tsinghua.edu.cn>
---
 kernel/bpf/verifier.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 085cbd5222737..18fb6267692c5 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -13763,13 +13763,6 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
 		return -EACCES;
 	}
 
-	/*
-	 * Accesses to untrusted PTR_TO_MEM are done through probe
-	 * instructions, hence no need to track offsets.
-	 */
-	if (base_type(ptr_reg->type) == PTR_TO_MEM && (ptr_reg->type & PTR_UNTRUSTED))
-		return 0;
-
 	switch (base_type(ptr_reg->type)) {
 	case PTR_TO_CTX:
 	case PTR_TO_MAP_VALUE:
@@ -13809,6 +13802,13 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
 		*dst_reg = *ptr_reg;
 	}
 
+	/*
+	 * Accesses to untrusted PTR_TO_MEM are done through probe
+	 * instructions, hence no need to track offsets.
+	 */
+	if (base_type(ptr_reg->type) == PTR_TO_MEM && (ptr_reg->type & PTR_UNTRUSTED))
+		return 0;
+
 	if (!check_reg_sane_offset_scalar(env, off_reg, ptr_reg->type) ||
 	    !check_reg_sane_offset_ptr(env, ptr_reg, ptr_reg->type))
 		return -EINVAL;
-- 
2.34.1

[PATCH bpf-next v3 3/3] selftests/bpf: Cover commuted pointer state propagation

[Yiyang Chen]

Add verifier coverage for the three cases affected by preserving the full
pointer state across scalar += pointer: stack frame number inheritance,
readonly-untrusted memory access, and dynptr data-slice invalidation.

Signed-off-by: Yiyang Chen <chenyy23@mails.tsinghua.edu.cn>
---
 .../testing/selftests/bpf/progs/dynptr_fail.c | 31 ++++++++++++++
 .../bpf/progs/mem_rdonly_untrusted.c          | 17 ++++++++
 .../bpf/progs/verifier_basic_stack.c          | 41 +++++++++++++++++++
 3 files changed, 89 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c
index 94489ac64da8e..7da4266bae09b 100644
--- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
+++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
@@ -1635,6 +1635,37 @@ static int callback(__u32 index, void *data)
         return 0;
 }
 
+/* A commuted add should preserve the parent id of a dynptr data slice.
+ */
+SEC("?raw_tp")
+__failure __msg("invalid mem access 'scalar'")
+int dynptr_slice_commuted_invalidate(void *ctx)
+{
+	struct bpf_dynptr ptr;
+	__u32 *slice, *derived;
+
+	bpf_ringbuf_reserve_dynptr(&ringbuf, sizeof(__u32), 0, &ptr);
+
+	slice = bpf_dynptr_data(&ptr, 0, sizeof(__u32));
+	if (!slice)
+		goto done;
+
+	asm volatile ("%[dst] = 0;"
+		"%[dst] += %[src];"
+		"%[src] = 0;"
+		: [dst]"=&r"(derived), [src]"+r"(slice)
+		:
+		: "memory");
+
+	bpf_ringbuf_discard_dynptr(&ptr, 0);
+	val = *derived;
+	return 0;
+
+done:
+	bpf_ringbuf_discard_dynptr(&ptr, 0);
+	return 0;
+}
+
 /* If the dynptr is written into in a callback function, its data
  * slices should be invalidated as well.
  */
diff --git a/tools/testing/selftests/bpf/progs/mem_rdonly_untrusted.c b/tools/testing/selftests/bpf/progs/mem_rdonly_untrusted.c
index 5b4453747c230..f166fff8f2176 100644
--- a/tools/testing/selftests/bpf/progs/mem_rdonly_untrusted.c
+++ b/tools/testing/selftests/bpf/progs/mem_rdonly_untrusted.c
@@ -226,4 +226,21 @@ int null_check(void *ctx)
 	return 0;
 }
 
+SEC("socket")
+__success
+__retval(1)
+int ldx_is_ok_commuted_addr(void *ctx)
+{
+	int v, *p, *derived;
+
+	v = 1;
+	p = bpf_rdonly_cast(&v, 0);
+	asm volatile ("%[dst] = 0;"
+		"%[dst] += %[src];"
+		: [dst]"=&r"(derived)
+		: [src]"r"(p)
+		: "memory");
+	return *derived;
+}
+
 char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/verifier_basic_stack.c b/tools/testing/selftests/bpf/progs/verifier_basic_stack.c
index fb62e09f21146..d3df7a9f1d8c8 100644
--- a/tools/testing/selftests/bpf/progs/verifier_basic_stack.c
+++ b/tools/testing/selftests/bpf/progs/verifier_basic_stack.c
@@ -97,4 +97,45 @@ __naked void misaligned_read_from_stack(void)
 "	::: __clobber_all);
 }
 
+SEC("socket")
+__description("stack pointer arithmetic preserves frame number")
+__failure __msg("R7 invalid mem access 'scalar'")
+__naked void stack_ptr_arith_preserves_frameno(void)
+{
+	asm volatile ("\
+		r3 = 0;						\
+		*(u64 *)(r10 - 8) = r3;			\
+		r1 = %[map_hash_8b] ll;			\
+		r2 = r10;					\
+		r2 += -8;					\
+		call %[bpf_map_lookup_elem];		\
+		if r0 != 0 goto +2;			\
+		r0 = 0;						\
+		exit;						\
+		r1 = r0;					\
+		r2 = 0;						\
+		r3 = 0;						\
+		call stack_ptr_arith_preserves_frameno_subprog;\
+		r0 = 0;						\
+		exit;						\
+	":
+	: __imm(bpf_map_lookup_elem),
+	  __imm_addr(map_hash_8b)
+	: __clobber_all);
+}
+
+static __used __naked void stack_ptr_arith_preserves_frameno_subprog(void)
+{
+	asm volatile ("\
+		*(u64 *)(r10 - 8) = r1;			\
+		r6 = -8;					\
+		r6 += r10;					\
+		*(u64 *)(r6 + 0) = r2;			\
+		r7 = *(u64 *)(r10 - 8);			\
+		*(u64 *)(r7 + 0) = r3;			\
+		r0 = 0;						\
+		exit;						\
+	"::: __clobber_all);
+}
+
 char _license[] SEC("license") = "GPL";
-- 
2.34.1

Re: [PATCH bpf-next v3 0/3] bpf: Preserve pointer state for commuted arithmetic

[Daniel Wade]

Hi Eduard, Yiyang,

Thanks for the pointer. I independently tested Yiyang's v3 series on the
source-matched f105f3631d51 base with the same kernel config used for my
original report.

Before the series, my impact witness reached chosen-address kernel
read/write and changed UID/GID 65534 with CAP_BPF and CAP_PERFMON to
UID/GID 0 in 3/3 runs. With all three v3 patches applied, the intended
untrusted-pointer positive control still passed, while the impact program
was rejected at "R0 pointer += pointer prohibited" before any address leak
or privilege transition in 3/3 runs.

I also ran the three focused cases added by the v3 series. All passed:

  mem_rdonly_untrusted/ldx_is_ok_commuted_addr
  dynptr/dynptr_slice_commuted_invalidate
  verifier_basic_stack/stack pointer arithmetic preserves frame number

Given that the affected code has shipped and the issue reaches kernel
read/write from a CAP_BPF and CAP_PERFMON context, should this be routed
through bpf and marked for stable rather than bpf-next?

For the v3 series:

Tested-by: Daniel Wade <danjwade95@gmail.com>

Regards,
Daniel