Patch to solve NULL pointer dereference in physmap_of.c

[Prins Anton (ST-CO/ENG1.1)]

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 at gmail.com] 
Sent: woensdag 21 november 2012 8:42
To: Prins Anton (ST-CO/ENG1.1)
Cc: linux-mtd at 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 at 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)

-- 
Best Regards,
Artem Bityutskiy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 6798 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.infradead.org/pipermail/linux-mtd/attachments/20121121/1c865966/attachment.p7s>