[PATCH net] net/sched: sch_fq: fix integer overflow of "credit"

[PATCH net] net/sched: sch_fq: fix integer overflow of "credit"

[Davide Caratti]

if sch_fq is configured with "initial quantum" having values greater than
INT_MAX, the first assignment of "credit" does signed integer overflow to
a very negative value.
In this situation, the syzkaller script provided by Cristoph triggers the
CPU soft-lockup warning even with few sockets. It's not an infinite loop,
but "credit" wasn't probably meant to be minus 2Gb for each new flow.
Capping "initial quantum" to INT_MAX proved to fix the issue.

Reported-by: Christoph Paasch <cpaasch@apple.com>
Link: https://github.com/multipath-tcp/mptcp_net-next/issues/377
Fixes: afe4fd062416 ("pkt_sched: fq: Fair Queue packet scheduler")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
 net/sched/sch_fq.c                            | 12 ++++++++--
 .../tc-testing/tc-tests/qdiscs/fq.json        | 22 +++++++++++++++++++
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_fq.c b/net/sched/sch_fq.c
index 48d14fb90ba0..12efbcfc2938 100644
--- a/net/sched/sch_fq.c
+++ b/net/sched/sch_fq.c
@@ -842,8 +842,16 @@ static int fq_change(struct Qdisc *sch, struct nlattr *opt,
 		}
 	}
 
-	if (tb[TCA_FQ_INITIAL_QUANTUM])
-		q->initial_quantum = nla_get_u32(tb[TCA_FQ_INITIAL_QUANTUM]);
+	if (tb[TCA_FQ_INITIAL_QUANTUM]) {
+		u32 initial_quantum = nla_get_u32(tb[TCA_FQ_INITIAL_QUANTUM]);
+
+		if (initial_quantum <= INT_MAX) {
+			q->initial_quantum = initial_quantum;
+		} else {
+			NL_SET_ERR_MSG_MOD(extack, "invalid initial quantum");
+			err = -EINVAL;
+		}
+	}
 
 	if (tb[TCA_FQ_FLOW_DEFAULT_RATE])
 		pr_warn_ratelimited("sch_fq: defrate %u ignored.\n",
diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq.json
index 8acb904d1419..3593fb8f79ad 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq.json
@@ -114,6 +114,28 @@
             "$IP link del dev $DUMMY type dummy"
         ]
     },
+    {
+        "id": "10f7",
+        "name": "Create FQ with invalid initial_quantum setting",
+        "category": [
+            "qdisc",
+            "fq"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [
+            "$IP link add dev $DUMMY type dummy || /bin/true"
+        ],
+        "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root fq initial_quantum 0x80000000",
+        "expExitCode": "2",
+        "verifyCmd": "$TC qdisc show dev $DUMMY",
+        "matchPattern": "qdisc fq 1: root.*initial_quantum 2048Mb",
+        "matchCount": "0",
+        "teardown": [
+            "$IP link del dev $DUMMY type dummy"
+        ]
+    },
     {
         "id": "9398",
         "name": "Create FQ with maxrate setting",
-- 
2.39.2

Re: [PATCH net] net/sched: sch_fq: fix integer overflow of "credit"

[Eric Dumazet]

On Mon, Apr 17, 2023 at 1:02 PM Davide Caratti <dcaratti@redhat.com> wrote:
>
> if sch_fq is configured with "initial quantum" having values greater than
> INT_MAX, the first assignment of "credit" does signed integer overflow to
> a very negative value.
> In this situation, the syzkaller script provided by Cristoph triggers the
> CPU soft-lockup warning even with few sockets. It's not an infinite loop,
> but "credit" wasn't probably meant to be minus 2Gb for each new flow.
> Capping "initial quantum" to INT_MAX proved to fix the issue.
>
> Reported-by: Christoph Paasch <cpaasch@apple.com>
> Link: https://github.com/multipath-tcp/mptcp_net-next/issues/377
> Fixes: afe4fd062416 ("pkt_sched: fq: Fair Queue packet scheduler")
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---

Reviewed-by: Eric Dumazet <edumazet@google.com>
Thanks.

Re: [PATCH net] net/sched: sch_fq: fix integer overflow of "credit"

[Jakub Kicinski]

On Mon, 17 Apr 2023 13:02:40 +0200 Davide Caratti wrote:
> +		u32 initial_quantum = nla_get_u32(tb[TCA_FQ_INITIAL_QUANTUM]);
> +
> +		if (initial_quantum <= INT_MAX) {
> +			q->initial_quantum = initial_quantum;
> +		} else {
> +			NL_SET_ERR_MSG_MOD(extack, "invalid initial quantum");
> +			err = -EINVAL;
> +		}

Please set the right policy in fq_policy[] instead.

Re: [PATCH net] net/sched: sch_fq: fix integer overflow of "credit"

[Eric Dumazet]

On Tue, Apr 18, 2023 at 4:30 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 17 Apr 2023 13:02:40 +0200 Davide Caratti wrote:
> > +             u32 initial_quantum = nla_get_u32(tb[TCA_FQ_INITIAL_QUANTUM]);
> > +
> > +             if (initial_quantum <= INT_MAX) {
> > +                     q->initial_quantum = initial_quantum;
> > +             } else {
> > +                     NL_SET_ERR_MSG_MOD(extack, "invalid initial quantum");
> > +                     err = -EINVAL;
> > +             }
>
> Please set the right policy in fq_policy[] instead.

Not sure these policies are available for old kernels (sch_fq was
added in linux-3.12) ?

Or have we backported all this infra already on stable kernels ?

commit d06a09b94c618c96ced584dd4611a888c8856b8d
Author: Johannes Berg <johannes.berg@intel.com>
Date:   Thu Apr 30 22:13:08 2020 +0200

    netlink: extend policy range validation

Re: [PATCH net] net/sched: sch_fq: fix integer overflow of "credit"

[Jakub Kicinski]

On Tue, 18 Apr 2023 12:26:13 +0200 Eric Dumazet wrote:
> > Please set the right policy in fq_policy[] instead.  
> 
> Not sure these policies are available for old kernels (sch_fq was
> added in linux-3.12) ?
> 
> Or have we backported all this infra already on stable kernels ?
> 
> commit d06a09b94c618c96ced584dd4611a888c8856b8d
> Author: Johannes Berg <johannes.berg@intel.com>
> Date:   Thu Apr 30 22:13:08 2020 +0200
> 
>     netlink: extend policy range validation

Good point, Davide, once the policy based fix hits the trees please
send this version of the fix to stable@ for 5.4 and older stable trees.

Re: [PATCH net] net/sched: sch_fq: fix integer overflow of "credit"

[Davide Caratti]

hello,

On Tue, Apr 18, 2023 at 4:43 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 18 Apr 2023 12:26:13 +0200 Eric Dumazet wrote:
> > > Please set the right policy in fq_policy[] instead.
> >
> > Not sure these policies are available for old kernels (sch_fq was
> > added in linux-3.12) ?

[...]

> Good point, Davide, once the policy based fix hits the trees please
> send this version of the fix to stable@ for 5.4 and older stable trees.

Sure, I will do that. And thanks for the review! :)
--
davide