Merge branch 'bpf-fix-two-holes-in-the-analysis-of-stack-arguments'

Alexei Starovoitov says: ==================== bpf: Fix two holes in the analysis of stack arguments From: Alexei Starovoitov <ast@kernel.org> Before the main pass, arg_track follows pointers to the stack through registers and spills. For each insn it tells which slots the insn can read, which slots it can write, and which slot it writes for certain. Scalar evolution and the removal of dead slots rely on the answer. In two cases a store to other memory counts as a certain write to a stack slot. Then scalar evolution misses a real write to the counter of a loop, and a prog that accesses a map value out of bounds loads. patch 1 "r10, or not a pointer to the stack" counts as "exactly r10". patch 3 A call does not change what is known about the pointers that are spilled in the frame of the caller. But a static subprog or a callback can replace such a pointer with a pointer to a map value. Valid progs are rejected too. Patches 2 and 4 are the tests. Patch 3 needs patch 1 for a spilled copy of r10. v1 -> v2: - One series. v1 was two series [1] [2], and CI applied [2] without [1] - Rebase on the bpf tree: patch 2 did not apply - Test a callee that overwrites a spilled copy of r10: the case in which patch 3 needs patch 1 (CI bot) - Change the two tests that match the log in patch 3, not in patch 4: they failed in between - Load the test with a callback only when progs are JITed - Make the tests shorter. None of them is in verifier_scev.c now - Patch 1: the example in the log stores a pointer to a map value, as the test does. With 0 the prog does not load [1] https://lore.kernel.org/bpf/20261031220615.804426-1-alexei.starovoitov@gmail.com/ [2] https://lore.kernel.org/bpf/20261101004657.3406576-1-alexei.starovoitov@gmail.com/ Signed-off-by: Alexei Starovoitov <ast@kernel.org> ==================== Link: https://patch.msgid.link/20261102055913.3598414-1-alexei.starovoitov@gmail.com Signed-off-by: Alexei Starovoitov <ast@kernel.org>
2026-11-07 06:17:02 +01:00 · 2026-11-02 17:53:14 +00:00 · 2026-11-02 17:53:14 +00:00 · 8ed2506a7c
commit 8ed2506a7c
parent 03b0138974 c9a45b2ede
3 changed files with 267 additions and 31 deletions
--- a/kernel/bpf/liveness.c
+++ b/kernel/bpf/liveness.c
@ -976,6 +976,19 @@ static struct arg_track arg_join_imprecise(struct arg_track a, struct arg_track
 	return (struct arg_track){ .mask = m, .frame = ARG_IMPRECISE };
 }

+/*
+ * 'a' or a value that is not a pointer to stack. A single fp-N gets a fake fp+0,
+ * to keep stack_use and prevent stack_def. fp+0 itself would stay a single offset.
+ */
+static struct arg_track arg_or_none(struct arg_track a)
+{
+	if (a.off_cnt != 1)
+		return a;
+	if (!a.off[0])
+		return (struct arg_track){ .frame = a.frame };
+	return arg_merge_offsets(a, arg_single(a.frame, 0));
+}
+
 /* Join two arg_track values at merge points */
 static struct arg_track __arg_track_join(struct arg_track a, struct arg_track b)
 {
@ -1000,20 +1013,10 @@ static struct arg_track __arg_track_join(struct arg_track a, struct arg_track b)
 	 */
 	if (a.frame == ARG_NONE && b.frame == ARG_NONE)
 		return a;
-	if (a.frame >= 0 && b.frame == ARG_NONE) {
-		/*
-		 * When joining single fp-N add fake fp+0 to
-		 * keep stack_use and prevent stack_def
-		 */
-		if (a.off_cnt == 1)
-			return arg_merge_offsets(a, arg_single(a.frame, 0));
-		return a;
-	}
-	if (b.frame >= 0 && a.frame == ARG_NONE) {
-		if (b.off_cnt == 1)
-			return arg_merge_offsets(b, arg_single(b.frame, 0));
-		return b;
-	}
+	if (a.frame >= 0 && b.frame == ARG_NONE)
+		return arg_or_none(a);
+	if (b.frame >= 0 && a.frame == ARG_NONE)
+		return arg_or_none(b);

 	return arg_join_imprecise(a, b);
 }
@ -1380,6 +1383,31 @@ static struct arg_track arg_track_deref(struct bpf_verifier_env *env, struct bpf
 	return (struct arg_track){ .frame = ARG_NONE };
 }

+/*
+ * True if the call at @insn_idx can pass a pointer to the frame at @depth to a static subprog
+ * or to a callback. They use it as a pointer to stack, a global subprog gets plain memory.
+ * It errs on the safe side: it looks at all argument registers and stack args, also at those
+ * that this callee doesn't take.
+ */
+static bool subprog_gets_local_fp(struct bpf_verifier_env *env, struct bpf_insn *insn, int insn_idx,
+				  struct arg_track *at, int depth)
+{
+	int r;
+
+	if (bpf_pseudo_call(insn)) {
+		r = bpf_find_subprog(env, insn_idx + insn->imm + 1);
+		if (r >= 0 && bpf_subprog_is_global(env, r))
+			return false;
+	} else if (!bpf_is_callx(insn) && !bpf_calls_callback(env, insn_idx)) {
+		return false;
+	}
+	/* R1-R5 and stack args */
+	for (r = BPF_REG_1; r < MAX_AT_TRACK_REGS; r++)
+		if ((r <= BPF_REG_5 || r >= MAX_BPF_REG) && can_be_local_fp(depth, r, &at[r]))
+			return true;
+	return false;
+}
+
 /*
 * Pure dataflow transfer function for arg_track state.
 * Updates at_out[] based on how the instruction modifies registers.
@ -1443,21 +1471,22 @@ static void arg_track_xfer(struct bpf_verifier_env *env, struct bpf_insn *insn,
 		*dst = none;
 	} else if (class == BPF_JMP && code == BPF_CALL) {
 		/*
-		 * at_stack_out[slot] is not cleared by the helper and subprog calls.
-		 * The fill_from_stack() may return the stale spill — which is an FP-derived arg_track
-		 * (the value that was originally spilled there). The loaded register then carries
-		 * a phantom FP-derived identity that doesn't correspond to what's actually in the slot.
-		 * This phantom FP pointer propagates forward, and wherever it's subsequently used
-		 * (as a helper argument, another store, etc.), it sets stack liveness bits.
-		 * Those bits correspond to stack accesses that don't actually happen.
-		 * So the effect is over-reporting stack liveness — marking slots as live that aren't
-		 * actually accessed. The verifier preserves more state than necessary across calls,
-		 * which is conservative.
+		 * Helpers, kfuncs and global subprogs can scratch stack slots. The verifier reads
+		 * a scalar from a scratched slot, but at_stack_out[slot] is not cleared, and
+		 * fill_from_stack() returns the stale spill: an FP-derived arg_track for a scalar.
+		 * A scalar can't be dereferenced, so the stale value can only set liveness bits
+		 * for stack accesses that don't happen. That is conservative.
 		 *
-		 * helpers can scratch stack slots, but they won't make a valid pointer out of it.
-		 * subprogs are allowed to write into parent slots, but they cannot write
-		 * _any_ FP-derived pointer into it (either their own or parent's FP).
+		 * Static subprogs and callbacks can't store an FP-derived pointer into the frame of
+		 * the caller, but they can store any other pointer there. The caller can fill it
+		 * and store through it. With the stale spill that store would be a definite write
+		 * to a stack slot, and it doesn't touch the stack. Callees are analyzed after the
+		 * caller, and they reach the whole frame from one pointer to it. So after the call
+		 * every slot holds what it held before or a value that is not FP-derived.
 		 */
+		if (subprog_gets_local_fp(env, insn, insn_idx, at_out, depth))
+			for (slot = 0; slot < nslots; slot++)
+				at_stack_out[slot] = __arg_track_join(at_stack_out[slot], none);
 		for (r = BPF_REG_0; r <= BPF_REG_5; r++)
 			at_out[r] = none;
 	} else if (class == BPF_LDX) {
--- a/tools/testing/selftests/bpf/progs/verifier_callx.c
+++ b/tools/testing/selftests/bpf/progs/verifier_callx.c
@ -289,6 +289,26 @@ __naked void callx_callee_writes_caller_stack(void)
 		: __clobber_all);
 }

+/* callee can store a pointer that is not FP-derived over a pointer spilled by the caller */
+SEC("socket")
+__success __log_level(2)
+__msg("r0 = *(u64 *)(r10 -16) // fp-16=fp0-8|fp0+0")
+__naked void callx_callee_can_overwrite_spilled_fp(void)
+{
+	asm volatile (
+		"r1 = r10;"
+		"r1 += -8;"
+		"*(u64 *)(r10 - 16) = r1;"
+		"r2 = %[write42] ll;"
+		"callx r2;"
+		"r0 = *(u64 *)(r10 - 16);"
+		"r0 = 0;"
+		"exit;"
+		:
+		: __imm_addr(write42)
+		: __clobber_all);
+}
+
 SEC("socket")
 __failure __msg("R1 has type scalar, expected func")
 __naked void callx_scalar(void)
--- a/tools/testing/selftests/bpf/progs/verifier_live_stack.c
+++ b/tools/testing/selftests/bpf/progs/verifier_live_stack.c
@ -1943,11 +1943,15 @@ static __used __naked void grandchild_deref(void)
 	::: __clobber_all);
 }

+/*
+ * The callee gets a pointer to the frame that holds the spills. It can store a pointer
+ * that is not FP-derived over them: after the call each of them is "fp-N or not FP-derived".
+ */
 SEC("socket")
 __log_level(2)
 __success
-__msg("14: (79) r1 = *(u64 *)(r10 -8) // r6=fp0-8 r7=fp1-16 fp-8=fp1-16 fp-16=fp0-8")
-__msg("15: (79) r0 = *(u64 *)(r1 +0) // r1=fp1-16 r6=fp0-8 r7=fp1-16 fp-8=fp1-16 fp-16=fp0-8")
+__msg("14: (79) r1 = *(u64 *)(r10 -8) // r6=fp0-8 r7=fp1-16 fp-8=fp1-16|fp1+0 fp-16=fp0-8|fp0+0")
+__msg("15: (79) r0 = *(u64 *)(r1 +0) // r1=fp1-16|fp1+0 r6=fp0-8 r7=fp1-16 fp-8=fp1-16|fp1+0")
 __msg("stack use/def subprog#1 mid_two_fp_threshold (d1,cs2):")
 __msg("14: (79) r1 = *(u64 *)(r10 -8)        ; use: fp1-8")
 __msg("15: (79) r0 = *(u64 *)(r1 +0)         ; use: fp1-16")
@ -1992,8 +1996,8 @@ static __used __naked void inner_nop_fptest(void)
 SEC("socket")
 __log_level(2)
 __success
-__msg("13: (79) r1 = *(u64 *)(r10 -8) // r6=fp0-8 r7=fp1-16 fp-8=fp1-16 fp-16=fp0-8")
-__msg("14: (79) r0 = *(u64 *)(r1 +0) // r1=fp1-16 r6=fp0-8 r7=fp1-16 fp-8=fp1-16 fp-16=fp0-8")
+__msg("13: (79) r1 = *(u64 *)(r10 -8) // r6=fp0-8 r7=fp1-16 fp-8=fp1-16|fp1+0 fp-16=fp0-8|fp0+0")
+__msg("14: (79) r0 = *(u64 *)(r1 +0) // r1=fp1-16|fp1+0 r6=fp0-8 r7=fp1-16 fp-8=fp1-16|fp1+0")
 __msg("stack use/def subprog#1 mid_one_fp_threshold (d1,cs2):")
 __msg("13: (79) r1 = *(u64 *)(r10 -8)        ; use: fp1-8")
 __msg("14: (79) r0 = *(u64 *)(r1 +0)         ; use: fp1-16")
@ -2565,6 +2569,189 @@ __naked void stack_or_non_stack_write(void)
 	: __clobber_all);
 }

+/* @insns start with fp-8 = 0 and r0 = pointer to map value */
+#define WITH_MAP_VALUE(insns)						\
+	asm volatile (							\
+	"*(u64 *)(r10 - 8) = 0;"					\
+	"r1 = %[array_map_8b] ll;"					\
+	"r2 = r10;"							\
+	"r2 += -8;"							\
+	"call %[bpf_map_lookup_elem];"					\
+	"if r0 == 0 goto 9f;"						\
+	insns								\
+"9:"									\
+	"exit;"								\
+	:								\
+	: __imm(bpf_map_lookup_elem),					\
+	  __imm(bpf_loop),						\
+	  __imm_addr(array_map_8b)					\
+	: __clobber_all)
+
+/*
+ * r1 and r2 are r10 or a pointer to map value, joined in both orders. The stores through them
+ * are not certain writes to fp-8 and fp-16: both slots are live before the stores.
+ */
+SEC("socket")
+__success __log_level(2)
+__msg("r1: fp0+0 + _ => fp0 ?")
+__msg("r2: _ + fp0+0 => fp0 ?")
+__naked void join_of_r10_and_map_value(void)
+{
+	WITH_MAP_VALUE(
+	"*(u64 *)(r10 - 16) = 0;"
+	"r1 = r10;"
+	"r2 = r0;"
+	"r2 += 16;"
+	"r3 = *(u64 *)(r0 + 0);"
+	"if r3 == 0 goto 1f;"
+	"r1 = r0;"
+	"r1 += 8;"
+	"r2 = r10;"
+"1:"
+	"*(u64 *)(r1 - 8) = 42;"
+	"*(u64 *)(r2 - 16) = 42;"
+	"r0 = *(u64 *)(r10 - 8);"
+	"r1 = *(u64 *)(r10 - 16);");
+}
+
+/* The counter in fp-8 is the offset in the map value r9. The store through r8 can change it. */
+#define LOOP_WITH_STORE_THROUGH_R8					\
+"8:"									\
+	"r1 = *(u64 *)(r10 - 8);"					\
+	"r2 = r9;"							\
+	"r2 += r1;"							\
+	"r0 = *(u8 *)(r2 + 0);"						\
+	"r1 += 1;"							\
+	"*(u64 *)(r10 - 8) = r1;"					\
+	"*(u64 *)(r8 + 0) = 4096;"					\
+	"if r1 < 8 goto 8b;"
+
+/*
+ * r7 is r10 or a pointer to map value. The store through r7 is not a certain write to fp-16:
+ * fp-16 can still hold &fp[-8], and then the store through r8 changes the counter of the loop.
+ */
+SEC("socket")
+__failure __msg("invalid access to map value, value_size=8 off=4096 size=1")
+__naked void join_of_r10_and_map_value_hidden_store(void)
+{
+	WITH_MAP_VALUE(
+	"r9 = r0;"
+	"r7 = r10;"
+	"r1 = *(u64 *)(r9 + 0);"
+	"if r1 == 0 goto 1f;"
+	"r7 = r9;"
+	"r7 += 16;"
+"1:"
+	"r1 = r10;"
+	"r1 += -8;"
+	"*(u64 *)(r10 - 16) = r1;"
+	"*(u64 *)(r7 - 16) = r9;"
+	"r8 = *(u64 *)(r10 - 16);"
+	LOOP_WITH_STORE_THROUGH_R8);
+}
+
+/* ctx[1] = ctx[0]. ctx is the second param, as in a callback of bpf_loop(). */
+static __used __naked void copy_first_to_second(void)
+{
+	asm volatile (
+	"r1 = *(u64 *)(r2 + 0);"
+	"*(u64 *)(r2 + 8) = r1;"
+	"r0 = 0;"
+	"exit;"
+	::: __clobber_all);
+}
+
+/*
+ * fp-16 holds &fp[-8]. The callee gets &fp[-24] and stores a pointer to map value to fp-16.
+ * The store through the filled pointer is not a certain write to fp-8: fp-8 is live before it.
+ */
+SEC("socket")
+__success __retval(0) __log_level(2)
+__msg("r1 = *(u64 *)(r10 -16) // fp-16=fp0-8|fp0+0")
+__msg("*(u64 *)(r1 +0) = 42{{ +}}; may_def: fp0-8")
+__naked void callee_overwrites_spilled_fp(void)
+{
+	WITH_MAP_VALUE(
+	"r1 = r10;"
+	"r1 += -8;"
+	"*(u64 *)(r10 - 16) = r1;"
+	"*(u64 *)(r10 - 24) = r0;"
+	"r2 = r10;"
+	"r2 += -24;"
+	"call copy_first_to_second;"
+	"r1 = *(u64 *)(r10 - 16);"
+	"*(u64 *)(r1 + 0) = 42;"
+	"r0 = *(u64 *)(r10 - 8);");
+}
+
+/* The same, but fp-16 holds a copy of r10, and the callee stores map value + 8 over it. */
+SEC("socket")
+__success __retval(0)
+__naked void callee_overwrites_spilled_r10(void)
+{
+	WITH_MAP_VALUE(
+	"*(u64 *)(r10 - 16) = r10;"
+	"r0 += 8;"
+	"*(u64 *)(r10 - 24) = r0;"
+	"r2 = r10;"
+	"r2 += -24;"
+	"call copy_first_to_second;"
+	"r1 = *(u64 *)(r10 - 16);"
+	"*(u64 *)(r1 - 8) = 42;"
+	"r0 = *(u64 *)(r10 - 8);");
+}
+
+/* The same, but the callee is a callback. */
+SEC("socket")
+__success __retval(0)
+__load_if_JITed()
+__naked void callback_overwrites_spilled_fp(void)
+{
+	WITH_MAP_VALUE(
+	"r1 = r10;"
+	"r1 += -8;"
+	"*(u64 *)(r10 - 16) = r1;"
+	"*(u64 *)(r10 - 24) = r0;"
+	"r1 = 1;"
+	"r2 = copy_first_to_second ll;"
+	"r3 = r10;"
+	"r3 += -24;"
+	"r4 = 0;"
+	"call %[bpf_loop];"
+	"r1 = *(u64 *)(r10 - 16);"
+	"*(u64 *)(r1 + 0) = 42;"
+	"r0 = *(u64 *)(r10 - 8);");
+}
+
+/*
+ * fp-16 holds &fp[-8], fp-24 holds &fp[-16], and the callee stores a pointer to map value to fp-24.
+ * fp-16 is still known after the call, and the store through r7 is not a certain write to it:
+ * r8 is &fp[-8], and the store through r8 changes the counter of the loop.
+ */
+SEC("socket")
+__failure __msg("invalid access to map value, value_size=8 off=4096 size=1")
+__naked void callee_overwrites_spilled_fp_hidden_store(void)
+{
+	WITH_MAP_VALUE(
+	"r9 = r0;"
+	"r1 = r10;"
+	"r1 += -8;"
+	"*(u64 *)(r10 - 16) = r1;"
+	"r1 += -8;"
+	"*(u64 *)(r10 - 24) = r1;"
+	"*(u64 *)(r10 - 32) = r9;"
+	"r2 = r10;"
+	"r2 += -32;"
+	"call copy_first_to_second;"
+	/* the fill keeps fp-16 live over the call */
+	"r1 = *(u64 *)(r10 - 16);"
+	"*(u64 *)(r10 - 16) = r1;"
+	"r7 = *(u64 *)(r10 - 24);"
+	"*(u64 *)(r7 + 0) = r9;"
+	"r8 = *(u64 *)(r10 - 16);"
+	LOOP_WITH_STORE_THROUGH_R8);
+}
+
 SEC("socket")
 __log_level(2)
 __flag(BPF_F_TEST_STATE_FREQ)