Re: [PATCH bpf-next 2/2] bpf, parisc: Add support for BPF_SDIV and BPF_SMOD in the parisc32 JIT

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The parisc32 JIT silently miscompiles the BPF_MOVSX (move with sign extension) instruction, treating it as a standard move.
- [High] `emit_call_libgcc_ll()` swaps the high and low 32-bit words of 64-bit arguments when passing them to libgcc helpers.
--

--- Patch [2]: [PATCH bpf-next 2/2] bpf, parisc: Add support for BPF_SDIV and BPF_SMOD in the parisc32 JIT ---
commit ef16e681a3d01d651d08cd8250b0ccd28e9d3647
Author: Nicholas Dudar <main.kalliope@gmail.com>

bpf, parisc: Add support for BPF_SDIV and BPF_SMOD in the parisc32 JIT

emit_alu_r32() and emit_alu_r64() call the unsigned millicode
($$divU/$$remU) and libgcc-style (hppa_div64()/hppa_div64_rem())
divide/remainder helpers for BPF_DIV and BPF_MOD regardless of the
BPF instruction's signedness, so the parisc32 JIT does not implement
signed BPF_SDIV and BPF_SMOD (off == 1). Signed ALU32 and ALU64
div/mod get an unsigned quotient and remainder rather than the
verifier's and the interpreter's signed result for negative operands.

Thread is_signed = (insn->off == 1) through emit_alu_r32()/
emit_alu_r64(), and on the signed path use the $$divI/$$remI signed
millicode routines (32-bit) or the hppa_sdiv64()/hppa_sdiv64_rem()
helpers added in patch 1 (64-bit) instead of their unsigned
counterparts. $$divI and $$remI are extern-declared and exported next
to $$divU/$$remU in arch/parisc/kernel/parisc_ksyms.c.
bpf_do_misc_fixups() rewrites the zero-divisor and INT_MIN/-1 cases out
of the instruction stream before the JIT runs.

> diff --git a/arch/parisc/net/bpf_jit_comp32.c b/arch/parisc/net/bpf_jit_comp32.c
> index 5ff0cf925fe95..eea0a34a3bfee 100644
> --- a/arch/parisc/net/bpf_jit_comp32.c
> +++ b/arch/parisc/net/bpf_jit_comp32.c
> @@ -562,12 +564,14 @@ static void emit_alu_i32(const s8 *dst, s32 imm,
>  }
>  
>  static void emit_alu_r64(const s8 *dst, const s8 *src,
> -			 struct hppa_jit_context *ctx, const u8 op)
> +			 struct hppa_jit_context *ctx, const u8 op,
> +			 bool is_signed)
>  {
>  	const s8 *tmp1 = regmap[TMP_REG_1];
>  	const s8 *tmp2 = regmap[TMP_REG_2];
>  	const s8 *rd;
>  	const s8 *rs = bpf_get_reg64(src, tmp2, ctx);
> +	void *func;
>  
>  	if (op == BPF_MOV)
>  		rd = bpf_get_reg64_ref(dst, tmp1, false, ctx);
> @@ -604,10 +608,12 @@ static void emit_alu_r64(const s8 *dst, const s8 *src,
>  		emit_call_libgcc_ll(__muldi3, rd, rs, op, ctx);
>  		break;
>  	case BPF_DIV:
> -		emit_call_libgcc_ll(&hppa_div64, rd, rs, op, ctx);
> +		func = is_signed ? &hppa_sdiv64 : &hppa_div64;
> +		emit_call_libgcc_ll(func, rd, rs, op, ctx);

[Severity: High]
This is a pre-existing issue, but does emit_call_libgcc_ll() swap the high
and low 32-bit words of 64-bit arguments when passing them to libgcc helpers?

For example, when calling hppa_sdiv64, emit_call_libgcc_ll() places the least
significant word (lo) into the first argument register (HPPA_REG_ARG0) and the
most significant word (hi) into the second (HPPA_REG_ARG1):

arch/parisc/net/bpf_jit_comp32.c:emit_call_libgcc_ll() {
    ...
    emit_hppa_copy(lo(arg0), HPPA_REG_ARG0, ctx);
    emit_hppa_copy(hi(arg0), HPPA_REG_ARG1, ctx);
    emit_hppa_copy(lo(arg1), HPPA_REG_ARG2, ctx);
    emit_hppa_copy(hi(arg1), HPPA_REG_ARG3, ctx);
    ...
}

Since PA-RISC is Big Endian, shouldn't the calling convention expect the most
significant word in the first register? This seems like it would cause 64-bit
ALU operations relying on libgcc helpers to compute entirely incorrect results.

>  		break;
>  	case BPF_MOD:
> -		emit_call_libgcc_ll(&hppa_div64_rem, rd, rs, op, ctx);
> +		func = is_signed ? &hppa_sdiv64_rem : &hppa_div64_rem;
> +		emit_call_libgcc_ll(func, rd, rs, op, ctx);
>  		break;

[ ... ]

> @@ -1163,12 +1172,12 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct hppa_jit_context *ctx,
>  			emit_imm32(tmp2, imm, ctx);
>  			src = tmp2;
>  		}
> -		emit_alu_r64(dst, src, ctx, BPF_OP(code));
> +		emit_alu_r64(dst, src, ctx, BPF_OP(code), off == 1);

[Severity: High]
This is a pre-existing issue, but does emit_alu_r64() silently miscompile
BPF_MOVSX (move with sign extension)?

When bpf_jit_emit_insn() intercepts BPF_ALU64 | BPF_MOV | BPF_X and passes it
to emit_alu_r64(), it does not validate if the offset is 8, 16, or 32 for the
sign extension. Then in emit_alu_r64() for BPF_MOV, it performs a simple copy:

arch/parisc/net/bpf_jit_comp32.c:emit_alu_r64() {
    ...
    case BPF_MOV:
        emit_hppa_copy(lo(rs), lo(rd), ctx);
        emit_hppa_copy(hi(rs), hi(rd), ctx);
        break;
    ...
}

Because bpf_jit_emit_insn() returns 0 (success) instead of rejecting the
unsupported instruction, the BPF core does not fall back to the interpreter.
Could this lead to silent data corruption when a BPF program relies on sign
extension, especially if the verifier assumes the extension has occurred for
bounds checking?

>  		break;
>  
>  	/* dst = -dst */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717190544.257306-1-main.kalliope@gmail.com?part=2