Merge branch 'bpf-preserve-pointer-state-for-commuted-arithmetic'

Yiyang Chen says:

====================
bpf: Preserve pointer state for commuted arithmetic

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

Patch 1 simplifies sanitize_err() before the pointer-state change. Patch 2
keeps the full pointer register state when the pointer operand is the source
of the add, preserving fields such as the stack frame number and parent id.
Patch 3 moves the untrusted PTR_TO_MEM early return after state propagation,
so scalar += untrusted_pointer remains usable through the probe-read path.
Patch 4 adds verifier selftests for stack frame number preservation,
readonly-untrusted memory access, and dynptr data-slice invalidation.

Changes in v4:
- Target the bpf fixes tree because the affected behavior is present in
  released kernels.
- Add Eduard's sanitize_err() cleanup and revised pointer-state patch, reusing
  the caller's temporary offset register instead of verifier-env scratch
  storage.
- Correct the stack-frame Fixes tag to the BPF-to-BPF call verification
  commit identified by Shung-Hsi.
- Fix the dynptr test comment style and retain the source-register clear so
  the test isolates parent-id propagation.
- Carry Daniel Wade's Tested-by and Eduard's selftest Acked-by from the v3
  thread.
- Rebase to bpf base 0ce37745d4.

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

v3: https://lore.kernel.org/bpf/cover.1784696371.git.chenyy23@mails.tsinghua.edu.cn/
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/
---
====================

Link: https://patch.msgid.link/20260729-c3-035-public-bpf-v4-v4-0-8ee297e2346b@mails.tsinghua.edu.cn
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>

kernel/bpf/verifier.c

@@ -13557,23 +13557,21 @@ static void sanitize_mark_insn_seen(struct bpf_verifier_env *env)
 		env->insn_aux_data[env->insn_idx].seen = env->pass_cnt;
 }
 
-static int sanitize_err(struct bpf_verifier_env *env,
-			const struct bpf_insn *insn, int reason,
-			const struct bpf_reg_state *off_reg,
-			const struct bpf_reg_state *dst_reg)
+static int sanitize_err(struct bpf_verifier_env *env, const struct bpf_insn *insn, int reason)
 {
 	static const char *err = "pointer arithmetic with it prohibited for !root";
 	const char *op = BPF_OP(insn->code) == BPF_ADD ? "add" : "sub";
 	u32 dst = insn->dst_reg, src = insn->src_reg;
+	struct bpf_reg_state *regs = cur_regs(env);
 
 	switch (reason) {
 	case REASON_BOUNDS:
 		verbose(env, "R%d has unknown scalar with mixed signed bounds, %s\n",
-			off_reg == dst_reg ? dst : src, err);
+			regs[src].type == SCALAR_VALUE ? src : dst, err);
 		break;
 	case REASON_TYPE:
 		verbose(env, "R%d has pointer with unsupported alu operation, %s\n",
-			off_reg == dst_reg ? src : dst, err);
+			regs[src].type == SCALAR_VALUE ? dst : src, err);
 		break;
 	case REASON_PATHS:
 		verbose(env, "R%d tried to %s from different maps, paths or scalars, %s\n",
@@ -13709,13 +13707,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:
@@ -13745,11 +13736,19 @@ 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. Callers guarantee that below does not overwrite off_reg.
 	 */
-	dst_reg->type = ptr_reg->type;
-	dst_reg->id = ptr_reg->id;
+	if (dst_reg != ptr_reg)
+		*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))
@@ -13762,7 +13761,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);
 	}
 
 	switch (opcode) {
@@ -13792,7 +13791,7 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
 		}
 		break;
 	case BPF_SUB:
-		if (dst_reg == off_reg) {
+		if (dst_reg != ptr_reg) {
 			/* scalar -= pointer.  Creates an unknown scalar */
 			verbose(env, "R%d tried to subtract pointer from scalar\n",
 				dst);
@@ -13855,7 +13854,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);
 	}
 
 	return 0;
@@ -14607,7 +14606,7 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env,
 	if (sanitize_needed(opcode)) {
 		ret = sanitize_val_alu(env, insn);
 		if (ret < 0)
-			return sanitize_err(env, insn, ret, NULL, NULL);
+			return sanitize_err(env, insn, ret);
 	}
 
 	/* Calculate sign/unsigned bounds and tnum for alu32 and alu64 bit ops.
@@ -14810,8 +14809,8 @@ static int adjust_reg_min_max_vals(struct bpf_verifier_env *env,
 				err = mark_chain_precision(env, insn->dst_reg);
 				if (err)
 					return err;
-				return adjust_ptr_min_max_vals(env, insn,
-							       src_reg, dst_reg);
+				off_reg = *dst_reg;
+				return adjust_ptr_min_max_vals(env, insn, src_reg, &off_reg);
 			}
 		} else if (ptr_reg) {
 			/* pointer += scalar */

tools/testing/selftests/bpf/progs/dynptr_fail.c

@@ -1635,6 +1635,36 @@ 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.
  */

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

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