Re: [RFC PATCH bpf-next v1 04/16] bpf: Add the typed_arena_cast instruction

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] bpf: typed arenas leak on late verification failure
- [Medium] bpf: diagnostic log state leak in check_alu_op() on typed arena cast
- [Low] bpf: comment style violation in backtrack_insn()
--

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

bpf: Add the typed_arena_cast instruction

This commit adds the typed_arena_cast instruction, which turns an untrusted
64-bit value into a verifier-trusted pointer to a typed arena object. The
verifier registers the typed arena on first sight, logs it, and lowers the
instruction after verification.

> diff --git a/kernel/bpf/backtrack.c b/kernel/bpf/backtrack.c
> index 0e38b9575328c..38984e52ee375 100644
> --- a/kernel/bpf/backtrack.c
> +++ b/kernel/bpf/backtrack.c
> @@ -328,11 +328,13 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx,
>  				if (sreg != BPF_REG_FP)
>  					bt_set_reg(bt, sreg);
>  			} else {
> -				/* dreg = K
> +				/* dreg = K, or dreg = typed_arena_cast(sreg, imm)
>  				 * dreg needs precision after this insn.
>  				 * Corresponding register is already marked
>  				 * as precise=true in this verifier state.
> -				 * No further markings in parent are necessary
> +				 * No further markings in parent are necessary;
> +				 * a cast yields a pointer that is safe for any
> +				 * value of sreg, which needs no precision.
>  				 */
                                 ^^^

[Severity: Low]
Does this multi-line comment violate the BPF subsystem comment formatting rules?
The opening /* should typically be on its own line for multi-line comments in
BPF subsystem code.

[ ... ]

> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index d3b8b626ec0f7..619f7c6a778d9 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -3085,6 +3085,8 @@ static void bpf_prog_free_deferred(struct work_struct *work)
>  	aux = container_of(work, struct bpf_prog_aux, work);
>  #ifdef CONFIG_BPF_SYSCALL
>  	bpf_free_kfunc_btf_tab(aux->kfunc_btf_tab);
> +	/* The typed arenas outlive the program; only the load's failure retracts them. */
> +	kfree(aux->typed_arenas);
>  #endif
>  #ifdef CONFIG_CGROUP_BPF
>  	if (aux->cgroup_atype != CGROUP_BPF_ATTACH_TYPE_INVALID)

[ ... ]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 03dbc0e003986..a0069983f1036 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -16993,7 +17173,9 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
>  			struct bpf_reg_state *dst_reg = regs + insn->dst_reg;
>  
>  			if (BPF_CLASS(insn->code) == BPF_ALU64) {
> -				if (insn->imm) {
> +				if (insn->off == BPF_TYPED_ARENA_CAST) {
> +					return check_typed_arena_cast(env, insn);
                                        ^^^^

[Severity: Medium]
Does this early return bypass the paired bpf_diag_mod_end(env) cleanup at the
end of check_alu_op()? Leaving the diagnostic state active here fails to record
the register modification in the diagnostic event log.

> +				} else if (insn->imm) {
>  					/* off == BPF_ADDR_SPACE_CAST */
>  					mark_reg_unknown(env, regs, insn->dst_reg);
>  					if (insn->imm == 1) /* cast from as(1) to as(0) */

[ ... ]

> @@ -20580,8 +20764,26 @@ static int resolve_func_ptrs(struct bpf_verifier_env *env)
>  }
>  
>  /* drop refcnt of maps used by the rejected program */
> +/*
> + * Drop the rejected program's typed arena references before its arena map
> + * reference. A typed arena nobody else registered is retracted; one that a
> + * loaded program registered lives on for the map's lifetime.
> + */
> +static void release_typed_arenas(struct bpf_verifier_env *env)
> +{
> +	struct bpf_prog_aux *aux = env->prog->aux;
> +	u32 i;
> +
> +	for (i = 0; i < aux->typed_arena_cnt; i++)
> +		bpf_typed_arena_put(bpf_prog_arena(env->prog), aux->typed_arenas[i]);
> +	kfree(aux->typed_arenas);
> +	aux->typed_arenas = NULL;
> +	aux->typed_arena_cnt = 0;
> +}
> +
>  static void release_maps(struct bpf_verifier_env *env)
>  {
> +	release_typed_arenas(env);
        ^^^^

[Severity: High]
Can this leak the typed arena references?

In bpf_check(), if verification fails after env->prog->aux->used_maps is
populated, the err_release_maps path bypasses calling release_maps(env):

err_release_maps:
	if (ret)
		release_insn_arrays(env);
	if (!env->prog->aux->used_maps)
		release_maps(env);

If release_maps(env) is skipped, release_typed_arenas(env) will not be called.
Then, bpf_prog_free_deferred() only does kfree(aux->typed_arenas) without
dropping the actual bpf_typed_arena_put() refcounts, permanently leaking
the typed arenas and their backing resources.

>  	__bpf_free_used_maps(env->prog->aux, env->used_maps,
>  			     env->used_map_cnt);
>  }

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