Re: [RFC PATCH bpf-next v1 02/11] bpf, arm64: Let the pad after a helper call have r0

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] bpf, arm64: memory leak of ctx.landing_ex in bpf_jit_free()
--

commit a9bbc452976e17e7771ffe7fb013d30ad6f17aba
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf, arm64: Let the pad after a helper call have r0

This commit adds landing pads after the epilogue for unwind_goto instructions
that follow helper or kfunc calls on arm64, copying the return value from x0
into x8 before branching to the pad. The extable fixup entries for these
calls are updated during a post-epilogue pass to point to the generated
landings.

> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> index b8fb49b00063c..284c1dd791ad7 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c

[ ... ]

> @@ -2387,6 +2447,16 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
>  	if (ctx.offset == NULL)
>  		goto out_off;
>  
> +	for (i = 1; i < prog->len; i++)
> +		if (bpf_is_unwind_goto(&prog->insnsi[i]) &&
> +		    unwind_goto_after_helper(&prog->insnsi[i]))
> +			nr_landings++;
> +	if (nr_landings) {
> +		ctx.landing_ex = kvzalloc_objs(int, nr_landings);
> +		if (ctx.landing_ex == NULL)
> +			goto out_off;
> +	}

[ ... ]

> @@ -2533,6 +2607,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
>  			prog->aux->priv_stack_ptr = NULL;
>  		}
>  		kvfree(ctx.offset);
> +		kvfree(ctx.landing_ex);
>  out_priv_stack:
>  		kfree(jit_data);
>  		prog->aux->jit_data = NULL;

[Severity: Medium]
Can this leak ctx.landing_ex if multi-function JIT compilation fails between
passes?

During the first pass of jit_subprogs() -> bpf_int_jit_compile() for a
subprogram (where prog->is_func is true and extra_pass is false), ctx is saved
in prog->aux->jit_data and the cleanup block is skipped so that ctx.landing_ex
and ctx.offset remain available for the second pass:

arch/arm64/net/bpf_jit_comp.c:bpf_int_jit_compile() {
    ...
	if (!prog->is_func || extra_pass) {
		...
	} else {
		jit_data->ctx = ctx;
		jit_data->ro_image = ro_image_ptr;
		jit_data->header = header;
		jit_data->ro_header = ro_header;
	}

	prog->bpf_func = (void *)ctx.ro_image + cfi_get_offset();
	prog->jited = 1;
	prog->jited_len = prog_size - cfi_get_offset();

	if (!prog->is_func || extra_pass) {
		...
		kvfree(ctx.offset);
		kvfree(ctx.landing_ex);
out_priv_stack:
		kfree(jit_data);
		prog->aux->jit_data = NULL;
	}
    ...
}

If a subsequent subprogram fails its initial JIT pass in jit_subprogs(), or if
an earlier subprogram fails during the extra_pass loop, jit_subprogs() jumps
to out_free and calls bpf_jit_free() on each subprogram. However,
bpf_jit_free() frees jit_data without freeing jit_data->ctx.landing_ex (or the
pre-existing jit_data->ctx.offset):

arch/arm64/net/bpf_jit_comp.c:bpf_jit_free() {
	if (prog->jited) {
		struct arm64_jit_data *jit_data = prog->aux->jit_data;
		...
		if (jit_data) {
			bpf_jit_binary_pack_finalize(jit_data->ro_header, jit_data->header);
			kfree(jit_data);
		}
    ...
}

Should bpf_jit_free() also call kvfree(jit_data->ctx.landing_ex) and
kvfree(jit_data->ctx.offset) before freeing jit_data?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793147364.git.memxor@gmail.com?part=2