Re: [PATCH bpf-next v3] bpf: log error code on trampoline unlink failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] If `bpf_trampoline_unlink_prog` fails to update the physical trampoline image, the detach path ignores the error and proceeds to free the BPF program, resulting in a Use-After-Free (UAF) when the active trampoline continues to call the freed memory.
--

--- Patch [1]: [PATCH bpf-next v3] bpf: log error code on trampoline unlink failure ---
commit 0b0618c3d340947a280eadeb940ccfebcad04f6f
Author: xu xin <xu.xin16@zte.com.cn>

bpf: log error code on trampoline unlink failure

This commit replaces silent WARN_ON_ONCE with WARN_ONCE to log the actual
error code from bpf_trampoline_unlink_prog() during link teardown.

> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 0ff9e3aa293d5..b4b4fc49bba79 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -3547,10 +3547,12 @@ static void bpf_tracing_link_release(struct bpf_link *link)
>  {
>  	struct bpf_tracing_link *tr_link =
>  		container_of(link, struct bpf_tracing_link, link.link);
> +	int err;
>  
> -	WARN_ON_ONCE(bpf_trampoline_unlink_prog(&tr_link->link.node,
> -						tr_link->trampoline,
> -						tr_link->tgt_prog));
> +	err = bpf_trampoline_unlink_prog(&tr_link->link.node,
> +					tr_link->trampoline,
> +					tr_link->tgt_prog);
> +	WARN_ONCE(err, "bpf_trampoline_unlink_prog returns error: %d\n", err);
>  
>  	bpf_trampoline_put(tr_link->trampoline);

[Severity: Critical]
This isn't a bug introduced by this patch, but since the commit message
mentions troubleshooting UAF crashes here, does this error path directly cause
that UAF?

If bpf_trampoline_update() fails inside __bpf_trampoline_unlink_prog(), the
program is removed from the software list, but the physical trampoline is not
updated:

__bpf_trampoline_unlink_prog() {
    ...
    bpf_trampoline_remove_prog(tr, node);
    return bpf_trampoline_update(tr, true /* lock_direct_mutex */, ops, data);
}

Because bpf_tracing_link_release() merely logs the error and continues, it
will proceed to tear down the link and free the BPF program memory.

Will the unmodified physical trampoline then blindly jump into the now-freed
BPF program on the next kernel function invocation?

> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index 129d07db117ec..9b9eea02dd58d 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -1004,12 +1004,15 @@ static void bpf_shim_tramp_link_release(struct bpf_link *link)
>  {
>  	struct bpf_shim_tramp_link *shim_link =
>  		container_of(link, struct bpf_shim_tramp_link, link.link);
> +	int err;
>  
>  	/* paired with 'shim_link->trampoline = tr' in bpf_trampoline_link_cgroup_shim */
>  	if (!shim_link->trampoline)
>  		return;
>  
> -	WARN_ON_ONCE(bpf_trampoline_unlink_prog(&shim_link->link.node, shim_link->trampoline, NULL));
> +	err = bpf_trampoline_unlink_prog(&shim_link->link.node, shim_link->trampoline, NULL);
> +	WARN_ONCE(err, "bpf_trampoline_unlink_prog returns error: %d\n", err);
> +
>  	bpf_trampoline_put(shim_link->trampoline);
>  }

[Severity: Critical]
This is a pre-existing issue, but this shim release callback appears to suffer
from the exact same flaw.

If the unlink fails, it ignores the error and continues, leaving the hardware
hook active for a program that is about to be freed.

> @@ -1720,15 +1723,16 @@ 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;
>  
>  	trampoline_lock_all();
>  
>  	for_each_mnode(mnode, link) {
>  		data->entry = &mnode->entry;
>  		bpf_trampoline_multi_attach_init(mnode->trampoline);
> -		WARN_ON_ONCE(__bpf_trampoline_unlink_prog(&mnode->node, mnode->trampoline,
> -					NULL, &trampoline_multi_ops, data));
> +		err = __bpf_trampoline_unlink_prog(&mnode->node, mnode->trampoline,NULL,
> +					&trampoline_multi_ops, data);
> +		WARN_ONCE(err, "__bpf_trampoline_unlink_prog returns error: %d\n", err);
>  	}

[Severity: Critical]
This is also a pre-existing issue.

Similar to the other paths, this loop ignores unlink failures. Can this allow
all attached programs to be freed even if their physical hooks remain active?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728142621001Ee2lrcnZpeNuxtxRES69Z@zte.com.cn?part=1