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

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

Pre-existing issues:
- [High] Incomplete fix: ARM64 load-acquire and all architectures' RMW atomics fail to clear the return register on fault
--

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

A load-acquire from an arena pointer gets an exception table entry, but the
entry is filled in as if it were a store. This patch uses
bpf_atomic_is_load_acq() so a load-acquire correctly takes the load path.

Cc: Peilin Ye <[email protected]>

> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c

[ ... ]

>  				 * 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 could this fix be missing other
RMW atomics on x86, as well as leaving similar vulnerabilities on ARM64 and
PowerPC?

For x86_64, RMW atomics like BPF_XCHG, BPF_CMPXCHG, and BPF_FETCH ops return a
value that must be cleared, but they seem to fall into the else branch in
populate_extable() and get fixup_reg = DONT_CLEAR. If a fault occurs on an
unmapped arena address, the exception handler skips the instruction without
clearing the return register. Since the verifier treats the result of these
atomic operations as an unconstrained scalar, a BPF program could stash a kernel
pointer in the return register beforehand, then leak it to userspace after the
fault.

Additionally, this issue appears to exist on other architectures.

On ARM64, add_exception_handler() in arch/arm64/net/bpf_jit_comp.c assumes all
BPF_STX instructions are stores, failing to clear dst_reg for load_acq and other
returning atomics:

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

On PowerPC, bpf_add_extable_entry() in arch/powerpc/net/bpf_jit_comp.c emits a
NOP fixup instead of clearing the destination register for any BPF_STX
instruction:

    fixup[0] = PPC_RAW_LI(dst_reg, 0);
    if (BPF_CLASS(code) == BPF_ST || BPF_CLASS(code) == BPF_STX)
        fixup[0] = PPC_RAW_NOP();

While load_acq might be unsupported on PowerPC arena pointers, value-returning
atomics like xchg and cmpxchg are supported and vulnerable to the same leak.

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