Re: [PATCH bpf-next 1/4] bpf: Inline bpf_iter_num_new() kfunc

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The inlined `bpf_iter_num_new` emits CPUv4 `BPF_MOVSX64_REG` instructions during the post-verification fixup pass. On architectures whose JIT compilers do not natively support CPUv4 (e.g. x86-32, MIPS32), `BPF_MOVSX` is miscompiled as a standard zero-extending 64-bit `MOV` because they ignore `insn->off`. This breaks the iterator's loop bounds check and causes a denial of service (unbounded BPF loop).
--

--- Patch [1]: [PATCH bpf-next 1/4] bpf: Inline bpf_iter_num_new() kfunc ---
commit a284b42200c5d5b485b4d27a28c08c05022dfa70
Author: Puranjay Mohan <puranjay@kernel.org>

bpf: Inline bpf_iter_num_new() kfunc
    
The numeric iterator kfuncs bpf_iter_num_{new,next,destroy}() back the
open-coded iterator macro bpf_for() and are emitted as regular kfunc
calls by the verifier. bpf_iter_num_new() is small and only touches the
on-stack iterator state, so the verifier can open-code it and avoid the
call overhead of setting up an iterator.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index de816063ae63d..f578cebbade4f 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -19715,6 +19715,40 @@ static void __fixup_collection_insert_kfunc(struct bpf_insn_aux_data *insn_aux,
>  	*cnt = 4;
>  }
>  
> +/*
> + * Inline bpf_iter_num_new(). R1 holds the pointer to the iterator, R2 and R3
> + * hold the (int) start and end arguments. Keep in sync with the kfunc in
> + * kernel/bpf/bpf_iter.c.
> + */
> +static int inline_bpf_iter_num_new(struct bpf_insn *insn_buf)
> +{
> +	/* if (start > end) goto einval; */
> +	insn_buf[0] = BPF_JMP32_REG(BPF_JSGT, BPF_REG_2, BPF_REG_3, 9);
> +	/* r0 = (s64)end - (s64)start; */
> +	insn_buf[1] = BPF_MOVSX64_REG(BPF_REG_0, BPF_REG_3, 32);
> +	insn_buf[2] = BPF_MOVSX64_REG(BPF_REG_4, BPF_REG_2, 32);

[Severity: High]
Does this code emit CPUv4 instructions (BPF_MOVSX64_REG) into the instruction
stream post-verification?

Since inline_bpf_iter_num_new() is called from bpf_fixup_kfunc_call() after
do_check() has completed, the verifier will not reject these instructions on
older architectures.

On architectures whose JIT compilers do not natively support CPUv4 (such as
x86-32 or MIPS32), BPF_MOVSX is miscompiled as a standard zero-extending
64-bit MOV because they ignore insn->off.

If start is a negative number and end is positive, start will be passed
zero-extended in the 64-bit register. A normal 64-bit MOV fails to sign-extend
this, and the subsequent subtraction r0 = end - start yields a large negative
64-bit integer instead of the actual positive distance.

> +	insn_buf[3] = BPF_ALU64_REG(BPF_SUB, BPF_REG_0, BPF_REG_4);
> +	/* if (r0 > BPF_MAX_LOOPS) goto e2big; */
> +	insn_buf[4] = BPF_JMP_IMM(BPF_JSGT, BPF_REG_0, BPF_MAX_LOOPS, 8);

[Severity: High]
Could the signed comparison BPF_JSGT fail to enforce the loop bounds if the
subtraction yields a large negative number on older JITs?

If the distance calculation evaluates as a negative 64-bit integer, this check
would evaluate to false, bypassing the BPF_MAX_LOOPS limit. This could allow
a BPF program to loop up to 4.29 billion times, potentially hanging the
kernel.

Since the if (start > end) check already ensures end >= start, would it be
safer to use standard 32-bit arithmetic? A 32-bit unsigned subtraction
(u32)end - (u32)start evaluates to the exact distance and correctly
zero-extends into the 64-bit register for the subsequent BPF_MAX_LOOPS check.

> +	/* s->cur = start - 1; */
> +	insn_buf[5] = BPF_ALU32_IMM(BPF_ADD, BPF_REG_2, -1);
> +	insn_buf[6] = BPF_STX_MEM(BPF_W, BPF_REG_1, BPF_REG_2, 0);
> +	/* s->end = end; */
> +	insn_buf[7] = BPF_STX_MEM(BPF_W, BPF_REG_1, BPF_REG_3, 4);
> +	/* return 0; */
> +	insn_buf[8] = BPF_MOV64_IMM(BPF_REG_0, 0);
> +	insn_buf[9] = BPF_JMP_A(5);
> +	/* einval: s->cur = s->end = 0; return -EINVAL; */
> +	insn_buf[10] = BPF_ST_MEM(BPF_DW, BPF_REG_1, 0, 0);
> +	insn_buf[11] = BPF_MOV64_IMM(BPF_REG_0, -EINVAL);
> +	insn_buf[12] = BPF_JMP_A(2);
> +	/* e2big: s->cur = s->end = 0; return -E2BIG; */
> +	insn_buf[13] = BPF_ST_MEM(BPF_DW, BPF_REG_1, 0, 0);
> +	insn_buf[14] = BPF_MOV64_IMM(BPF_REG_0, -E2BIG);
> +
> +	return 15;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715130430.318421-1-puranjay@kernel.org?part=1