Re: [PATCH bpf-next 1/2] bpf: Print breakdown of insns processed by subprogs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Out-of-bounds read in `bpf_subprog_is_global()` when printing verification stats for programs with hidden subprograms.
--

commit 08128f883e58301e0f97b1d4a94656be63b8f790
Author: Paul Chaignon <paul.chaignon@gmail.com>

bpf: Print breakdown of insns processed by subprogs

This patch tracks the number of instructions processed by each global
function (subprog) during BPF verification. It then reports this
breakdown in the verifier logs under BPF_LOG_STATS.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 03f9e16c2abe..b48389b48eb6 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -18288,6 +18295,13 @@ static void print_verification_stats(struct bpf_verifier_env *env)
>  				verbose(env, "+");
>  		}
>  		verbose(env, "\n");
> +		verbose(env, "insns processed %d", env->subprog_info[0].insn_processed);
> +		for (i = 1; i < env->subprog_cnt; i++) {
> +			if (!bpf_subprog_is_global(env, i))

Could this introduce a regression due to an out-of-bounds read on the
func_info_aux array if there are hidden subprograms?

At load time, if BTF is present, env->prog->aux->func_info_aux is allocated
with exactly func_info_cnt elements, and env->subprog_cnt initially matches
func_info_cnt.

During verification, if a feature like bpf_throw is used without an explicit
callback, add_hidden_subprog() is called which increments env->subprog_cnt
but does not resize the func_info_aux array.

Since this loop iterates up to the updated env->subprog_cnt, when i reaches
the index of a hidden subprogram, bpf_subprog_is_global() will access
aux[i].linkage where i >= func_info_cnt:

kernel/bpf/verifier.c:bpf_subprog_is_global() {
	struct bpf_func_info_aux *aux = env->prog->aux->func_info_aux;

	return aux && aux[subprog].linkage == BTF_FUNC_GLOBAL;
}

Is it possible to skip hidden subprograms here, or limit the check to
func_info_cnt?

> +				continue;
> +			verbose(env, "+%d", env->subprog_info[i].insn_processed);
> +		}
> +		verbose(env, "\n");
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/5c3514dbc879f22ba3f9eff8a2c730e13bd0572c.1777388107.git.paul.chaignon@gmail.com?part=1