diff --git a/kernel/bpf/liveness.c b/kernel/bpf/liveness.c index e657e6d95408..75d9af4921de 100644 --- 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) { diff --git a/tools/testing/selftests/bpf/progs/verifier_callx.c b/tools/testing/selftests/bpf/progs/verifier_callx.c index 7e59188d9fca..a12bc421e75c 100644 --- 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) diff --git a/tools/testing/selftests/bpf/progs/verifier_live_stack.c b/tools/testing/selftests/bpf/progs/verifier_live_stack.c index 5f9b11a81260..2ae5c60150dd 100644 --- 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)