Same problem as on x86-64: add_exception_handler() folds "there is no destination register to clear" and "this is a store" into one DONT_CLEAR value ... if (BPF_CLASS(insn->code) != BPF_LDX && !bpf_atomic_is_load_acq(insn)) dst_reg = DONT_CLEAR; ... which ex_handler_bpf() then reads back as the access direction: bool is_write = (dst_reg == DONT_CLEAR); A RMW carrying BPF_FETCH is both. emit_lse_atomic() reads the old value into src_reg for BPF_{ADD,AND,OR,XOR} | BPF_FETCH and BPF_XCHG, and into r0 for BPF_CMPXCHG, so a fault over an unmapped arena page is correctly reported as a WRITE but leaves that register holding a stale value instead of the 0 that every other BPF_PROBE_* access delivers. Same as on x86-64, add a separate ARENA_WRITE bit for the direction. FIXUP_REG is now filled in by the callers of add_exception_handler(), the BPF_PROBE_ATOMIC one deriving it from bpf_atomic_load_reg(), so that the helper only has to determine the direction. This is how the riscv64 JIT already does it, and it stops the two store callers from handing in a dst_reg that was only going to be overwritten with DONT_CLEAR anyway. Fixes: e612b5c1d3ee ("bpf, arm64: Add support for lse atomics in bpf_arena") Signed-off-by: Daniel Borkmann Cc: Puranjay Mohan --- v1 -> v2: - move bpf_atomic_load_reg into build_insn (Eduard) arch/arm64/net/bpf_jit_comp.c | 44 ++++++++++++++++++++++++----------- 1 file changed, 30 insertions(+), 14 deletions(-) diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c index d14d297ebb96..74b4083791da 100644 --- a/arch/arm64/net/bpf_jit_comp.c +++ b/arch/arm64/net/bpf_jit_comp.c @@ -1082,23 +1082,27 @@ static void build_epilogue(struct jit_ctx *ctx, bool was_classic) * * Bit layout of `fixup` (32-bit): * - * +-----------+--------+-----------+-----------+----------+ - * | 31-27 | 26-22 | 21 | 20-16 | 15-0 | - * | | | | | | - * | FIXUP_REG | Unused | ARENA_ACC | ARENA_REG | OFFSET | - * +-----------+--------+-----------+-----------+----------+ + * +-----------+--------+-------------+-----------+-----------+----------+ + * | 31-27 | 26-23 | 22 | 21 | 20-16 | 15-0 | + * | | | | | | | + * | FIXUP_REG | Unused | ARENA_WRITE | ARENA_ACC | ARENA_REG | OFFSET | + * +-----------+--------+-------------+-----------+-----------+----------+ * * - OFFSET (16 bits): Offset used to compute address for Load/Store instruction. * - ARENA_REG (5 bits): Register that is used to calculate the address for load/store when * accessing the arena region. * - ARENA_ACCESS (1 bit): This bit is set when the faulting instruction accessed the arena region. + * - ARENA_WRITE (1 bit): This bit is set when the faulting instruction wrote to the arena region. + * It is independent of FIXUP_REG, since a read-modify-write both writes to + * memory and reads the old value into a register. * - FIXUP_REG (5 bits): Destination register for the load instruction (cleared on fault) or set to - * DONT_CLEAR if it is a store instruction. + * DONT_CLEAR if the instruction does not read into a register. */ #define BPF_FIXUP_OFFSET_MASK GENMASK(15, 0) #define BPF_FIXUP_ARENA_REG_MASK GENMASK(20, 16) #define BPF_ARENA_ACCESS BIT(21) +#define BPF_ARENA_WRITE BIT(22) #define BPF_FIXUP_REG_MASK GENMASK(31, 27) #define DONT_CLEAR 5 /* Unused ARM64 register from BPF's POV */ @@ -1109,7 +1113,7 @@ bool ex_handler_bpf(const struct exception_table_entry *ex, s16 off = FIELD_GET(BPF_FIXUP_OFFSET_MASK, ex->fixup); int arena_reg = FIELD_GET(BPF_FIXUP_ARENA_REG_MASK, ex->fixup); bool is_arena = !!(ex->fixup & BPF_ARENA_ACCESS); - bool is_write = (dst_reg == DONT_CLEAR); + bool is_write = !!(ex->fixup & BPF_ARENA_WRITE); unsigned long addr; if (is_arena) { @@ -1132,7 +1136,7 @@ static int add_exception_handler(const struct bpf_insn *insn, { off_t ins_offset; s16 off = insn->off; - bool is_arena; + bool is_arena, is_write; int arena_reg; unsigned long pc; struct exception_table_entry *ex; @@ -1181,15 +1185,18 @@ static int add_exception_handler(const struct bpf_insn *insn, /* * A load-acquire is of BPF_STX class, but reads from src_reg into * dst_reg like a BPF_LDX does, hence it must not be treated as a store - * here. + * here. A read-modify-write carrying BPF_FETCH is reported as a write + * even though it does have a register to clear, see the callers. */ - if (BPF_CLASS(insn->code) != BPF_LDX && !bpf_atomic_is_load_acq(insn)) - dst_reg = DONT_CLEAR; + is_write = BPF_CLASS(insn->code) != BPF_LDX && + !bpf_atomic_is_load_acq(insn); ex->fixup = FIELD_PREP(BPF_FIXUP_REG_MASK, dst_reg); if (is_arena) { ex->fixup |= BPF_ARENA_ACCESS; + if (is_write) + ex->fixup |= BPF_ARENA_WRITE; /* * insn->src_reg/dst_reg holds the address in the arena region with upper 32-bits * being zero because of a preceding addr_space_cast(r, 0x0, 0x1) instruction. @@ -1889,7 +1896,7 @@ static int build_insn(const struct bpf_verifier_env *env, const struct bpf_insn break; } - ret = add_exception_handler(insn, ctx, dst); + ret = add_exception_handler(insn, ctx, DONT_CLEAR); if (ret) return ret; break; @@ -1956,7 +1963,7 @@ static int build_insn(const struct bpf_verifier_env *env, const struct bpf_insn break; } - ret = add_exception_handler(insn, ctx, dst); + ret = add_exception_handler(insn, ctx, DONT_CLEAR); if (ret) return ret; break; @@ -1979,7 +1986,16 @@ static int build_insn(const struct bpf_verifier_env *env, const struct bpf_insn return ret; if (BPF_MODE(insn->code) == BPF_PROBE_ATOMIC) { - ret = add_exception_handler(insn, ctx, dst); + /* + * A load-acquire reads into dst_reg, and a read-modify-write + * carrying BPF_FETCH reads the old value into src_reg, or into + * r0 for a BPF_CMPXCHG. Clear that register on fault, the + * remaining atomics have no destination register. + */ + int load_reg = bpf_atomic_load_reg(insn); + + ret = add_exception_handler(insn, ctx, load_reg < 0 ? + DONT_CLEAR : bpf2a64[load_reg]); if (ret) return ret; } -- 2.43.0