[PATCH] io: adc: stm32-adc: fix potential NULL pointer dereference in stm32_adc_probe()

[PATCH] io: adc: stm32-adc: fix potential NULL pointer dereference in stm32_adc_probe()

[Zhang Shurong]

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(-)

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);
 
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] io: adc: stm32-adc: fix potential NULL pointer dereference in stm32_adc_probe()

[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);
>  


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] io: adc: stm32-adc: fix potential NULL pointer dereference in stm32_adc_probe()

[Zhang Shurong]

在 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




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] io: adc: stm32-adc: fix potential NULL pointer dereference in stm32_adc_probe()

[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
> 
> 
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] io: adc: stm32-adc: fix potential NULL pointer dereference in stm32_adc_probe()

[Zhang 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




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] io: adc: stm32-adc: fix potential NULL pointer dereference in stm32_adc_probe()

[Jonathan Cameron]

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
> 
> 
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel