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

[sashiko-bot@kernel.org]

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