Re: [PATCH v3] bpf: Fix UAF in bpf_trampoline_multi_attach_free on update failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Ignoring errors during BPF link detachment leads to a Use-After-Free of the `bpf_prog` when ftrace or trampoline updates fail.
- [Critical] The patch is incomplete: it fixes a UAF of the trampoline image when `bpf_trampoline_update()` fails, but leaves an identical UAF if ftrace direct modifications fail during multi-detach.
--

--- Patch [1]: [PATCH v3] bpf: Fix UAF in bpf_trampoline_multi_attach_free on update failure ---
commit 569677dafd5f99230e4a9c93925fbe240c10aadd
Author: Hui Zhu <zhuhui@kylinos.cn>
bpf: Fix UAF in bpf_trampoline_multi_attach_free on update failure

This patch modifies bpf_trampoline_multi_attach_free() to only free
old_image if it differs from cur_image, aiming to prevent a use-after-free 
when bpf_trampoline_update() fails. It also changes 
bpf_trampoline_multi_detach() to return void.

> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index e07af35ed0402..b76d162517e36 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
[ ... ]
> @@ -1632,7 +1632,18 @@ static void bpf_trampoline_multi_attach_init(struct bpf_trampoline *tr)
>  
>  static void bpf_trampoline_multi_attach_free(struct bpf_trampoline *tr)
>  {
> -	if (tr->multi_attach.old_image)
> +	/*
> +	 * Only free old_image if it is no longer the active image.
> +	 * When bpf_trampoline_update() fails before modify_fentry_multi()/
> +	 * unregister_fentry_multi() is called, cur_image is unchanged
> +	 * (cur_image == old_image) and ftrace still points to it. Freeing
> +	 * it would cause a UAF when ftrace calls into the freed memory.
> +	 * On success, cur_image is either a new image or NULL, so
> +	 * old_image != cur_image correctly identifies a stale image that
> +	 * is safe to free.
> +	 */
> +	if (tr->multi_attach.old_image &&
> +	    tr->multi_attach.old_image != tr->cur_image)
>  		bpf_tramp_image_put(tr->multi_attach.old_image);

[Severity: Critical]
This is a pre-existing issue, but does this logic leave another window for
a use-after-free if ftrace modifications fail?

During bpf_trampoline_multi_detach(), if __bpf_trampoline_unlink_prog()
succeeds, it updates tr->cur_image to a new image or NULL. If a subsequent 
call to update ftrace fails (for example, a memory allocation failure inside 
update_ftrace_direct_del()), ftrace is not updated and continues to point 
to old_image.

Because tr->cur_image was updated, the condition here evaluates to true. 
This appears to erroneously free old_image while ftrace is still using it, 
potentially causing a crash when the traced function is executed.

[ ... ]
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 891897f8a1b35..29260951aa871 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -3687,7 +3687,7 @@ static void bpf_tracing_multi_link_release(struct bpf_link *link)
>  	struct bpf_tracing_multi_link *tr_link =
>  		container_of(link, struct bpf_tracing_multi_link, link);
>  
> -	WARN_ON_ONCE(bpf_trampoline_multi_detach(link->prog, tr_link));
> +	bpf_trampoline_multi_detach(link->prog, tr_link);
>  }

[Severity: Critical]
This is a pre-existing issue, but by treating detachment as always
successful here, do we risk a use-after-free of the bpf_prog itself?

If __bpf_trampoline_unlink_prog() fails inside bpf_trampoline_multi_detach(),
the error is logged with a warning, but execution continues. In this scenario, 
the active trampoline image still contains a jump to the BPF program.

Since bpf_tracing_multi_link_release() proceeds to destroy the bpf_link 
and drops the final reference to the bpf_prog, the program memory is freed. 
However, the ftrace hook remains active and points to the freed bpf_prog, 
which can trigger a crash when the traced function is called.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260810022304.211544-1-hui.zhu@linux.dev?part=1