Re: [PATCH bpf v1 05/10] bpf: Reject resilient lock operations in rbtree callbacks

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The restriction against calling lock operations inside rbtree callbacks can be bypassed by nesting callbacks or calling subprograms, because in_rbtree_lock_required_cb() only checks the immediate top frame.
--

commit c5914f67e15078bc12af13429620d2a2a4de2b11
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Reject resilient lock operations in rbtree callbacks

This patch attempts to enforce the root's lock requirement for resilient
locks in rbtree callbacks. It aims to prevent use-after-free conditions
when nodes are removed and freed by other CPUs during the in-progress tree
walk.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index f540279ff4abb..32d31fa67036b 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -13241,6 +13241,11 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_call_arg_me
>  		{
>  			int flags = PROCESS_RES_LOCK;
>  
> +			if (in_rbtree_lock_required_cb(env)) {
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This isn't a bug introduced by this patch, but does
in_rbtree_lock_required_cb() fully protect against this bypass?

Looking at in_rbtree_lock_required_cb(), it only checks the immediate top
frame:

kernel/bpf/verifier.c:in_rbtree_lock_required_cb() {
    ...
	callee = state->frame[state->curframe];

	if (!callee->in_callback_fn)
		return false;

	kfunc_btf_id = insn[callee->callsite].imm;
	return is_rbtree_lock_required_kfunc(kfunc_btf_id);
}

If a BPF program calls a custom rbtree comparison callback, and inside that
callback it calls a static subprogram (or a nested callback like bpf_loop()),
would state->curframe's in_callback_fn be false (or its kfunc_btf_id not
match the rbtree add kfunc)?

Could a program use this nested frame to successfully unlock the resilient
lock via a global map value (BPF_PSEUDO_MAP_VALUE), temporarily dropping the
lock and allowing another CPU to concurrently remove and free the nodes being
traversed by bpf_rbtree_add(), leading to a Use-After-Free?

> +				verbose(env, "can't res_spin_{lock,unlock} in rbtree cb\n");
> +				return -EACCES;
> +			}
> +
>  			if (reg->type != PTR_TO_MAP_VALUE && reg->type != (PTR_TO_BTF_ID | MEM_ALLOC)) {
>  				verbose(env, "%s doesn't point to map value or allocated object\n",
>  					reg_arg_name(env, argno));

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903144433.1716731-1-memxor@gmail.com?part=5