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

[Paul Chaignon <paul.chaignon@gmail.com>]

On Wed, Apr 29, 2026 at 04:01:25PM +0200, Paul Chaignon wrote:
> On Wed, Apr 29, 2026 at 02:07:33AM +0200, Kumar Kartikeya Dwivedi wrote:
> > On Wed Apr 29, 2026 at 1:55 AM CEST, Kumar Kartikeya Dwivedi wrote:
> > > On Wed Apr 29, 2026 at 1:08 AM CEST, sashiko-bot wrote:
> > >> 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?
> > >
> > > Don't see it happening, since hidden subprogs never get verified, so we won't
> > > invoke bpf_subprog_is_global() with such a subprog index.
> > 
> > Ah, no, stupid me. We get here after fixing up and adding the hidden subprog. So
> > we can still do OOB since subprog_cnt includes the hidden_subprog_cnt. How about
> > the following as a fix? I checked over other places where we iterate over all of
> > the subprogs and those look fine, so instead of changing bpf_subprog_is_global()
> > we can adjust this function to only consider real subprogs. Didn't compile test.
> 
> That's a nice find! I also doubted it initially as we have that pattern
> everywhere.
> 
> It looks like this would be a fix for commit 335d1c5b5452 ("bpf:
> Implement support for adding hidden subprogs") (or technically, the next
> commit as 335d1c5b5452 didn't have any user). So maybe I can resend as a
> first patch (with you as a co-author) and the following diff (took the
> opportunity to simplify the logic on the assumption that we always have
> the main "subprog").

Just saw that subprog_info is statically allocated (contrary to
func_info_aux) so we wouldn't get an OOB on the existing code, just
some garbage "+0" on the stack depths.

> 
> Not sure if it would need to be sent to bpf instead of bpf-next. Maybe
> keep the patchset on bpf-next, but add "Cc: stable@vger.kernel.org"?

[...]