[PATCH bpf-next 0/2] bpf: Fix two trampoline image UAFs in bpf_trampoline_multi_detach()

[PATCH bpf-next 0/2] bpf: Fix two trampoline image UAFs in bpf_trampoline_multi_detach()

[Hui Zhu]

From: Hui Zhu <zhuhui@kylinos.cn>

While auditing the error paths added by commit aef4dfa790b2 ("bpf: Add
bpf_trampoline_multi_attach/detach functions"), I found two independent
ways bpf_trampoline_multi_detach() can free a trampoline image that
ftrace is still actively calling into.

Patch 1 fixes the case where __bpf_trampoline_unlink_prog() itself
fails before touching ftrace (e.g. -ENOMEM building the new image):
cur_image is left equal to old_image, but
bpf_trampoline_multi_attach_free() frees old_image unconditionally.

Patch 2 fixes the narrower case where the per-trampoline unlink
succeeds (cur_image has already been speculatively advanced) but the
later batched update_ftrace_direct_del()/update_ftrace_direct_mod()
call fails. Patch 1's old_image != cur_image check alone isn't enough
here since ftrace was never actually updated; the affected
trampolines need to be rolled back to old_image instead of freed.

Both paths are already WARN_ON_ONCE()'d and need an allocation or
ftrace failure to trigger, so they're unlikely to fire in practice,
but when they do, the current code frees a live image out from under
ftrace.

Hui Zhu (2):
  bpf: Fix stale old_image UAF on trampoline update failure
  bpf: Fix UAF in bpf_trampoline_multi_detach when ftrace update fails

 kernel/bpf/trampoline.c | 54 +++++++++++++++++++++++++++++++++++------
 1 file changed, 46 insertions(+), 8 deletions(-)

-- 
2.43.0

[PATCH bpf-next 1/2] bpf: Fix stale old_image UAF on trampoline update failure

[Hui Zhu]

From: Hui Zhu <zhuhui@kylinos.cn>

In bpf_trampoline_multi_detach(), if __bpf_trampoline_unlink_prog()
fails (e.g. -ENOMEM in bpf_tramp_image_alloc), bpf_trampoline_update()
returns before touching ftrace, so cur_image still equals old_image
and ftrace still points to it. The failure is only WARN_ON_ONCE'd and
the loop continues; afterwards bpf_trampoline_multi_attach_free()
unconditionally frees old_image via bpf_tramp_image_put(). Once the
RCU grace period elapses, the still-active image is freed while
ftrace keeps calling into it - a UAF that can crash the kernel or be
exploited via slab reuse.

The attach path avoids this with bpf_trampoline_multi_attach_rollback(),
but that can't be reused here: on this failure path cur_image ==
old_image, so calling it would free the active image the same way.
Re-adding the prog to the trampoline's hlist isn't safe either, since
bpf_tracing_multi_link_release() frees the link (and its embedded
tramp_node) immediately after detach returns.

Fix it by only freeing old_image in bpf_trampoline_multi_attach_free()
when it differs from cur_image. On success cur_image is updated to a
new image (or NULL), so old_image != cur_image correctly identifies a
stale image. On this failure path they're equal, so the image is left
alone until a later successful update replaces it.

Fixes: aef4dfa790b2 ("bpf: Add bpf_trampoline_multi_attach/detach functions")
Signed-off-by: Hui Zhu <zhuhui@kylinos.cn>
---
 kernel/bpf/trampoline.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 6eadf64f7ec9..a78fbf726fad 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -1585,7 +1585,17 @@ 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);
 
 	tr->multi_attach.old_image = NULL;
-- 
2.43.0

Re: [PATCH bpf-next 1/2] bpf: Fix stale old_image UAF on trampoline update failure

[sashiko-bot]

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

New issues:
- [Low] BPF subsystem coding style violation in multi-line comment.

Pre-existing issues:
- [Critical] Use-After-Free of trampoline image on detach failure path when ftrace deletion fails.
- [Critical] Use-After-Free of BPF program memory on detach failure path when trampoline unlink fails.
- [Critical] Use-After-Free of trampoline image on attach rollback failure.
--

--- Patch [1]: [PATCH bpf-next 1/2] bpf: Fix stale old_image UAF on trampoline update failure ---
commit add7893089763b05b38780203210dc401d0f2955
Author: Hui Zhu <zhuhui@kylinos.cn>

bpf: Fix stale old_image UAF on trampoline update failure

This patch addresses a use-after-free where old_image could be freed
even when bpf_trampoline_update() fails. It fixes the problem by verifying
that old_image and cur_image are different before allowing the memory to
be freed.

> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index 6eadf64f7ec90..a78fbf726fad5 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -1585,7 +1585,17 @@ 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.

[Severity: Low]
Does this violate the BPF subsystem coding style? Multi-line comments
should have the opening /* on its own line.

> +	 * 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 safely handle the case
where ftrace deletion fails in bpf_trampoline_multi_detach()?

If __bpf_trampoline_unlink_prog() succeeds, unregister_fentry_multi()
sets tr->cur_image = NULL.

Then update_ftrace_direct_del() is called, but it can fail (e.g., if
hash_sub allocation fails). If it fails, the error is ignored and ftrace
continues pointing to old_image:

bpf_trampoline_multi_detach() {
    ...
    if (ftrace_hash_count(data->unreg))
        WARN_ON_ONCE(update_ftrace_direct_del(&direct_ops, data->unreg));
    ...
}

When bpf_trampoline_multi_attach_free() executes, old_image != cur_image
evaluates to true (non-NULL != NULL), causing old_image to be freed.
Could ftrace then trigger and execute the freed memory?


[Severity: Critical]
This is a pre-existing issue, but is there a use-after-free of the BPF
program memory if the trampoline unlink fails during detach?

If __bpf_trampoline_unlink_prog() fails (e.g., returns -ENOMEM) in
bpf_trampoline_multi_detach(), the failure is ignored:

bpf_trampoline_multi_detach() {
    ...
    WARN_ON_ONCE(__bpf_trampoline_unlink_prog(&mnode->node, mnode->trampoline,
                                              NULL, &trampoline_multi_ops, data));
    ...
}

The detach function returns, and bpf_tracing_multi_link_release()
unconditionally frees the BPF program memory:

bpf_tracing_multi_link_release() {
    ...
    WARN_ON_ONCE(bpf_trampoline_multi_detach(link->prog, tr_link));
    bpf_tracing_multi_link_free(tr_link);
    ...
}

However, because unlink failed, ftrace still points to old_image, which
contains hardcoded assembly jumping into the now-freed BPF program memory.
Could this result in a use-after-free of the BPF program memory if ftrace
is triggered?


[Severity: Critical]
This is a pre-existing issue, but can the trampoline image be freed while
ftrace is still using it during a failed attach rollback?

In bpf_trampoline_multi_attach(), if update_ftrace_direct_mod() fails,
the rollback sequence attempts update_ftrace_direct_del():

bpf_trampoline_multi_attach() {
    ...
    if (err) {
        if (ftrace_hash_count(data->reg))
            WARN_ON_ONCE(update_ftrace_direct_del(&direct_ops, data->reg));
        goto rollback_unlink;
    }
    ...
}

If update_ftrace_direct_del() fails, ftrace remains configured to call
the newly allocated cur_image.

Then bpf_trampoline_multi_attach_rollback() unconditionally frees
tr->cur_image:

bpf_trampoline_multi_attach_rollback() {
    if (tr->cur_image)
        bpf_tramp_image_put(tr->cur_image);
    ...
}

Could ftrace execute the freed cur_image in this scenario?

>  
>  	tr->multi_attach.old_image = NULL;
>  }

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

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

[Hui Zhu]

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 &&
+		    tr->multi_attach.old_image != tr->cur_image &&
+		    ((err_unreg && !tr->cur_image) ||
+		     (err_mod && tr->cur_image)))
+			bpf_trampoline_multi_attach_rollback(tr);
+		else
+			bpf_trampoline_multi_attach_free(tr);
+	}
 
 	trampoline_unlock_all();
 
-- 
2.43.0

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

[sashiko-bot]

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