Re: [RFC bpf-next 1/2] bpf, mips: Factor register moves into helpers

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The MIPS BPF JIT silently miscompiles `BPF_MOVSX` (move with sign extension) instructions as standard zero-extended or 64-bit copies.
--

--- Patch [1]: [RFC bpf-next 1/2] bpf, mips: Factor register moves into helpers ---
commit a341e9c2445aa10a781c4264cc9b57117f7c9711
Author: Nicholas Dudar <main.kalliope@gmail.com>

bpf, mips: Factor register moves into helpers

This patch refactors the MIPS BPF JIT by introducing 32-bit and 64-bit
register-move helpers to remove duplicated backend-specific handling for
MOV instructions. It preserves the verifier-inserted zero-extension marker
behavior.

> diff --git a/arch/mips/net/bpf_jit_comp32.c b/arch/mips/net/bpf_jit_comp32.c
> index 40a878b672f5d..bfe73b023983e 100644
> --- a/arch/mips/net/bpf_jit_comp32.c
> +++ b/arch/mips/net/bpf_jit_comp32.c

[ ... ]

> @@ -1485,8 +1501,7 @@ int build_insn(const struct bpf_insn *insn, struct jit_context *ctx)
>  			/* Special mov32 for zext */
>  			emit_mov_i(ctx, hi(dst), 0);
>  		} else {
> -			emit_mov_r(ctx, lo(dst), lo(src));
> -			emit_zext_ver(ctx, dst);
> +			emit_mov_r32(ctx, dst, src);

[Severity: Critical]
This is a pre-existing issue, but does this switch case in build_insn()
silently miscompile BPF_MOVSX (move with sign extension) instructions?

When the BPF program uses a move with sign extension (insn->off != 0), this
code generation appears to perform a standard zero-extended copy instead.
Since the verifier tracks bounds assuming the value was sign-extended,
could this discrepancy allow a program to bypass verifier bounds checks
and perform out-of-bounds memory accesses?

Since you are refactoring the MOV handlers here, should emit_mov_r32() be
updated to take insn->off and implement the proper sign extension logic?

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

[ ... ]

> diff --git a/arch/mips/net/bpf_jit_comp64.c b/arch/mips/net/bpf_jit_comp64.c
> index fa7e9aa37f498..45fee6f6b87e9 100644
> --- a/arch/mips/net/bpf_jit_comp64.c
> +++ b/arch/mips/net/bpf_jit_comp64.c

[ ... ]

> @@ -742,7 +754,7 @@ int build_insn(const struct bpf_insn *insn, struct jit_context *ctx)
>  		break;
>  	/* dst = src (64-bit) */
>  	case BPF_ALU64 | BPF_MOV | BPF_X:
> -		emit_mov_r(ctx, dst, src);
> +		emit_mov_r64(ctx, dst, src);

[Severity: Critical]
This is also a pre-existing issue, but does the 64-bit MOV handler similarly
ignore insn->off in build_insn()?

Like the 32-bit case, if a program uses a BPF_MOVSX instruction, does this
result in a standard 64-bit copy instead of the required sign extension?

>  		break;
>  	/* dst = -dst (64-bit) */
>  	case BPF_ALU64 | BPF_NEG:

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