| 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
| ||
|
Message-ID: <DB3PR0402MB3916311773BADACCC5DC3A7CF5010@DB3PR0402MB3916.eurprd04.prod.outlook.com>
Date: Thu, 23 May 2019 09:35:48 +0000
From: Anson Huang <anson.huang@....com>
To: Russell King - ARM Linux admin <linux@...linux.org.uk>
CC: "catalin.marinas@....com" <catalin.marinas@....com>,
"will.deacon@....com" <will.deacon@....com>,
"shawnguo@...nel.org" <shawnguo@...nel.org>,
"s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
"kernel@...gutronix.de" <kernel@...gutronix.de>,
"festevam@...il.com" <festevam@...il.com>,
"maxime.ripard@...tlin.com" <maxime.ripard@...tlin.com>,
"olof@...om.net" <olof@...om.net>,
"agross@...nel.org" <agross@...nel.org>,
"jagan@...rulasolutions.com" <jagan@...rulasolutions.com>,
"bjorn.andersson@...aro.org" <bjorn.andersson@...aro.org>,
Leonard Crestez <leonard.crestez@....com>,
"dinguyen@...nel.org" <dinguyen@...nel.org>,
"enric.balletbo@...labora.com" <enric.balletbo@...labora.com>,
Aisheng Dong <aisheng.dong@....com>,
Abel Vesa <abel.vesa@....com>,
"l.stach@...gutronix.de" <l.stach@...gutronix.de>,
"tglx@...utronix.de" <tglx@...utronix.de>,
"robh@...nel.org" <robh@...nel.org>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
dl-linux-imx <linux-imx@....com>
Subject: RE: [PATCH V6 1/2] soc: imx: Add SCU SoC info driver support
Hi, Russell
> -----Original Message-----
> From: Russell King - ARM Linux admin [mailto:linux@...linux.org.uk]
> Sent: Thursday, May 23, 2019 5:10 PM
> To: Anson Huang <anson.huang@....com>
> Cc: catalin.marinas@....com; will.deacon@....com;
> shawnguo@...nel.org; s.hauer@...gutronix.de; kernel@...gutronix.de;
> festevam@...il.com; maxime.ripard@...tlin.com; olof@...om.net;
> agross@...nel.org; jagan@...rulasolutions.com;
> bjorn.andersson@...aro.org; Leonard Crestez <leonard.crestez@....com>;
> dinguyen@...nel.org; enric.balletbo@...labora.com; Aisheng Dong
> <aisheng.dong@....com>; Abel Vesa <abel.vesa@....com>;
> l.stach@...gutronix.de; tglx@...utronix.de; robh@...nel.org;
> gregkh@...uxfoundation.org; linux-arm-kernel@...ts.infradead.org; linux-
> kernel@...r.kernel.org; dl-linux-imx <linux-imx@....com>
> Subject: Re: [PATCH V6 1/2] soc: imx: Add SCU SoC info driver support
>
> On Thu, May 23, 2019 at 04:53:46AM +0000, Anson Huang wrote:
> > Add i.MX SCU SoC info driver to support i.MX8QXP SoC, introduce driver
> > dependency into Kconfig as CONFIG_IMX_SCU must be selected to support
> > i.MX SCU SoC driver, also need to use platform driver model to make
> > sure IMX_SCU driver is probed before i.MX SCU SoC driver.
>
> This seems buggy...
We have to use platform driver model, but there is no such soc device in DT file,
so we created the platform device in this driver directly.
>
> > +static int imx_scu_soc_probe(struct platform_device *pdev) {
> > + struct soc_device_attribute *soc_dev_attr;
> > + struct soc_device *soc_dev;
> > + u32 id, val;
> > + int ret;
> > +
> > + ret = imx_scu_get_handle(&soc_ipc_handle);
> > + if (ret)
> > + return ret;
> > +
> > + soc_dev_attr = devm_kzalloc(&pdev->dev, sizeof(*soc_dev_attr),
> > + GFP_KERNEL);
> > + if (!soc_dev_attr)
> > + return -ENOMEM;
> > +
> > + soc_dev_attr->family = "Freescale i.MX";
> > +
> > + ret = of_property_read_string(of_root,
> > + "model",
> > + &soc_dev_attr->machine);
> > + if (ret)
> > + return ret;
> > +
> > + id = imx_scu_soc_id();
> > +
> > + /* format soc_id value passed from SCU firmware */
> > + val = id & 0x1f;
> > + soc_dev_attr->soc_id = val ? kasprintf(GFP_KERNEL, "0x%x", val)
> > + : "unknown";
>
> soc_id can either be an allocated string, or it can be a static string of
> "unknown".
>
> > + if (!soc_dev_attr->soc_id)
> > + return -ENOMEM;
> > +
> > + /* format revision value passed from SCU firmware */
> > + val = (id >> 5) & 0xf;
> > + val = (((val >> 2) + 1) << 4) | (val & 0x3);
> > + soc_dev_attr->revision = val ? kasprintf(GFP_KERNEL,
> > + "%d.%d",
> > + (val >> 4) & 0xf,
> > + val & 0xf) : "unknown";
>
> revision here is the same.
>
> > + if (!soc_dev_attr->revision) {
> > + ret = -ENOMEM;
> > + goto free_soc_id;
> > + }
> > +
> > + soc_dev = soc_device_register(soc_dev_attr);
> > + if (IS_ERR(soc_dev)) {
> > + ret = PTR_ERR(soc_dev);
> > + goto free_revision;
> > + }
> > +
> > + return 0;
> > +
> > +free_revision:
> > + kfree(soc_dev_attr->revision);
> > +free_soc_id:
> > + kfree(soc_dev_attr->soc_id);
>
> Yet here you blindly kfree(), which means you could be doing in effect
> kfree("unknown") - which will likely result in a kernel oops.
>
> Either always make revision/soc_id an allocated string, or use some static
> storage for the strings (they're only small, static storage is likely to be much
> more efficient in this case, and there can only be one of these in any case.)
You are right, actually the "unknown" message is NOT helpful at all, so in next version,
I will just check the return value from SCU firmware, if it fail, just skip this driver probe,
If it success, it will just print out the soc_id and revision passing from SCU firmware, so no
need to have "unknown" string here.
+++ b/drivers/soc/imx/soc-imx-scu.c
@@ -27,7 +27,7 @@ struct imx_sc_msg_misc_get_soc_id {
} data;
} __packed;
-static u32 imx_scu_soc_id(void)
+static int imx_scu_soc_id(void)
{
struct imx_sc_msg_misc_get_soc_id msg;
struct imx_sc_rpc_msg *hdr = &msg.hdr;
@@ -44,8 +44,7 @@ static u32 imx_scu_soc_id(void)
ret = imx_scu_call_rpc(soc_ipc_handle, &msg, true);
if (ret) {
pr_err("%s: get soc info failed, ret %d\n", __func__, ret);
- /* return 0 means getting revision failed */
- return 0;
+ return ret;
}
return msg.data.resp.id;
@@ -55,7 +54,8 @@ static int imx_scu_soc_probe(struct platform_device *pdev)
{
struct soc_device_attribute *soc_dev_attr;
struct soc_device *soc_dev;
- u32 id, val;
+ u32 val;
+ int id;
int ret;
ret = imx_scu_get_handle(&soc_ipc_handle);
@@ -76,21 +76,22 @@ static int imx_scu_soc_probe(struct platform_device *pdev)
return ret;
id = imx_scu_soc_id();
+ if (id < 0)
+ return -EINVAL;
/* format soc_id value passed from SCU firmware */
val = id & 0x1f;
- soc_dev_attr->soc_id = val ? kasprintf(GFP_KERNEL, "0x%x", val)
- : "unknown";
+ soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "0x%x", val);
if (!soc_dev_attr->soc_id)
return -ENOMEM;
/* format revision value passed from SCU firmware */
val = (id >> 5) & 0xf;
val = (((val >> 2) + 1) << 4) | (val & 0x3);
- soc_dev_attr->revision = val ? kasprintf(GFP_KERNEL,
- "%d.%d",
- (val >> 4) & 0xf,
- val & 0xf) : "unknown";
+ soc_dev_attr->revision = kasprintf(GFP_KERNEL,
+ "%d.%d",
+ (val >> 4) & 0xf,
+ val & 0xf);
if (!soc_dev_attr->revision) {
Thanks,
Anson.
>
> > + return ret;
> > +}
> > +
> > +static struct platform_driver imx_scu_soc_driver = {
> > + .driver = {
> > + .name = IMX_SCU_SOC_DRIVER_NAME,
> > + },
> > + .probe = imx_scu_soc_probe,
> > +};
> > +
> > +static int __init imx_scu_soc_init(void) {
> > + struct platform_device *imx_scu_soc_pdev;
> > + struct device_node *np;
> > + int ret;
> > +
> > + np = of_find_compatible_node(NULL, NULL, "fsl,imx-scu");
> > + if (!np)
> > + return -ENODEV;
> > +
> > + of_node_put(np);
> > +
> > + ret = platform_driver_register(&imx_scu_soc_driver);
> > + if (ret)
> > + return ret;
> > +
> > + imx_scu_soc_pdev =
> > +
> platform_device_register_simple(IMX_SCU_SOC_DRIVER_NAME,
> > + -1,
> > + NULL,
> > + 0);
> > + if (IS_ERR(imx_scu_soc_pdev))
> > + platform_driver_unregister(&imx_scu_soc_driver);
> > +
> > + return PTR_ERR_OR_ZERO(imx_scu_soc_pdev);
> > +}
> > +device_initcall(imx_scu_soc_init);
> > --
> > 2.7.4
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@...ts.infradead.org
> > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists
> > .infradead.org%2Fmailman%2Flistinfo%2Flinux-arm-
> kernel&data=02%7C0
> >
> 1%7Canson.huang%40nxp.com%7C6ef296866ce243d9473c08d6df5e6b5f%7C
> 686ea1d
> >
> 3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636941993959195561&sdat
> a=mXdow
> >
> fLshJ%2BQlyLJ%2BbyYwuDUXh3CwOBAiZ%2BvVED%2FRh4%3D&reserve
> d=0
> >
>
> --
> RMK's Patch system:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.
> armlinux.org.uk%2Fdeveloper%2Fpatches%2F&data=02%7C01%7Canso
> n.huang%40nxp.com%7C6ef296866ce243d9473c08d6df5e6b5f%7C686ea1d3
> bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636941993959195561&sdata
> =rsrH%2F9pqdLZUJ0E%2BWUa2Kf71rtOcNDyjJ81R8Ex6yQs%3D&reserve
> d=0
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down
> 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
Powered by blists - more mailing lists