Re: [PATCH bpf-next v2 4/8] bpf, x86: refactor BPF_ST management in do_jit

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Improper break scope in nested switch causes JIT to emit malformed instructions for BPF_ST with BPF_DW targeting the 6th argument.
--

commit 4f81840cd1222105beefd9a592e593df3c32c344
Author: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>

bpf, x86: refactor BPF_ST management in do_jit

This commit refactors BPF_ST instruction generation in the x86 JIT by
grouping memory access operations under a single fall-through block and
utilizing an inner switch on the BPF instruction size. This prepares the
code for adding KASAN checks.

> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 0981791014eb3..943a0f315cf2b 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
[ ... ]
> @@ -2300,41 +2300,50 @@ static int do_jit(struct bpf_verifier_env *env, struct bpf_prog *bpf_prog, int *
[ ... ]
>  		case BPF_ST | BPF_MEM | BPF_DW:
> -			if (dst_reg == BPF_REG_PARAMS && insn->off == -8) {
> -				/* Arg 6: store immediate in r9 register */
> -				emit_mov_imm64(&prog, X86_REG_R9, imm32 >> 31, (u32)imm32);
> +			switch (BPF_SIZE(insn->code)) {
[ ... ]
> +			case BPF_DW:
> +				if (dst_reg == BPF_REG_PARAMS &&
> +				    insn->off == -8) {
> +					/* Arg 6: store immediate in r9 register */
> +					emit_mov_imm64(&prog, X86_REG_R9,
> +						       imm32 >> 31, (u32)imm32);
> +					break;

[Severity: Critical]
Does this break statement now only exit the newly introduced inner switch
rather than the outer switch?

In the original code, this break would exit the outer switch entirely and
skip the remainder of the BPF_ST logic. Now that it only breaks the inner
switch, execution appears to fall through to the common suffix below:

    insn_off = insn->off;
    ...
    EMIT(imm32, bpf_size_to_x86_bytes(BPF_SIZE(insn->code)));

Could this cause the JIT to incorrectly emit the user-controlled imm32 value
directly into the executable instruction stream as raw x86 instructions,
potentially leading to a kernel crash or arbitrary code execution?

> +				}
> +				EMIT2(add_1mod(0x48, dst_reg), 0xC7);
>  				break;
>  			}
>  
> -st:			insn_off = insn->off;
> +			insn_off = insn->off;
>  			if (dst_reg == BPF_REG_PARAMS) {
>  				/*
>  				 * Args 7+: reverse BPF negative offsets to
>  				 * x86 positive rsp offsets.
>  				 * BPF off=-16 → [rsp+0], off=-24 → [rsp+8], ...
>  				 */
> -				insn_off = outgoing_arg_base - outgoing_rsp - insn_off - 16;
> +				insn_off = outgoing_arg_base - outgoing_rsp -
> +					   insn_off - 16;
>  				dst_reg = BPF_REG_FP;
>  			}
>  			if (is_imm8(insn_off))

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604-kasan-v2-0-c066e627fda8@bootlin.com?part=4