Patchwork Patch to solve NULL pointer dereference in physmap_of.c

login
register
mail settings
Submitter Prins Anton (ST-CO/ENG1.1)
Date Nov. 22, 2012, 3:20 p.m.
Message ID <85D877DD6EE67B4A9FCA9B9C3A4865670C3AF1D4E4@SI-MBX14.de.bosch.com>
Download mbox | patch
Permalink /patch/201066/
State New
Headers show

Comments

Prins Anton (ST-CO/ENG1.1) - Nov. 22, 2012, 3:20 p.m.
[PATCH] mtd: maps/physmap_of.c: change error checking to prevent a NULL pointer dereference if no DTS tuple is mappable

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
If no one of the supplied tuples is mappable (info->list_size == 0) and info->cmtd will not be set.
But it is used in mtd_device_parse_register, OOPS... if should generate an error in this case!

[From: Anton Prins <anton.prins@nl.bosch.com>]

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)
Artem Bityutskiy - Nov. 30, 2012, 11:51 a.m.
Hi, sorry for long delay, but would you please re-send this in a nice
form:

1. Wrap commit message lines to 80 characters.
2. Make the patch applicable with 'git am' - this one does not apply.

Thanks!

On Thu, 2012-11-22 at 16:20 +0100, Prins Anton (ST-CO/ENG1.1) wrote:
> [PATCH] mtd: maps/physmap_of.c: change error checking to prevent a NULL pointer dereference if no DTS tuple is mappable
> 
> 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
> If no one of the supplied tuples is mappable (info->list_size == 0) and info->cmtd will not be set.
> But it is used in mtd_device_parse_register, OOPS... if should generate an error in this case!
> 
> [From: Anton Prins <anton.prins@nl.bosch.com>]
> 
> diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c
> index 2e6fb68..f6de444 100644
> --- a/drivers/mtd/maps/physmap_of.c
> +++ b/drivers/mtd/maps/physmap_of.c
> @@ -268,6 +268,7 @@ static int __devinit of_flash_probe(struct platform_device *dev)
>         }
> 
>         err = 0;
> +       info->cmtd = NULL;
>         if (info->list_size == 1) {
>                 info->cmtd = info->list[0].mtd;
>         } else if (info->list_size > 1) {
> @@ -276,9 +277,10 @@ static int __devinit of_flash_probe(struct platform_device *dev)
>                  */
>                 info->cmtd = mtd_concat_create(mtd_list, info->list_size,
>                                                dev_name(&dev->dev));
-               if (info->cmtd == NULL)
> -                       err = -ENXIO;
>         }
> +       if (info->cmtd == NULL)
> +               err = -ENXIO;
> +
>         if (err)
>                 goto err_out;
> 
> -----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)
>
Prins Anton (ST-CO/ENG1.1) - Dec. 3, 2012, 7:31 a.m.
Sorry I'm in a learning curve... now used 'git format-patch'.
This made it working for 'git am'; so hopefully this time its fine!

Artem, thanks for your support!

Met vriendelijke groeten | Best Regards, 
Anton Prins

-----Original Message-----
From: Artem Bityutskiy [mailto:dedekind1@gmail.com] 
Sent: vrijdag 30 november 2012 12:51
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
Hi, sorry for long delay, but would you please re-send this in a nice
form:

1. Wrap commit message lines to 80 characters.
2. Make the patch applicable with 'git am' - this one does not apply.
Thanks!

On Thu, 2012-11-22 at 16:20 +0100, Prins Anton (ST-CO/ENG1.1) wrote:
> [PATCH] mtd: maps/physmap_of.c: change error checking to prevent a NULL pointer dereference if no DTS tuple is mappable
> 
> 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
> If no one of the supplied tuples is mappable (info->list_size == 0) and info->cmtd will not be set.
> But it is used in mtd_device_parse_register, OOPS... if should generate an error in this case!
> 
> [From: Anton Prins <anton.prins@nl.bosch.com>]
> 
> diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c
> index 2e6fb68..f6de444 100644
> --- a/drivers/mtd/maps/physmap_of.c
> +++ b/drivers/mtd/maps/physmap_of.c
> @@ -268,6 +268,7 @@ static int __devinit of_flash_probe(struct platform_device *dev)
>         }
> 
>         err = 0;
> +       info->cmtd = NULL;
>         if (info->list_size == 1) {
>                 info->cmtd = info->list[0].mtd;
>         } else if (info->list_size > 1) {
> @@ -276,9 +277,10 @@ static int __devinit of_flash_probe(struct platform_device *dev)
>                  */
>                 info->cmtd = mtd_concat_create(mtd_list, info->list_size,
>                                                dev_name(&dev->dev));
-               if (info->cmtd == NULL)
> -                       err = -ENXIO;
>         }
> +       if (info->cmtd == NULL)
> +               err = -ENXIO;
> +
>         if (err)
>                 goto err_out;
> 
> -----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)
>
Artem Bityutskiy - Dec. 10, 2012, 1:31 p.m.
On Mon, 2012-12-03 at 08:31 +0100, Prins Anton (ST-CO/ENG1.1) wrote:
> Sorry I'm in a learning curve... now used 'git format-patch'.
> This made it working for 'git am'; so hopefully this time its fine!
> 
> Artem, thanks for your support!

A lot better, but is still not ideal. You did not run checkpatch.pl,
otherwise you would notice this warning:

checkpatch.pl results for patch "[PATCH] mtd: maps/physmap_of.c: error checking to prevent a NULL  pointer dereference"                                                                                                             
                                                                                                                                                                                                                                    
ERROR:MISSING_SIGN_OFF: Missing Signed-off-by: line(s)                                                                                                                                                                              
                                                                                                                                                                                                                                    
total: 1 errors, 0 warnings, 19 lines checked                                                                                                                                                                                       

I've added the S-o-b line for you and pushed the patch to l2-mtd.git,
thanks!
Prins Anton (ST-CO/ENG1.1) - Dec. 11, 2012, 8:34 a.m.
Great, I was not aware I was allowed to sign-off. Thx

Met vriendelijke groeten | Best Regards, 
Anton Prins

-----Original Message-----
From: Artem Bityutskiy [mailto:dedekind1@gmail.com] 
Sent: maandag 10 december 2012 14:31
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 Mon, 2012-12-03 at 08:31 +0100, Prins Anton (ST-CO/ENG1.1) wrote:
> Sorry I'm in a learning curve... now used 'git format-patch'.
> This made it working for 'git am'; so hopefully this time its fine!
> 
> Artem, thanks for your support!

A lot better, but is still not ideal. You did not run checkpatch.pl,
otherwise you would notice this warning:

checkpatch.pl results for patch "[PATCH] mtd: maps/physmap_of.c: error checking to prevent a NULL  pointer dereference"                                                                                                             
                                                                                                                                                                                                                                    
ERROR:MISSING_SIGN_OFF: Missing Signed-off-by: line(s)                                                                                                                                                                              
                                                                                                                                                                                                                                    
total: 1 errors, 0 warnings, 19 lines checked                                                                                                                                                                                       

I've added the S-o-b line for you and pushed the patch to l2-mtd.git,
thanks!
Artem Bityutskiy - Dec. 11, 2012, 8:49 a.m.
On Tue, 2012-12-11 at 09:34 +0100, Prins Anton (ST-CO/ENG1.1) wrote:
> Great, I was not aware I was allowed to sign-off. Thx

You should do this in kernel. The idea is to make the chain of people
the patch went through visible. By putting your signed-off you confirm
that this is your patch. By putting my signed-off I confirm that I took
this patch from you. Sometimes there are long s-o-f chains.

Patch

diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c
index 2e6fb68..f6de444 100644
--- a/drivers/mtd/maps/physmap_of.c
+++ b/drivers/mtd/maps/physmap_of.c
@@ -268,6 +268,7 @@  static int __devinit of_flash_probe(struct platform_device *dev)
        }

        err = 0;
+       info->cmtd = NULL;
        if (info->list_size == 1) {
                info->cmtd = info->list[0].mtd;
        } else if (info->list_size > 1) {
@@ -276,9 +277,10 @@  static int __devinit of_flash_probe(struct platform_device *dev)
                 */
                info->cmtd = mtd_concat_create(mtd_list, info->list_size,
                                               dev_name(&dev->dev));
-               if (info->cmtd == NULL)
-                       err = -ENXIO;
        }
+       if (info->cmtd == NULL)
+               err = -ENXIO;
+
        if (err)
                goto err_out;

-----Original Message-----