Re: [PATCH bpf-next 3/6] bpf, x86: Clear fetch destination on faulting arena atomic

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

Pre-existing issues:
- [High] The fix for clearing the fetch destination register on a faulting arena read-modify-write atomic is incomplete and misses arm64 and s390 architectures.
--

--- Patch [3]: [PATCH bpf-next 3/6] bpf, x86: Clear fetch destination on faulting arena atomic ---
commit d051fde5d669db0d464e3f70d0cdcf647b528ab3
Author: Daniel Borkmann <[email protected]>

bpf, x86: Clear fetch destination on faulting arena atomic

This commit fixes an issue on x86 where faulting arena read-modify-write
atomic instructions with a fetch operation failed to clear the destination
register. It introduces a new ARENA_WRITE bit in the exception table
metadata to correctly report the fault direction independently of the
register clearing logic.

> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 8dddb5d7af21b..d920772af7d5f 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c

[Severity: High]
This is a pre-existing issue, but does this same bug also need to be fixed
in the arm64 and s390 JIT implementations?

On arm64, add_exception_handler() hardcodes DONT_CLEAR for all instructions
except BPF_LDX and BPF_LOAD_ACQ, which seems to leave the fetch register
uncleared for faulting BPF_PROBE_ATOMIC instructions:

arch/arm64/net/bpf_jit_comp.c:add_exception_handler() {
	if (BPF_CLASS(insn->code) != BPF_LDX && !bpf_atomic_is_load_acq(insn))
		dst_reg = DONT_CLEAR;

	ex->fixup = FIELD_PREP(BPF_FIXUP_REG_MASK, dst_reg);
}

On s390, bpf_jit_probe_atomic_pre() never sets probe->reg, leaving it
initialized to -1. This seems to cause ex_handler_bpf() to skip clearing any
register:

arch/s390/net/bpf_jit_comp.c:bpf_jit_probe_atomic_pre() {
	if (BPF_MODE(insn->code) != BPF_PROBE_ATOMIC)
		return;

	/* lgrl %r1,kern_arena */
	EMIT6_PCREL_RILB(0xc4080000, REG_W1, jit->kern_arena);
	/* agr %r1,%dst */
	EMIT4(0xb9080000, REG_W1, insn->dst_reg);
	probe->arena_reg = REG_W1;
	probe->prg = jit->prg;
}

Could this result in leaking stale or uninitialized memory to the BPF
program when an arena atomic read-modify-write instruction faults on an
unmapped page on these architectures?

> @@ -2384,15 +2389,29 @@ st:			insn_off = insn->off;
>  				    bpf_atomic_is_load_acq(insn)) {
>  					arena_reg = reg2pt_regs[src_reg];
>  					fixup_reg = reg2pt_regs[dst_reg];
> +					is_write = false;
>  				} else {
> +					/*
> +					 * A store has no destination register to clear,
> +					 * except for a read-modify-write with BPF_FETCH,
> +					 * which also reads the old value into src_reg, or
> +					 * into r0 for a BPF_CMPXCHG. Either way the access
> +					 * is still reported as a write.
> +					 */
> +					int load_reg = bpf_atomic_load_reg(insn);
> +
>  					arena_reg = reg2pt_regs[dst_reg];
> -					fixup_reg = DONT_CLEAR;
> +					fixup_reg = load_reg < 0 ? DONT_CLEAR :
> +						    reg2pt_regs[load_reg];
> +					is_write = true;
>  				}

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