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

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

[Shailendra Verma]

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

Signed-off-by: Shailendra Verma <shailendra.v-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 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

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 503 bytes --]

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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Uwe Kleine-König]

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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> > ---
> >  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/  |

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1654 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Uwe Kleine-König]

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

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1656 bytes --]

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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> > > > > ---
> > > > >  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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 343 bytes --]

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>

I don't mind either way, but since Thierry is the maintainer of this
driver, I respect his preference.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]