| Message ID | 20221215034800.275502-2-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] soc: imx: imx8m-blk-ctrl: Fix NULL pointer dereference | expand |
On Thu, Dec 15, 2022 at 04:48:00AM +0100, Marek Vasut wrote: > Check domain->power_dev = dev_pm_domain_attach_by_name() return value using > IS_ERR_OR_NULL() instead of plain IS_ERR(), and fail if domain->power_dev is > either error or NULL. > > In case a power domain attached by dev_pm_domain_attach_by_name() is not > described in DT, dev_pm_domain_attach_by_name() returns NULL, which is > then used a few lines below in dev_set_name(domain->power_dev, ...);, > which leads to NULL pointer dereference. > > Fixes: 556f5cf9568a ("soc: imx: add i.MX8MP HSIO blk-ctrl") > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Adam Ford <aford173@gmail.com> > Cc: Benjamin Gaignard <benjamin.gaignard@collabora.com> > Cc: Fabio Estevam <festevam@gmail.com> > Cc: Lucas Stach <l.stach@pengutronix.de> > Cc: Marco Felsch <m.felsch@pengutronix.de> > Cc: Marek Vasut <marex@denx.de> > Cc: NXP Linux Team <linux-imx@nxp.com> > Cc: Paul Elder <paul.elder@ideasonboard.com> > Cc: Peng Fan <peng.fan@nxp.com> > Cc: Pengutronix Kernel Team <kernel@pengutronix.de> > Cc: Sascha Hauer <s.hauer@pengutronix.de> > Cc: Shawn Guo <shawnguo@kernel.org> > To: linux-arm-kernel@lists.infradead.org > --- > drivers/soc/imx/imx8mp-blk-ctrl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/soc/imx/imx8mp-blk-ctrl.c b/drivers/soc/imx/imx8mp-blk-ctrl.c > index b7d4161fcda9c..916bf0b3bc975 100644 > --- a/drivers/soc/imx/imx8mp-blk-ctrl.c > +++ b/drivers/soc/imx/imx8mp-blk-ctrl.c > @@ -553,7 +553,7 @@ static int imx8mp_blk_ctrl_probe(struct platform_device *pdev) > > domain->power_dev = > dev_pm_domain_attach_by_name(dev, data->gpc_name); > - if (IS_ERR(domain->power_dev)) { > + if (IS_ERR_OR_NULL(domain->power_dev)) { > dev_err_probe(dev, PTR_ERR(domain->power_dev), > "failed to attach power domain %s\n", > data->gpc_name); Same problem as with the other patch, you shouldn't use 0 as error value. Sascha
On 22-12-15, Sascha Hauer wrote: > On Thu, Dec 15, 2022 at 04:48:00AM +0100, Marek Vasut wrote: > > Check domain->power_dev = dev_pm_domain_attach_by_name() return value using > > IS_ERR_OR_NULL() instead of plain IS_ERR(), and fail if domain->power_dev is > > either error or NULL. > > > > In case a power domain attached by dev_pm_domain_attach_by_name() is not > > described in DT, dev_pm_domain_attach_by_name() returns NULL, which is > > then used a few lines below in dev_set_name(domain->power_dev, ...);, > > which leads to NULL pointer dereference. > > > > Fixes: 556f5cf9568a ("soc: imx: add i.MX8MP HSIO blk-ctrl") > > Signed-off-by: Marek Vasut <marex@denx.de> > > --- > > Cc: Adam Ford <aford173@gmail.com> > > Cc: Benjamin Gaignard <benjamin.gaignard@collabora.com> > > Cc: Fabio Estevam <festevam@gmail.com> > > Cc: Lucas Stach <l.stach@pengutronix.de> > > Cc: Marco Felsch <m.felsch@pengutronix.de> > > Cc: Marek Vasut <marex@denx.de> > > Cc: NXP Linux Team <linux-imx@nxp.com> > > Cc: Paul Elder <paul.elder@ideasonboard.com> > > Cc: Peng Fan <peng.fan@nxp.com> > > Cc: Pengutronix Kernel Team <kernel@pengutronix.de> > > Cc: Sascha Hauer <s.hauer@pengutronix.de> > > Cc: Shawn Guo <shawnguo@kernel.org> > > To: linux-arm-kernel@lists.infradead.org > > --- > > drivers/soc/imx/imx8mp-blk-ctrl.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/soc/imx/imx8mp-blk-ctrl.c b/drivers/soc/imx/imx8mp-blk-ctrl.c > > index b7d4161fcda9c..916bf0b3bc975 100644 > > --- a/drivers/soc/imx/imx8mp-blk-ctrl.c > > +++ b/drivers/soc/imx/imx8mp-blk-ctrl.c > > @@ -553,7 +553,7 @@ static int imx8mp_blk_ctrl_probe(struct platform_device *pdev) > > > > domain->power_dev = > > dev_pm_domain_attach_by_name(dev, data->gpc_name); > > - if (IS_ERR(domain->power_dev)) { > > + if (IS_ERR_OR_NULL(domain->power_dev)) { > > dev_err_probe(dev, PTR_ERR(domain->power_dev), > > "failed to attach power domain %s\n", > > data->gpc_name); > > Same problem as with the other patch, you shouldn't use 0 as error > value. This and IIRC Lucas already had a patch to fix. Regards, Marco > > Sascha > > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | >
On 12/15/22 10:04, Marco Felsch wrote: > On 22-12-15, Sascha Hauer wrote: >> On Thu, Dec 15, 2022 at 04:48:00AM +0100, Marek Vasut wrote: >>> Check domain->power_dev = dev_pm_domain_attach_by_name() return value using >>> IS_ERR_OR_NULL() instead of plain IS_ERR(), and fail if domain->power_dev is >>> either error or NULL. >>> >>> In case a power domain attached by dev_pm_domain_attach_by_name() is not >>> described in DT, dev_pm_domain_attach_by_name() returns NULL, which is >>> then used a few lines below in dev_set_name(domain->power_dev, ...);, >>> which leads to NULL pointer dereference. >>> >>> Fixes: 556f5cf9568a ("soc: imx: add i.MX8MP HSIO blk-ctrl") >>> Signed-off-by: Marek Vasut <marex@denx.de> >>> --- >>> Cc: Adam Ford <aford173@gmail.com> >>> Cc: Benjamin Gaignard <benjamin.gaignard@collabora.com> >>> Cc: Fabio Estevam <festevam@gmail.com> >>> Cc: Lucas Stach <l.stach@pengutronix.de> >>> Cc: Marco Felsch <m.felsch@pengutronix.de> >>> Cc: Marek Vasut <marex@denx.de> >>> Cc: NXP Linux Team <linux-imx@nxp.com> >>> Cc: Paul Elder <paul.elder@ideasonboard.com> >>> Cc: Peng Fan <peng.fan@nxp.com> >>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de> >>> Cc: Sascha Hauer <s.hauer@pengutronix.de> >>> Cc: Shawn Guo <shawnguo@kernel.org> >>> To: linux-arm-kernel@lists.infradead.org >>> --- >>> drivers/soc/imx/imx8mp-blk-ctrl.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/soc/imx/imx8mp-blk-ctrl.c b/drivers/soc/imx/imx8mp-blk-ctrl.c >>> index b7d4161fcda9c..916bf0b3bc975 100644 >>> --- a/drivers/soc/imx/imx8mp-blk-ctrl.c >>> +++ b/drivers/soc/imx/imx8mp-blk-ctrl.c >>> @@ -553,7 +553,7 @@ static int imx8mp_blk_ctrl_probe(struct platform_device *pdev) >>> >>> domain->power_dev = >>> dev_pm_domain_attach_by_name(dev, data->gpc_name); >>> - if (IS_ERR(domain->power_dev)) { >>> + if (IS_ERR_OR_NULL(domain->power_dev)) { >>> dev_err_probe(dev, PTR_ERR(domain->power_dev), >>> "failed to attach power domain %s\n", >>> data->gpc_name); >> >> Same problem as with the other patch, you shouldn't use 0 as error >> value. > > This and IIRC Lucas already had a patch to fix. Seems there is a fix for 8m-blk-ctrl, not 8mp , but the fix would then be identical.
On Sat, Dec 17, 2022 at 5:26 PM Marek Vasut <marex@denx.de> wrote: > > On 12/15/22 10:04, Marco Felsch wrote: > > On 22-12-15, Sascha Hauer wrote: > >> On Thu, Dec 15, 2022 at 04:48:00AM +0100, Marek Vasut wrote: > >>> Check domain->power_dev = dev_pm_domain_attach_by_name() return value using > >>> IS_ERR_OR_NULL() instead of plain IS_ERR(), and fail if domain->power_dev is > >>> either error or NULL. > >>> > >>> In case a power domain attached by dev_pm_domain_attach_by_name() is not > >>> described in DT, dev_pm_domain_attach_by_name() returns NULL, which is > >>> then used a few lines below in dev_set_name(domain->power_dev, ...);, > >>> which leads to NULL pointer dereference. > >>> > >>> Fixes: 556f5cf9568a ("soc: imx: add i.MX8MP HSIO blk-ctrl") > >>> Signed-off-by: Marek Vasut <marex@denx.de> > >>> --- > >>> Cc: Adam Ford <aford173@gmail.com> > >>> Cc: Benjamin Gaignard <benjamin.gaignard@collabora.com> > >>> Cc: Fabio Estevam <festevam@gmail.com> > >>> Cc: Lucas Stach <l.stach@pengutronix.de> > >>> Cc: Marco Felsch <m.felsch@pengutronix.de> > >>> Cc: Marek Vasut <marex@denx.de> > >>> Cc: NXP Linux Team <linux-imx@nxp.com> > >>> Cc: Paul Elder <paul.elder@ideasonboard.com> > >>> Cc: Peng Fan <peng.fan@nxp.com> > >>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de> > >>> Cc: Sascha Hauer <s.hauer@pengutronix.de> > >>> Cc: Shawn Guo <shawnguo@kernel.org> > >>> To: linux-arm-kernel@lists.infradead.org > >>> --- > >>> drivers/soc/imx/imx8mp-blk-ctrl.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/soc/imx/imx8mp-blk-ctrl.c b/drivers/soc/imx/imx8mp-blk-ctrl.c > >>> index b7d4161fcda9c..916bf0b3bc975 100644 > >>> --- a/drivers/soc/imx/imx8mp-blk-ctrl.c > >>> +++ b/drivers/soc/imx/imx8mp-blk-ctrl.c > >>> @@ -553,7 +553,7 @@ static int imx8mp_blk_ctrl_probe(struct platform_device *pdev) > >>> > >>> domain->power_dev = > >>> dev_pm_domain_attach_by_name(dev, data->gpc_name); > >>> - if (IS_ERR(domain->power_dev)) { > >>> + if (IS_ERR_OR_NULL(domain->power_dev)) { > >>> dev_err_probe(dev, PTR_ERR(domain->power_dev), > >>> "failed to attach power domain %s\n", > >>> data->gpc_name); > >> > >> Same problem as with the other patch, you shouldn't use 0 as error > >> value. > > > > This and IIRC Lucas already had a patch to fix. > > Seems there is a fix for 8m-blk-ctrl, not 8mp , but the fix would then > be identical. I think this might be the patch from Lucas that Marco was referencing: https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=671618&archive=both&state=* adam
On 12/18/22 14:29, Adam Ford wrote: Hi, >>>>> diff --git a/drivers/soc/imx/imx8mp-blk-ctrl.c b/drivers/soc/imx/imx8mp-blk-ctrl.c >>>>> index b7d4161fcda9c..916bf0b3bc975 100644 >>>>> --- a/drivers/soc/imx/imx8mp-blk-ctrl.c >>>>> +++ b/drivers/soc/imx/imx8mp-blk-ctrl.c >>>>> @@ -553,7 +553,7 @@ static int imx8mp_blk_ctrl_probe(struct platform_device *pdev) >>>>> >>>>> domain->power_dev = >>>>> dev_pm_domain_attach_by_name(dev, data->gpc_name); >>>>> - if (IS_ERR(domain->power_dev)) { >>>>> + if (IS_ERR_OR_NULL(domain->power_dev)) { >>>>> dev_err_probe(dev, PTR_ERR(domain->power_dev), >>>>> "failed to attach power domain %s\n", >>>>> data->gpc_name); >>>> >>>> Same problem as with the other patch, you shouldn't use 0 as error >>>> value. >>> >>> This and IIRC Lucas already had a patch to fix. >> >> Seems there is a fix for 8m-blk-ctrl, not 8mp , but the fix would then >> be identical. > > I think this might be the patch from Lucas that Marco was referencing: > > https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=671618&archive=both&state=* Since Linux 6.1 does not even boot on i.MX8MP and crashes before even printing anything on console (unless you use earlycon), it would be good to apply the 1/3 or "[PATCH] soc: imx: imx8mp-blk-ctrl: Do not set power domain name" I sent yesterday.
On 22-12-18, Marek Vasut wrote: > On 12/18/22 14:29, Adam Ford wrote: > > Hi, > > > > > > > diff --git a/drivers/soc/imx/imx8mp-blk-ctrl.c b/drivers/soc/imx/imx8mp-blk-ctrl.c > > > > > > index b7d4161fcda9c..916bf0b3bc975 100644 > > > > > > --- a/drivers/soc/imx/imx8mp-blk-ctrl.c > > > > > > +++ b/drivers/soc/imx/imx8mp-blk-ctrl.c > > > > > > @@ -553,7 +553,7 @@ static int imx8mp_blk_ctrl_probe(struct platform_device *pdev) > > > > > > > > > > > > domain->power_dev = > > > > > > dev_pm_domain_attach_by_name(dev, data->gpc_name); > > > > > > - if (IS_ERR(domain->power_dev)) { > > > > > > + if (IS_ERR_OR_NULL(domain->power_dev)) { > > > > > > dev_err_probe(dev, PTR_ERR(domain->power_dev), > > > > > > "failed to attach power domain %s\n", > > > > > > data->gpc_name); > > > > > > > > > > Same problem as with the other patch, you shouldn't use 0 as error > > > > > value. > > > > > > > > This and IIRC Lucas already had a patch to fix. > > > > > > Seems there is a fix for 8m-blk-ctrl, not 8mp , but the fix would then > > > be identical. > > > > I think this might be the patch from Lucas that Marco was referencing: > > > > https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=671618&archive=both&state=* > > Since Linux 6.1 does not even boot on i.MX8MP and crashes before even > printing anything on console (unless you use earlycon), it would be good to > apply the 1/3 or "[PATCH] soc: imx: imx8mp-blk-ctrl: Do not set power domain > name" I sent yesterday. Or just pick https://patchwork.kernel.org/project/linux-arm-kernel/patch/20220826191305.3215706-1-l.stach@pengutronix.de/ which has already Peng's review. Up to the maintainer which he want to use but we should fix that. Regards, Marco
diff --git a/drivers/soc/imx/imx8mp-blk-ctrl.c b/drivers/soc/imx/imx8mp-blk-ctrl.c index b7d4161fcda9c..916bf0b3bc975 100644 --- a/drivers/soc/imx/imx8mp-blk-ctrl.c +++ b/drivers/soc/imx/imx8mp-blk-ctrl.c @@ -553,7 +553,7 @@ static int imx8mp_blk_ctrl_probe(struct platform_device *pdev) domain->power_dev = dev_pm_domain_attach_by_name(dev, data->gpc_name); - if (IS_ERR(domain->power_dev)) { + if (IS_ERR_OR_NULL(domain->power_dev)) { dev_err_probe(dev, PTR_ERR(domain->power_dev), "failed to attach power domain %s\n", data->gpc_name);
Check domain->power_dev = dev_pm_domain_attach_by_name() return value using IS_ERR_OR_NULL() instead of plain IS_ERR(), and fail if domain->power_dev is either error or NULL. In case a power domain attached by dev_pm_domain_attach_by_name() is not described in DT, dev_pm_domain_attach_by_name() returns NULL, which is then used a few lines below in dev_set_name(domain->power_dev, ...);, which leads to NULL pointer dereference. Fixes: 556f5cf9568a ("soc: imx: add i.MX8MP HSIO blk-ctrl") Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: Adam Ford <aford173@gmail.com> Cc: Benjamin Gaignard <benjamin.gaignard@collabora.com> Cc: Fabio Estevam <festevam@gmail.com> Cc: Lucas Stach <l.stach@pengutronix.de> Cc: Marco Felsch <m.felsch@pengutronix.de> Cc: Marek Vasut <marex@denx.de> Cc: NXP Linux Team <linux-imx@nxp.com> Cc: Paul Elder <paul.elder@ideasonboard.com> Cc: Peng Fan <peng.fan@nxp.com> Cc: Pengutronix Kernel Team <kernel@pengutronix.de> Cc: Sascha Hauer <s.hauer@pengutronix.de> Cc: Shawn Guo <shawnguo@kernel.org> To: linux-arm-kernel@lists.infradead.org --- drivers/soc/imx/imx8mp-blk-ctrl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)