Re: [PATCH bpf-next v2 3/6] bpf, x86: Fix exception table metadata for arena load-acquire

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The x86 JIT exception table metadata fix for `BPF_PROBE_ATOMIC` successfully addresses `BPF_LOAD_ACQ` but incorrectly handles read-modify-write (RMW) fetch operations (e.g., `BPF_CMPXCHG`, `BPF_XCHG`, `BPF_ADD | BPF_FETCH`), leading to silent lock acquisition successes and potential information leaks.
- [High] The ARM64 and s390 BPF JITs fail to properly populate exception table metadata for arena atomic instructions, leaving both `BPF_LOAD_ACQ` (on ARM64) and RMW fetch atomics (on both architectures) broken on unmapped page faults.
--

--- Patch [3]: [PATCH bpf-next v2 3/6] bpf, x86: Fix exception table metadata for arena load-acquire ---
commit 45e0ae10aa75994903c64f22211117edeb408932
Author: Daniel Borkmann <[email protected]>

bpf, x86: Fix exception table metadata for arena load-acquire

This commit fixes an issue where load-acquire instructions on arena
pointers were incorrectly treated as stores by the exception table
logic on x86. It ensures the destination register is properly cleared
and the correct address register is reported during a fault.

> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 01e7ce569c1ed..88ed95b2eaa72 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 {
>  					arena_reg = reg2pt_regs[dst_reg];
>  					fixup_reg = DONT_CLEAR;
>  				}

[Severity: High]
This isn't a bug introduced by this patch, but it seems read-modify-write
(RMW) fetch operations like BPF_CMPXCHG, BPF_XCHG, and BPF_ADD | BPF_FETCH
might also be incorrectly handled here.

Since RMW fetch atomics belong to the BPF_STX class and are not LOAD_ACQ,
they incorrectly fall into this else branch in do_jit() where fixup_reg is
set to DONT_CLEAR.

If a BPF program executes a BPF_CMPXCHG on an unmapped arena page, the
exception handler will intercept the fault but leave the destination register
uncleared.

Could this allow a lock acquisition loop checking if the register matches the
expected value to falsely assume the lock was acquired, potentially bypassing
BPF concurrency controls?

Does this also risk leaking the uncleared pointer?


[Severity: High]
This is a pre-existing issue, but similar logic appears to be missing for
ARM64 and s390 JIT implementations for arena atomic instructions.

In arch/arm64/net/bpf_jit_comp.c:add_exception_handler():

	if (BPF_CLASS(insn->code) != BPF_LDX)
		dst_reg = DONT_CLEAR;

Does ARM64 also fail to identify BPF_LOAD_ACQ and RMW atomics as instructions
that require clearing a destination register, treating them as DONT_CLEAR?

This could trigger the exact same bug fixed for x86 in this patch.

Additionally, in arch/s390/net/bpf_jit_comp.c:bpf_jit_probe_atomic_pre():

	if (BPF_MODE(insn->code) != BPF_PROBE_ATOMIC)
		return;

For RMW atomics on s390, the function seems to fail to assign a register to be
cleared, meaning the exception handler does not clear any register upon an
unmapped page fault.

Could this also lead to silent lock acquisition bypass on s390?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3