Message ID | 20230724014625.4087030-1-linma@zju.edu.cn |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:9010:0:b0:3e4:2afc:c1 with SMTP id l16csp1530987vqg; Sun, 23 Jul 2023 19:38:02 -0700 (PDT) X-Google-Smtp-Source: APBJJlFCJVJb1dC/Pd00ov4VI96WjPO51dgzYspMNGqI2cqXuDPfTGEV7SNb2b1zM/NrjXEAgjuJ X-Received: by 2002:a05:6808:b12:b0:3a3:fd7d:1e33 with SMTP id s18-20020a0568080b1200b003a3fd7d1e33mr7878447oij.53.1690166281942; Sun, 23 Jul 2023 19:38:01 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1690166281; cv=none; d=google.com; s=arc-20160816; b=VR5ZVgu1GqksGHXSJX92MN+F3MsIEWG0HMNTZSi/ZN3WxDHWH3HwacEcuTNChg7NA9 L5/HEJcICiJvVVaEDrs4wLAzAtwwBlOXOi8WPDt/vD2sXjNLFopYTf24EYBHhdRZZDW5 phDOOYd3BJjeqKHMMsjb5KN32P5QHUg/YHDsYsdAEa7W/ID6AAgeNyFjv0PqgVjpLm5q 9GeoO8o/x5wvXjiKKL6x4AY0YG/n3J6mC9mpMvZTlNGlA50bxYrMUy2xgd7zv5G9azRg qMRtBM+CTiQXa+2J3j82M/j9GHoD8DsrYJLNypLSpcoHOk6qtEDkWRyT5q8A2ELbP5B/ Hyzw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:message-id:date:subject:cc:to:from; bh=CGChArUp9e3YZjQA64tmB/R9YpQUS1LLCn29iKojH+Y=; fh=9fIMXUWIIPByQg1CyMsg3pjzz3KuHDuP0y2yn3Ep7h0=; b=vdHfT05OmNNsYcx3nuNJZhSDvCp0avW6KYsZvSs16/13UaX1aizffMGXBLDQ4oUdqV hCBibetBw9l79/1nrnrzHpwiGqAiShHCJ75f7DdslG4jodd9M1Jn4ITVx549jj5C658H 92Za2T9l4pgFVAUO9HEqDUjQP8ynPdAxRwCjYuC7VxserBtXvXJD0l2l0idGzwRtT3Rh 4xxUOkoRCry2bKzqSc980UX5ziL5+Tp1QxXY/RZiPBcHe5qU+LExcQzGkZ4DLzAxQZ82 HeAjpydVwAByygV6uOuEHLGMwuLrtgdZArqmpXQ5P8P66IbqfY3M1K7VREbAt9pY6zjR 42rA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id z19-20020a63e113000000b005638183ab90si6727437pgh.611.2023.07.23.19.37.48; Sun, 23 Jul 2023 19:38:01 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229468AbjGXCG1 (ORCPT <rfc822;kautuk.consul.80@gmail.com> + 99 others); Sun, 23 Jul 2023 22:06:27 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41684 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231742AbjGXCGN (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Sun, 23 Jul 2023 22:06:13 -0400 Received: from zg8tndyumtaxlji0oc4xnzya.icoremail.net (zg8tndyumtaxlji0oc4xnzya.icoremail.net [46.101.248.176]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 129A97290; Sun, 23 Jul 2023 18:59:43 -0700 (PDT) Received: from localhost.localdomain (unknown [183.128.129.228]) by mail-app2 (Coremail) with SMTP id by_KCgBXPxrz171kq+dwCg--.54437S4; Mon, 24 Jul 2023 09:46:28 +0800 (CST) From: Lin Ma <linma@zju.edu.cn> To: jhs@mojatatu.com, xiyou.wangcong@gmail.com, jiri@resnulli.us, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, netdev@vger.kernel.org, linux-kernel@vger.kernel.org Cc: Lin Ma <linma@zju.edu.cn> Subject: [PATCH v2] net/sched: mqprio: Add length check for TCA_MQPRIO_{MAX/MIN}_RATE64 Date: Mon, 24 Jul 2023 09:46:25 +0800 Message-Id: <20230724014625.4087030-1-linma@zju.edu.cn> X-Mailer: git-send-email 2.17.1 X-CM-TRANSID: by_KCgBXPxrz171kq+dwCg--.54437S4 X-Coremail-Antispam: 1UD129KBjvJXoW7ZrWfXr1UKr4kWw4UCFWkXrb_yoW8Cr15pF ykJryxtFWDCFn7J393Cws7ZFZY9wsrCF42gFy5Zw4rArn8Wa4aga48WFW29r13Jr4rGws3 Jr1qya47urn0vFDanT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDU0xBIdaVrnRJUUUvm14x267AKxVW8JVW5JwAFc2x0x2IEx4CE42xK8VAvwI8IcIk0 rVWrJVCq3wAFIxvE14AKwVWUJVWUGwA2ocxC64kIII0Yj41l84x0c7CEw4AK67xGY2AK02 1l84ACjcxK6xIIjxv20xvE14v26w1j6s0DM28EF7xvwVC0I7IYx2IY6xkF7I0E14v26rxl 6s0DM28EF7xvwVC2z280aVAFwI0_GcCE3s1l84ACjcxK6I8E87Iv6xkF7I0E14v26rxl6s 0DM2AIxVAIcxkEcVAq07x20xvEncxIr21l5I8CrVACY4xI64kE6c02F40Ex7xfMcIj6xII jxv20xvE14v26r1j6r18McIj6I8E87Iv67AKxVWUJVW8JwAm72CE4IkC6x0Yz7v_Jr0_Gr 1lF7xvr2IYc2Ij64vIr41lF7I21c0EjII2zVCS5cI20VAGYxC7M4IIrI8v6xkF7I0E8cxa n2IY04v7MxkIecxEwVAFwVWkMxAIw28IcxkI7VAKI48JMxC20s026xCaFVCjc4AY6r1j6r 4UMI8I3I0E5I8CrVAFwI0_Jr0_Jr4lx2IqxVCjr7xvwVAFwI0_JrI_JrWlx4CE17CEb7AF 67AKxVWUtVW8ZwCIc40Y0x0EwIxGrwCI42IY6xIIjxv20xvE14v26r1j6r1xMIIF0xvE2I x0cI8IcVCY1x0267AKxVWUJVW8JwCI42IY6xAIw20EY4v20xvaj40_Jr0_JF4lIxAIcVC2 z280aVAFwI0_Jr0_Gr1lIxAIcVC2z280aVCY1x0267AKxVW8JVW8JrUvcSsGvfC2KfnxnU UI43ZEXa7VUbLiSPUUUUU== X-CM-SenderInfo: qtrwiiyqvtljo62m3hxhgxhubq/ X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1772200041009718134 X-GMAIL-MSGID: 1772267799327883143 |
Series |
[v2] net/sched: mqprio: Add length check for TCA_MQPRIO_{MAX/MIN}_RATE64
|
|
Commit Message
Lin Ma
July 24, 2023, 1:46 a.m. UTC
The nla_for_each_nested parsing in function mqprio_parse_nlattr() does
not check the length of the nested attribute. This can lead to an
out-of-attribute read and allow a malformed nlattr (e.g., length 0) to
be viewed as 8 byte integer and passed to priv->max_rate/min_rate.
This patch adds the check based on nla_len() when check the nla_type(),
which ensures that the length of these two attribute must equals
sizeof(u64).
Fixes: 4e8b86c06269 ("mqprio: Introduce new hardware offload mode and shaper in mqprio")
Signed-off-by: Lin Ma <linma@zju.edu.cn>
---
V1 -> V2: do check with != rather than < as suggested
seperate the check and give clearer error message
net/sched/sch_mqprio.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
Comments
On 23/07/2023 22:46, Lin Ma wrote: > The nla_for_each_nested parsing in function mqprio_parse_nlattr() does > not check the length of the nested attribute. This can lead to an > out-of-attribute read and allow a malformed nlattr (e.g., length 0) to > be viewed as 8 byte integer and passed to priv->max_rate/min_rate. > > This patch adds the check based on nla_len() when check the nla_type(), > which ensures that the length of these two attribute must equals > sizeof(u64). > > Fixes: 4e8b86c06269 ("mqprio: Introduce new hardware offload mode and shaper in mqprio") > Signed-off-by: Lin Ma <linma@zju.edu.cn> Reviewed-by: Victor Nogueira <victor@mojatatu.com>
On Mon, 24 Jul 2023 09:46:25 +0800 Lin Ma wrote: > The nla_for_each_nested parsing in function mqprio_parse_nlattr() does > not check the length of the nested attribute. This can lead to an > out-of-attribute read and allow a malformed nlattr (e.g., length 0) to > be viewed as 8 byte integer and passed to priv->max_rate/min_rate. > > This patch adds the check based on nla_len() when check the nla_type(), > which ensures that the length of these two attribute must equals > sizeof(u64). How do you run get_maintainer? You didn't CC the author of the code.
Hello Jakub, > > The nla_for_each_nested parsing in function mqprio_parse_nlattr() does > > not check the length of the nested attribute. This can lead to an > > out-of-attribute read and allow a malformed nlattr (e.g., length 0) to > > be viewed as 8 byte integer and passed to priv->max_rate/min_rate. > > > > This patch adds the check based on nla_len() when check the nla_type(), > > which ensures that the length of these two attribute must equals > > sizeof(u64). > > How do you run get_maintainer? You didn't CC the author of the code. That's weird, I just ran code below and send this patch to all 9 emails poped out. # ./scripts/get_maintainer.pl net/sched/sch_mqprio.c Jamal Hadi Salim <jhs@mojatatu.com> (maintainer:TC subsystem) Cong Wang <xiyou.wangcong@gmail.com> (maintainer:TC subsystem) Jiri Pirko <jiri@resnulli.us> (maintainer:TC subsystem) "David S. Miller" <davem@davemloft.net> (maintainer:NETWORKING [GENERAL]) Eric Dumazet <edumazet@google.com> (maintainer:NETWORKING [GENERAL]) Jakub Kicinski <kuba@kernel.org> (maintainer:NETWORKING [GENERAL]) Paolo Abeni <pabeni@redhat.com> (maintainer:NETWORKING [GENERAL]) netdev@vger.kernel.org (open list:TC subsystem) linux-kernel@vger.kernel.org (open list) Can you tell me which one is missing and I will resend the patch to him. Thanks Lin
On Tue, 25 Jul 2023 08:15:39 +0800 (GMT+08:00) Lin Ma wrote: > > > The nla_for_each_nested parsing in function mqprio_parse_nlattr() does > > > not check the length of the nested attribute. This can lead to an > > > out-of-attribute read and allow a malformed nlattr (e.g., length 0) to > > > be viewed as 8 byte integer and passed to priv->max_rate/min_rate. > > > > > > This patch adds the check based on nla_len() when check the nla_type(), > > > which ensures that the length of these two attribute must equals > > > sizeof(u64). > > > > How do you run get_maintainer? You didn't CC the author of the code. > > That's weird, I just ran code below and send this patch to all 9 emails poped out. > > # ./scripts/get_maintainer.pl net/sched/sch_mqprio.c Joe, here's another case. Lin Ma, you need to run the script on the file generated by git format-patch, rather than the file path. That gives better coverage for keywords included in the commit message (especially the Fixes tag). Please rerun it on the patch and repost with the right CC list.
Hi Jakub, > > > > The nla_for_each_nested parsing in function mqprio_parse_nlattr() does > > > > not check the length of the nested attribute. This can lead to an > > > > out-of-attribute read and allow a malformed nlattr (e.g., length 0) to > > > > be viewed as 8 byte integer and passed to priv->max_rate/min_rate. > > > > > > > > This patch adds the check based on nla_len() when check the nla_type(), > > > > which ensures that the length of these two attribute must equals > > > > sizeof(u64). > > > > > > How do you run get_maintainer? You didn't CC the author of the code. > > > > That's weird, I just ran code below and send this patch to all 9 emails poped out. > > > > # ./scripts/get_maintainer.pl net/sched/sch_mqprio.c > > Joe, here's another case. > > Lin Ma, you need to run the script on the file generated by > git format-patch, rather than the file path. That gives better > coverage for keywords included in the commit message (especially > the Fixes tag). Please rerun it on the patch and repost with > the right CC list. Copy that. Sorry for the inconvenience that was raised by that. Will resend the patch with the correct CC list ASAP. Regards Lin > -- > pw-bot: cr
On Mon, 2023-07-24 at 17:56 -0700, Jakub Kicinski wrote: > On Tue, 25 Jul 2023 08:15:39 +0800 (GMT+08:00) Lin Ma wrote: > > > > The nla_for_each_nested parsing in function mqprio_parse_nlattr() does > > > > not check the length of the nested attribute. This can lead to an > > > > out-of-attribute read and allow a malformed nlattr (e.g., length 0) to > > > > be viewed as 8 byte integer and passed to priv->max_rate/min_rate. > > > > > > > > This patch adds the check based on nla_len() when check the nla_type(), > > > > which ensures that the length of these two attribute must equals > > > > sizeof(u64). > > > > > > How do you run get_maintainer? You didn't CC the author of the code. > > > > That's weird, I just ran code below and send this patch to all 9 emails poped out. > > > > # ./scripts/get_maintainer.pl net/sched/sch_mqprio.c > > Joe, here's another case. What do you think the "case" is here? Do you think John Fastabend, who hasn't touched the file in 7+ years should be cc'd? Why? > Lin Ma, you need to run the script on the file generated by > git format-patch, rather than the file path. That gives better > coverage for keywords included in the commit message (especially > the Fixes tag). Please rerun it on the patch and repost with > the right CC list.
On Mon, 24 Jul 2023 20:59:53 -0700 Joe Perches wrote: > > Joe, here's another case. > > What do you think the "case" is here? > > Do you think John Fastabend, who hasn't touched the file in 7+ years > should be cc'd? Why? Nope. The author of the patch under Fixes.
On Tue, 2023-07-25 at 12:38 -0700, Jakub Kicinski wrote: > On Mon, 24 Jul 2023 20:59:53 -0700 Joe Perches wrote: > > > Joe, here's another case. > > > > What do you think the "case" is here? > > > > Do you think John Fastabend, who hasn't touched the file in 7+ years > > should be cc'd? Why? > > Nope. The author of the patch under Fixes. It adds that already since 2019. commit 2f5bd343694ed53b3abc4a616ce975505271afe7 Author: Joe Perches <joe@perches.com> Date: Wed Dec 4 16:50:29 2019 -0800 scripts/get_maintainer.pl: add signatures from Fixes: <badcommit> lines in commit message A Fixes: lines in a commit message generally indicate that a previous commit was inadequate for whatever reason. The signers of the previous inadequate commit should also be cc'd on this new commit so update get_maintainer to find the old commit and add the original signers.
On Tue, 25 Jul 2023 12:50:00 -0700 Joe Perches wrote: > > > What do you think the "case" is here? > > > > > > Do you think John Fastabend, who hasn't touched the file in 7+ years > > > should be cc'd? Why? > > > > Nope. The author of the patch under Fixes. > > It adds that already since 2019. Which is awesome! But for that to work you have to run get_maintainer on the patchfile not the file paths. Lin Ma used: # ./scripts/get_maintainer.pl net/sched/sch_mqprio.c That's what I keep complaining about. People run get_maintainer on paths and miss out on all the get_maintainer features which need to see the commit message.
On Tue, 2023-07-25 at 12:56 -0700, Jakub Kicinski wrote: > On Tue, 25 Jul 2023 12:50:00 -0700 Joe Perches wrote: > > > > What do you think the "case" is here? > > > > > > > > Do you think John Fastabend, who hasn't touched the file in 7+ years > > > > should be cc'd? Why? > > > > > > Nope. The author of the patch under Fixes. > > > > It adds that already since 2019. > > Which is awesome! But for that to work you have to run get_maintainer > on the patchfile not the file paths. Lin Ma used: > > # ./scripts/get_maintainer.pl net/sched/sch_mqprio.c > > That's what I keep complaining about. People run get_maintainer on > paths and miss out on all the get_maintainer features which need > to see the commit message. Which is useful when editing a file but not when sending a patch. get_maintainer does _both_. Again, there's no issue with get_maintainer, but there is in its use. If there's a real issue, it's with people and their knowledge of process documentation. It's _not_ the tool itself as you stated.
diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c index ab69ff7577fc..f1d141a6d0aa 100644 --- a/net/sched/sch_mqprio.c +++ b/net/sched/sch_mqprio.c @@ -290,6 +290,13 @@ static int mqprio_parse_nlattr(struct Qdisc *sch, struct tc_mqprio_qopt *qopt, "Attribute type expected to be TCA_MQPRIO_MIN_RATE64"); return -EINVAL; } + + if (nla_len(attr) != sizeof(u64)) { + NL_SET_ERR_MSG_ATTR(extack, attr, + "Attribute TCA_MQPRIO_MIN_RATE64 expected to have 8 bytes length"); + return -EINVAL; + } + if (i >= qopt->num_tc) break; priv->min_rate[i] = nla_get_u64(attr); @@ -312,6 +319,13 @@ static int mqprio_parse_nlattr(struct Qdisc *sch, struct tc_mqprio_qopt *qopt, "Attribute type expected to be TCA_MQPRIO_MAX_RATE64"); return -EINVAL; } + + if (nla_len(attr) != sizeof(u64)) { + NL_SET_ERR_MSG_ATTR(extack, attr, + "Attribute TCA_MQPRIO_MAX_RATE64 expected to have 8 bytes length"); + return -EINVAL; + } + if (i >= qopt->num_tc) break; priv->max_rate[i] = nla_get_u64(attr);