Skip to contentSkip to conversation

net: sched: sch: Bounds check priority

PATCH Applied Kees Cook 4 participants 4 messages Jan 27, 2023 – Jan 31, 2023

Subsystems (3)
Networking Network drivers Traffic control (tc)
lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH 1/1 net: sched: sch: Bounds check priority

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 <[email protected]>
Cc Cong Wang <[email protected]>
Cc Jiri Pirko <[email protected]>
Cc "David S. Miller" <[email protected]>
Cc Eric Dumazet <[email protected]>
Cc Jakub Kicinski <[email protected]>
Cc Paolo Abeni <[email protected]>
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
---
 net/sched/sch_htb.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
net/sched/sch_htb.c +4 -1
index f46643850df8..cc28e41fb745 100644
@@ -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)
Signature · 1 line
2.34.1
Reviewed-by on v1 1/1
#1 lore raw
Quoted 11 lines · Kees Cook

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];
| ^~~~~~

...

Quoted 4 lines
Signed-off-by Kees Cook <[email protected]>
---
 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 <[email protected]>
Quoted 19 lines
net/sched/sch_htb.c +4 -1
index f46643850df8..cc28e41fb745 100644
@@ -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)
Signature · 1 line
2.34.1
Reviewed-by on v1 1/1
#2 lore raw
In reply to Kees Cook
Quoted 10 lines · Kees Cook

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 <[email protected]>

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.

CI Applied
#3 lore raw
In reply to Kees Cook
This patch was applied to netdev/net.git (master) by Paolo Abeni <[email protected]>:

Hello:

This patch was applied to netdev/net.git (master)
by Paolo Abeni <[email protected]>:

Quoted 6 lines · 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)

Quoted 6 lines
      |                             ~~~~~~~~~~~~~~~^~~~~~

../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!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html