Skip to contentSkip to conversation

question about potential null pointer dereference

mfd In review Gustavo A. R. Silva 3 participants 10 messages May 23, 2017 – May 24, 2017

Subsystems (2)
Multifunction devices (MFD) Wolfson microelectronics drivers
lore
Share & tools
lore

Gustavo A. R. Silva

author
#0 lore raw

Hello everybody,

While looking into Coverity ID 1408830 I ran into the following piece of code at drivers/mfd/wm831x-spi.c:26

26static int wm831x_spi_probe(struct spi_device *spi) 27{

 28        struct wm831x_pdata *pdata = dev_get_platdata(&spi->dev);
 29        const struct spi_device_id *id = spi_get_device_id(spi);
 30        const struct of_device_id *of_id;
 31        struct wm831x *wm831x;
 32        enum wm831x_parent type;
 33        int ret;

34
35 if (spi->dev.of_node) {
36 of_id = of_match_device(wm831x_of_match, &spi->dev);
37 type = (enum wm831x_parent)of_id->data;
38 } else {
39 type = (enum wm831x_parent)id->driver_data;
40 }
41
42 wm831x = devm_kzalloc(&spi->dev, sizeof(struct wm831x),
GFP_KERNEL);
43 if (wm831x == NULL)
44 return -ENOMEM;
45
46 spi->mode = SPI_MODE_0;
47

 48        spi_set_drvdata(spi, wm831x);
 49        wm831x->dev = &spi->dev;
 50        wm831x->type = type;

51
52 wm831x->regmap = devm_regmap_init_spi(spi, &wm831x_regmap_config);
53 if (IS_ERR(wm831x->regmap)) {
54 ret = PTR_ERR(wm831x->regmap);
55 dev_err(wm831x->dev, "Failed to allocate register
map: %d\n",
56 ret);
57 return ret;
58 }
59
60 if (pdata)
61 memcpy(&wm831x->pdata, pdata, sizeof(*pdata));
62
63 return wm831x_device_init(wm831x, spi->irq);
64}

The issue here is that there is a potential NULL pointer dereference at line 37, in case function of_match_device() returns NULL.

Maybe a patch like the following could be applied in order to avoid any chance of a NULL pointer dereference:

index c332e28..7b227c9 100644

drivers/mfd/wm831x-spi.c +4 -0
@@ -34,6 +34,10 @@ static int wm831x_spi_probe(struct spi_device *spi)
 
        if (spi->dev.of_node) {
                of_id = of_match_device(wm831x_of_match, &spi->dev);
+               if (!of_id) {
+                       dev_err(&spi->dev, "Failed to find matching id\n");
+                       return -EINVAL;
+               }
                type = (enum wm831x_parent)of_id->data;
        } else {
                type = (enum wm831x_parent)id->driver_data;

What do you think?

I'd really appreciate any comment on this.

Thank you!
--
Gustavo A. R. Silva

#1 lore raw
Quoted 48 lines · Gustavo A. R. Silva

Hello everybody,

While looking into Coverity ID 1408830 I ran into the following piece of code at drivers/mfd/wm831x-spi.c:26

26static int wm831x_spi_probe(struct spi_device *spi) 27{

 28        struct wm831x_pdata *pdata = dev_get_platdata(&spi->dev);
 29        const struct spi_device_id *id = spi_get_device_id(spi);
 30        const struct of_device_id *of_id;
 31        struct wm831x *wm831x;
 32        enum wm831x_parent type;
 33        int ret;

34
35 if (spi->dev.of_node) {
36 of_id = of_match_device(wm831x_of_match, &spi->dev);
37 type = (enum wm831x_parent)of_id->data;
38 } else {
39 type = (enum wm831x_parent)id->driver_data;
40 }
41
42 wm831x = devm_kzalloc(&spi->dev, sizeof(struct wm831x),
GFP_KERNEL);
43 if (wm831x == NULL)
44 return -ENOMEM;
45
46 spi->mode = SPI_MODE_0;
47

 48        spi_set_drvdata(spi, wm831x);
 49        wm831x->dev = &spi->dev;
 50        wm831x->type = type;

51
52 wm831x->regmap = devm_regmap_init_spi(spi, &wm831x_regmap_config);
53 if (IS_ERR(wm831x->regmap)) {
54 ret = PTR_ERR(wm831x->regmap);
55 dev_err(wm831x->dev, "Failed to allocate register map:
%d\n",
56 ret);
57 return ret;
58 }
59
60 if (pdata)
61 memcpy(&wm831x->pdata, pdata, sizeof(*pdata));
62
63 return wm831x_device_init(wm831x, spi->irq);
64}

Quoted 5 lines

The issue here is that there is a potential NULL pointer dereference at line 37, in case function of_match_device() returns NULL.

Maybe a patch like the following could be applied in order to avoid any chance of a NULL pointer dereference:

I do not believe it's possible for of_match_device() to return NULL in this case.

However, if you wanted to submit a patch checking for it, it would not be rejected.

drivers/mfd/wm831x-spi.c static int wm831x_spi_probe(struct spi_device *spi) · line 38

Quoted 9 lines

index c332e28..7b227c9 100644

drivers/mfd/wm831x-spi.c +2 -0
@@ -34,6 +34,10 @@ static int wm831x_spi_probe(struct spi_device *spi)
 
        if (spi->dev.of_node) {
                of_id = of_match_device(wm831x_of_match, &spi->dev);
+               if (!of_id) {
+                       dev_err(&spi->dev, "Failed to find matching id\n");

"Failed to match device"

drivers/mfd/wm831x-spi.c static int wm831x_spi_probe(struct spi_device *spi) · line 39

Quoted 1 line
drivers/mfd/wm831x-spi.c +1 -0
+                       return -EINVAL;

-ENODEV

Quoted 9 lines
drivers/mfd/wm831x-spi.c +0 -0
                type = (enum wm831x_parent)of_id->data;
        } else {
                type = (enum wm831x_parent)id->driver_data;

What do you think?

I'd really appreciate any comment on this.

Thank you!

Signature · 4 lines
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
#2 lore raw
Quoted 4 lines · Lee Jones
Quoted 3 lines · Gustavo A. R. Silva

The issue here is that there is a potential NULL pointer dereference at line 37, in case function of_match_device() returns NULL.

Quoted 8 lines
Quoted 2 lines

Maybe a patch like the following could be applied in order to avoid any chance of a NULL pointer dereference:

I do not believe it's possible for of_match_device() to return NULL in this case.

However, if you wanted to submit a patch checking for it, it would not be rejected.

Personally I would prefer to have a NULL pointer check in there.

Thanks,
Charles

author
#3 lore raw

Hi guys,

Quoting Charles Keepax <[email protected]>:

Quoted 9 lines · Lee Jones
Quoted 8 lines · Gustavo A. R. Silva
Quoted 1 line

The issue here is that there is a potential NULL pointer

dereference at line

Quoted 4 lines

37, in case function of_match_device() returns NULL.

Maybe a patch like the following could be applied in order to avoid any chance of a NULL pointer dereference:

Quoted 8 lines
Quoted 5 lines

I do not believe it's possible for of_match_device() to return NULL in this case.

However, if you wanted to submit a patch checking for it, it would not be rejected.

Personally I would prefer to have a NULL pointer check in there.

I'll send a patch shortly.

Thank you both.
--
Gustavo A. R. Silva

author
#4 lore raw

PATCH 1/1 mfd: add null check before pointer dereference

Add null check before dereferencing pointer of_id in order to avoid a potential NULL pointer dereference.

Addresses-Coverity-ID: 1408830

Cc Charles Keepax <[email protected]>
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/mfd/wm831x-spi.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/mfd/wm831x-spi.c +4 -0
index c332e28..7b227c9 100644
@@ -34,6 +34,10 @@ static int wm831x_spi_probe(struct spi_device *spi)
 
 	if (spi->dev.of_node) {
 		of_id = of_match_device(wm831x_of_match, &spi->dev);
+		if (!of_id) {
+			dev_err(&spi->dev, "Failed to find matching id\n");
+			return -EINVAL;
+		}
 		type = (enum wm831x_parent)of_id->data;
 	} else {
 		type = (enum wm831x_parent)id->driver_data;
Signature · 1 line
2.5.0
Acked-by on v1 1/1
#5 lore raw

Re: [PATCH] mfd: add null check before pointer dereference

Quoted 7 lines · Gustavo A. R. Silva

Add null check before dereferencing pointer of_id in order to avoid a potential NULL pointer dereference.

Addresses-Coverity-ID: 1408830

Cc Charles Keepax <[email protected]>
Signed-off-by Gustavo A. R. Silva <[email protected]>

---

Acked-by Charles Keepax <[email protected]>

Looks good although is there not probably the same issue in wm831x-i2c.c? Might be worth fixing up both of them.

Thanks,
Charles

#6 lore raw

Re: [PATCH] mfd: add null check before pointer dereference

In reply to Gustavo A. R. Silva

Please use the $SUBJECT line expected by the subsystem.

The following command can help with this:

  `git log --oneline -- <SUBSYSTEM>`
Quoted 15 lines

Add null check before dereferencing pointer of_id in order to avoid a potential NULL pointer dereference.

Addresses-Coverity-ID: 1408830

Cc Charles Keepax <[email protected]>
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/mfd/wm831x-spi.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/mfd/wm831x-spi.c +0 -0
index c332e28..7b227c9 100644

drivers/mfd/wm831x-spi.c static int wm831x_spi_probe(struct spi_device *spi) · line 40

Quoted 7 lines
drivers/mfd/wm831x-spi.c +4 -0
@@ -34,6 +34,10 @@ static int wm831x_spi_probe(struct spi_device *spi)
 
 	if (spi->dev.of_node) {
 		of_id = of_match_device(wm831x_of_match, &spi->dev);
+		if (!of_id) {
+			dev_err(&spi->dev, "Failed to find matching id\n");
+			return -EINVAL;
+		}

I already mentioned why this isn't suitable.

Please see my pre-review.

Quoted 3 lines
drivers/mfd/wm831x-spi.c +0 -0
 		type = (enum wm831x_parent)of_id->data;
 	} else {
 		type = (enum wm831x_parent)id->driver_data;
Signature · 4 lines
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
author
#7 lore raw

Re: [PATCH] mfd: add null check before pointer dereference

Hi Lee,

Quoting Lee Jones <[email protected]>:

Quoted 6 lines

Please use the $SUBJECT line expected by the subsystem.

The following command can help with this:

  `git log --oneline -- <SUBSYSTEM>`

I get it.

Quoted 25 lines
Quoted 15 lines

Add null check before dereferencing pointer of_id in order to avoid a potential NULL pointer dereference.

Addresses-Coverity-ID: 1408830

Cc Charles Keepax <[email protected]>
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/mfd/wm831x-spi.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/mfd/wm831x-spi.c +0 -0
index c332e28..7b227c9 100644

drivers/mfd/wm831x-spi.c static int wm831x_spi_probe(struct spi_device *spi) · line 40

Quoted 7 lines
drivers/mfd/wm831x-spi.c +4 -0
@@ -34,6 +34,10 @@ static int wm831x_spi_probe(struct spi_device *spi)
 
 	if (spi->dev.of_node) {
 		of_id = of_match_device(wm831x_of_match, &spi->dev);
+		if (!of_id) {
+			dev_err(&spi->dev, "Failed to find matching id\n");
+			return -EINVAL;
+		}

I already mentioned why this isn't suitable.

Quoted 2 lines

Please see my pre-review.

You are right. Let me fix that.

Thanks
--
Gustavo A. R. Silva

author
#8 lore raw

PATCH v2 1/1 mfd: wm831x: add null check before pointer dereference

Add NULL check before dereferencing pointer of_id in order to avoid a potential NULL pointer dereference.

Addresses-Coverity-ID: 1408830

Cc Lee Jones <[email protected]>
Cc Charles Keepax <[email protected]>
Signed-off-by Gustavo A. R. Silva <[email protected]>

---
Changes in v2:
Update error log and return value.

 drivers/mfd/wm831x-spi.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/mfd/wm831x-spi.c +4 -0
index c332e28..018ce65 100644
@@ -34,6 +34,10 @@ static int wm831x_spi_probe(struct spi_device *spi)
 
 	if (spi->dev.of_node) {
 		of_id = of_match_device(wm831x_of_match, &spi->dev);
+		if (!of_id) {
+			dev_err(&spi->dev, "Failed to match device\n");
+			return -ENODEV;
+		}
 		type = (enum wm831x_parent)of_id->data;
 	} else {
 		type = (enum wm831x_parent)id->driver_data;
Signature · 1 line
2.5.0
Applied
#9 lore raw

Re: [PATCH v2] mfd: wm831x: add null check before pointer dereference

Quoted 7 lines · Gustavo A. R. Silva

Add NULL check before dereferencing pointer of_id in order to avoid a potential NULL pointer dereference.

Addresses-Coverity-ID: 1408830

Cc Lee Jones <[email protected]>
Cc Charles Keepax <[email protected]>
Signed-off-by Gustavo A. R. Silva <[email protected]>
Quoted 6 lines

---
Changes in v2:
Update error log and return value.

 drivers/mfd/wm831x-spi.c | 4 ++++
 1 file changed, 4 insertions(+)

Applied, thanks.

Quoted 15 lines
drivers/mfd/wm831x-spi.c +4 -0
index c332e28..018ce65 100644
@@ -34,6 +34,10 @@ static int wm831x_spi_probe(struct spi_device *spi)
 
 	if (spi->dev.of_node) {
 		of_id = of_match_device(wm831x_of_match, &spi->dev);
+		if (!of_id) {
+			dev_err(&spi->dev, "Failed to match device\n");
+			return -ENODEV;
+		}
 		type = (enum wm831x_parent)of_id->data;
 	} else {
 		type = (enum wm831x_parent)id->driver_data;
Signature · 4 lines
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog