Re: [PATCH bpf-next 22/26] bpf: Remove scalar ids from regsafe()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] bpf: alpha-renaming of struct_ops ctx_arg_info ref_id in
  check_ids() [1]
--

commit 4ddf1d7e2a622751a128b84cb218611e091c605f
Author: Alexei Starovoitov <ast@kernel.org>

bpf: Remove scalar ids from regsafe()

This commit removes check_scalar_ids() and the scalar ID comparison logic from
regsafe(), as scalar registers no longer carry IDs.

> diff --git a/kernel/bpf/states.c b/kernel/bpf/states.c
> index b606763496552..48359e789faad 100644
> --- a/kernel/bpf/states.c
> +++ b/kernel/bpf/states.c
> @@ -58,7 +58,7 @@ static bool check_ids(u32 old_id, u32 cur_id, struct bpf_idmap *idmap)
>  		return true;
>  
>  	/* a name of what the caller has, see BPF_INHERITED_ID */
> -	if (((old_id | cur_id) & ~BPF_ADD_CONST) >= BPF_INHERITED_ID)
> +	if ((old_id | cur_id) >= BPF_INHERITED_ID)
>  		return old_id == cur_id;

[Severity: Critical]
This problem wasn't introduced by this patch, but can check_ids() [1]
map an unrelated reference ID in cur->refs[] to a struct_ops __ref context
argument's fixed aux->ctx_arg_info[i].ref_id during refsafe()?

In do_check_common(), the verifier acquires a reference for each __ref
struct_ops context argument and stores the literal ID in
aux->ctx_arg_info[i].ref_id, while BPF_REG_1 (PTR_TO_CTX) keeps id == 0.
When the program later loads that argument from PTR_TO_CTX, btf_ctx_access()
populates info->ref_id from ctx_arg_info[i].ref_id, and __check_ctx_access()
checks whether that literal ID is still in env->cur_state->refs[]:

kernel/bpf/verifier.c:__check_ctx_access() {
	...
		if (base_type(info->reg_type) == PTR_TO_BTF_ID) {
			if (info->ref_id &&
			    !find_reference_state(env->cur_state, info->ref_id)) {
				verbose(env, "invalid bpf_context access off=%d. Reference may already be released\n",
					off);
				return -EACCES;
			}
		}
	...
}

Because ctx_arg_info[i].ref_id is a normal ID below BPF_INHERITED_ID and is
not anchored to PTR_TO_CTX, refsafe() -> check_ids() allows alpha-renaming it.
If an earlier branch keeps the initial context reference (id = 1) and a later
branch releases id = 1, acquires an unrelated reference (id = 2), and reaches
the same pruning point with PTR_TO_CTX still live, check_ids() maps cur_id = 2
to old_id = 1 and prunes the second branch.  If the code after the merge point
reloads the __ref argument from PTR_TO_CTX, the second path uses or releases
the already-released context object, causing a use-after-free, double free, or
reference leak.

[1] https://sashiko.dev/#/bug/linux-3dbabad3-179e-4dd4-a907-20f15cf47b12

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109064715.2382452-1-alexei.starovoitov@gmail.com?part=22