Re: [PATCH bpf-next 2/2] bpf: Fix UAF in bpf_trampoline_multi_detach when ftrace update fails

[Jiri Olsa <olsajiri@gmail.com>]

On Thu, Jul 16, 2026 at 04:43:39PM +0800, Hui Zhu wrote:
> From: Hui Zhu <zhuhui@kylinos.cn>
> 
> After the per-mnode unlink loop, the return values of the batch
> update_ftrace_direct_del()/update_ftrace_direct_mod() calls are only
> WARN_ON_ONCE()'d, not checked. bpf_trampoline_multi_attach_free() is
> then called unconditionally for every mnode, freeing old_image
> whenever the single-point unlink succeeded (old_image != cur_image).
> 
> If the batch update fails, ftrace still points to old_image for the
> affected IPs, so freeing it is a UAF.
> 
> Capture the two return values and, for mnodes whose single-point
> unlink succeeded but whose batch update failed, call
> bpf_trampoline_multi_attach_rollback() instead of
> bpf_trampoline_multi_attach_free(). cur_image == NULL identifies the
> unreg path (set by unregister_fentry_multi when total == 0),
> cur_image != NULL identifies the modify path (set by
> modify_fentry_multi when total > 0), so err_unreg/err_mod can be
> matched to the right mnodes. Rollback restores cur_image = old_image,
> the image ftrace is still actually calling.
> 
> This mirrors the existing error handling in
> bpf_trampoline_multi_attach()'s rollback_unlink path.
> 
> Fixes: aef4dfa790b2 ("bpf: Add bpf_trampoline_multi_attach/detach functions")
> Signed-off-by: Hui Zhu <zhuhui@kylinos.cn>
> ---
>  kernel/bpf/trampoline.c | 42 ++++++++++++++++++++++++++++++++++-------
>  1 file changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index a78fbf726fad..44ef25beb4cc 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -1723,7 +1723,7 @@ int bpf_trampoline_multi_detach(struct bpf_prog *prog, struct bpf_tracing_multi_
>  {
>  	struct bpf_tracing_multi_data *data = &link->data;
>  	struct bpf_tracing_multi_node *mnode;
> -	int i;
> +	int i, err_unreg = 0, err_mod = 0;
>  
>  	trampoline_lock_all();
>  
> @@ -1734,13 +1734,41 @@ int bpf_trampoline_multi_detach(struct bpf_prog *prog, struct bpf_tracing_multi_
>  					NULL, &trampoline_multi_ops, data));
>  	}
>  
> -	if (ftrace_hash_count(data->unreg))
> -		WARN_ON_ONCE(update_ftrace_direct_del(&direct_ops, data->unreg));
> -	if (ftrace_hash_count(data->modify))
> -		WARN_ON_ONCE(update_ftrace_direct_mod(&direct_ops, data->modify, true));
> +	if (ftrace_hash_count(data->unreg)) {
> +		err_unreg = update_ftrace_direct_del(&direct_ops, data->unreg);
> +		WARN_ON_ONCE(err_unreg);
> +	}
> +	if (ftrace_hash_count(data->modify)) {
> +		err_mod = update_ftrace_direct_mod(&direct_ops, data->modify, true);
> +		WARN_ON_ONCE(err_mod);
> +	}
>  
> -	for_each_mnode(mnode, link)
> -		bpf_trampoline_multi_attach_free(mnode->trampoline);
> +	for_each_mnode(mnode, link) {
> +		struct bpf_trampoline *tr = mnode->trampoline;
> +
> +		/* If the batch ftrace update failed for this mnode's path,
> +		 * ftrace still points to old_image. Use rollback to restore
> +		 * cur_image to old_image (putting the new cur_image if any)
> +		 * so the trampoline keeps the image ftrace is calling.
> +		 *
> +		 * This relies on update_ftrace_direct_del/mod being atomic:
> +		 * on failure, NO IPs in the hash are modified in ftrace (all
> +		 * validation/allocation happens before any ftrace record is
> +		 * touched). If this assumption is broken in the future (i.e.,
> +		 * partial success becomes possible), this rollback logic would
> +		 * need to be revisited.
> +		 *
> +		 * cur_image == NULL indicates the unreg path (total == 0);
> +		 * cur_image != NULL indicates the modify path (total > 0).
> +		 */
> +		if (tr->multi_attach.old_image &&

I think tr->multi_attach.old_image is always != NULL in here?

> +		    tr->multi_attach.old_image != tr->cur_image &&
> +		    ((err_unreg && !tr->cur_image) ||
> +		     (err_mod && tr->cur_image)))
> +			bpf_trampoline_multi_attach_rollback(tr);

it's good that we won't free the image that's used, but after the link
is detached, its program will be freed, so I expect the trampoline execution
is likely to crash anyway.. did you try to simulate the errors?

I remember suggesting to increase the prog's refcount to prevent that
but I never sent that change.. I think also standard trampolines have
the same issue

> +		else
> +			bpf_trampoline_multi_attach_free(tr);
> +	}
>  
>  	trampoline_unlock_all();

also we could have bpf_trampoline_multi_detach returning void,
without the WARN_ON_ONCE in bpf_tracing_multi_link_release,
we already have the warnings in here

thanks,
jirka