> commit 86d3e38ceefc101816dc27eeae8be587b534aee8
> Author: Hui Zhu <[email protected]>
>
> 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 <[email protected]>
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
