| Message ID | tencent_994DA85912C937E3B5405BA960B31ED90A08@qq.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | io: adc: stm32-adc: fix potential NULL pointer dereference in stm32_adc_probe() | expand |
On Sat, 15 Jul 2023 23:55:50 +0800 Zhang Shurong <zhang_shurong@foxmail.com> wrote: > of_match_device() may fail and returns a NULL pointer. > > Fix this by checking the return value of of_match_device(). > > Fixes: 64ad7f6438f3 ("iio: adc: stm32: introduce compatible data cfg") > Signed-off-by: Zhang Shurong <zhang_shurong@foxmail.com> Hi Zhang, I'm not sure we can actually make this bug happen. Do you have a way of triggering it? The driver is only probed on devices where that match will work. Also, assuming the match table is the same one associated with this probe function, then us priv->cfg = of_device_get_match_data() and check the output of that which is what we really care about. Jonathan > --- > drivers/iio/adc/stm32-adc-core.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/adc/stm32-adc-core.c b/drivers/iio/adc/stm32-adc-core.c > index 48f02dcc81c1..70011fdbf5f6 100644 > --- a/drivers/iio/adc/stm32-adc-core.c > +++ b/drivers/iio/adc/stm32-adc-core.c > @@ -706,6 +706,8 @@ static int stm32_adc_probe(struct platform_device *pdev) > struct stm32_adc_priv *priv; > struct device *dev = &pdev->dev; > struct device_node *np = pdev->dev.of_node; > + const struct of_device_id *of_id; > + > struct resource *res; > u32 max_rate; > int ret; > @@ -718,8 +720,11 @@ static int stm32_adc_probe(struct platform_device *pdev) > return -ENOMEM; > platform_set_drvdata(pdev, &priv->common); > > - priv->cfg = (const struct stm32_adc_priv_cfg *) > - of_match_device(dev->driver->of_match_table, dev)->data; > + of_id = of_match_device(dev->driver->of_match_table, dev); > + if (!of_id) > + return -ENODEV; > + > + priv->cfg = (const struct stm32_adc_priv_cfg *)of_id->data; > priv->nb_adc_max = priv->cfg->num_adcs; > spin_lock_init(&priv->common.lock); >
在 2023年7月17日星期一 CST 上午12:08:21，Jonathan Cameron 写道： > On Sat, 15 Jul 2023 23:55:50 +0800 > > Zhang Shurong <zhang_shurong@foxmail.com> wrote: > > of_match_device() may fail and returns a NULL pointer. > > > > Fix this by checking the return value of of_match_device(). > > > > Fixes: 64ad7f6438f3 ("iio: adc: stm32: introduce compatible data cfg") > > Signed-off-by: Zhang Shurong <zhang_shurong@foxmail.com> > > Hi Zhang, > > I'm not sure we can actually make this bug happen. Do you have > a way of triggering it? The driver is only probed on devices where > that match will work. > > Also, assuming the match table is the same one associated with this probe > function, then us priv->cfg = of_device_get_match_data() and check the > output of that which is what we really care about. > > Jonathan > > > --- > > > > drivers/iio/adc/stm32-adc-core.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iio/adc/stm32-adc-core.c > > b/drivers/iio/adc/stm32-adc-core.c index 48f02dcc81c1..70011fdbf5f6 > > 100644 > > --- a/drivers/iio/adc/stm32-adc-core.c > > +++ b/drivers/iio/adc/stm32-adc-core.c > > @@ -706,6 +706,8 @@ static int stm32_adc_probe(struct platform_device > > *pdev)> > > struct stm32_adc_priv *priv; > > struct device *dev = &pdev->dev; > > struct device_node *np = pdev->dev.of_node; > > > > + const struct of_device_id *of_id; > > + > > > > struct resource *res; > > u32 max_rate; > > int ret; > > > > @@ -718,8 +720,11 @@ static int stm32_adc_probe(struct platform_device > > *pdev)> > > return -ENOMEM; > > > > platform_set_drvdata(pdev, &priv->common); > > > > - priv->cfg = (const struct stm32_adc_priv_cfg *) > > - of_match_device(dev->driver->of_match_table, dev)->data; > > + of_id = of_match_device(dev->driver->of_match_table, dev); > > + if (!of_id) > > + return -ENODEV; > > + > > + priv->cfg = (const struct stm32_adc_priv_cfg *)of_id->data; > > > > priv->nb_adc_max = priv->cfg->num_adcs; > > spin_lock_init(&priv->common.lock); Hello Jonathan, I think we can make it happen by designing the platform device in a way that its name aligns with that of the driver. In such a scenario, when the driver is probed, the of_match_device function will return null. You can verify this functionality by reviewing the following function: static int platform_match(struct device *dev, struct device_driver *drv) Best regards, Shurong
On Mon, 28 Aug 2023 23:02:07 +0800 Zhang Shurong <zhang_shurong@foxmail.com> wrote: > 在 2023年7月17日星期一 CST 上午12:08:21，Jonathan Cameron 写道： > > On Sat, 15 Jul 2023 23:55:50 +0800 > > > > Zhang Shurong <zhang_shurong@foxmail.com> wrote: > > > of_match_device() may fail and returns a NULL pointer. > > > > > > Fix this by checking the return value of of_match_device(). > > > > > > Fixes: 64ad7f6438f3 ("iio: adc: stm32: introduce compatible data cfg") > > > Signed-off-by: Zhang Shurong <zhang_shurong@foxmail.com> > > > > Hi Zhang, > > > > I'm not sure we can actually make this bug happen. Do you have > > a way of triggering it? The driver is only probed on devices where > > that match will work. > > > > Also, assuming the match table is the same one associated with this probe > > function, then us priv->cfg = of_device_get_match_data() and check the > > output of that which is what we really care about. > > > > Jonathan > > > > > --- > > > > > > drivers/iio/adc/stm32-adc-core.c | 9 +++++++-- > > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/iio/adc/stm32-adc-core.c > > > b/drivers/iio/adc/stm32-adc-core.c index 48f02dcc81c1..70011fdbf5f6 > > > 100644 > > > --- a/drivers/iio/adc/stm32-adc-core.c > > > +++ b/drivers/iio/adc/stm32-adc-core.c > > > @@ -706,6 +706,8 @@ static int stm32_adc_probe(struct platform_device > > > *pdev)> > > > struct stm32_adc_priv *priv; > > > struct device *dev = &pdev->dev; > > > struct device_node *np = pdev->dev.of_node; > > > > > > + const struct of_device_id *of_id; > > > + > > > > > > struct resource *res; > > > u32 max_rate; > > > int ret; > > > > > > @@ -718,8 +720,11 @@ static int stm32_adc_probe(struct platform_device > > > *pdev)> > > > return -ENOMEM; > > > > > > platform_set_drvdata(pdev, &priv->common); > > > > > > - priv->cfg = (const struct stm32_adc_priv_cfg *) > > > - of_match_device(dev->driver->of_match_table, dev)->data; > > > + of_id = of_match_device(dev->driver->of_match_table, dev); > > > + if (!of_id) > > > + return -ENODEV; > > > + > > > + priv->cfg = (const struct stm32_adc_priv_cfg *)of_id->data; > > > > > > priv->nb_adc_max = priv->cfg->num_adcs; > > > spin_lock_init(&priv->common.lock); > Hello Jonathan, > > I think we can make it happen by designing the platform device in a way that > its name aligns with that of the driver. In such a scenario, when the driver > is probed, the of_match_device function will return null. You can verify this > functionality by reviewing the following function: > > static int platform_match(struct device *dev, struct device_driver *drv) I don't think we care about that case. If there is a real example of why someone would do this then that would be a different matter. Jonathan > > Best regards, > Shurong > > >
在 2023年8月29日星期二 CST 上午12:16:05，Jonathan Cameron 写道： > On Mon, 28 Aug 2023 23:02:07 +0800 > > Zhang Shurong <zhang_shurong@foxmail.com> wrote: > > 在 2023年7月17日星期一 CST 上午12:08:21，Jonathan Cameron 写道： > > > > > On Sat, 15 Jul 2023 23:55:50 +0800 > > > > > > Zhang Shurong <zhang_shurong@foxmail.com> wrote: > > > > of_match_device() may fail and returns a NULL pointer. > > > > > > > > Fix this by checking the return value of of_match_device(). > > > > > > > > Fixes: 64ad7f6438f3 ("iio: adc: stm32: introduce compatible data cfg") > > > > Signed-off-by: Zhang Shurong <zhang_shurong@foxmail.com> > > > > > > Hi Zhang, > > > > > > I'm not sure we can actually make this bug happen. Do you have > > > a way of triggering it? The driver is only probed on devices where > > > that match will work. > > > > > > Also, assuming the match table is the same one associated with this > > > probe > > > function, then us priv->cfg = of_device_get_match_data() and check the > > > output of that which is what we really care about. > > > > > > Jonathan > > > > > > > --- > > > > > > > > drivers/iio/adc/stm32-adc-core.c | 9 +++++++-- > > > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/iio/adc/stm32-adc-core.c > > > > b/drivers/iio/adc/stm32-adc-core.c index 48f02dcc81c1..70011fdbf5f6 > > > > 100644 > > > > --- a/drivers/iio/adc/stm32-adc-core.c > > > > +++ b/drivers/iio/adc/stm32-adc-core.c > > > > @@ -706,6 +706,8 @@ static int stm32_adc_probe(struct platform_device > > > > *pdev)> > > > > > > > > struct stm32_adc_priv *priv; > > > > struct device *dev = &pdev->dev; > > > > struct device_node *np = pdev->dev.of_node; > > > > > > > > + const struct of_device_id *of_id; > > > > + > > > > > > > > struct resource *res; > > > > u32 max_rate; > > > > int ret; > > > > > > > > @@ -718,8 +720,11 @@ static int stm32_adc_probe(struct platform_device > > > > *pdev)> > > > > > > > > return -ENOMEM; > > > > > > > > platform_set_drvdata(pdev, &priv->common); > > > > > > > > - priv->cfg = (const struct stm32_adc_priv_cfg *) > > > > - of_match_device(dev->driver->of_match_table, dev)->data; > > > > + of_id = of_match_device(dev->driver->of_match_table, dev); > > > > + if (!of_id) > > > > + return -ENODEV; > > > > + > > > > + priv->cfg = (const struct stm32_adc_priv_cfg *)of_id->data; > > > > > > > > priv->nb_adc_max = priv->cfg->num_adcs; > > > > spin_lock_init(&priv->common.lock); > > > > Hello Jonathan, > > > > I think we can make it happen by designing the platform device in a way > > that its name aligns with that of the driver. In such a scenario, when > > the driver is probed, the of_match_device function will return null. You > > can verify this functionality by reviewing the following function: > > > > static int platform_match(struct device *dev, struct device_driver *drv) > > I don't think we care about that case. If there is a real example of > why someone would do this then that would be a different matter. > > Jonathan > > > Best regards, > > Shurong I just think it might be more appropriate to return the correct error code in this situation. I agree with your assessment that it is an abnormal case. Therefore, it is perfectly fine if you decide not to select this patch. Thanks for your kind reply. Shurong
On Tue, 29 Aug 2023 00:35:36 +0800 Zhang Shurong <zhang_shurong@foxmail.com> wrote: > 在 2023年8月29日星期二 CST 上午12:16:05，Jonathan Cameron 写道： > > On Mon, 28 Aug 2023 23:02:07 +0800 > > > > Zhang Shurong <zhang_shurong@foxmail.com> wrote: > > > 在 2023年7月17日星期一 CST 上午12:08:21，Jonathan Cameron 写道： > > > > > > > On Sat, 15 Jul 2023 23:55:50 +0800 > > > > > > > > Zhang Shurong <zhang_shurong@foxmail.com> wrote: > > > > > of_match_device() may fail and returns a NULL pointer. > > > > > > > > > > Fix this by checking the return value of of_match_device(). > > > > > > > > > > Fixes: 64ad7f6438f3 ("iio: adc: stm32: introduce compatible data cfg") > > > > > Signed-off-by: Zhang Shurong <zhang_shurong@foxmail.com> > > > > > > > > Hi Zhang, > > > > > > > > I'm not sure we can actually make this bug happen. Do you have > > > > a way of triggering it? The driver is only probed on devices where > > > > that match will work. > > > > > > > > Also, assuming the match table is the same one associated with this > > > > probe > > > > function, then us priv->cfg = of_device_get_match_data() and check the > > > > output of that which is what we really care about. > > > > > > > > Jonathan > > > > > > > > > --- > > > > > > > > > > drivers/iio/adc/stm32-adc-core.c | 9 +++++++-- > > > > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/iio/adc/stm32-adc-core.c > > > > > b/drivers/iio/adc/stm32-adc-core.c index 48f02dcc81c1..70011fdbf5f6 > > > > > 100644 > > > > > --- a/drivers/iio/adc/stm32-adc-core.c > > > > > +++ b/drivers/iio/adc/stm32-adc-core.c > > > > > @@ -706,6 +706,8 @@ static int stm32_adc_probe(struct platform_device > > > > > *pdev)> > > > > > > > > > > struct stm32_adc_priv *priv; > > > > > struct device *dev = &pdev->dev; > > > > > struct device_node *np = pdev->dev.of_node; > > > > > > > > > > + const struct of_device_id *of_id; > > > > > + > > > > > > > > > > struct resource *res; > > > > > u32 max_rate; > > > > > int ret; > > > > > > > > > > @@ -718,8 +720,11 @@ static int stm32_adc_probe(struct platform_device > > > > > *pdev)> > > > > > > > > > > return -ENOMEM; > > > > > > > > > > platform_set_drvdata(pdev, &priv->common); > > > > > > > > > > - priv->cfg = (const struct stm32_adc_priv_cfg *) > > > > > - of_match_device(dev->driver->of_match_table, dev)->data; > > > > > + of_id = of_match_device(dev->driver->of_match_table, dev); > > > > > + if (!of_id) > > > > > + return -ENODEV; > > > > > + > > > > > + priv->cfg = (const struct stm32_adc_priv_cfg *)of_id->data; > > > > > > > > > > priv->nb_adc_max = priv->cfg->num_adcs; > > > > > spin_lock_init(&priv->common.lock); > > > > > > Hello Jonathan, > > > > > > I think we can make it happen by designing the platform device in a way > > > that its name aligns with that of the driver. In such a scenario, when > > > the driver is probed, the of_match_device function will return null. You > > > can verify this functionality by reviewing the following function: > > > > > > static int platform_match(struct device *dev, struct device_driver *drv) > > > > I don't think we care about that case. If there is a real example of > > why someone would do this then that would be a different matter. > > > > Jonathan > > > > > Best regards, > > > Shurong > I just think it might be more appropriate to return the correct error code in > this situation. I agree with your assessment that it is an abnormal case. > Therefore, it is perfectly fine if you decide not to select this patch. > > Thanks for your kind reply. > Applied but with a modified commit message to describe this as hardening rather than a fix. Also dropped the fixes tag as I don't really want this to be picked up for stable. Thanks, Jonathan > Shurong > > >
diff --git a/drivers/iio/adc/stm32-adc-core.c b/drivers/iio/adc/stm32-adc-core.c index 48f02dcc81c1..70011fdbf5f6 100644 --- a/drivers/iio/adc/stm32-adc-core.c +++ b/drivers/iio/adc/stm32-adc-core.c @@ -706,6 +706,8 @@ static int stm32_adc_probe(struct platform_device *pdev) struct stm32_adc_priv *priv; struct device *dev = &pdev->dev; struct device_node *np = pdev->dev.of_node; + const struct of_device_id *of_id; + struct resource *res; u32 max_rate; int ret; @@ -718,8 +720,11 @@ static int stm32_adc_probe(struct platform_device *pdev) return -ENOMEM; platform_set_drvdata(pdev, &priv->common); - priv->cfg = (const struct stm32_adc_priv_cfg *) - of_match_device(dev->driver->of_match_table, dev)->data; + of_id = of_match_device(dev->driver->of_match_table, dev); + if (!of_id) + return -ENODEV; + + priv->cfg = (const struct stm32_adc_priv_cfg *)of_id->data; priv->nb_adc_max = priv->cfg->num_adcs; spin_lock_init(&priv->common.lock);
of_match_device() may fail and returns a NULL pointer. Fix this by checking the return value of of_match_device(). Fixes: 64ad7f6438f3 ("iio: adc: stm32: introduce compatible data cfg") Signed-off-by: Zhang Shurong <zhang_shurong@foxmail.com> --- drivers/iio/adc/stm32-adc-core.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)