Re: [PATCH bpf-next v6 1/6] bpf: Track verifier instruction stats for each subprogram

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] A NULL pointer dereference occurs in `account_current_path()` when handling an allocation failure during `pop_stack()`.
--

--- Patch [1]: [PATCH bpf-next v6 1/6] bpf: Track verifier instruction stats for each subprogram ---
commit 026daf56a558e1775b6ccd02d21953d3b59b6a28
Author: Kumar Kartikeya Dwivedi <[email protected]>

bpf: Track verifier instruction stats for each subprogram
    
This patch tracks verifier instruction counts for each subprogram
independently. It records both own and inclusive instruction counts,
updating the subtotals during function returns and when a path terminates
or is pruned.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 7439afdc851a7..88e7ea6fbe73a 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -9807,6 +9809,37 @@ static int set_task_work_schedule_callback_state(struct bpf_verifier_env *env,
>  
>  static bool is_rbtree_lock_required_kfunc(u32 btf_id);
>  
> +static void account_processed_insn(struct bpf_verifier_env *env)
> +{
> +	struct bpf_func_state *frame = cur_func(env);
> +
> +	env->insn_processed++;
> +	frame->insns_subtotal++;
> +	env->subprog_info[frame->subprogno].insns_own++;
> +}
> +
> +static void account_processed_insns(struct bpf_verifier_env *env,
> +				    struct bpf_func_state *callee,
> +				    struct bpf_func_state *caller)
> +{
> +	u32 insns = callee->insns_subtotal;
                    ^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Could `callee` be NULL here on error paths?

If a memory allocation failure happens during `pop_stack()` (for example, if
`kzalloc_obj()` fails while allocating a new frame during the verifier state
copy), the state can be partially copied, leaving `state->frame[i]` as NULL
for some `i <= state->curframe`.

When `do_check_common()` handles this error, it unconditionally calls
`account_current_path()`. The loop in `account_current_path()` iterates up to
`state->curframe` and passes `state->frame[frame]` to
`account_processed_insns()`, which would then unconditionally dereference the
NULL pointer.

Should we check `if (!callee)` before attempting to read its subtotal?

> +
> +	env->subprog_info[callee->subprogno].insns_total += insns;
> +	if (caller)
> +		caller->insns_subtotal += insns;
> +	callee->insns_subtotal = 0;
> +}
> +
> +static void account_current_path(struct bpf_verifier_env *env)
> +{
> +	struct bpf_verifier_state *state = env->cur_state;
> +	int frame;
> +
> +	for (frame = state->curframe; frame >= 0; frame--)
> +		account_processed_insns(env, state->frame[frame],
> +					frame ? state->frame[frame - 1] : NULL);
> +}

[ ... ]

> @@ -18680,6 +18717,7 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
>  
>  	ret = do_check(env);
>  out:
> +	account_current_path(env);
>  	if (!ret && pop_log)
>  		bpf_vlog_reset(&env->log, 0);
>  	free_states(env);

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