Re: [PATCH bpf 1/2] bpf: Do not release trampoline image in case off unregister error

Re: [PATCH bpf 1/2] bpf: Do not release trampoline image in case off unregister error

[Jiri Olsa]

On Fri, Apr 24, 2026 at 04:12:31PM +0000, sashiko-bot@kernel.org wrote:
> Sashiko AI review found 3 potential issue(s):
> - [Critical] Use-After-Free of BPF programs due to leaked trampoline image
> - [High] Permanent memory leak of trampoline executable image
> - [High] Use-After-Free of ftrace_ops and bpf_trampoline
> --
> 
> commit 3c4d55e24f28fa5f7f6dd937603366805532d1a4
> Author: Jiri Olsa <jolsa@kernel.org>
> 
> bpf: Do not release trampoline image in case off unregister error
> 
> If unregister_fentry fails we still have trampoline image attached
> to a function, so releasing it could trigger crash. Releasing the
> trampoline image only when the unregister succeeds.
> 
> > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> > index f02254a21585fa..01082ecc5c4f5f 100644
> > --- a/kernel/bpf/trampoline.c
> > +++ b/kernel/bpf/trampoline.c
> > @@ -618,8 +618,10 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
> >  
> >  	if (total == 0) {
> >  		err = unregister_fentry(tr, orig_flags, tr->cur_image->image);
> > -		bpf_tramp_image_put(tr->cur_image);
> > -		tr->cur_image = NULL;
> > +		if (!err) {
> > +			bpf_tramp_image_put(tr->cur_image);
> > +			tr->cur_image = NULL;
> > +		}
> 
> If unregister_fentry() fails, the ftrace or fentry hook remains active because
> the image isn't put. Does the teardown path, like bpf_tracing_link_release(),
> still proceed to tear down the link and free the bpf_prog structures?
> 
> If the BPF programs are freed but the leaked JIT executable image is still
> attached and active, could subsequent calls to the hooked kernel function
> execute the trampoline image, dereference the hardcoded pointers, and execute
> freed BPF programs?
> 
> When unregister_fentry() fails and bpf_tramp_image_put() is skipped, does the
> error propagate up to the caller where bpf_trampoline_put() might eventually
> free the parent struct bpf_trampoline?
> 
> If the parent trampoline is destroyed without freeing tr->cur_image, does this
> leak the struct bpf_tramp_image and its associated JIT memory page?
> 
> Additionally, if unregister_fentry() fails for an ftrace-managed trampoline,
> tr->fops appears to remain registered in the ftrace subsystem.
> 
> If the link teardown ignores the error and unconditionally calls
> bpf_trampoline_put(tr), will direct_ops_free() free tr->fops and tr?
> 
> Could this leave the ftrace subsystem with dangling pointers in its internal
> tracking structures, potentially causing a use-after-free regression during
> subsequent ftrace operations?

ah right, released bpf_prog and bpf_trampoline will cause crash later,
maybe we could just inc bpf_prog ref and skip bpf_trampoline_put like
below for the tracing link release

jirka


---
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 3b1f0ba02f61..c609f15b146d 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3505,9 +3505,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);
 
-	WARN_ON_ONCE(bpf_trampoline_unlink_prog(&tr_link->link,
+	if (WARN_ON_ONCE(bpf_trampoline_unlink_prog(&tr_link->link,
 						tr_link->trampoline,
-						tr_link->tgt_prog));
+						tr_link->tgt_prog))) {
+		bpf_prog_inc(tr_link->link.link.prog);
+		return;
+	}
 
 	bpf_trampoline_put(tr_link->trampoline);