Re: [PATCH bpf-next v2 2/3] bpf: Fix prog UAF in bpf_trampoline_multi_attach() register-path rollback

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

New issues:
- [Medium] Memory leak of pinned BPF program on consecutive ftrace update failures.

Pre-existing issues:
- [High] Use-after-free of BPF program if trampoline image allocation fails during detach.
--

--- Patch [2]: [PATCH bpf-next v2 2/3] bpf: Fix prog UAF in bpf_trampoline_multi_attach() register-path rollback ---
commit d47fe23104c85648ed4814b819490191dd04b447
Author: Hui Zhu <[email protected]>

bpf: Fix prog UAF in bpf_trampoline_multi_attach() register-path rollback

This commit fixes a use-after-free in bpf_trampoline_multi_attach() where
ftrace rollback failures could lead to the trampoline image and attached
program being unconditionally freed while still active. It resolves this by
pinning the program to the current image if the register-path undo fails.

> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index c08d1a09e6381..7fac27374ece1 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c

[ ... ]

> @@ -1744,8 +1746,47 @@ int bpf_trampoline_multi_attach(struct bpf_prog *prog, u32 *ids,
>  
>  rollback_unlink:
>  	for_each_mnode_cnt(mnode, link, rollback_cnt) {

[ ... ]

> +		if (via_register && err_undo_reg && rtr->cur_image) {
> +			WARN_ON_ONCE(rtr->cur_image->pinned_prog);
> +			bpf_prog_inc(prog);
> +			rtr->cur_image->pinned_prog = prog;

[Severity: Medium]
Can this result in a memory leak if multiple operations fail on the same
trampoline?

Since pinned_prog is a single pointer, if multiple programs attached to the
same trampoline experience ftrace update failures, the previous program
appears to be overwritten without releasing its reference.

Is there a way to track multiple pinned programs to prevent permanently
leaking the bpf_prog and its associated resources in this rollback path?

> +		} else {
> +			bpf_trampoline_multi_attach_rollback(rtr);
> +		}
>  	}
>  
>  	trampoline_unlock_all();

[Severity: High]
This isn't a bug introduced by this patch, but does a similar use-after-free
exist during detach if trampoline image allocation fails?

If a user detaches a program but bpf_tramp_image_alloc() fails (e.g. from
memory pressure), __bpf_trampoline_unlink_prog() removes the program from
the active list but leaves the image unchanged:

kernel/bpf/trampoline.c:__bpf_trampoline_unlink_prog() {
	...
	bpf_trampoline_remove_prog(tr, node);
	return bpf_trampoline_update(tr, true /* lock_direct_mutex */, ops, data);
}

In bpf_trampoline_multi_detach(), the rollback logic requires old_image to
differ from cur_image to pin the program:

kernel/bpf/trampoline.c:bpf_trampoline_multi_detach() {
	...
		if (tr->multi_attach.old_image &&
		    tr->multi_attach.old_image != tr->cur_image &&
	...
}

Because the image allocation failed, old_image == cur_image, which bypasses
the program pinning logic. The caller bpf_tracing_multi_link_release() will
then unconditionally drop the program reference via bpf_prog_put(). 

Could this allow ftrace to execute the freed BPF program memory since the
active ftrace image still contains direct calls to it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2