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

On Wed, 29 Apr 2026 at 17:53, Paul Chaignon <[email protected]> wrote:
>
> 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 <[email protected]>
> > > >>
> > > >> 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.

Yeah, I don't think it was buggy before this, e.g. adjust_btf_func()
explicitly avoids touching func_info for this reason.
Just feel free to fold the diff into your commit and resend, no need
to send a separate change or make me co author.

>
> >
> > 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: [email protected]"?
>
> [...]
>