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

Groups > linux.kernel > #1569390 > unrolled thread

[PATCH] I2c: busses - Fix possible NULL derefrence.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH] I2c: busses - Fix possible NULL derefrence. Shailendra Verma <shailendra.v@samsung.com> - 2017-01-30 06:10 +0100
    Re: [PATCH] I2c: busses - Fix possible NULL derefrence. Thierry Reding <thierry.reding@gmail.com> - 2017-01-30 08:20 +0100
      Re: [PATCH] I2c: busses - Fix possible NULL derefrence. Uwe Kleine-König          <u.kleine-koenig@pengutronix.de> - 2017-01-30 09:30 +0100
        Re: [PATCH] I2c: busses - Fix possible NULL derefrence. Thierry Reding <thierry.reding@gmail.com> - 2017-01-30 10:00 +0100
          Re: [PATCH] I2c: busses - Fix possible NULL derefrence. Uwe Kleine-König          <u.kleine-koenig@pengutronix.de> - 2017-01-30 12:20 +0100
            Re: [PATCH] I2c: busses - Fix possible NULL derefrence. Thierry Reding <thierry.reding@gmail.com> - 2017-01-30 12:30 +0100

#1569390 — [PATCH] I2c: busses - Fix possible NULL derefrence.

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

Signed-off-by: Shailendra Verma <shailendra.v@samsung.com>
---
 drivers/i2c/busses/i2c-tegra.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 4af9bba..93ac1e1 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -920,6 +920,10 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	tegra_i2c_parse_dt(i2c_dev);
 
 	i2c_dev->hw = of_device_get_match_data(&pdev->dev);
+	if (!i2c_dev->hw) {
+		dev_err(&pdev->dev, "no device match found\n");
+		return -ENODEV;
+	}
 	i2c_dev->is_dvc = of_device_is_compatible(pdev->dev.of_node,
 						  "nvidia,tegra20-i2c-dvc");
 	init_completion(&i2c_dev->msg_complete);
-- 
1.7.9.5

[toc] | [next] | [standalone]

#1569431

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

On Mon, Jan 30, 2017 at 10:33:07AM +0530, Shailendra Verma wrote:
> of_device_get_match_data could return NULL, and so can cause
> a NULL pointer dereference later.
> 
> Signed-off-by: Shailendra Verma <shailendra.v@samsung.com>
> ---
>  drivers/i2c/busses/i2c-tegra.c |    4 ++++
>  1 file changed, 4 insertions(+)

This will never happen. Any match in the OF table that would cause the
->probe() to occur has a valid .data pointer associated with it.

Thierry

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

#1569460

Hello,

On Mon, Jan 30, 2017 at 08:12:17AM +0100, Thierry Reding wrote:
> On Mon, Jan 30, 2017 at 10:33:07AM +0530, Shailendra Verma wrote:
> > of_device_get_match_data could return NULL, and so can cause
> > a NULL pointer dereference later.
> > 
> > Signed-off-by: Shailendra Verma <shailendra.v@samsung.com>
> > ---
> >  drivers/i2c/busses/i2c-tegra.c |    4 ++++
> >  1 file changed, 4 insertions(+)
> 
> This will never happen. Any match in the OF table that would cause the
> ->probe() to occur has a valid .data pointer associated with it.

Theoretically you could (I think) bind that driver to a node with

	compatible = "tegra-i2c";

Anyhow, even if today there was no possibility this could happen, that's
something that might easily be changed by a future change. So I doubt
"this will never happen" stays true for sure and being defensive is a
good idea. And even a BUG would be better than a silent NULL pointer
dereference.

Just my € 0.02
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

#1569470

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

On Mon, Jan 30, 2017 at 09:07:15AM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Mon, Jan 30, 2017 at 08:12:17AM +0100, Thierry Reding wrote:
> > On Mon, Jan 30, 2017 at 10:33:07AM +0530, Shailendra Verma wrote:
> > > of_device_get_match_data could return NULL, and so can cause
> > > a NULL pointer dereference later.
> > > 
> > > Signed-off-by: Shailendra Verma <shailendra.v@samsung.com>
> > > ---
> > >  drivers/i2c/busses/i2c-tegra.c |    4 ++++
> > >  1 file changed, 4 insertions(+)
> > 
> > This will never happen. Any match in the OF table that would cause the
> > ->probe() to occur has a valid .data pointer associated with it.
> 
> Theoretically you could (I think) bind that driver to a node with
> 
> 	compatible = "tegra-i2c";

That's not a valid compatible string and I don't think this could end up
anywhere that would make the driver bind. Even if it did I think it'd be
good to crash rather than error out to make it very obvious that you've
made a mistake that needs to be immediately fixed.

If you error out it's much more likely that people won't notice.

> Anyhow, even if today there was no possibility this could happen, that's
> something that might easily be changed by a future change. So I doubt
> "this will never happen" stays true for sure and being defensive is a
> good idea.

Let's revisit this again *if* this ever becomes a real issue. There's no
use in adding dead code to the kernel to handle hypothetical use-cases.

> And even a BUG would be better than a silent NULL pointer dereference.

I've never encountered a NULL pointer dereference that was silent. =)

Thierry

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

#1569613

On Mon, Jan 30, 2017 at 09:54:55AM +0100, Thierry Reding wrote:
> On Mon, Jan 30, 2017 at 09:07:15AM +0100, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Mon, Jan 30, 2017 at 08:12:17AM +0100, Thierry Reding wrote:
> > > On Mon, Jan 30, 2017 at 10:33:07AM +0530, Shailendra Verma wrote:
> > > > of_device_get_match_data could return NULL, and so can cause
> > > > a NULL pointer dereference later.
> > > > 
> > > > Signed-off-by: Shailendra Verma <shailendra.v@samsung.com>
> > > > ---
> > > >  drivers/i2c/busses/i2c-tegra.c |    4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > 
> > > This will never happen. Any match in the OF table that would cause the
> > > ->probe() to occur has a valid .data pointer associated with it.
> > 
> > Theoretically you could (I think) bind that driver to a node with
> > 
> > 	compatible = "tegra-i2c";
> 
> That's not a valid compatible string and I don't think this could end up
> anywhere that would make the driver bind. Even if it did I think it'd be

Look at platform_match() in drivers/base/platform.c. If
of_driver_match_device fails it might still match based on
strcmp(pdev->name, drv->name).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

#1569622

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

On Mon, Jan 30, 2017 at 12:15:53PM +0100, Uwe Kleine-König wrote:
> On Mon, Jan 30, 2017 at 09:54:55AM +0100, Thierry Reding wrote:
> > On Mon, Jan 30, 2017 at 09:07:15AM +0100, Uwe Kleine-König wrote:
> > > Hello,
> > > 
> > > On Mon, Jan 30, 2017 at 08:12:17AM +0100, Thierry Reding wrote:
> > > > On Mon, Jan 30, 2017 at 10:33:07AM +0530, Shailendra Verma wrote:
> > > > > of_device_get_match_data could return NULL, and so can cause
> > > > > a NULL pointer dereference later.
> > > > > 
> > > > > Signed-off-by: Shailendra Verma <shailendra.v@samsung.com>
> > > > > ---
> > > > >  drivers/i2c/busses/i2c-tegra.c |    4 ++++
> > > > >  1 file changed, 4 insertions(+)
> > > > 
> > > > This will never happen. Any match in the OF table that would cause the
> > > > ->probe() to occur has a valid .data pointer associated with it.
> > > 
> > > Theoretically you could (I think) bind that driver to a node with
> > > 
> > > 	compatible = "tegra-i2c";
> > 
> > That's not a valid compatible string and I don't think this could end up
> > anywhere that would make the driver bind. Even if it did I think it'd be
> 
> Look at platform_match() in drivers/base/platform.c. If
> of_driver_match_device fails it might still match based on
> strcmp(pdev->name, drv->name).

pdev->name is never influenced by the compatible string. The only way
you could create a device that would match this driver is if you were to
manually create it using of_platform_device_create() or similar,
something which we can easily prevent (or revert should anyone ever get
such code into the kernel again).

Thierry

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web