Re: [PATCH bpf-next v2 07/11] LoongArch: BPF: Support atomics on arena pointers

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Faulting arena atomic operations fail to clear their destination registers, defeating the verifier's zero-extension tracking and enabling arbitrary out-of-bounds memory access.
- [High] Missing exception table entry for the `sc` (Store Conditional) instruction in `BPF_PROBE_ATOMIC` `cmpxchg` allows trivial kernel panics.

Pre-existing issues:
- [High] LoongArch's `ex_handler_bpf` does not report arena violations, breaking standard BPF arena semantics.
--

commit 445332124b454806c0eb995caa9dad9e7668feba
Author: George Guo <guodongtai@kylinos.cn>

LoongArch: BPF: Support atomics on arena pointers

This commit implements atomic operations on arena pointers (BPF_PROBE_ATOMIC)
for LoongArch. It adds exception table entries to handle page faults for
these memory accesses.

> diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
> index 4a3b632c1fde5..a7f2d45aef751 100644
> --- a/arch/loongarch/net/bpf_jit.c
> +++ b/arch/loongarch/net/bpf_jit.c

[ ... ]

> @@ -585,12 +600,14 @@ static int emit_atomic_rmw(const struct bpf_insn *insn, struct jit_ctx *ctx)
>  	case BPF_CMPXCHG:
>  		move_reg(ctx, t2, r0);
>  		if (isdw) {
> +			ll_idx = ctx->idx;
>  			emit_insn(ctx, lld, r0, t1, 0);
>  			emit_insn(ctx, bne, t2, r0, 4);
>  			move_reg(ctx, t3, src);
>  			emit_insn(ctx, scd, t3, t1, 0);

[Severity: High]
Does this Store Conditional (scd) instruction need an exception table entry?
If a BPF program executes a BPF_CMPXCHG operation on a read-only or 
Copy-On-Write (COW) arena page, the Load Linked (lld) instruction will
succeed because it is a read, but the subsequent Store Conditional
instruction could trigger a page fault due to the write violation. Since
there is no exception table entry registered for this scd instruction,
would this result in an unhandled kernel page fault oops?

>  			emit_insn(ctx, beq, t3, LOONGARCH_GPR_ZERO, -4);
>  		} else {

[ ... ]

> @@ -600,12 +617,42 @@ static int emit_atomic_rmw(const struct bpf_insn *insn, struct jit_ctx *ctx)
>  			emit_insn(ctx, beq, t3, LOONGARCH_GPR_ZERO, -6);
>  			emit_zext_32(ctx, r0, true);
>  		}
> +		/*
> +		 * On arena the ll may fault (unmapped page); the page-fault
> +		 * handler restarts the program at @resume.  Only the ll needs an
> +		 * entry: if it faults the sc is never reached, and once the ll
> +		 * succeeds the page is mapped so the sc cannot fault.  Resume
> +		 * past the whole ll/sc loop.
> +		 */
> +		if (arena) {
> +			ret = __add_exception_handler(insn, ctx,
> +						      REG_DONT_CLEAR_MARKER,
> +						      ll_idx, ctx->idx);

[Severity: Critical]
Should the actual destination register (r0) be cleared here instead of
using REG_DONT_CLEAR_MARKER? If a fault occurs on the arena pointer, the
destination register retains its old value instead of being cleared.
Because the verifier assumes the register has been bounded (e.g., upper
32-bits zeroed), leaving the hardware register with an unbounded value
might allow out-of-bounds memory accesses.

> +			if (ret)
> +				return ret;
> +		}
>  		break;
>  	default:
>  		pr_err_once("bpf-jit: invalid atomic read-modify-write opcode %02x\n", imm);
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * For the single-instruction am* ops the memory access is the last
> +	 * emitted instruction; register its exception entry before emitting the
> +	 * deferred zero-extend so the fault resumes past it.  cmpxchg handled
> +	 * its own entry above.
> +	 */
> +	if (arena && imm != BPF_CMPXCHG) {
> +		ret = __add_exception_handler(insn, ctx, REG_DONT_CLEAR_MARKER,
> +					      ctx->idx - 1, ctx->idx + (zext ? 1 : 0));

[Severity: Critical]
Is it safe to pass REG_DONT_CLEAR_MARKER for fetch atomics? Similar to
the cmpxchg case, if an exception occurs on an invalid arena pointer,
the source register is not cleared. This can cause a divergence from
the verifier's zero extension tracking and potentially allow out-of-bounds
memory accesses.

> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (zext)
> +		emit_zext_32(ctx, src, true);
> +
>  	return 0;
>  }

[ ... ]

> @@ -616,10 +663,37 @@ static int emit_atomic_ld_st(const struct bpf_insn *insn, struct jit_ctx *ctx)
>  	const u8 dst = regmap[insn->dst_reg];
>  	const s16 off = insn->off;
>  	const s32 imm = insn->imm;
> +	const bool arena = BPF_MODE(insn->code) == BPF_PROBE_ATOMIC;
> +	int ret;
>  
>  	switch (imm) {
>  	/* dst_reg = load_acquire(src_reg + off16) */
>  	case BPF_LOAD_ACQ:
> +		if (arena) {
> +			/* t1 = src + off + arena_vm_start; load from [t1]. */
> +			move_imm(ctx, t1, off, false);
> +			emit_insn(ctx, addd, t1, src, t1);
> +			emit_insn(ctx, addd, t1, t1, REG_ARENA);
> +			switch (BPF_SIZE(insn->code)) {
> +			case BPF_B:
> +				emit_insn(ctx, ldbu, dst, t1, 0);
> +				break;
> +			case BPF_H:
> +				emit_insn(ctx, ldhu, dst, t1, 0);
> +				break;
> +			case BPF_W:
> +				emit_insn(ctx, ldwu, dst, t1, 0);
> +				break;
> +			case BPF_DW:
> +				emit_insn(ctx, ldd, dst, t1, 0);
> +				break;
> +			}
> +			ret = add_exception_handler(insn, ctx, REG_DONT_CLEAR_MARKER);

[Severity: Critical]
Could we pass the actual destination register rather than
REG_DONT_CLEAR_MARKER here? If the load acquisition faults on an unmapped
arena pointer, the destination register will not be cleared. The verifier
expects it to be cleared or bounded, which could allow out-of-bounds
memory reads if the register keeps its prior unbounded value.

> +			if (ret)
> +				return ret;
> +			emit_insn(ctx, dbar, 0b10100);
> +			break;
> +		}

[ ... ]

> @@ -708,10 +807,6 @@ static bool is_signed_bpf_cond(u8 cond)
>  	       cond == BPF_JSGE || cond == BPF_JSLE;
>  }
>  
> -#define BPF_FIXUP_REG_MASK	GENMASK(31, 27)
> -#define BPF_FIXUP_OFFSET_MASK	GENMASK(26, 0)
> -#define REG_DONT_CLEAR_MARKER	0
> -
>  bool ex_handler_bpf(const struct exception_table_entry *ex,
>  		    struct pt_regs *regs)
>  {
> @@ -725,12 +820,21 @@ bool ex_handler_bpf(const struct exception_table_entry *ex,
>  	return true;
>  }

[Severity: High]
This is a pre-existing issue, but does ex_handler_bpf() need to report
arena violations to user space? If a BPF program accesses an out-of-bounds
arena pointer (such as the guard bands), this handler clears the
destination register and resumes execution without delivering a SIGSEGV.
Standard BPF arena semantics usually require calling
bpf_prog_report_arena_violation() in this path to ensure the user space
process is properly signaled.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702022322.51033-1-dongtai.guo@linux.dev?part=7