Messages in this thread | | | Date | Sat, 28 Jan 2023 14:43:28 +0100 | From | Simon Horman <> | Subject | Re: [PATCH] net: sched: sch: Bounds check priority |
| |
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 >
| |