| Message ID | 85D877DD6EE67B4A9FCA9B9C3A4865670C3AF1D4E4@SI-MBX14.de.bosch.com |
|---|---|
| State | New, archived |
| Headers | show |
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) >
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) >
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!
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!
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.
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-----