Message ID | 20230723074504.3706691-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 l16csp1151882vqg; Sun, 23 Jul 2023 01:01:41 -0700 (PDT) X-Google-Smtp-Source: APBJJlF/ern1JI8Mo4qOwt+8lMna9Zm0wz3BlfarLgRWJvgRKjFy99uJm6/DXtXh69CPIAsL3iaB X-Received: by 2002:a17:906:53d9:b0:992:47d7:35d7 with SMTP id p25-20020a17090653d900b0099247d735d7mr6929543ejo.14.1690099301668; Sun, 23 Jul 2023 01:01:41 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1690099301; cv=none; d=google.com; s=arc-20160816; b=wL7Z2Z7DTnj2yDoPrQ9qPFnShG4RSnSF0lBC70JroHrTh4TgdV4t7md2vMLaY9Yq9K 6FS2IdbCUlewMgEevtptLJEFvVOPmZLgP4/HgfK2TiqUhSR+gNKY14o/kKEmTHc1copc RvLYpGThhwvywphE7cPhLj8MXun36+kgV1/QsipCujyLeqD/Dx6Ly17JLHsIpUumVKgB Xe+V+RN1VNgZvHcMAQnNMqCmqD7lHaohp4N1A6NLIjgG9P7C3NFE6BqYzYLYxixTR4gU ERP/G1f/pbpz5DmWFKUF9tn0ZjwSLkKs1F0U1+NMBOCwh4Fc/nPMWCpnaQsquhQzrDZH norQ== 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=ArhAwS5Pftpcoe8vLiTn7LqKhODX4RM0E8qHGNJZ4Hs=; fh=NJzMN4GVIfLdYHzurVEQeo1huJPBRJb0+2OqHXkr4V4=; b=odCs9Sa7hBMKmgAviJgHf4l6ro5ZDUFYccDoOZnrGpZJHTQ8bcDr2xPdt87NXfeSQy YyVgn9tmXoVaOJb2Pgls+0T4kmtyMeyRmdY89VTWz3V6hvjY+B/q6lRnH1rWN79qUODs YPVi4qdG7wLHrd5xPTaMdYK2DdsF/fVvAnideDj5btI9xDCsQKh/b6Va8qnaVoFvBRkX JRfytwPkAmkfEw6JxkECMF0P6/Ozqzy4g/3aZGArrT4RQW518YQ9SNR5KbrZZm+DpHjU enQFRIb1+OUzFzJ4Sa2M1eXcsQfeFMaV5zU5jNBJWHt0HNb/wAKWVjryQs4E59X61GNV sP2Q== 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 y8-20020a170906470800b009882f2a8aabsi4965035ejq.551.2023.07.23.01.01.17; Sun, 23 Jul 2023 01:01:41 -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 S229646AbjGWHpV (ORCPT <rfc822;assdfgzxcv4@gmail.com> + 99 others); Sun, 23 Jul 2023 03:45:21 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46508 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229469AbjGWHpU (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Sun, 23 Jul 2023 03:45:20 -0400 X-Greylist: delayed 230 seconds by postgrey-1.37 at lindbergh.monkeyblade.net; Sun, 23 Jul 2023 00:45:17 PDT Received: from zg8tmtu5ljg5lje1ms4xmtka.icoremail.net (zg8tmtu5ljg5lje1ms4xmtka.icoremail.net [159.89.151.119]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id AE4DD19B3 for <linux-kernel@vger.kernel.org>; Sun, 23 Jul 2023 00:45:17 -0700 (PDT) Received: from localhost.localdomain (unknown [36.24.99.225]) by mail-app3 (Coremail) with SMTP id cC_KCgD3_g6B2rxkkDN_Cw--.46290S4; Sun, 23 Jul 2023 15:45:05 +0800 (CST) From: Lin Ma <linma@zju.edu.cn> To: jgg@ziepe.ca, leon@kernel.org, markzhang@nvidia.com, michaelgur@nvidia.com, ohartoov@nvidia.com, chenzhongjin@huawei.com, yuancan@huawei.com, linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org Cc: Lin Ma <linma@zju.edu.cn> Subject: [PATCH v1] RDMA/nldev: Add length check for IFLA_BOND_ARP_IP_TARGET parsing Date: Sun, 23 Jul 2023 15:45:04 +0800 Message-Id: <20230723074504.3706691-1-linma@zju.edu.cn> X-Mailer: git-send-email 2.17.1 X-CM-TRANSID: cC_KCgD3_g6B2rxkkDN_Cw--.46290S4 X-Coremail-Antispam: 1UD129KBjvJXoWxJr1Dtr45WrW8WF48ur1UAwb_yoW8JFW3pF W0qFy7KF47JF13Ga1Dta1kWFWa93W7ZFyagFWDt343urn8Xw1S9345CFyYvFWDArWkA3W2 vF15Z34j9FZ2qr7anT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDU0xBIdaVrnRJUUUvC14x267AKxVW8JVW5JwAFc2x0x2IEx4CE42xK8VAvwI8IcIk0 rVWrJVCq3wAFIxvE14AKwVWUJVWUGwA2ocxC64kIII0Yj41l84x0c7CEw4AK67xGY2AK02 1l84ACjcxK6xIIjxv20xvE14v26w1j6s0DM28EF7xvwVC0I7IYx2IY6xkF7I0E14v26r4U JVWxJr1l84ACjcxK6I8E87Iv67AKxVW0oVCq3wA2z4x0Y4vEx4A2jsIEc7CjxVAFwI0_Gc CE3s1le2I262IYc4CY6c8Ij28IcVAaY2xG8wAqx4xG64xvF2IEw4CE5I8CrVC2j2WlYx0E 2Ix0cI8IcVAFwI0_JrI_JrylYx0Ex4A2jsIE14v26r1j6r4UMcvjeVCFs4IE7xkEbVWUJV W8JwACjcxG0xvY0x0EwIxGrwACjI8F5VA0II8E6IAqYI8I648v4I1lFIxGxcIEc7CjxVA2 Y2ka0xkIwI1lc2xSY4AK67AK6r4xMxAIw28IcxkI7VAKI48JMxC20s026xCaFVCjc4AY6r 1j6r4UMI8I3I0E5I8CrVAFwI0_Jr0_Jr4lx2IqxVCjr7xvwVAFwI0_JrI_JrWlx4CE17CE b7AF67AKxVWUtVW8ZwCIc40Y0x0EwIxGrwCI42IY6xIIjxv20xvE14v26r1j6r1xMIIF0x vE2Ix0cI8IcVCY1x0267AKxVW8JVWxJwCI42IY6xAIw20EY4v20xvaj40_Jr0_JF4lIxAI cVC2z280aVAFwI0_Jr0_Gr1lIxAIcVC2z280aVCY1x0267AKxVW8JVW8JrUvcSsGvfC2Kf nxnUUI43ZEXa7VU10tC7UUUUU== X-CM-SenderInfo: qtrwiiyqvtljo62m3hxhgxhubq/ X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,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: 1772197565238824885 X-GMAIL-MSGID: 1772197565238824885 |
Series |
[v1] RDMA/nldev: Add length check for IFLA_BOND_ARP_IP_TARGET parsing
|
|
Commit Message
Lin Ma
July 23, 2023, 7:45 a.m. UTC
The nla_for_each_nested parsing in function
nldev_stat_set_counter_dynamic_doit() does not check the length of the
attribute. This can lead to an out-of-attribute read and allow a
malformed nlattr (e.g., length 0) to be viewed as a 4 byte integer.
This patch adds the check based on nla_len() just as other code does,
see how bond_changelink (drivers/net/bonding/bond_netlink.c) parses
IFLA_BOND_NS_IP6_TARGET.
Fixes: 3c3c1f141639 ("RDMA/nldev: Allow optional-counter status configuration through RDMA netlink")
Signed-off-by: Lin Ma <linma@zju.edu.cn>
---
drivers/infiniband/core/nldev.c | 5 +++++
1 file changed, 5 insertions(+)
Comments
On Sun, Jul 23, 2023 at 03:45:04PM +0800, Lin Ma wrote: > The nla_for_each_nested parsing in function > nldev_stat_set_counter_dynamic_doit() does not check the length of the > attribute. This can lead to an out-of-attribute read and allow a > malformed nlattr (e.g., length 0) to be viewed as a 4 byte integer. 1. Subject of this patch doesn't really match the change. 2. See my comment on your i40e patch. https://lore.kernel.org/netdev/20230724174435.GA11388@unreal/ Thanks
Hello Leon, > > On Sun, Jul 23, 2023 at 03:45:04PM +0800, Lin Ma wrote: > > The nla_for_each_nested parsing in function > > nldev_stat_set_counter_dynamic_doit() does not check the length of the > > attribute. This can lead to an out-of-attribute read and allow a > > malformed nlattr (e.g., length 0) to be viewed as a 4 byte integer. > > 1. Subject of this patch doesn't really match the change. My bad, a stupid mistake. I will fix that and prepare another patch. > 2. See my comment on your i40e patch. > https://lore.kernel.org/netdev/20230724174435.GA11388@unreal/ > Yeah I have seen that. Just as Jakub said, empty netlink attributes are valid (they are viewed as flag). The point is that different attribute has different length requirement. For this specific code, the RDMA_NLDEV_ATTR_STAT_HWCOUNTERS attribute is a nested one whose inner attributes should be NLA_U32. But as you can see in variable nldev_policy, the description does not use nested policy to enfore that, which results in the bug discussed in my commit message. [RDMA_NLDEV_ATTR_STAT_HWCOUNTERS] = { .type = NLA_NESTED }, The elegant fix could be add the nested policy description to nldev_policy while this is toublesome as no existing nla_attr has been given to this nested nlattr. Hence, add the length check is the simplest solution and you can see such nla_len check code all over the kernel. > Thanks Regards Lin
On Tue, Jul 25, 2023 at 08:11:58AM +0800, Lin Ma wrote: > Hello Leon, > > > > > On Sun, Jul 23, 2023 at 03:45:04PM +0800, Lin Ma wrote: > > > The nla_for_each_nested parsing in function > > > nldev_stat_set_counter_dynamic_doit() does not check the length of the > > > attribute. This can lead to an out-of-attribute read and allow a > > > malformed nlattr (e.g., length 0) to be viewed as a 4 byte integer. > > > > 1. Subject of this patch doesn't really match the change. > > My bad, a stupid mistake. I will fix that and prepare another patch. > > > 2. See my comment on your i40e patch. > > https://lore.kernel.org/netdev/20230724174435.GA11388@unreal/ > > > > Yeah I have seen that. Just as Jakub said, empty netlink attributes are valid > (they are viewed as flag). The point is that different attribute has different > length requirement. For this specific code, the RDMA_NLDEV_ATTR_STAT_HWCOUNTERS > attribute is a nested one whose inner attributes should be NLA_U32. But as you > can see in variable nldev_policy, the description does not use nested policy to > enfore that, which results in the bug discussed in my commit message. > > [RDMA_NLDEV_ATTR_STAT_HWCOUNTERS] = { .type = NLA_NESTED }, > > The elegant fix could be add the nested policy description to nldev_policy while > this is toublesome as no existing nla_attr has been given to this nested nlattr. > Hence, add the length check is the simplest solution and you can see such nla_len > check code all over the kernel. Right, and this is what bothers me. I would more than happy to change nla_for_each_nested() to be something like nla_for_each_nested_type(...., sizeof(u32)), which will skip empty lines, for code which can't have them. Thanks > > > Thanks > > Regards > Lin
Hi Leon, > > Right, and this is what bothers me. > > I would more than happy to change nla_for_each_nested() to be something > like nla_for_each_nested_type(...., sizeof(u32)), which will skip empty > lines, for code which can't have them. > > Thanks > Well, nla_for_each_nested_type(...., sizeof(u32)) seems good if the nested attribute is sure to contain only one type of attribute with constant length. (like the case of RDMA_NLDEV_ATTR_STAT_HWCOUNTERS). I accept that it is another elegant solution here. But efforts are needed to make sure if this is true for other cases. Regards Lin
On Tue, 25 Jul 2023 08:25:57 +0300 Leon Romanovsky wrote: > > Yeah I have seen that. Just as Jakub said, empty netlink attributes are valid > > (they are viewed as flag). The point is that different attribute has different > > length requirement. For this specific code, the RDMA_NLDEV_ATTR_STAT_HWCOUNTERS > > attribute is a nested one whose inner attributes should be NLA_U32. But as you > > can see in variable nldev_policy, the description does not use nested policy to > > enfore that, which results in the bug discussed in my commit message. > > > > [RDMA_NLDEV_ATTR_STAT_HWCOUNTERS] = { .type = NLA_NESTED }, > > > > The elegant fix could be add the nested policy description to nldev_policy while > > this is toublesome as no existing nla_attr has been given to this nested nlattr. > > Hence, add the length check is the simplest solution and you can see such nla_len > > check code all over the kernel. > > Right, and this is what bothers me. > > I would more than happy to change nla_for_each_nested() to be something > like nla_for_each_nested_type(...., sizeof(u32)), which will skip empty > lines, for code which can't have them. In general the idea of auto-skipping stuff kernel doesn't recognize is a bit old school. Better direction would be extending the policy validation to cover use cases for such loops.
On Tue, Jul 25, 2023 at 10:14:05AM -0700, Jakub Kicinski wrote: > On Tue, 25 Jul 2023 08:25:57 +0300 Leon Romanovsky wrote: > > > Yeah I have seen that. Just as Jakub said, empty netlink attributes are valid > > > (they are viewed as flag). The point is that different attribute has different > > > length requirement. For this specific code, the RDMA_NLDEV_ATTR_STAT_HWCOUNTERS > > > attribute is a nested one whose inner attributes should be NLA_U32. But as you > > > can see in variable nldev_policy, the description does not use nested policy to > > > enfore that, which results in the bug discussed in my commit message. > > > > > > [RDMA_NLDEV_ATTR_STAT_HWCOUNTERS] = { .type = NLA_NESTED }, > > > > > > The elegant fix could be add the nested policy description to nldev_policy while > > > this is toublesome as no existing nla_attr has been given to this nested nlattr. > > > Hence, add the length check is the simplest solution and you can see such nla_len > > > check code all over the kernel. > > > > Right, and this is what bothers me. > > > > I would more than happy to change nla_for_each_nested() to be something > > like nla_for_each_nested_type(...., sizeof(u32)), which will skip empty > > lines, for code which can't have them. > > In general the idea of auto-skipping stuff kernel doesn't recognize > is a bit old school. Better direction would be extending the policy > validation to cover use cases for such loops. I'm all in for any solution which will help for average developer to write netlink code without mistakes. Thanks
Hello there, > > > > Yeah I have seen that. Just as Jakub said, empty netlink attributes are valid > > > > (they are viewed as flag). The point is that different attribute has different > > > > length requirement. For this specific code, the RDMA_NLDEV_ATTR_STAT_HWCOUNTERS > > > > attribute is a nested one whose inner attributes should be NLA_U32. But as you > > > > can see in variable nldev_policy, the description does not use nested policy to > > > > enfore that, which results in the bug discussed in my commit message. > > > > > > > > [RDMA_NLDEV_ATTR_STAT_HWCOUNTERS] = { .type = NLA_NESTED }, > > > > > > > > The elegant fix could be add the nested policy description to nldev_policy while > > > > this is toublesome as no existing nla_attr has been given to this nested nlattr. > > > > Hence, add the length check is the simplest solution and you can see such nla_len > > > > check code all over the kernel. > > > > > > Right, and this is what bothers me. > > > > > > I would more than happy to change nla_for_each_nested() to be something > > > like nla_for_each_nested_type(...., sizeof(u32)), which will skip empty > > > lines, for code which can't have them. > > > > In general the idea of auto-skipping stuff kernel doesn't recognize > > is a bit old school. Better direction would be extending the policy > > validation to cover use cases for such loops. > > I'm all in for any solution which will help for average developer to write > netlink code without mistakes. > > Thanks I have just come out a new solution for such length issues. Please see * https://lore.kernel.org/all/20230731121247.3972783-1-linma@zju.edu.cn/T/#u * https://lore.kernel.org/all/20230731121324.3973136-1-linma@zju.edu.cn/T/#u I'm not sure adding additional validation logic in the main nlattr code is the best solution. Still, after investigating the code, the len field can be very suitable for handling the NLA_NESTED cases here. And the developer can do manual parsing with better nla_policy-based checking too. If this idea is applied, I will also write a script to clean up other nla_len patches based on the nla_policy check. Regards Lin
On Mon, Jul 31, 2023 at 08:33:02PM +0800, Lin Ma wrote: > Hello there, > > > > > > Yeah I have seen that. Just as Jakub said, empty netlink attributes are valid > > > > > (they are viewed as flag). The point is that different attribute has different > > > > > length requirement. For this specific code, the RDMA_NLDEV_ATTR_STAT_HWCOUNTERS > > > > > attribute is a nested one whose inner attributes should be NLA_U32. But as you > > > > > can see in variable nldev_policy, the description does not use nested policy to > > > > > enfore that, which results in the bug discussed in my commit message. > > > > > > > > > > [RDMA_NLDEV_ATTR_STAT_HWCOUNTERS] = { .type = NLA_NESTED }, > > > > > > > > > > The elegant fix could be add the nested policy description to nldev_policy while > > > > > this is toublesome as no existing nla_attr has been given to this nested nlattr. > > > > > Hence, add the length check is the simplest solution and you can see such nla_len > > > > > check code all over the kernel. > > > > > > > > Right, and this is what bothers me. > > > > > > > > I would more than happy to change nla_for_each_nested() to be something > > > > like nla_for_each_nested_type(...., sizeof(u32)), which will skip empty > > > > lines, for code which can't have them. > > > > > > In general the idea of auto-skipping stuff kernel doesn't recognize > > > is a bit old school. Better direction would be extending the policy > > > validation to cover use cases for such loops. > > > > I'm all in for any solution which will help for average developer to write > > netlink code without mistakes. > > > > Thanks > > I have just come out a new solution for such length issues. Please see > * https://lore.kernel.org/all/20230731121247.3972783-1-linma@zju.edu.cn/T/#u > * https://lore.kernel.org/all/20230731121324.3973136-1-linma@zju.edu.cn/T/#u > > I'm not sure adding additional validation logic in the main nlattr code is > the best solution. Still, after investigating the code, the len field can > be very suitable for handling the NLA_NESTED cases here. And the developer > can do manual parsing with better nla_policy-based checking too. > > If this idea is applied, I will also write a script to clean up other > nla_len patches based on the nla_policy check. It looks like Jakub didn't like the idea and we will need to add your sizeof checks all other the place. Thanks > > Regards > Lin
diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c index d5d3e4f0de77..74635c23b371 100644 --- a/drivers/infiniband/core/nldev.c +++ b/drivers/infiniband/core/nldev.c @@ -1989,6 +1989,11 @@ static int nldev_stat_set_counter_dynamic_doit(struct nlattr *tb[], nla_for_each_nested(entry_attr, tb[RDMA_NLDEV_ATTR_STAT_HWCOUNTERS], rem) { + if (nla_len(entry_attr) < sizeof(index)) { + ret = -EINVAL; + goto out; + } + index = nla_get_u32(entry_attr); if ((index >= stats->num_counters) || !(stats->descs[index].flags & IB_STAT_FLAG_OPTIONAL)) {