[PATCH bpf-next v2 3/6] bpf, x86: Fix exception table metadata for arena load-acquire
Daniel Borkmann <[email protected]>
| Newsgroups | org.kernel.vger.bpf |
|---|---|
| Message-ID | <[email protected]> |
A load-acquire from an arena pointer is converted to BPF_PROBE_ATOMIC and
gets an exception table entry, but the entry is filled in as if it were a
store, since populate_extable() decides based on instruction class alone
and a load-acquire is of BPF_STX class:
if (BPF_CLASS(insn->code) == BPF_LDX) {
arena_reg = reg2pt_regs[src_reg];
fixup_reg = reg2pt_regs[dst_reg];
} else {
arena_reg = reg2pt_regs[dst_reg];
fixup_reg = DONT_CLEAR;
}
For a load-acquire dst_reg holds the loaded value and src_reg holds the
address, so both assignments in the else branch are wrong. On a fault
over an unmapped arena page ex_handler_bpf() then:
- computes the reported address from the value register instead
of the address register
- reports the access as a WRITE, since it derives the direction
from fixup_reg == DONT_CLEAR
- leaves dst_reg untouched, so the program continues with a stale
value instead of the 0 that BPF_PROBE_* loads deliver
The access itself is emitted correctly, emit_atomic_ld_st_index() uses
src_reg as the address, so this is a broken probe contract and a wrong
diagnostic rather than a memory safety issue.
Use bpf_atomic_is_load_acq() helper so a load-acquire takes the load path.
Fixes: 5341c9a4d833 ("bpf, x86: Support load-acquire and store-release instructions")
Signed-off-by: Daniel Borkmann <[email protected]>
Cc: Peilin Ye <[email protected]>
---
arch/x86/net/bpf_jit_comp.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 01e7ce569c1e..88ed95b2eaa7 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -2331,8 +2331,13 @@ st: insn_off = insn->off;
* BPF_PROBE_ATOMIC) before being used for the memory access. Pass
* the reg holding the unmodified 32-bit address to
* ex_handler_bpf().
+ *
+ * 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.
*/
- if (BPF_CLASS(insn->code) == BPF_LDX) {
+ if (BPF_CLASS(insn->code) == BPF_LDX ||
+ bpf_atomic_is_load_acq(insn)) {
arena_reg = reg2pt_regs[src_reg];
fixup_reg = reg2pt_regs[dst_reg];
} else {
--
2.43.0