* Re: [PATCH bpf-next 1/2] bpf: Print breakdown of insns processed by subprogs [not found] ` <20260428230844.EFA64C2BCAF@smtp.kernel.org> @ 2026-04-28 23:55 ` Kumar Kartikeya Dwivedi 2026-04-29 0:07 ` Kumar Kartikeya Dwivedi 0 siblings, 1 reply; 5+ messages in thread From: Kumar Kartikeya Dwivedi @ 2026-04-28 23:55 UTC (permalink / raw) To: sashiko, Paul Chaignon; +Cc: bpf 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Print breakdown of insns processed by subprogs 2026-04-28 23:55 ` [PATCH bpf-next 1/2] bpf: Print breakdown of insns processed by subprogs Kumar Kartikeya Dwivedi @ 2026-04-29 0:07 ` Kumar Kartikeya Dwivedi 2026-04-29 14:01 ` Paul Chaignon 0 siblings, 1 reply; 5+ messages in thread From: Kumar Kartikeya Dwivedi @ 2026-04-29 0:07 UTC (permalink / raw) To: Kumar Kartikeya Dwivedi, sashiko, Paul Chaignon; +Cc: bpf 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. diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index b48389b48eb6..b9266e3d46c7 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -18281,22 +18281,24 @@ static int do_check_main(struct bpf_verifier_env *env) static void print_verification_stats(struct bpf_verifier_env *env) { + /* Skip over hidden subprogs which are not verified. */ + int subprog_cnt = env->subprog_cnt - env->hidden_subprog_cnt; int i; if (env->log.level & BPF_LOG_STATS) { verbose(env, "verification time %lld usec\n", div_u64(env->verification_time, 1000)); verbose(env, "stack depth "); - for (i = 0; i < env->subprog_cnt; i++) { + for (i = 0; i < subprog_cnt; i++) { u32 depth = env->subprog_info[i].stack_depth; verbose(env, "%d", depth); - if (i + 1 < env->subprog_cnt) + if (i + 1 < subprog_cnt) verbose(env, "+"); } verbose(env, "\n"); verbose(env, "insns processed %d", env->subprog_info[0].insn_processed); - for (i = 1; i < env->subprog_cnt; i++) { + for (i = 1; i < subprog_cnt; i++) { if (!bpf_subprog_is_global(env, i)) continue; verbose(env, "+%d", env->subprog_info[i].insn_processed); ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Print breakdown of insns processed by subprogs 2026-04-29 0:07 ` Kumar Kartikeya Dwivedi @ 2026-04-29 14:01 ` Paul Chaignon 2026-04-29 15:53 ` Paul Chaignon 0 siblings, 1 reply; 5+ messages in thread From: Paul Chaignon @ 2026-04-29 14:01 UTC (permalink / raw) To: Kumar Kartikeya Dwivedi; +Cc: sashiko, bpf 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"). 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"? diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 03f9e16c2abe..8dfe7da76258 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -18274,19 +18274,15 @@ static int do_check_main(struct bpf_verifier_env *env) static void print_verification_stats(struct bpf_verifier_env *env) { - int i; + /* Skip over hidden subprogs which are not verified. */ + int i, subprog_cnt = env->subprog_cnt - env->hidden_subprog_cnt; if (env->log.level & BPF_LOG_STATS) { verbose(env, "verification time %lld usec\n", div_u64(env->verification_time, 1000)); - verbose(env, "stack depth "); - for (i = 0; i < env->subprog_cnt; i++) { - u32 depth = env->subprog_info[i].stack_depth; - - verbose(env, "%d", depth); - if (i + 1 < env->subprog_cnt) - verbose(env, "+"); - } + verbose(env, "stack depth %d", env->subprog_info[0].stack_depth); + for (i = 1; i < subprog_cnt; i++) + verbose(env, "+%d", env->subprog_info[i].stack_depth); verbose(env, "\n"); } verbose(env, "processed %d insns (limit %d) max_states_per_insn %d " ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Print breakdown of insns processed by subprogs 2026-04-29 14:01 ` Paul Chaignon @ 2026-04-29 15:53 ` Paul Chaignon 2026-04-29 22:17 ` Kumar Kartikeya Dwivedi 0 siblings, 1 reply; 5+ messages in thread From: Paul Chaignon @ 2026-04-29 15:53 UTC (permalink / raw) To: Kumar Kartikeya Dwivedi; +Cc: sashiko, bpf 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"? [...] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Print breakdown of insns processed by subprogs 2026-04-29 15:53 ` Paul Chaignon @ 2026-04-29 22:17 ` Kumar Kartikeya Dwivedi 0 siblings, 0 replies; 5+ messages in thread From: Kumar Kartikeya Dwivedi @ 2026-04-29 22:17 UTC (permalink / raw) To: Paul Chaignon; +Cc: sashiko, bpf On Wed, 29 Apr 2026 at 17:53, Paul Chaignon <paul.chaignon@gmail.com> 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 <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. 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: stable@vger.kernel.org"? > > [...] > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-04-29 22:18 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <5c3514dbc879f22ba3f9eff8a2c730e13bd0572c.1777388107.git.paul.chaignon@gmail.com>
[not found] ` <20260428230844.EFA64C2BCAF@smtp.kernel.org>
2026-04-28 23:55 ` [PATCH bpf-next 1/2] bpf: Print breakdown of insns processed by subprogs Kumar Kartikeya Dwivedi
2026-04-29 0:07 ` Kumar Kartikeya Dwivedi
2026-04-29 14:01 ` Paul Chaignon
2026-04-29 15:53 ` Paul Chaignon
2026-04-29 22:17 ` Kumar Kartikeya Dwivedi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox