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

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

[Hui Zhu]

From: Hui Zhu <zhuhui@kylinos.cn>

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 calls into it.  Freeing
old_image in that case causes a UAF.

Only free old_image when it differs from cur_image.

Also change bpf_trampoline_multi_detach() to return void and drop
the WARN_ON_ONCE at its call site.

Signed-off-by: Hui Zhu <zhuhui@kylinos.cn>
---
 include/linux/bpf.h      |  9 ++++-----
 kernel/bpf/trampoline.c  | 17 ++++++++++++++---
 kernel/trace/bpf_trace.c |  2 +-
 3 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 73bacfc6444d..3255c6f5ea6b 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1518,8 +1518,8 @@ int arch_prepare_bpf_dispatcher(void *image, void *buf, s64 *funcs, int num_func
 
 int bpf_trampoline_multi_attach(struct bpf_prog *prog, u32 *ids,
 				struct bpf_tracing_multi_link *link);
-int bpf_trampoline_multi_detach(struct bpf_prog *prog,
-				struct bpf_tracing_multi_link *link);
+void bpf_trampoline_multi_detach(struct bpf_prog *prog,
+				 struct bpf_tracing_multi_link *link);
 void bpf_trampoline_set_flags(struct bpf_trampoline *tr, u32 flags);
 
 /*
@@ -1639,10 +1639,9 @@ static inline int bpf_trampoline_multi_attach(struct bpf_prog *prog, u32 *ids,
 {
 	return -ENOTSUPP;
 }
-static inline int bpf_trampoline_multi_detach(struct bpf_prog *prog,
-					      struct bpf_tracing_multi_link *link)
+static inline void bpf_trampoline_multi_detach(struct bpf_prog *prog,
+					       struct bpf_tracing_multi_link *link)
 {
-	return -ENOTSUPP;
 }
 static inline void bpf_trampoline_set_flags(struct bpf_trampoline *tr, u32 flags) {}
 #endif
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index ed7999ad6c66..544ab41a3c25 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -1595,7 +1595,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);
 
 	tr->multi_attach.old_image = NULL;
@@ -1719,7 +1730,8 @@ int bpf_trampoline_multi_attach(struct bpf_prog *prog, u32 *ids,
 	return err;
 }
 
-int bpf_trampoline_multi_detach(struct bpf_prog *prog, struct bpf_tracing_multi_link *link)
+void bpf_trampoline_multi_detach(struct bpf_prog *prog,
+				 struct bpf_tracing_multi_link *link)
 {
 	struct bpf_tracing_multi_data *data = &link->data;
 	struct bpf_tracing_multi_node *mnode;
@@ -1749,7 +1761,6 @@ int bpf_trampoline_multi_detach(struct bpf_prog *prog, struct bpf_tracing_multi_
 		bpf_trampoline_put(mnode->trampoline);
 
 	clear_tracing_multi_data(data);
-	return 0;
 }
 
 #undef for_each_mnode_cnt
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 891897f8a1b3..29260951aa87 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);
 }
 
 static void bpf_tracing_multi_link_dealloc(struct bpf_link *link)
-- 
2.53.0

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

[sashiko-bot]

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

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

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 4060 bytes --]

> commit 86d3e38ceefc101816dc27eeae8be587b534aee8
> Author: Hui Zhu <zhuhui@kylinos.cn>
>
>     bpf: Fix UAF in bpf_trampoline_multi_attach_free on update failure
>
>     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 calls into it.  Freeing
>     old_image in that case causes a UAF.
>
>     Only free old_image when it differs from cur_image.
>
>     Also change bpf_trampoline_multi_detach() to return void and drop
>     the WARN_ON_ONCE at its call site.
>
>     Signed-off-by: Hui Zhu <zhuhui@kylinos.cn>

This looks like a bug fix.  Should this include a Fixes: tag pointing
at the commit that introduced bpf_trampoline_multi_attach_free(), so
the backport scope for this UAF is clear?

> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index b4a10c9878cf..f4e8d372253a 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h

[ ... ]

> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index e07af35ed040..b76d162517e3 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);

Does the comment accurately describe when old_image is safe to free?
Looking at modify_fentry_multi() and unregister_fentry_multi(), they
assign cur_image before ftrace is reprogrammed:

kernel/bpf/trampoline.c:modify_fentry_multi()
    ftrace_hash_add(data->modify, data->entry, ip, addr);
    tr->cur_image = im;

kernel/bpf/trampoline.c:unregister_fentry_multi()
    ftrace_hash_add(data->unreg, data->entry, ip, addr);
    tr->cur_image = NULL;

Both return 0 immediately. The actual ftrace updates happen later in
bpf_trampoline_multi_detach():

    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));

    for_each_mnode(mnode, link)
            bpf_trampoline_multi_attach_free(mnode->trampoline);

If update_ftrace_direct_del() fails with -ENOMEM (via hash_sub() ->
alloc_and_copy_ftrace_hash() -> alloc_ftrace_hash()), it goes to
out_unlock before the fixup, leaving direct_functions still mapping
tr->ip to old_image->image. Similarly, if update_ftrace_direct_mod()
fails in register_ftrace_function_nolock() or
__ftrace_hash_update_ipmodify(), it never reaches the loop that would
update direct_functions to point to the new image.

In both cases, cur_image has already been changed (to NULL or to im),
so old_image != cur_image would pass and old_image would be freed even
though ftrace's direct_functions still points to it. Wouldn't this
still be the UAF the commit message describes?

[ ... ]

> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 891897f8a1b3..29260951aa87 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31350581565