Re: [PATCH bpf-next v1 03/20] bpf: Refuse bpf_task_park() in callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 2 related pre-existing issues it does not
introduce:
- [Critical] bpf: missing in_rbtree_lock_required_cb() check in
  check_func_arg() [1]
- [Critical] bpf: missing parent_id propagation in check_kfunc_call()
  [2]
--

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

bpf: Refuse bpf_task_park() in callbacks

This commit rejects calls to bpf_task_park() inside synchronous,
asynchronous, and exception callback frames by walking the active frames in
the verifier state.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index e90cc127aedb8..b42782c7f209d 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -11222,6 +11222,31 @@ static void invalidate_non_owning_refs(struct bpf_verifier_env *env)
>  	}));
>  }
>  
> +/*
> + * bpf_task_park() is confined to the main body of the program and the
> + * functions it calls directly. A callback runs from kernel code that holds
> + * state of its own around the call, an iteration position, a work item or
> + * timer that owns the program only through Tasks Trace RCU, an exiting task,
> + * none of it audited against a sleep that drops the program's protection.
> + * Refuse the park in any callback frame, synchronous, asynchronous or
> + * exception. Asynchronous callbacks are verified as a state of their own
> + * with the callback as frame 0, so the walk over the frames sees them too.
> + */
> +static bool in_callback_frames(struct bpf_verifier_env *env)
> +{
> +	struct bpf_verifier_state *vstate = env->cur_state;
> +	int i;
> +
> +	for (i = 0; i <= vstate->curframe; i++) {
> +		struct bpf_func_state *frame = vstate->frame[i];
> +
> +		if (frame->in_callback_fn || frame->in_async_callback_fn ||
> +		    frame->in_exception_callback_fn)
> +			return true;
> +	}
> +	return false;
> +}

[Severity: Critical]
This problem wasn't introduced by this patch, but can an rbtree comparator
callback corrupt the tree via [1] because check_func_arg() skips
in_rbtree_lock_required_cb() for bpf_rbtree_add_impl()?

kernel/bpf/verifier.c:check_func_arg() {
	...
	case ARG_PTR_TO_RB_NODE:
		if (is_bpf_rbtree_add_kfunc(meta->func_id)) {
			if (reg->type != (PTR_TO_BTF_ID | MEM_ALLOC)) {
				verbose(env, "%s expected pointer to allocated object\n",
					reg_arg_name(env, argno));
				return -EINVAL;
			}
			if (!reg_is_referenced(env, reg)) {
				verbose(env, "allocated object must be referenced\n");
				return -EINVAL;
			}
		} else {
			...
			if (in_rbtree_lock_required_cb(env)) {
				verbose(env, "%s not allowed in rbtree cb\n", meta->func_name);
				return -EINVAL;
			}
		}
}

While the outer __bpf_rbtree_add() is mid-traversal inside bpf_call_cb(),
the comparator callback can acquire an owning node reference (for example,
via bpf_list_pop_front()) and invoke bpf_rbtree_add_impl() with a second
comparator subprogram. The nested insertion links the node and rebalances
the tree under the already-held lock, leaving the outer __bpf_rbtree_add()
to finish linking with stale parent, link, and leftmost state when the
callback returns.

> @@ -16366,6 +16391,12 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  	}
>  
>  	if (is_bpf_task_park_kfunc(meta.func_id)) {
> +		if (in_callback_frames(env)) {
> +			verbose(env, "bpf_task_park() is not allowed in a callback\n");
> +			bpf_diag_policy(env, insn_idx, "kfunc bpf_task_park", "the call is in a callback",
> +					"Call bpf_task_park() from the main program or from a function it calls directly, not from a callback.");
> +			return -EINVAL;
> +		}
>  		err = check_park_stack_objects(env, insn_idx);
>  		if (err)
>  			return err;

[Severity: Critical]
This problem wasn't introduced by this patch, but does check_kfunc_call()
fail to track the lifetime of a returned PTR_TO_MEM register [2] when
the kfunc argument is a borrowed pointer?

kernel/bpf/verifier.c:check_kfunc_call() {
			...
			mark_reg_known_zero(env, regs, BPF_REG_0);
			regs[BPF_REG_0].type = PTR_TO_MEM;
			regs[BPF_REG_0].mem_size = meta.ret_mem.size;
			...
			if (meta.ref_obj.id) {
				err = validate_ref_obj(env, &meta.ref_obj);
				if (err)
					return err;
				regs[BPF_REG_0].parent_id = meta.ref_obj.id;
			}
}

For a borrowed pointer argument, meta.ref_obj.id is 0 and the parent
object's lifetime is recorded in meta.ref_obj.parent_id instead. Ignoring
meta.ref_obj.parent_id leaves regs[BPF_REG_0].parent_id unset, so releasing
or invalidating the underlying parent object does not invalidate R0,
allowing a use-after-free through the returned PTR_TO_MEM pointer.

[1] https://sashiko.dev/#/bug/linux-eb46b3eb-92d0-4fac-b401-30327c613b94
[2] https://sashiko.dev/#/bug/linux-3da483d6-33c8-4529-a20e-81cdfb7a8ed3

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794269506.git.memxor@gmail.com?part=3