Re: [PATCH bpf-next v2 3/3] bpf: Fix prog UAF in __bpf_trampoline_unlink_prog() on update failure

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

Pre-existing issues:
- [Critical] Unaddressed Use-After-Free for BPF_TRAMP_REPLACE programs if bpf_arch_text_poke() fails in __bpf_trampoline_unlink_prog.
- [Medium] Dangling pointer and Denial of Service if bpf_arch_text_poke() fails during BPF_TRAMP_REPLACE program attachment.
--

--- Patch [3]: [PATCH bpf-next v2 3/3] bpf: Fix prog UAF in __bpf_trampoline_unlink_prog() on update failure ---
commit 58c3b5c6808d88a8686306a02c5a18f2f2dd6edf
Author: Hui Zhu <[email protected]>

bpf: Fix prog UAF in __bpf_trampoline_unlink_prog() on update failure

This commit fixes a use-after-free issue in __bpf_trampoline_unlink_prog()
by pinning the program to tr->cur_image when bpf_trampoline_update() fails.
This ensures the program outlives the link and prevents a use-after-free
when the image is executed.

> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index 7fac27374ece1..6071261fd66b8 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -991,7 +991,25 @@ static int __bpf_trampoline_unlink_prog(struct bpf_tramp_node *node,
>  		return err;
>  	}

[Severity: Critical]
This is a pre-existing issue, but does the BPF_TRAMP_REPLACE branch above
suffer from a similar Use-After-Free if bpf_arch_text_poke() fails?

If the text poke fails, this branch returns early and bypasses the newly
added UAF mitigation logic. Because the poke failed, the target function
still contains a live jump to the extension program's JITed code. The caller
(bpf_tracing_link_release) will log a warning and proceed to unconditionally
drop its reference to the bpf_prog, freeing it. Will this result in a
use-after-free the next time the target function is executed?

[Severity: Medium]
This is also a pre-existing issue, but is there a similar failure handling
problem during attachment in __bpf_trampoline_link_prog()?

kernel/bpf/trampoline.c:__bpf_trampoline_link_prog() {
    ...
		tr->extension_prog = node->link->prog;
		return bpf_arch_text_poke(tr->func.addr, BPF_MOD_NOP,
    ...
}

If bpf_arch_text_poke() fails here, tr->extension_prog remains non-NULL
but the program reference is eventually dropped, leaving a dangling pointer.
Any future attach attempts to this trampoline will hit the early
if (tr->extension_prog) check and permanently return -EBUSY. Could this lead
to a local denial of service for the affected tracing hook?

>  	bpf_trampoline_remove_prog(tr, node);
> -	return bpf_trampoline_update(tr, true /* lock_direct_mutex */, ops, data);
> +	err = bpf_trampoline_update(tr, true /* lock_direct_mutex */, ops, data);
> +	/*
> +	 * If the update above failed, tr->cur_image is unchanged, i.e. ftrace
> +	 * (or the direct jump, for the standard ops) is still actually
> +	 * calling into an image with node->link->prog's call baked into its
> +	 * machine code, even though this unlink is being reported as failed.
> +	 * Every caller of this function only WARN_ON_ONCE()'s a failure here
> +	 * and then unconditionally frees the underlying bpf_prog, which
> +	 * would be a use-after-free the next time that image is executed.
> +	 * Pin an extra reference on the prog onto tr->cur_image so it
> +	 * outlives this link, mirroring the fix applied to
> +	 * bpf_trampoline_multi_attach()/bpf_trampoline_multi_detach().
> +	 */
> +	if (err && tr->cur_image) {
> +		WARN_ON_ONCE(tr->cur_image->pinned_prog);
> +		bpf_prog_inc(node->link->prog);
> +		tr->cur_image->pinned_prog = node->link->prog;
> +	}
> +	return err;
>  }

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