Message ID | 20221117090514.118296-1-tanghui20@huawei.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:f944:0:0:0:0:0 with SMTP id q4csp293165wrr; Thu, 17 Nov 2022 01:10:07 -0800 (PST) X-Google-Smtp-Source: AA0mqf70HyRlR64Mh0A85/23BeMaUyNaCvHNAU71JpKai8ulXrdKdMgf2+wvOVeBMMBHEiaerQUQ X-Received: by 2002:a65:6808:0:b0:43c:758:5190 with SMTP id l8-20020a656808000000b0043c07585190mr1238259pgt.339.1668676207551; Thu, 17 Nov 2022 01:10:07 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1668676207; cv=none; d=google.com; s=arc-20160816; b=fe7Fy0lPVYYDMgypm/AOVqqNrOIbld/E4bY1Foapy0FuVbpwws+isyNZetAaFvn7qP JAIqgFK1jXuFXuH5F7np64274SPq++AK+AP+eesvDf1wF0Vkjct/C6yREmTpkon20sU+ 3IrGmkRtWOtrxZ17D8HeEc6V+XvDJrmQY0IBcN97FjZBW9w/9UGJRWdIVNwQHHe1oS/j YZJ2cDt3tasyJWIvqqDIYmgBFd3mWAYmpBojOI/5h7T9UCUkARLdgdrsJQ1Grzvelfhy CKyJyZgzxivpYnqI0ceDHe+jlGz6YmBwG0/a3qoLFqoxiVseM9poyP4KUE+ItLtKRLVh CtkQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:mime-version:message-id:date:subject:cc:to:from; bh=XcflEfAsl1kER3opwufULZWmC+IaH1Y5ZdTQRPhe4lU=; b=bQ+UB3tPUhJP7XUWHOkREPcjcNz/FLDlSlt8DjNuKmcsDUNxfw5Eav7NFPYxpsVlBr xQZXAtojixcmOQTrG/c/ck3fdnZVSEOGSIVASo5ZbJj19VFQThCTBFxOs2BT3jvd2Akn SklHLPtir9dsC8IaU1YxEn5Kll1C/6qd3o8jhfNA7PRUOZBCH8sz3kljvN73qIY47NO1 1T8KvJCwVkVY1/lJdPNLiXECxf7XLo6FnEdZ3ArfBotr3MDDmbuTjpEw1IWXAzBH2Yp+ UPm5wTf0m/jCWIs/Kvrjfo5t6o10ySKD7WlkPv54KwDNSjUTfNvwBcbOeBhOJdGjQ+F9 Plcw== 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; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=huawei.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id l17-20020a637011000000b0046a1ae37df5si555974pgc.147.2022.11.17.01.09.54; Thu, 17 Nov 2022 01:10:07 -0800 (PST) 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; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=huawei.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S239712AbiKQJIi (ORCPT <rfc822;just.gull.subs@gmail.com> + 99 others); Thu, 17 Nov 2022 04:08:38 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:36212 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S239715AbiKQJIf (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 17 Nov 2022 04:08:35 -0500 Received: from szxga03-in.huawei.com (szxga03-in.huawei.com [45.249.212.189]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id CE4F0A1B9; Thu, 17 Nov 2022 01:08:34 -0800 (PST) Received: from dggemv711-chm.china.huawei.com (unknown [172.30.72.56]) by szxga03-in.huawei.com (SkyGuard) with ESMTP id 4NCYsT1ZPJzJnpT; Thu, 17 Nov 2022 17:05:17 +0800 (CST) Received: from kwepemm600005.china.huawei.com (7.193.23.191) by dggemv711-chm.china.huawei.com (10.1.198.66) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.31; Thu, 17 Nov 2022 17:08:26 +0800 Received: from ubuntu1804.huawei.com (10.67.175.30) by kwepemm600005.china.huawei.com (7.193.23.191) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.31; Thu, 17 Nov 2022 17:08:26 +0800 From: Hui Tang <tanghui20@huawei.com> To: <davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>, <mw@semihalf.com>, <linux@armlinux.org.uk>, <andrew@lunn.ch>, <pabeni@redhat.com> CC: <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <yusongping@huawei.com> Subject: [PATCH net v2] net: mdio-ipq4019: fix possible invalid pointer dereference Date: Thu, 17 Nov 2022 17:05:14 +0800 Message-ID: <20221117090514.118296-1-tanghui20@huawei.com> X-Mailer: git-send-email 2.17.1 MIME-Version: 1.0 Content-Type: text/plain X-Originating-IP: [10.67.175.30] X-ClientProxiedBy: dggems702-chm.china.huawei.com (10.3.19.179) To kwepemm600005.china.huawei.com (7.193.23.191) X-CFilter-Loop: Reflected X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_PASS 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: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1749537411846688266?= X-GMAIL-MSGID: =?utf-8?q?1749733822574382151?= |
Series |
[net,v2] net: mdio-ipq4019: fix possible invalid pointer dereference
|
|
Commit Message
Hui Tang
Nov. 17, 2022, 9:05 a.m. UTC
priv->eth_ldo_rdy is saved the return value of devm_ioremap_resource(),
which !IS_ERR() should be used to check.
Fixes: 23a890d493e3 ("net: mdio: Add the reset function for IPQ MDIO driver")
Signed-off-by: Hui Tang <tanghui20@huawei.com>
---
v1 -> v2: set priv->eth_ldo_rdy NULL, if devm_ioremap_resource() failed
---
drivers/net/mdio/mdio-ipq4019.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
Comments
On Thu, Nov 17, 2022 at 05:05:14PM +0800, Hui Tang wrote: > priv->eth_ldo_rdy is saved the return value of devm_ioremap_resource(), > which !IS_ERR() should be used to check. > > Fixes: 23a890d493e3 ("net: mdio: Add the reset function for IPQ MDIO driver") > Signed-off-by: Hui Tang <tanghui20@huawei.com> > --- > v1 -> v2: set priv->eth_ldo_rdy NULL, if devm_ioremap_resource() failed > --- > drivers/net/mdio/mdio-ipq4019.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c > index 4eba5a91075c..dfd1647eac36 100644 > --- a/drivers/net/mdio/mdio-ipq4019.c > +++ b/drivers/net/mdio/mdio-ipq4019.c > @@ -231,8 +231,11 @@ static int ipq4019_mdio_probe(struct platform_device *pdev) > /* The platform resource is provided on the chipset IPQ5018 */ > /* This resource is optional */ > res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > - if (res) > + if (res) { > priv->eth_ldo_rdy = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(priv->eth_ldo_rdy)) > + priv->eth_ldo_rdy = NULL; > + } As i said, please add devm_ioremap_resource_optional(). Follow the concept of devm_clk_get_optional(), devm_gpiod_get_optional(), devm_reset_control_get_optional(), devm_reset_control_get_optional(), platform_get_irq_byname_optional() etc. All these will not return an error if the resource you are trying to get does not exist. They instead return NULL, or something which other API members understand as does not exist, but thats O.K. These functions however do return errors for real problem, ENOMEM, EINVAL etc. These should not be ignored. You should then use this new function for all your other patches where the resource is optional. Andrew
On 2022/11/17 21:57, Andrew Lunn wrote: > On Thu, Nov 17, 2022 at 05:05:14PM +0800, Hui Tang wrote: >> priv->eth_ldo_rdy is saved the return value of devm_ioremap_resource(), >> which !IS_ERR() should be used to check. >> >> Fixes: 23a890d493e3 ("net: mdio: Add the reset function for IPQ MDIO driver") >> Signed-off-by: Hui Tang <tanghui20@huawei.com> >> --- >> v1 -> v2: set priv->eth_ldo_rdy NULL, if devm_ioremap_resource() failed >> --- >> drivers/net/mdio/mdio-ipq4019.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c >> index 4eba5a91075c..dfd1647eac36 100644 >> --- a/drivers/net/mdio/mdio-ipq4019.c >> +++ b/drivers/net/mdio/mdio-ipq4019.c >> @@ -231,8 +231,11 @@ static int ipq4019_mdio_probe(struct platform_device *pdev) >> /* The platform resource is provided on the chipset IPQ5018 */ >> /* This resource is optional */ >> res = platform_get_resource(pdev, IORESOURCE_MEM, 1); >> - if (res) >> + if (res) { >> priv->eth_ldo_rdy = devm_ioremap_resource(&pdev->dev, res); >> + if (IS_ERR(priv->eth_ldo_rdy)) >> + priv->eth_ldo_rdy = NULL; >> + } > > As i said, please add devm_ioremap_resource_optional(). Follow the > concept of devm_clk_get_optional(), devm_gpiod_get_optional(), > devm_reset_control_get_optional(), devm_reset_control_get_optional(), > platform_get_irq_byname_optional() etc. > > All these will not return an error if the resource you are trying to > get does not exist. They instead return NULL, or something which other > API members understand as does not exist, but thats O.K. > > These functions however do return errors for real problem, ENOMEM, > EINVAL etc. These should not be ignored. > > You should then use this new function for all your other patches where > the resource is optional. I finally understand what you mean now. I need add devm_ioremap_resource_optional() helper, which return NULL if the resource does not exist, and if it return other errors we need to deal with errors. In my case, it returns -ENOMEM if the resource does not exist. So, the code should be as follows, is that right? + void __iomem *devm_ioremap_resource_optional(struct device *dev, + const struct resource *res) + { + void __iomem *base; + + base = __devm_ioremap_resource(dev, res, DEVM_IOREMAP); + if (IS_ERR(base) && PTR_ERR(base) == -ENOMEM) + return NULL; + + return base; + } [...] res = platform_get_resource(pdev, IORESOURCE_MEM, 1); - if (res) + if (res) { + priv->eth_ldo_rdy = devm_ioremap_resource_optional(&pdev->dev, res) + if (IS_ERR(priv->eth_ldo_rdy)) + return PTR_ERR(priv->eth_ldo_rdy); + } [...] thanks.
> So, the code should be as follows, is that right? > > + void __iomem *devm_ioremap_resource_optional(struct device *dev, > + const struct resource *res) > + { > + void __iomem *base; > + > + base = __devm_ioremap_resource(dev, res, DEVM_IOREMAP); > + if (IS_ERR(base) && PTR_ERR(base) == -ENOMEM) > + return NULL; > + > + return base; > + } > > > [...] > res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > - if (res) > + if (res) { > + priv->eth_ldo_rdy = devm_ioremap_resource_optional(&pdev->dev, res) > + if (IS_ERR(priv->eth_ldo_rdy)) > + return PTR_ERR(priv->eth_ldo_rdy); > + } > [...] Yes, that is the basic concept. The only thing i might change is the double meaning of -ENOMEM. __devm_ioremap_resource() allocates memory, and if that memory allocation fails, it returns -ENOMEM. If the resource does not exist, it also returns -ENOMEM. So you cannot tell these two error conditions apart. Most of the other get_foo() calls return -ENODEV if the gpio/regulator/clock does not exist, so you can tell if you are out of memory. But ioremap is specifically about memory so -ENOMEM actually makes sense. If you are out of memory, it seems likely the problem is not going to go away quickly, so the next allocation will also fail, and hopefully the error handling will then work. So i don't think it is major issue. So yes, go with the code above. Andrew
On 2022/11/18 21:44, Andrew Lunn wrote: >> So, the code should be as follows, is that right? >> >> + void __iomem *devm_ioremap_resource_optional(struct device *dev, >> + const struct resource *res) >> + { >> + void __iomem *base; >> + >> + base = __devm_ioremap_resource(dev, res, DEVM_IOREMAP); >> + if (IS_ERR(base) && PTR_ERR(base) == -ENOMEM) >> + return NULL; >> + >> + return base; >> + } >> >> >> [...] >> res = platform_get_resource(pdev, IORESOURCE_MEM, 1); >> - if (res) >> + if (res) { >> + priv->eth_ldo_rdy = devm_ioremap_resource_optional(&pdev->dev, res) >> + if (IS_ERR(priv->eth_ldo_rdy)) >> + return PTR_ERR(priv->eth_ldo_rdy); >> + } >> [...] > > Yes, that is the basic concept. > > The only thing i might change is the double meaning of -ENOMEM. > __devm_ioremap_resource() allocates memory, and if that memory > allocation fails, it returns -ENOMEM. If the resource does not exist, > it also returns -ENOMEM. So you cannot tell these two error conditions > apart. Most of the other get_foo() calls return -ENODEV if the > gpio/regulator/clock does not exist, so you can tell if you are out of > memory. But ioremap is specifically about memory so -ENOMEM actually > makes sense. > > If you are out of memory, it seems likely the problem is not going to > go away quickly, so the next allocation will also fail, and hopefully > the error handling will then work. So i don't think it is major > issue. So yes, go with the code above. > Hi, Andrew My new patchset is ready, but I just found out that another patch has been applied to netdev/net.git. Can I solve the problem in present way? And I will add devm_ioremap_resource_optional() helper later to optimize related drivers. How about this? Thanks.
> Hi, Andrew > > My new patchset is ready, but I just found out that another patch has been > applied to netdev/net.git. Can I solve the problem in present way? And I > will add devm_ioremap_resource_optional() helper later to optimize related > drivers. How about this? This is one of those harder to merge changes. patches to lib/devres.c generally go via GregKH. Networking changes are merged via the netdev list. Did you find this issue via a static analyser? I assume you are running it over the entire tree and are finding problems in multiple subsystems? So devm_ioremap_resource_optional() is potentially going to be needed in lots of places? One way to get this merged is to cross post the patch adding devm_ioremap_resource_optional() and ask GregKH to ACK it, and then get netdev to merge it. You can then use it within the netdev subsystem. What you cannot do is use it in other subsystems until the next kernel cycle when it will be globally available. So three patches. One adding devm_ioremap_resource_optional(), one to revert the 'fix', and one with the real fix using devm_ioremap_resource_optional(). Andrew
On 2022/11/21 22:36, Andrew Lunn wrote: >> Hi, Andrew >> >> My new patchset is ready, but I just found out that another patch has been >> applied to netdev/net.git. Can I solve the problem in present way? And I >> will add devm_ioremap_resource_optional() helper later to optimize related >> drivers. How about this? > > This is one of those harder to merge changes. patches to lib/devres.c > generally go via GregKH. Networking changes are merged via the netdev > list. > > Did you find this issue via a static analyser? I assume you are > running it over the entire tree and are finding problems in multiple > subsystems? So devm_ioremap_resource_optional() is potentially going > to be needed in lots of places? Yes, I grep the entire drives, some drivers is really going to be needed for devm_ioremap_resource_optional() case. For example: drivers/mmc/host/mtk-sd.c drivers/mmc/host/sdhci-st.c drivers/ufs/host/ufs-qcom.c drivers/mfd/bcm2835-pm.c net/mdio/mdio-ipq4019.c drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > One way to get this merged is to cross post the patch adding > devm_ioremap_resource_optional() and ask GregKH to ACK it, and then > get netdev to merge it. You can then use it within the netdev > subsystem. What you cannot do is use it in other subsystems until the > next kernel cycle when it will be globally available. > > So three patches. One adding devm_ioremap_resource_optional(), one to > revert the 'fix', and one with the real fix using > devm_ioremap_resource_optional(). Thanks
diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c index 4eba5a91075c..dfd1647eac36 100644 --- a/drivers/net/mdio/mdio-ipq4019.c +++ b/drivers/net/mdio/mdio-ipq4019.c @@ -231,8 +231,11 @@ static int ipq4019_mdio_probe(struct platform_device *pdev) /* The platform resource is provided on the chipset IPQ5018 */ /* This resource is optional */ res = platform_get_resource(pdev, IORESOURCE_MEM, 1); - if (res) + if (res) { priv->eth_ldo_rdy = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(priv->eth_ldo_rdy)) + priv->eth_ldo_rdy = NULL; + } bus->name = "ipq4019_mdio"; bus->read = ipq4019_mdio_read;