From: Alexei Starovoitov A fix of the main pass for big endian that stands alone. The patches that follow refine more registers and stack slots at conditional jumps, so more progs would run into it. *(u64 *)(r10 - 8) = r1 r2 = *(u32 *)(r10 - 8) r2 gets the low 32 bits of r1 on little endian and the upper 32 bits on big endian. In two cases the verifier gives r2 the range of the low 32 bits on big endian too. The fill itself is right: check_stack_read_fixed_off() asks bpf_stack_narrow_access_ok() where the low bytes of a spill are. A fill from elsewhere gets an unbounded scalar. After the 32-bit spill '*(u32 *)(r10 - 8) = r1' the half-slot at r10 - 8 is STACK_SPILL and spilled_ptr describes r1, whose low 32 bits it holds on both byte orders. Two functions take a 64-bit spill for such a slot: - __clean_func_state() marks a dead half-slot STACK_POISON. If that is the one at r10 - 4, what remains of a 64-bit spill has the slot types of a 32-bit spill. - stacksafe() skips the bytes that are STACK_MISC in the old state. An old 32-bit spill and a current 64-bit spill are then compared by regsafe() of spilled_ptr. That is right on little endian, where the half-slot at r10 - 8 has the low 32 bits of a 64-bit spill. On big endian it has the upper 32 bits. Keep such a 64-bit spill whole in __clean_func_state(). In stacksafe() compare two spills only if the bytes of the old one are the low bytes of the current one. bpf_stack_narrow_access_ok() decides in both. Two tests in verifier_spill_fill.c, one per function. They load on little endian and are rejected on big endian. Fixes: be23266b4a08 ("bpf: 4-byte precise clean_verifier_state") Fixes: 6715df8d5d24 ("bpf: Allow reads from uninit stack") Signed-off-by: Alexei Starovoitov --- kernel/bpf/states.c | 19 ++++++++++++ .../selftests/bpf/progs/verifier_spill_fill.c | 31 +++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/kernel/bpf/states.c b/kernel/bpf/states.c index efc0870b85c2..b60676349655 100644 --- a/kernel/bpf/states.c +++ b/kernel/bpf/states.c @@ -168,6 +168,16 @@ static void __clean_func_state(struct bpf_verifier_env *env, stype == STACK_IRQ_FLAG) continue; + /* + * When the half-slot closer to FP is dead, what remains of a + * 64-bit spill looks like a 32-bit spill: the low 32 bits of + * the value. On big endian the other half-slot holds the upper + * 32 bits. + */ + if (hi_live && bpf_spill_size(bpf_stack_slot(st, i)) == BPF_REG_SIZE && + !bpf_stack_narrow_access_ok(0, BPF_REG_SIZE / 2, BPF_REG_SIZE)) + continue; + /* * Only scalar spills can be degraded to raw stack bytes * when their high half is dead. Pointer spills need the @@ -446,6 +456,15 @@ static bool stacksafe(struct bpf_verifier_env *env, struct bpf_func_state *old, if (!cur_slot) return false; + /* + * The old spill is the first bytes of a wider spill. On big endian + * these are not the low bytes of the value. + */ + if (old_type == STACK_SPILL && bpf_is_spilled_reg(cur_slot) && + !bpf_stack_narrow_access_ok(0, bpf_spill_size(old_slot), + bpf_spill_size(cur_slot))) + return false; + /* * 64 and 32-bit scalar spills vs MISC/INVALID slots and vice versa. * Load from MISC/INVALID slots produces unbound scalar. diff --git a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c index 0baa8e06b8d1..623ed5ef3630 100644 --- a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c +++ b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c @@ -1451,6 +1451,37 @@ __naked void imprecise_scalar_spill_half_dead(void) : __clobber_all); } +/* + * The first half-slot of a 64-bit spill holds its low 32 bits on little endian only. + * The test spills to r10 - 8. The first four bytes of the slot are an offset into one byte. + */ +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ +#define FIRST_HALF_IS_LOW __success +#else +#define FIRST_HALF_IS_LOW __failure __msg("access may be outside object bounds") +#endif +#define FIRST_HALF(name, stores) \ +SEC("raw_tp") FIRST_HALF_IS_LOW \ +__naked void name(void) \ +{ \ + asm volatile ( \ + "call %[bpf_ktime_get_ns];" \ + stores \ + "r7 = *(u32 *)(r10 - 8); r1 = %[single_byte_buf] ll; r1 += r7; r0 = *(u8 *)(r1 + 0);" \ + "out_%=: r0 = 0; exit;" \ + : \ + : __imm(bpf_ktime_get_ns), __imm_addr(single_byte_buf) \ + : __clobber_all); \ +} + +/* The low 32 bits of the value are 0. The second half-slot is dead after the jump. */ +FIRST_HALF(first_half_of_spill_second_half_dead, + "r0 <<= 32; *(u64 *)(r10 - 8) = r0; if r0 == 0 goto out_%=;") +/* One path has a 32-bit spill of r6, whose low 32 bits are 0, the other one a 64-bit spill */ +FIRST_HALF(spill_of_64_bits_vs_spill_of_32_bits, + "r6 = r0; r6 <<= 32; if r0 > 5 goto 1f; *(u32 *)(r10 - 8) = r6; goto 2f;" + "1: r6 = 0x7ffffff000000000 ll; *(u64 *)(r10 - 8) = r6; 2:") + /* check valid spill/fill, ptr to tp buffer */ SEC("raw_tracepoint.w") __success -- 2.55.0