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

[PATCH net v2] 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.

v2: validation of "initial quantum" is done in fq_policy, instead of open
    coding in fq_change() _ suggested by Jakub Kicinski

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")
Reviewed-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
 net/sched/sch_fq.c                            |  6 ++++-
 .../tc-testing/tc-tests/qdiscs/fq.json        | 22 +++++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/net/sched/sch_fq.c b/net/sched/sch_fq.c
index 48d14fb90ba0..f59a2cb2c803 100644
--- a/net/sched/sch_fq.c
+++ b/net/sched/sch_fq.c
@@ -779,13 +779,17 @@ static int fq_resize(struct Qdisc *sch, u32 log)
 	return 0;
 }
 
+static struct netlink_range_validation iq_range = {
+	.max = INT_MAX,
+};
+
 static const struct nla_policy fq_policy[TCA_FQ_MAX + 1] = {
 	[TCA_FQ_UNSPEC]			= { .strict_start_type = TCA_FQ_TIMER_SLACK },
 
 	[TCA_FQ_PLIMIT]			= { .type = NLA_U32 },
 	[TCA_FQ_FLOW_PLIMIT]		= { .type = NLA_U32 },
 	[TCA_FQ_QUANTUM]		= { .type = NLA_U32 },
-	[TCA_FQ_INITIAL_QUANTUM]	= { .type = NLA_U32 },
+	[TCA_FQ_INITIAL_QUANTUM]	= NLA_POLICY_FULL_RANGE(NLA_U32, &iq_range),
 	[TCA_FQ_RATE_ENABLE]		= { .type = NLA_U32 },
 	[TCA_FQ_FLOW_DEFAULT_RATE]	= { .type = NLA_U32 },
 	[TCA_FQ_FLOW_MAX_RATE]		= { .type = NLA_U32 },
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 v2] net/sched: sch_fq: fix integer overflow of "credit"

[Pedro Tammela]

On 20/04/2023 11:59, Davide Caratti 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.
> 
> v2: validation of "initial quantum" is done in fq_policy, instead of open
>      coding in fq_change() _ suggested by Jakub Kicinski
> 
> 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")
> Reviewed-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---
>   net/sched/sch_fq.c                            |  6 ++++-
>   .../tc-testing/tc-tests/qdiscs/fq.json        | 22 +++++++++++++++++++
>   2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/net/sched/sch_fq.c b/net/sched/sch_fq.c
> index 48d14fb90ba0..f59a2cb2c803 100644
> --- a/net/sched/sch_fq.c
> +++ b/net/sched/sch_fq.c
> @@ -779,13 +779,17 @@ static int fq_resize(struct Qdisc *sch, u32 log)
>   	return 0;
>   }
>   
> +static struct netlink_range_validation iq_range = {
> +	.max = INT_MAX,
> +};
> +
>   static const struct nla_policy fq_policy[TCA_FQ_MAX + 1] = {
>   	[TCA_FQ_UNSPEC]			= { .strict_start_type = TCA_FQ_TIMER_SLACK },
>   
>   	[TCA_FQ_PLIMIT]			= { .type = NLA_U32 },
>   	[TCA_FQ_FLOW_PLIMIT]		= { .type = NLA_U32 },
>   	[TCA_FQ_QUANTUM]		= { .type = NLA_U32 },
> -	[TCA_FQ_INITIAL_QUANTUM]	= { .type = NLA_U32 },
> +	[TCA_FQ_INITIAL_QUANTUM]	= NLA_POLICY_FULL_RANGE(NLA_U32, &iq_range),
>   	[TCA_FQ_RATE_ENABLE]		= { .type = NLA_U32 },
>   	[TCA_FQ_FLOW_DEFAULT_RATE]	= { .type = NLA_U32 },
>   	[TCA_FQ_FLOW_MAX_RATE]		= { .type = NLA_U32 },
> 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",

You probably don't want to backport the test as well? If so I would 
break this patch into two.

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

[Jakub Kicinski]

On Thu, 20 Apr 2023 13:25:33 -0300 Pedro Tammela wrote:
> >           "id": "9398",
> >           "name": "Create FQ with maxrate setting",  
> 
> You probably don't want to backport the test as well? If so I would 
> break this patch into two.

IIRC the preference of stable folks is to backport more not fewer
selftests. Practicality of that aside, I think the patch is good as is.

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

[Pedro Tammela]

On 20/04/2023 20:24, Jakub Kicinski wrote:
> On Thu, 20 Apr 2023 13:25:33 -0300 Pedro Tammela wrote:
>>>            "id": "9398",
>>>            "name": "Create FQ with maxrate setting",
>>
>> You probably don't want to backport the test as well? If so I would
>> break this patch into two.
> 
> IIRC the preference of stable folks is to backport more not fewer
> selftests. Practicality of that aside, I think the patch is good as is.

My concern here was mostly due to conflicts with the tdc code base. 
Davide explained to me privately that he will take care of this so it's 
all good.

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

[Davide Caratti]

hi,

On Fri, Apr 21, 2023 at 4:24 PM Pedro Tammela <pctammela@mojatatu.com> wrote:
>
> On 20/04/2023 20:24, Jakub Kicinski wrote:

[...]

> > IIRC the preference of stable folks is to backport more not fewer
> > selftests. Practicality of that aside, I think the patch is good as is.
>
> My concern here was mostly due to conflicts with the tdc code base.
> Davide explained to me privately that he will take care of this so it's
> all good.

right, and I should color this reply-to-all button in a way that
catches better my attention :)

the tdc code will be a merge conflict on kernels that don't have [1]
(v6.0 and v5.x), so I will skip the tdc testcase for the stable
backport (unless somebody has objections).

thanks,
-- 
davide

[1] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=9e274718cc050874761

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 20 Apr 2023 16:59:46 +0200 you 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.
> 
> [...]

Here is the summary with links:
  - [net,v2] net/sched: sch_fq: fix integer overflow of "credit"
    https://git.kernel.org/netdev/net/c/7041101ff6c3

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html