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

On 9/8/26 23:33, [email protected] wrote:
> 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?
> 

The potential UAF issue will be fixed by Hui's patch [1].

[1]
https://lore.kernel.org/bpf/aaa3829e11e2e26bcd3bda9ee6df7a0101a718ac.1786412280.git.zhuhui@kylinos.cn/

>> +	}
>>  
>> -	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?
> 

False positive. If the target bpf prog is tail_call_reachable, the
tr->flags must keep BPF_TRAMP_F_TAIL_CALL_CTX always.

Thanks,
Leon

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