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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Rolling back the trampoline image during detach introduces a memory leak of the trampoline image and its executable page.

Pre-existing issues:
- [Critical] Failing to update ftrace during detach causes an exploitable Use-After-Free of BPF program memory.
--

--- Patch [2]: [PATCH bpf-next 2/2] bpf: Fix UAF in bpf_trampoline_multi_detach when ftrace update fails ---
commit 74c497eb28abb2878db6016026226008d3d6f7a6
Author: Hui Zhu <zhuhui@kylinos.cn>
Subject: bpf: Fix UAF in bpf_trampoline_multi_detach when ftrace update fails

This patch updates bpf_trampoline_multi_detach to capture ftrace update
failures and uses a rollback mechanism to prevent freeing the old trampoline
image while ftrace still points to it.

> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c

[ ... ]

> @@ -1734,13 +1734,41 @@ int bpf_trampoline_multi_detach(struct bpf_prog *prog, struct bpf_tracing_multi_
> -	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.
[ ... ]
> +		 */
> +		if (tr->multi_attach.old_image &&
> +		    tr->multi_attach.old_image != tr->cur_image &&
> +		    ((err_unreg && !tr->cur_image) ||
> +		     (err_mod && tr->cur_image)))
> +			bpf_trampoline_multi_attach_rollback(tr);

[Severity: High]
Does this rollback leak the trampoline image memory?

In a successful detach, tr->cur_image is typically set to NULL. By calling
bpf_trampoline_multi_attach_rollback(), we restore tr->cur_image to
tr->multi_attach.old_image instead of freeing it.

When the link is subsequently destroyed, bpf_trampoline_put() is called.
Because the BPF program was already unlinked from the trampoline earlier in
this function via __bpf_trampoline_unlink_prog(), tr->progs_hlist is
completely empty. 

This causes bpf_trampoline_put() to directly free the trampoline structure:

kernel/bpf/trampoline.c:bpf_trampoline_put() {
    ...
    if (hlist_empty(&tr->progs_hlist[BPF_TRAMP_FENTRY]) &&
        ...) {
        ...
        kfree(tr);
    }
}

Because bpf_trampoline_put() expects tr->cur_image to be NULL when no
programs remain, it does not free it. Is there another mechanism that
will eventually clean up this restored image, or is it permanently leaked?

[Severity: Critical]
This is a pre-existing issue, but does keeping the old image active still
lead to a use-after-free of the BPF program itself?

Even though the rollback preserves the trampoline image so ftrace can safely
call it without faulting, the BPF program was already unlinked by
__bpf_trampoline_unlink_prog().

Since bpf_trampoline_multi_detach() unconditionally returns 0, the caller
proceeds with tearing down the link, ignoring the ftrace detachment failure:

kernel/trace/bpf_trace.c:bpf_tracing_multi_link_release() {
    WARN_ON_ONCE(bpf_trampoline_multi_detach(link->prog, tr_link));
}

This continues to bpf_link_dealloc() which calls bpf_prog_put(link->prog),
freeing the BPF program's memory.

Because the ftrace hook could not be removed, won't ftrace eventually fire
and execute the preserved old_image, which contains hardcoded direct calls
to the now-freed BPF program?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784191209.git.zhuhui@kylinos.cn?part=2