| Message ID | 20230127224036.never.561-kees@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | de5ca4c3852f896cacac2bf259597aab5e17d9e3 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: sched: sch: Bounds check priority | expand |
On Fri, Jan 27, 2023 at 02:40:37PM -0800, Kees Cook wrote: > Nothing was explicitly bounds checking the priority index used to access > clpriop[]. WARN and bail out early if it's pathological. Seen with GCC 13: > > ../net/sched/sch_htb.c: In function 'htb_activate_prios': > ../net/sched/sch_htb.c:437:44: warning: array subscript [0, 31] is outside array bounds of 'struct htb_prio[8]' [-Warray-bounds=] > 437 | if (p->inner.clprio[prio].feed.rb_node) > | ~~~~~~~~~~~~~~~^~~~~~ > ../net/sched/sch_htb.c:131:41: note: while referencing 'clprio' > 131 | struct htb_prio clprio[TC_HTB_NUMPRIO]; > | ^~~~~~ > ... > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > net/sched/sch_htb.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) I'm not sure what will happen if we hit the 'break' case. But I also think that warning and bailing out is an improvement on whatever happens now if that scenario is hit. Reviewed-by: Simon Horman <simon.horman@corigine.com> > diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c > index f46643850df8..cc28e41fb745 100644 > --- a/net/sched/sch_htb.c > +++ b/net/sched/sch_htb.c > @@ -431,7 +431,10 @@ static void htb_activate_prios(struct htb_sched *q, struct htb_class *cl) > while (cl->cmode == HTB_MAY_BORROW && p && mask) { > m = mask; > while (m) { > - int prio = ffz(~m); > + unsigned int prio = ffz(~m); > + > + if (WARN_ON_ONCE(prio > ARRAY_SIZE(p->inner.clprio))) > + break; > m &= ~(1 << prio); > > if (p->inner.clprio[prio].feed.rb_node) > -- > 2.34.1 >
On Fri, Jan 27, 2023 at 02:40:37PM -0800, Kees Cook wrote: > Nothing was explicitly bounds checking the priority index used to access > clpriop[]. WARN and bail out early if it's pathological. Seen with GCC 13: > > ../net/sched/sch_htb.c: In function 'htb_activate_prios': > ../net/sched/sch_htb.c:437:44: warning: array subscript [0, 31] is outside array bounds of 'struct htb_prio[8]' [-Warray-bounds=] > 437 | if (p->inner.clprio[prio].feed.rb_node) > | ~~~~~~~~~~~~~~~^~~~~~ > ../net/sched/sch_htb.c:131:41: note: while referencing 'clprio' > 131 | struct htb_prio clprio[TC_HTB_NUMPRIO]; > | ^~~~~~ Reviewed-by: Cong Wang <cong.wang@bytedance.com> We already have a check in htb_change_class(): 2056 if ((cl->prio = hopt->prio) >= TC_HTB_NUMPRIO) 2057 cl->prio = TC_HTB_NUMPRIO - 1; so this patch is just to make GCC 13 happy. Thanks.
Hello: This patch was applied to netdev/net.git (master) by Paolo Abeni <pabeni@redhat.com>: On Fri, 27 Jan 2023 14:40:37 -0800 you wrote: > Nothing was explicitly bounds checking the priority index used to access > clpriop[]. WARN and bail out early if it's pathological. Seen with GCC 13: > > ../net/sched/sch_htb.c: In function 'htb_activate_prios': > ../net/sched/sch_htb.c:437:44: warning: array subscript [0, 31] is outside array bounds of 'struct htb_prio[8]' [-Warray-bounds=] > 437 | if (p->inner.clprio[prio].feed.rb_node) > | ~~~~~~~~~~~~~~~^~~~~~ > ../net/sched/sch_htb.c:131:41: note: while referencing 'clprio' > 131 | struct htb_prio clprio[TC_HTB_NUMPRIO]; > | ^~~~~~ > > [...] Here is the summary with links: - net: sched: sch: Bounds check priority https://git.kernel.org/netdev/net/c/de5ca4c3852f You are awesome, thank you!
diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c index f46643850df8..cc28e41fb745 100644 --- a/net/sched/sch_htb.c +++ b/net/sched/sch_htb.c @@ -431,7 +431,10 @@ static void htb_activate_prios(struct htb_sched *q, struct htb_class *cl) while (cl->cmode == HTB_MAY_BORROW && p && mask) { m = mask; while (m) { - int prio = ffz(~m); + unsigned int prio = ffz(~m); + + if (WARN_ON_ONCE(prio > ARRAY_SIZE(p->inner.clprio))) + break; m &= ~(1 << prio); if (p->inner.clprio[prio].feed.rb_node)
Nothing was explicitly bounds checking the priority index used to access clpriop[]. WARN and bail out early if it's pathological. Seen with GCC 13: ../net/sched/sch_htb.c: In function 'htb_activate_prios': ../net/sched/sch_htb.c:437:44: warning: array subscript [0, 31] is outside array bounds of 'struct htb_prio[8]' [-Warray-bounds=] 437 | if (p->inner.clprio[prio].feed.rb_node) | ~~~~~~~~~~~~~~~^~~~~~ ../net/sched/sch_htb.c:131:41: note: while referencing 'clprio' 131 | struct htb_prio clprio[TC_HTB_NUMPRIO]; | ^~~~~~ Cc: Jamal Hadi Salim <jhs@mojatatu.com> Cc: Cong Wang <xiyou.wangcong@gmail.com> Cc: Jiri Pirko <jiri@resnulli.us> Cc: "David S. Miller" <davem@davemloft.net> Cc: Eric Dumazet <edumazet@google.com> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Paolo Abeni <pabeni@redhat.com> Cc: netdev@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- net/sched/sch_htb.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)