| Message ID | 20240808042858.2768309-1-make24@iscas.ac.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soc: imx: imx8m-blk-ctrl: Fix NULL pointer dereference | expand |
Hi Ma, thank you for the patch. On 24-08-08, Ma Ke wrote: > Check bc->bus_power_dev = dev_pm_domain_attach_by_name() return value using > IS_ERR_OR_NULL() instead of plain IS_ERR(), and fail if bc->bus_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, which leads to NULL pointer dereference. Argh.. there are other users of this API getting this wrong too. This make me wonder why dev_pm_domain_attach_by_name() return NULL instead of the error code returned by of_property_match_string(). IMHO to fix once and for all users we should fix the return code of dev_pm_domain_attach_by_name(). Regards, Marco > Found by code review. > > Cc: stable@vger.kernel.org > Fixes: 1a1da28544fd ("soc: imx: imx8m-blk-ctrl: Defer probe if 'bus' genpd is not yet ready") > Signed-off-by: Ma Ke <make24@iscas.ac.cn> > --- > drivers/pmdomain/imx/imx8m-blk-ctrl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pmdomain/imx/imx8m-blk-ctrl.c b/drivers/pmdomain/imx/imx8m-blk-ctrl.c > index ca942d7929c2..d46fb5387148 100644 > --- a/drivers/pmdomain/imx/imx8m-blk-ctrl.c > +++ b/drivers/pmdomain/imx/imx8m-blk-ctrl.c > @@ -212,7 +212,7 @@ static int imx8m_blk_ctrl_probe(struct platform_device *pdev) > return -ENOMEM; > > bc->bus_power_dev = dev_pm_domain_attach_by_name(dev, "bus"); > - if (IS_ERR(bc->bus_power_dev)) { > + if (IS_ERR_OR_NULL(bc->bus_power_dev)) { > if (PTR_ERR(bc->bus_power_dev) == -ENODEV) > return dev_err_probe(dev, -EPROBE_DEFER, > "failed to attach power domain \"bus\"\n"); > -- > 2.25.1 > > >
Hi Ma, On 24-08-08, Ma Ke wrote: > Check bc->bus_power_dev = dev_pm_domain_attach_by_name() return value using > IS_ERR_OR_NULL() instead of plain IS_ERR(), and fail if bc->bus_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, which leads to NULL pointer dereference. the same comment I wrote previously applies to this patch. Regards, Marco > > Found by code review. > > Cc: stable@vger.kernel.org > Fixes: 1a1da28544fd ("soc: imx: imx8m-blk-ctrl: Defer probe if 'bus' genpd is not yet ready") > Signed-off-by: Ma Ke <make24@iscas.ac.cn> > --- > drivers/pmdomain/imx/imx8m-blk-ctrl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pmdomain/imx/imx8m-blk-ctrl.c b/drivers/pmdomain/imx/imx8m-blk-ctrl.c > index ca942d7929c2..d46fb5387148 100644 > --- a/drivers/pmdomain/imx/imx8m-blk-ctrl.c > +++ b/drivers/pmdomain/imx/imx8m-blk-ctrl.c > @@ -212,7 +212,7 @@ static int imx8m_blk_ctrl_probe(struct platform_device *pdev) > return -ENOMEM; > > bc->bus_power_dev = dev_pm_domain_attach_by_name(dev, "bus"); > - if (IS_ERR(bc->bus_power_dev)) { > + if (IS_ERR_OR_NULL(bc->bus_power_dev)) { > if (PTR_ERR(bc->bus_power_dev) == -ENODEV) > return dev_err_probe(dev, -EPROBE_DEFER, > "failed to attach power domain \"bus\"\n"); > -- > 2.25.1 > > >
On Thu, Aug 8, 2024, at 08:12, Marco Felsch wrote: > > On 24-08-08, Ma Ke wrote: >> Check bc->bus_power_dev = dev_pm_domain_attach_by_name() return value using >> IS_ERR_OR_NULL() instead of plain IS_ERR(), and fail if bc->bus_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, which leads to NULL pointer dereference. > > Argh.. there are other users of this API getting this wrong too. This > make me wonder why dev_pm_domain_attach_by_name() return NULL instead of > the error code returned by of_property_match_string(). > > IMHO to fix once and for all users we should fix the return code of > dev_pm_domain_attach_by_name(). Agreed, in general any use of IS_ERR_OR_NULL() indicates that there is a bad API that should be fixed instead, and this is probably the case for genpd_dev_pm_attach_by_id(). One common use that is widely accepted is returning NULL when a subsystem is completely disabled. In this case an IS_ERR() check returns false on a NULL pointer and the returned structure should be opaque so callers are unable to dereference that NULL pointer. genpd_dev_pm_attach_by_{id,name}() is documented to also return a NULL pointer when no PM domain is needed, but they return a normal 'struct device' that can easily be used in an unsafe way after checking for IS_ERR(). Fortunately it seems that there are only a few callers at the moment, so coming up with a safer interface is still possible. Arnd
On Thu, 8 Aug 2024 at 08:53, Arnd Bergmann <arnd@arndb.de> wrote: > > On Thu, Aug 8, 2024, at 08:12, Marco Felsch wrote: > > > > On 24-08-08, Ma Ke wrote: > >> Check bc->bus_power_dev = dev_pm_domain_attach_by_name() return value using > >> IS_ERR_OR_NULL() instead of plain IS_ERR(), and fail if bc->bus_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, which leads to NULL pointer dereference. > > > > Argh.. there are other users of this API getting this wrong too. This > > make me wonder why dev_pm_domain_attach_by_name() return NULL instead of > > the error code returned by of_property_match_string(). > > > > IMHO to fix once and for all users we should fix the return code of > > dev_pm_domain_attach_by_name(). > > Agreed, in general any use of IS_ERR_OR_NULL() indicates that there > is a bad API that should be fixed instead, and this is probably the > case for genpd_dev_pm_attach_by_id(). > > One common use that is widely accepted is returning NULL when > a subsystem is completely disabled. In this case an IS_ERR() > check returns false on a NULL pointer and the returned structure > should be opaque so callers are unable to dereference that > NULL pointer. > > genpd_dev_pm_attach_by_{id,name}() is documented to also return > a NULL pointer when no PM domain is needed, but they return > a normal 'struct device' that can easily be used in an unsafe > way after checking for IS_ERR(). > > Fortunately it seems that there are only a few callers at the > moment, so coming up with a safer interface is still possible. I am not sure it's worth the effort, but I may be wrong. It's been a bit tricky to keep the interfaces above consistent with the legacy interface (dev_pm_domain_attach()). Moreover, we need a way to allow a PM domain to be optional. By returning NULL (or 0), we are telling the consumer that there is no PM domain described that we can attach the device to. Kind regards Uffe
On Thu, 8 Aug 2024 at 06:29, Ma Ke <make24@iscas.ac.cn> wrote: > > Check bc->bus_power_dev = dev_pm_domain_attach_by_name() return value using > IS_ERR_OR_NULL() instead of plain IS_ERR(), and fail if bc->bus_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, which leads to NULL pointer dereference. > > Found by code review. > > Cc: stable@vger.kernel.org > Fixes: 1a1da28544fd ("soc: imx: imx8m-blk-ctrl: Defer probe if 'bus' genpd is not yet ready") > Signed-off-by: Ma Ke <make24@iscas.ac.cn> > --- > drivers/pmdomain/imx/imx8m-blk-ctrl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pmdomain/imx/imx8m-blk-ctrl.c b/drivers/pmdomain/imx/imx8m-blk-ctrl.c > index ca942d7929c2..d46fb5387148 100644 > --- a/drivers/pmdomain/imx/imx8m-blk-ctrl.c > +++ b/drivers/pmdomain/imx/imx8m-blk-ctrl.c > @@ -212,7 +212,7 @@ static int imx8m_blk_ctrl_probe(struct platform_device *pdev) > return -ENOMEM; > > bc->bus_power_dev = dev_pm_domain_attach_by_name(dev, "bus"); > - if (IS_ERR(bc->bus_power_dev)) { > + if (IS_ERR_OR_NULL(bc->bus_power_dev)) { > if (PTR_ERR(bc->bus_power_dev) == -ENODEV) > return dev_err_probe(dev, -EPROBE_DEFER, > "failed to attach power domain \"bus\"\n"); The corresponding else clause uses PTR_ERR(bc->bus_power_dev), would that trigger a splat too? > -- > 2.25.1 > Kind regards Uffe
On 24-08-13, Ulf Hansson wrote: > On Thu, 8 Aug 2024 at 08:53, Arnd Bergmann <arnd@arndb.de> wrote: > > > > On Thu, Aug 8, 2024, at 08:12, Marco Felsch wrote: > > > > > > On 24-08-08, Ma Ke wrote: > > >> Check bc->bus_power_dev = dev_pm_domain_attach_by_name() return value using > > >> IS_ERR_OR_NULL() instead of plain IS_ERR(), and fail if bc->bus_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, which leads to NULL pointer dereference. > > > > > > Argh.. there are other users of this API getting this wrong too. This > > > make me wonder why dev_pm_domain_attach_by_name() return NULL instead of > > > the error code returned by of_property_match_string(). > > > > > > IMHO to fix once and for all users we should fix the return code of > > > dev_pm_domain_attach_by_name(). > > > > Agreed, in general any use of IS_ERR_OR_NULL() indicates that there > > is a bad API that should be fixed instead, and this is probably the > > case for genpd_dev_pm_attach_by_id(). > > > > One common use that is widely accepted is returning NULL when > > a subsystem is completely disabled. In this case an IS_ERR() > > check returns false on a NULL pointer and the returned structure > > should be opaque so callers are unable to dereference that > > NULL pointer. > > > > genpd_dev_pm_attach_by_{id,name}() is documented to also return > > a NULL pointer when no PM domain is needed, but they return > > a normal 'struct device' that can easily be used in an unsafe > > way after checking for IS_ERR(). > > > > Fortunately it seems that there are only a few callers at the > > moment, so coming up with a safer interface is still possible. > > I am not sure it's worth the effort, but I may be wrong. > > It's been a bit tricky to keep the interfaces above consistent with > the legacy interface (dev_pm_domain_attach()). Moreover, we need a way > to allow a PM domain to be optional. By returning NULL (or 0), we are > telling the consumer that there is no PM domain described that we can > attach the device to. Other subsystems like GPIO, regulator have a ..._optional API for this, could this be an option? Regards, Marco > > Kind regards > Uffe >
On Wed, 14 Aug 2024 at 12:58, Marco Felsch <m.felsch@pengutronix.de> wrote: > > On 24-08-13, Ulf Hansson wrote: > > On Thu, 8 Aug 2024 at 08:53, Arnd Bergmann <arnd@arndb.de> wrote: > > > > > > On Thu, Aug 8, 2024, at 08:12, Marco Felsch wrote: > > > > > > > > On 24-08-08, Ma Ke wrote: > > > >> Check bc->bus_power_dev = dev_pm_domain_attach_by_name() return value using > > > >> IS_ERR_OR_NULL() instead of plain IS_ERR(), and fail if bc->bus_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, which leads to NULL pointer dereference. > > > > > > > > Argh.. there are other users of this API getting this wrong too. This > > > > make me wonder why dev_pm_domain_attach_by_name() return NULL instead of > > > > the error code returned by of_property_match_string(). > > > > > > > > IMHO to fix once and for all users we should fix the return code of > > > > dev_pm_domain_attach_by_name(). > > > > > > Agreed, in general any use of IS_ERR_OR_NULL() indicates that there > > > is a bad API that should be fixed instead, and this is probably the > > > case for genpd_dev_pm_attach_by_id(). > > > > > > One common use that is widely accepted is returning NULL when > > > a subsystem is completely disabled. In this case an IS_ERR() > > > check returns false on a NULL pointer and the returned structure > > > should be opaque so callers are unable to dereference that > > > NULL pointer. > > > > > > genpd_dev_pm_attach_by_{id,name}() is documented to also return > > > a NULL pointer when no PM domain is needed, but they return > > > a normal 'struct device' that can easily be used in an unsafe > > > way after checking for IS_ERR(). > > > > > > Fortunately it seems that there are only a few callers at the > > > moment, so coming up with a safer interface is still possible. > > > > I am not sure it's worth the effort, but I may be wrong. > > > > It's been a bit tricky to keep the interfaces above consistent with > > the legacy interface (dev_pm_domain_attach()). Moreover, we need a way > > to allow a PM domain to be optional. By returning NULL (or 0), we are > > telling the consumer that there is no PM domain described that we can > > attach the device to. > > Other subsystems like GPIO, regulator have a ..._optional API for this, > could this be an option? If we were in a position of re-implementing the interfaces from scratch, then probably yes. At this point, I am not so sure the curns to have it are worth the benefit. Keep in mind that the legacy dev_pm_domain_attach() is already optional and it's used by common bus level code. Kind regards Uffe
diff --git a/drivers/pmdomain/imx/imx8m-blk-ctrl.c b/drivers/pmdomain/imx/imx8m-blk-ctrl.c index ca942d7929c2..d46fb5387148 100644 --- a/drivers/pmdomain/imx/imx8m-blk-ctrl.c +++ b/drivers/pmdomain/imx/imx8m-blk-ctrl.c @@ -212,7 +212,7 @@ static int imx8m_blk_ctrl_probe(struct platform_device *pdev) return -ENOMEM; bc->bus_power_dev = dev_pm_domain_attach_by_name(dev, "bus"); - if (IS_ERR(bc->bus_power_dev)) { + if (IS_ERR_OR_NULL(bc->bus_power_dev)) { if (PTR_ERR(bc->bus_power_dev) == -ENODEV) return dev_err_probe(dev, -EPROBE_DEFER, "failed to attach power domain \"bus\"\n");
Check bc->bus_power_dev = dev_pm_domain_attach_by_name() return value using IS_ERR_OR_NULL() instead of plain IS_ERR(), and fail if bc->bus_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, which leads to NULL pointer dereference. Found by code review. Cc: stable@vger.kernel.org Fixes: 1a1da28544fd ("soc: imx: imx8m-blk-ctrl: Defer probe if 'bus' genpd is not yet ready") Signed-off-by: Ma Ke <make24@iscas.ac.cn> --- drivers/pmdomain/imx/imx8m-blk-ctrl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)