Re: [PATCH v12 1/8] powerpc/bpf: fix buffer overflow in JIT for large BPF programs

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

Pre-existing issues:
- [High] bpf_jit_binary_pack_finalize leaks modmem charge and uses uninitialized size on error path.
--

--- Patch [1]: [PATCH v12 1/8] powerpc/bpf: fix buffer overflow in JIT for large BPF programs ---
commit 86d46c787bf7b94485f88f3d4dc3220927360140
Author: Abhishek Dubey <[email protected]>

powerpc/bpf: fix buffer overflow in JIT for large BPF programs

This patch fixes a buffer overflow in the PowerPC BPF JIT compiler
caused by incorrect handling of exit_addr during pass-0 size calculation.
It ensures the epilogue instructions are conservatively accounted for
in the initial allocation and iterates the code-generation loop until
the program size converges.

Link: https://lore.kernel.org/bpf/[email protected]/T/#mfcb23909d977b949727cca4f59ee56a13fd69b92

> diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
> index 7b07b43575f11..8bee2674d4a90 100644
> --- a/arch/powerpc/net/bpf_jit_comp.c
> +++ b/arch/powerpc/net/bpf_jit_comp.c

[ ... ]

> @@ -306,10 +306,13 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
>  	code_base = (u32 *)(image + FUNCTION_DESCR_SIZE);
>  	fcode_base = (u32 *)(fimage + FUNCTION_DESCR_SIZE);
>  
> -	/* Code generation passes 1-2 */
> -	for (pass = 1; pass < 3; pass++) {
> +	/* Code generation passes 1-2+, loop until program size converges. */
> +	for (pass = 1; pass <= CODEGEN_MAX_PASSES; pass++) {
> +		u32 prev_proglen = proglen;
> +
>  		/* Now build the prologue, body code & epilogue for real. */
>  		cgctx.idx = 0;
> +		cgctx.exentry_idx = 0;
>  		cgctx.alt_exit_addr = 0;
>  		bpf_jit_build_prologue(code_base, &cgctx);
>  		if (bpf_jit_build_body(fp, code_base, fcode_base, &cgctx, addrs, pass,
> @@ -318,11 +321,26 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
>  			bpf_jit_binary_pack_free(fhdr, hdr);
>  			goto out_err;
>  		}
> +		addrs[fp->len] = cgctx.idx * 4;
>  		bpf_jit_build_epilogue(code_base, &cgctx);
>  
> +		proglen = cgctx.idx * 4;
> +
>  		if (bpf_jit_enable > 1)
>  			pr_info("Pass %d: shrink = %d, seen = 0x%x\n", pass,
> -				proglen - (cgctx.idx * 4), cgctx.seen);
> +				prev_proglen - proglen, cgctx.seen);
> +
> +		/* Check if program size has converged, but ensure minimum passes */
> +		if (pass >= CODEGEN_MIN_PASSES && proglen == prev_proglen)
> +			break;
> +
> +		if (pass == CODEGEN_MAX_PASSES && proglen != prev_proglen) {
> +			pr_err("BPF JIT: Program did not converge after %d passes\n",
> +								CODEGEN_MAX_PASSES);
> +			bpf_arch_text_copy(&fhdr->size, &hdr->size, sizeof(hdr->size));
> +			bpf_jit_binary_pack_free(fhdr, hdr);
> +			goto out_err;
> +		}

[Severity: High]
This isn't a bug introduced by this patch, but since we are touching the JIT
error paths here, I noticed a potential leak in bpf_jit_binary_pack_finalize().

Does the error path in kernel/bpf/core.c:bpf_jit_binary_pack_finalize() leak
the global JIT memory charge?

When bpf_arch_text_copy() fails, the error path cleans up the memory
allocations but appears to completely omit the corresponding
bpf_jit_uncharge_modmem() call:

kernel/bpf/core.c:bpf_jit_binary_pack_finalize() {
	...
	kvfree(rw_header);

	if (IS_ERR(ptr)) {
		bpf_prog_pack_free(ro_header, ro_header->size);
		return PTR_ERR(ptr);
	}
	...
}

Additionally, could ro_header->size be uninitialized if bpf_arch_text_copy()
fails early, potentially causing bpf_prog_pack_free() to corrupt the
pack bitmap?

>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1