Patchwork Patch to solve NULL pointer dereference in physmap_of.c

login
register
mail settings
Submitter Prins Anton (ST-CO/ENG1.1)
Date Nov. 9, 2012, 7:45 a.m.
Message ID <85D877DD6EE67B4A9FCA9B9C3A4865670C3ADE0635@SI-MBX14.de.bosch.com>
Download mbox | patch
Permalink /patch/197989/
State New
Headers show

Comments

Prins Anton (ST-CO/ENG1.1) - Nov. 9, 2012, 7:45 a.m.
commit 0905a6f4aec377123e94d2260f2f7a0d867e19be
Author: Anton Prins <anton.prins@nl.bosch.com>
Date:   Fri Nov 9 10:12:58 2012 +0100

    Correct error checking to prevent a NULL pointer dereference

    The problem only occurs if the DTS is not correct, the requested mapping is not reserved on the parent bus.
    In this special case the count is 1, but the list_size after mapping is 0. list_size 0 should generate an error!
Artem Bityutskiy - Nov. 21, 2012, 7:42 a.m.
On Fri, 2012-11-09 at 08:45 +0100, Prins Anton (ST-CO/ENG1.1) wrote:
> commit 0905a6f4aec377123e94d2260f2f7a0d867e19be
> Author: Anton Prins <anton.prins@nl.bosch.com>
> Date:   Fri Nov 9 10:12:58 2012 +0100
> 
>     Correct error checking to prevent a NULL pointer dereference
> 
>     The problem only occurs if the DTS is not correct, the requested mapping is not reserved on the parent bus.
>     In this special case the count is 1, but the list_size after mapping is 0. list_size 0 should generate an error!

Sorry, I do not really understand which problem this patch solves, could
you please improve the commit message and re-send?

> 
> diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c
> index 2e6fb68..83d121e 100644
> --- a/drivers/mtd/maps/physmap_of.c
> +++ b/drivers/mtd/maps/physmap_of.c
> @@ -267,13 +267,14 @@ static int __devinit of_flash_probe(struct platform_device *dev)
>                 info->list[i].mtd->dev.parent = &dev->dev;
>         }
> 

It seems the error condition should be checked and acted upon here. What
you looks more like making the code less readable.

> -       err = 0;
>         if (info->list_size == 1) {
> +               err = 0;
>                 info->cmtd = info->list[0].mtd;
>         } else if (info->list_size > 1) {
>                 /*
>                  * We detected multiple devices. Concatenate them together.
>                  */
> +               err = 0;
>                 info->cmtd = mtd_concat_create(mtd_list, info->list_size,
>                                                dev_name(&dev->dev));
>                 if (info->cmtd == NULL)
Prins Anton (ST-CO/ENG1.1) - Nov. 21, 2012, 8:43 a.m.
Hi Artem,

This patch solves a NULL pointer dereference, this may occur if the tuple is not mappable (jumps to continue in the for-loop).
Out of the loop possible results are: 
- info->list_size == 0  if no of the tuples is mappable
- info->list_size == 1
- info->list_size > 1

In the case no tuple is mappable (info->list_size == 0) info->cmtd is not set but used in mtd_device_parse_register...OOPS.

Indeed you also check for 'info->list_size == 0' for me it doesn't differ in readability.
My choice was to reset the error-code only If I'm sure the list_size is >= 1.

Another option, maybe the best... is to check for info->cmtd == NULL @ line 296.

Met vriendelijke groeten | Best Regards, 
Anton Prins

-----Original Message-----
From: Artem Bityutskiy [mailto:dedekind1@gmail.com] 
Sent: woensdag 21 november 2012 8:42
To: Prins Anton (ST-CO/ENG1.1)
Cc: linux-mtd@lists.infradead.org
Subject: Re: Patch to solve NULL pointer dereference in physmap_of.c
On Fri, 2012-11-09 at 08:45 +0100, Prins Anton (ST-CO/ENG1.1) wrote:
> commit 0905a6f4aec377123e94d2260f2f7a0d867e19be
> Author: Anton Prins <anton.prins@nl.bosch.com>
> Date:   Fri Nov 9 10:12:58 2012 +0100
> 
>     Correct error checking to prevent a NULL pointer dereference
> 
>     The problem only occurs if the DTS is not correct, the requested mapping is not reserved on the parent bus.
>     In this special case the count is 1, but the list_size after mapping is 0. list_size 0 should generate an error!

Sorry, I do not really understand which problem this patch solves, could
you please improve the commit message and re-send?

> 
> diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c
> index 2e6fb68..83d121e 100644
> --- a/drivers/mtd/maps/physmap_of.c
> +++ b/drivers/mtd/maps/physmap_of.c
> @@ -267,13 +267,14 @@ static int __devinit of_flash_probe(struct platform_device *dev)
>                 info->list[i].mtd->dev.parent = &dev->dev;
>         }
> 

It seems the error condition should be checked and acted upon here. What
you looks more like making the code less readable.

> -       err = 0;
>         if (info->list_size == 1) {
> +               err = 0;
>                 info->cmtd = info->list[0].mtd;
>         } else if (info->list_size > 1) {
>                 /*
>                  * We detected multiple devices. Concatenate them together.
>                  */
> +               err = 0;
>                 info->cmtd = mtd_concat_create(mtd_list, info->list_size,
>                                                dev_name(&dev->dev));
>                 if (info->cmtd == NULL)

Patch

diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c
index 2e6fb68..83d121e 100644
--- a/drivers/mtd/maps/physmap_of.c
+++ b/drivers/mtd/maps/physmap_of.c
@@ -267,13 +267,14 @@  static int __devinit of_flash_probe(struct platform_device *dev)
                info->list[i].mtd->dev.parent = &dev->dev;
        }

-       err = 0;
        if (info->list_size == 1) {
+               err = 0;
                info->cmtd = info->list[0].mtd;
        } else if (info->list_size > 1) {
                /*
                 * We detected multiple devices. Concatenate them together.
                 */
+               err = 0;
                info->cmtd = mtd_concat_create(mtd_list, info->list_size,
                                               dev_name(&dev->dev));
                if (info->cmtd == NULL)