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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4e8324cf-e6de-acff-5e30-373d015a3cb4@mojatatu.com>
Date:   Thu, 20 Apr 2023 13:25:33 -0300
From:   Pedro Tammela <pctammela@...atatu.com>
To:     Davide Caratti <dcaratti@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>, Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH net v2] net/sched: sch_fq: fix integer overflow of
 "credit"

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@...le.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@...gle.com>
> Signed-off-by: Davide Caratti <dcaratti@...hat.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.