Re: [PATCH bpf-next v2 1/3] bpf: Fix UAF in bpf_trampoline_multi_detach on update failure

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

New issues:
- [Low] Multi-line comment opening `/*` is not on its own line.

Pre-existing issues:
- [High] Incomplete fix: Use-After-Free of `bpf_prog` when `__bpf_trampoline_unlink_prog()` fails.
- [High] Use-After-Free of trampoline image in `bpf_trampoline_multi_attach` error path.
--

--- Patch [1]: [PATCH bpf-next v2 1/3] bpf: Fix UAF in bpf_trampoline_multi_detach on update failure ---
commit 26ad7c4ab94aa890dec18cbc899359aabf65c015
Author: Hui Zhu <[email protected]>

    bpf: Fix UAF in bpf_trampoline_multi_detach on update failure
    
    The commit attempts to fix Use-After-Free issues in the error paths of
    bpf_trampoline_multi_detach() by changing when old_image is freed and
    introducing a rollback mechanism when ftrace updates fail. It pins the
    bpf_prog onto the image to avoid freeing it while ftrace might still
    direct calls to it.

> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index ed7999ad6c66c..c08d1a09e6381 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
[ ... ]
> @@ -1719,11 +1760,11 @@ int bpf_trampoline_multi_attach(struct bpf_prog *prog, u32 *ids,
>  	return err;
>  }
>  
> -int bpf_trampoline_multi_detach(struct bpf_prog *prog, struct bpf_tracing_multi_link *link)
> +void bpf_trampoline_multi_detach(struct bpf_prog *prog, struct bpf_tracing_multi_link *link)
>  {
>  	struct bpf_tracing_multi_data *data = &link->data;
>  	struct bpf_tracing_multi_node *mnode;
> -	int i, err;
> +	int i, err, err_unreg = 0, err_mod = 0;
>  
>  	trampoline_lock_all();
>  
> @@ -1735,13 +1776,65 @@ int bpf_trampoline_multi_detach(struct bpf_prog *prog, struct bpf_tracing_multi_
>  		WARN_ONCE(err, "__bpf_trampoline_unlink_prog failed: %d\n", err);
>  	}
>  
> -	if (ftrace_hash_count(data->unreg))
> -		WARN_ON_ONCE(update_ftrace_direct_del(&direct_ops, data->unreg));
> -	if (ftrace_hash_count(data->modify))
> -		WARN_ON_ONCE(update_ftrace_direct_mod(&direct_ops, data->modify, true));
> +	if (ftrace_hash_count(data->unreg)) {
> +		err_unreg = update_ftrace_direct_del(&direct_ops, data->unreg);
> +		WARN_ON_ONCE(err_unreg);
> +	}
> +	if (ftrace_hash_count(data->modify)) {
> +		err_mod = update_ftrace_direct_mod(&direct_ops, data->modify, true);
> +		WARN_ON_ONCE(err_mod);
> +	}
>  
> -	for_each_mnode(mnode, link)
> -		bpf_trampoline_multi_attach_free(mnode->trampoline);
> +	for_each_mnode(mnode, link) {
> +		struct bpf_trampoline *tr = mnode->trampoline;
> +
> +		/* If the batch ftrace update failed for this mnode's path,
> +		 * ftrace still points to old_image. Use rollback to restore

[Severity: Low]
Does this multi-line comment violate the BPF subsystem guidelines? The
opening /* of a multi-line comment must be on its own line.

> +		 * cur_image to old_image (putting the new cur_image if any)
> +		 * so the trampoline keeps the image ftrace is calling.
[ ... ]
> +		 * image if the trampoline is torn down first instead (see
> +		 * bpf_trampoline_put()).
> +		 */
> +		if (tr->multi_attach.old_image &&
> +		    tr->multi_attach.old_image != tr->cur_image &&
> +		    ((err_unreg && !tr->cur_image) ||
> +		     (err_mod && tr->cur_image))) {

[Severity: High]
This is a pre-existing issue, but does this incomplete fix lead to a
Use-After-Free of the bpf_prog structure if __bpf_trampoline_unlink_prog()
fails?

When __bpf_trampoline_unlink_prog() fails, old_image equals cur_image,
so this condition evaluates to false, bypassing the logic to pin the
bpf_prog onto the retained image. This allows the caller to release its
reference to the bpf_prog and free it, even though the active ftrace
trampoline image still contains a direct machine-code call to the freed
prog.

A similar missing pinning logic exists in the non-multi caller of
__bpf_trampoline_unlink_prog() where it directly returns the failure of
bpf_trampoline_update() without pinning the prog:

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);
}

> +			WARN_ON_ONCE(tr->multi_attach.old_image->pinned_prog);
> +			bpf_prog_inc(prog);
> +			tr->multi_attach.old_image->pinned_prog = prog;
> +			bpf_trampoline_multi_attach_rollback(tr);
> +		} else {
> +			bpf_trampoline_multi_attach_free(tr);
> +		}
> +	}
>  
>  	trampoline_unlock_all();

[Severity: High]
This isn't a bug introduced by this patch, but does the error path in
bpf_trampoline_multi_attach() cause a Use-After-Free of the trampoline
image?

If update_ftrace_direct_del() fails during rollback:

kernel/bpf/trampoline.c:bpf_trampoline_multi_attach() {
    ...
	if (ftrace_hash_count(data->modify)) {
		err = update_ftrace_direct_mod(&direct_ops, data->modify, true);
		if (err) {
			if (ftrace_hash_count(data->reg))
				WARN_ON_ONCE(update_ftrace_direct_del(&direct_ops, data->reg));
			goto rollback_unlink;
		}
	}
    ...
}

It proceeds blindly to rollback_unlink, which calls
bpf_trampoline_multi_attach_rollback():

kernel/bpf/trampoline.c:bpf_trampoline_multi_attach_rollback() {
    ...
	if (tr->cur_image)
		bpf_tramp_image_put(tr->cur_image);
    ...
}

Does this unconditionally free the newly allocated trampoline images even
when update_ftrace_direct_del() fails, leading to a UAF since ftrace
is still actively pointing to them?

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