Re: [PATCH v11 8/8] powerpc64/bpf: fix percpu private stack leak on JIT failure

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

Pre-existing issues:
- [Critical] In `bpf_jit_binary_pack_finalize()`, if `bpf_arch_text_copy()` fails, it uses uninitialized `ro_header->size` to free the pack (causing bitmap corruption) and fails to uncharge modmem.
- [Critical] If `bpf_jit_binary_pack_finalize()` fails during `bpf_jit_free()`, the JIT memory pack is double-freed.
- [High] The ARM64 BPF JIT compiler has the exact same percpu private stack memory leak on failure paths that this patch fixes for PowerPC.
--

--- Patch [8]: [PATCH  v11 8/8] powerpc64/bpf: fix percpu private stack leak on JIT failure ---
commit e1be4be6e638db0714dd1f4ef6a5f98e6f888db6
Author: Abhishek Dubey <[email protected]>

powerpc64/bpf: fix percpu private stack leak on JIT failure

The existing conditional statement in bpf_int_jit_compile() frees the
percpu private stack at out_addrs only when the image buffer was never
allocated.

Closes: https://lore.kernel.org/bpf/[email protected]

> diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
> index 2cc1124ed5303..395f1458db2e6 100644
> --- a/arch/powerpc/net/bpf_jit_comp.c
> +++ b/arch/powerpc/net/bpf_jit_comp.c
> @@ -404,7 +404,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
>  				(void *)fimage + FUNCTION_DESCR_SIZE);
>  
>  out_addrs:
> -		if (!image && priv_stack_ptr) {
> +		if (!fp->jited && priv_stack_ptr) {
>  			fp->aux->priv_stack_ptr = NULL;
>  			free_percpu(priv_stack_ptr);
>  		}

[Severity: High]
This isn't a bug introduced by this patch, but the ARM64 BPF JIT compiler
has the exact same percpu private stack memory leak on failure paths that
this patch addresses for PowerPC.

In arch/arm64/net/bpf_jit_comp.c:bpf_int_jit_compile():

    out_off:
		if (!ro_header && priv_stack_ptr) {
			free_percpu(priv_stack_ptr);
			prog->aux->priv_stack_ptr = NULL;
		}

On failure paths jumping from out_free_hdr, ro_header is not NULL, causing
the percpu stack free to be skipped. Could this also exhaust percpu memory
on ARM64?

[Severity: Critical]
This is a pre-existing issue, but if bpf_jit_binary_pack_finalize() fails
during bpf_jit_free(), does this double-free the JIT memory pack?

In arch/powerpc/net/bpf_jit_comp.c:bpf_jit_free():

		if (jit_data) {
			bpf_jit_binary_pack_finalize(jit_data->fhdr,
                                                     jit_data->hdr);
			kvfree(jit_data->addrs);
			kfree(jit_data);
		}
		hdr = bpf_jit_binary_pack_hdr(fp);
		bpf_jit_binary_pack_free(hdr, NULL);

If jit_subprogs() aborts early and calls bpf_jit_free() on subprograms,
bpf_jit_binary_pack_finalize() is called. If its internal text copy fails,
it internally frees ro_header. However, the return value of
bpf_jit_binary_pack_finalize() is ignored here, and bpf_jit_binary_pack_free()
is called again, which seems to double-free the same pack structure.

The same pattern appears to exist in arch/x86/net/bpf_jit_comp.c:bpf_jit_free()
as well.

[Severity: Critical]
This is a pre-existing issue, but does bpf_jit_binary_pack_finalize() use
uninitialized memory if bpf_arch_text_copy() fails?

In kernel/bpf/core.c:bpf_jit_binary_pack_finalize():

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

If bpf_arch_text_copy() fails, ro_header->size has not been initialized.
Could this clear arbitrary memory regions in the shared pack allocator bitmap
and permanently leak the module memory charge since bpf_jit_uncharge_modmem()
is missing on this error path?

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