Re: [PATCH bpf-next 04/13] bpf: Add tracing_multi link support for bpf progs

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] A text poke failure during the detachment of a `tracing_multi` link from BPF programs causes a Use-After-Free of the trampoline executable image and a double free.
- [Medium] Trampoline flags are not rolled back when `__bpf_trampoline_link_prog` fails during `bpf_trampoline_multi_attach`, permanently corrupting the state of the shared trampoline.
--

--- Patch [4]: [PATCH bpf-next 04/13] bpf: Add tracing_multi link support for bpf progs ---
commit b93dc01dd11e0331789988b67d9503ac3b565c51
Author: Leon Hwang <[email protected]>

    bpf: Add tracing_multi link support for bpf progs
    
    Similar to the tracing_multi link support for kernel functions, add
    support for bpf progs by reusing the 'ids' fields as BTF ids, and adding
    fds array to indicate the target bpf prog. Then, for each pair of fd+id,
    they will be the attach targets of tracing_multi link.

> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index eddd259d37760..fc51ea2428bee 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -1572,12 +1572,23 @@ static int update_fentry_multi(struct bpf_trampoline *tr, u32 orig_flags,
>  			       struct bpf_tramp_image *im, struct ftrace_hash *hash,
>  			       struct bpf_tracing_multi_data *data)
>  {
> -	unsigned long addr = (unsigned long)(im ? im->image : tr->cur_image->image);
> +	if (tr->func.ftrace_managed) {
> +		unsigned long addr = (unsigned long)(im ? im->image : tr->cur_image->image);
>  
> -	if (bpf_trampoline_use_jmp(tr->flags))
> -		addr = ftrace_jmp_set(addr);
> +		if (bpf_trampoline_use_jmp(tr->flags))
> +			addr = ftrace_jmp_set(addr);
> +
> +		ftrace_hash_add(hash, data->entry, tr->ip, addr);
> +	} else {
> +		void *old_addr = tr->cur_image ? tr->cur_image->image : NULL;
> +		void *new_addr = im ? im->image : NULL;
> +		int ret;
> +
> +		ret = bpf_trampoline_update_fentry(tr, orig_flags, old_addr, new_addr);
> +		if (ret)
> +			return ret;

[Severity: High]
If bpf_trampoline_update_fentry() fails here, we return the error and bypass
updating tr->cur_image. In bpf_trampoline_multi_detach(), if
__bpf_trampoline_unlink_prog() fails, it logs a warning but continues and
unconditionally calls bpf_trampoline_multi_attach_free(). 

Since tr->cur_image wasn't updated, does bpf_trampoline_multi_attach_free()
end up freeing the currently active executable image, leading to a
use-after-free and a double free on the next attach/detach?

> +	}
>  
> -	ftrace_hash_add(hash, data->entry, tr->ip, addr);
>  	tr->cur_image = im;
>  	return 0;
>  }

[ ... ]

> @@ -1691,6 +1715,9 @@ int bpf_trampoline_multi_attach(struct bpf_prog *prog, u32 *ids,
>  	for_each_mnode(mnode, link) {
>  		bpf_trampoline_multi_attach_init(mnode->trampoline);
>  
> +		if (progs && progs[i]->aux->tail_call_reachable)
> +			mnode->trampoline->flags |= BPF_TRAMP_F_TAIL_CALL_CTX;
> +

[Severity: Medium]
If __bpf_trampoline_link_prog() fails below, the error handler sets
rollback_cnt = i and skips rolling back the current node i.

Does this leave the BPF_TRAMP_F_TAIL_CALL_CTX flag permanently set on the
trampoline?

>  		data->entry = &mnode->entry;
>  		err = __bpf_trampoline_link_prog(&mnode->node, mnode->trampoline, NULL,
>  						 &trampoline_multi_ops, data);

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