Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1267675 > unrolled thread

[PATCH] i2c: rcar: fix a possible NULL dereference

Back to article view | Back to linux.kernel

Contents

  [PATCH] i2c: rcar: fix a possible NULL dereference LABBE Corentin <clabbe.montjoie@gmail.com> - 2015-11-12 08:30 +0100
    Re: [PATCH] i2c: rcar: fix a possible NULL dereference Uwe Kleine-König   <u.kleine-koenig@pengutronix.de> - 2015-11-12 08:50 +0100
      Re: [PATCH] i2c: rcar: fix a possible NULL dereference Wolfram Sang <wsa@the-dreams.de> - 2015-11-12 09:00 +0100
        Re: [PATCH] i2c: rcar: fix a possible NULL dereference Uwe Kleine-König   <u.kleine-koenig@pengutronix.de> - 2015-11-12 09:10 +0100
          Re: [PATCH] i2c: rcar: fix a possible NULL dereference Wolfram Sang <wsa@the-dreams.de> - 2015-11-12 09:50 +0100
            Re: [PATCH] i2c: rcar: fix a possible NULL dereference Uwe Kleine-König   <u.kleine-koenig@pengutronix.de> - 2015-11-12 10:10 +0100
              Re: [PATCH] i2c: rcar: fix a possible NULL dereference Wolfram Sang <wsa@the-dreams.de> - 2015-11-12 10:20 +0100

#1267675 — [PATCH] i2c: rcar: fix a possible NULL dereference

of_match_device could return NULL, and so cause a NULL pointer
dereference later.

Reported-by: coverity (CID 1130036)
Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com>
---
 drivers/i2c/busses/i2c-rcar.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index b0ae560..d2bdbda 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -639,6 +639,7 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	u32 bus_speed;
 	int irq, ret;
+	const struct of_device_id *of_id;
 
 	priv = devm_kzalloc(dev, sizeof(struct rcar_i2c_priv), GFP_KERNEL);
 	if (!priv)
@@ -653,7 +654,10 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 	bus_speed = 100000; /* default 100 kHz */
 	of_property_read_u32(dev->of_node, "clock-frequency", &bus_speed);
 
-	priv->devtype = (enum rcar_i2c_type)of_match_device(rcar_i2c_dt_ids, dev)->data;
+	of_id = of_match_device(rcar_i2c_dt_ids, dev);
+	if (!of_id)
+		return -ENODEV;
+	priv->devtype = (enum rcar_i2c_type)of_id->data;
 
 	ret = rcar_i2c_clock_calculate(priv, bus_speed, dev);
 	if (ret < 0)
-- 
2.4.10

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1267687

Hello,

On Thu, Nov 12, 2015 at 08:25:09AM +0100, LABBE Corentin wrote:
> of_match_device could return NULL, and so cause a NULL pointer
> dereference later.
> 
> Reported-by: coverity (CID 1130036)
> Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com>
> ---
>  drivers/i2c/busses/i2c-rcar.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
> index b0ae560..d2bdbda 100644
> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -639,6 +639,7 @@ static int rcar_i2c_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	u32 bus_speed;
>  	int irq, ret;
> +	const struct of_device_id *of_id;
>  
>  	priv = devm_kzalloc(dev, sizeof(struct rcar_i2c_priv), GFP_KERNEL);
>  	if (!priv)
> @@ -653,7 +654,10 @@ static int rcar_i2c_probe(struct platform_device *pdev)
>  	bus_speed = 100000; /* default 100 kHz */
>  	of_property_read_u32(dev->of_node, "clock-frequency", &bus_speed);
>  
> -	priv->devtype = (enum rcar_i2c_type)of_match_device(rcar_i2c_dt_ids, dev)->data;
> +	of_id = of_match_device(rcar_i2c_dt_ids, dev);
> +	if (!of_id)
> +		return -ENODEV;
> +	priv->devtype = (enum rcar_i2c_type)of_id->data;

This is nearly an open coding of of_device_get_match_data. Maybe using

	priv->devtype = (enum rcar_i2c_type)of_device_get_match_data(dev)

if good enough? 

Other than that, the NULL pointer dereference should only happen if the
device was bound using the driver name. That might be worth to point out
in the commit log. So maybe make (in a separate patch) the probe
function fail when probed by name?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1267696

[Multipart message — attachments visible in raw view] — view raw

On Thu, Nov 12, 2015 at 08:44:47AM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Thu, Nov 12, 2015 at 08:25:09AM +0100, LABBE Corentin wrote:
> > of_match_device could return NULL, and so cause a NULL pointer
> > dereference later.
> > 
> > Reported-by: coverity (CID 1130036)
> > Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com>
> > ---
> >  drivers/i2c/busses/i2c-rcar.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
> > index b0ae560..d2bdbda 100644
> > --- a/drivers/i2c/busses/i2c-rcar.c
> > +++ b/drivers/i2c/busses/i2c-rcar.c
> > @@ -639,6 +639,7 @@ static int rcar_i2c_probe(struct platform_device *pdev)
> >  	struct device *dev = &pdev->dev;
> >  	u32 bus_speed;
> >  	int irq, ret;
> > +	const struct of_device_id *of_id;
> >  
> >  	priv = devm_kzalloc(dev, sizeof(struct rcar_i2c_priv), GFP_KERNEL);
> >  	if (!priv)
> > @@ -653,7 +654,10 @@ static int rcar_i2c_probe(struct platform_device *pdev)
> >  	bus_speed = 100000; /* default 100 kHz */
> >  	of_property_read_u32(dev->of_node, "clock-frequency", &bus_speed);
> >  
> > -	priv->devtype = (enum rcar_i2c_type)of_match_device(rcar_i2c_dt_ids, dev)->data;
> > +	of_id = of_match_device(rcar_i2c_dt_ids, dev);
> > +	if (!of_id)
> > +		return -ENODEV;
> > +	priv->devtype = (enum rcar_i2c_type)of_id->data;
> 
> This is nearly an open coding of of_device_get_match_data. Maybe using
> 
> 	priv->devtype = (enum rcar_i2c_type)of_device_get_match_data(dev)
> 
> if good enough? 
> 
> Other than that, the NULL pointer dereference should only happen if the
> device was bound using the driver name. That might be worth to point out
> in the commit log. So maybe make (in a separate patch) the probe
> function fail when probed by name?

RCar is a DT only platform.

[toc] | [prev] | [next] | [standalone]

#1267702

Hello Wolfram,

On Thu, Nov 12, 2015 at 08:52:38AM +0100, Wolfram Sang wrote:
> On Thu, Nov 12, 2015 at 08:44:47AM +0100, Uwe Kleine-König wrote:
> > Other than that, the NULL pointer dereference should only happen if the
> > device was bound using the driver name. That might be worth to point out
> > in the commit log. So maybe make (in a separate patch) the probe
> > function fail when probed by name?
> 
> RCar is a DT only platform.

Does this imply that no checking is needed? I'm not sure.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1267720

[Multipart message — attachments visible in raw view] — view raw

On Thu, Nov 12, 2015 at 09:09:26AM +0100, Uwe Kleine-König wrote:
> Hello Wolfram,
> 
> On Thu, Nov 12, 2015 at 08:52:38AM +0100, Wolfram Sang wrote:
> > On Thu, Nov 12, 2015 at 08:44:47AM +0100, Uwe Kleine-König wrote:
> > > Other than that, the NULL pointer dereference should only happen if the
> > > device was bound using the driver name. That might be worth to point out
> > > in the commit log. So maybe make (in a separate patch) the probe
> > > function fail when probed by name?
> > 
> > RCar is a DT only platform.
> 
> Does this imply that no checking is needed? I'm not sure.

I don't see how this driver could get probed otherwise. That being said,
for the "better safe than sorry" approach, I'd accept your suggestion.

[toc] | [prev] | [next] | [standalone]

#1267736

On Thu, Nov 12, 2015 at 09:48:37AM +0100, Wolfram Sang wrote:
> On Thu, Nov 12, 2015 at 09:09:26AM +0100, Uwe Kleine-König wrote:
> > Hello Wolfram,
> > 
> > On Thu, Nov 12, 2015 at 08:52:38AM +0100, Wolfram Sang wrote:
> > > On Thu, Nov 12, 2015 at 08:44:47AM +0100, Uwe Kleine-König wrote:
> > > > Other than that, the NULL pointer dereference should only happen if the
> > > > device was bound using the driver name. That might be worth to point out
> > > > in the commit log. So maybe make (in a separate patch) the probe
> > > > function fail when probed by name?
> > > 
> > > RCar is a DT only platform.
> > 
> > Does this imply that no checking is needed? I'm not sure.
> 
> I don't see how this driver could get probed otherwise. That being said,
> for the "better safe than sorry" approach, I'd accept your suggestion.

Try adding the following to a device tree:

	i2c-rcar {

	}

This creates a platform device with name=i2c-rcar which makes
platform_match() yield a match for your driver.

(The other alternative is a device with this name created by a platform
file (even if this is not an rcar machine) doesn't trigger that problem
here I think, because there is a platform_device_id with a matching
name).

See also 

	http://article.gmane.org/gmane.linux.kernel/2083641

for a similar discussion.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1267743

[Multipart message — attachments visible in raw view] — view raw

> Try adding the following to a device tree:
> 
> 	i2c-rcar {
> 
> 	}

"Better safe than sorry" won again ;)

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web