[U-Boot] [PATCH] mtd: pxa3xx_nand: Correct allocation and init bug

[U-Boot] [PATCH] mtd: pxa3xx_nand: Correct allocation and init bug

[Kevin Smith]

Correct a null pointer dereference in board_nand_init().  Zeroed
memory was allocated, then immediately dereferenced, which is a
null dereference.  The dereference is completely removed, since
this pointer is later initialized in alloc_nand_resources.

The allocation size is reduced from what was introduced from the
Linux kernel, as U-boot uses the statically allocated nand_info
instead of needing to dynamically allocate an mtd_info instance.

Also, some pointer math was corrected in the initialization of
the nand_chip pointer.

Signed-off-by: Kevin Smith <kevin.smith@elecsyscorp.com>
Cc: Stefan Roese <sr@denx.de>
Cc: Luka Perkov <luka.perkov@sartura.hr>
Cc: Scott Wood <scottwood@freescale.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 1565a9a..e5ea5c2 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -1486,8 +1486,8 @@ static int alloc_nand_resource(struct pxa3xx_nand_info *info)
 	info->variant = pxa3xx_nand_get_variant();
 	for (cs = 0; cs < pdata->num_cs; cs++) {
 		mtd = &nand_info[cs];
-		chip = (struct nand_chip *)info +
-			sizeof(struct pxa3xx_nand_host);
+		chip = (struct nand_chip *)
+			((u8 *)&info[1] + sizeof(*host) * cs);
 		host = (struct pxa3xx_nand_host *)chip;
 		info->host[cs] = host;
 		host->mtd = mtd;
@@ -1600,19 +1600,12 @@ void board_nand_init(void)
 	struct pxa3xx_nand_host *host;
 	int ret;
 
-	info = kzalloc(sizeof(*info) + (sizeof(struct mtd_info) +
-					sizeof(*host)) *
-		       CONFIG_SYS_MAX_NAND_DEVICE, GFP_KERNEL);
+	info = kzalloc(sizeof(*info) +
+				sizeof(*host) * CONFIG_SYS_MAX_NAND_DEVICE,
+			GFP_KERNEL);
 	if (!info)
 		return;
 
-	/*
-	 * If CONFIG_SYS_NAND_SELF_INIT is defined, each driver is responsible
-	 * for instantiating struct nand_chip, while drivers/mtd/nand/nand.c
-	 * still provides a "struct mtd_info nand_info" instance.
-	 */
-	info->host[0]->mtd = &nand_info[0];
-
 	ret = pxa3xx_nand_probe(info);
 	if (ret)
 		return;
-- 
2.4.6

[U-Boot] [PATCH] mtd: pxa3xx_nand: Correct allocation and init bug

[Scott Wood]

On Fri, 2015-10-23 at 17:49 +0000, Kevin Smith wrote:
> Correct a null pointer dereference in board_nand_init().  Zeroed
> memory was allocated, then immediately dereferenced, which is a
> null dereference.  The dereference is completely removed, since
> this pointer is later initialized in alloc_nand_resources.
> 
> The allocation size is reduced from what was introduced from the
> Linux kernel, as U-boot uses the statically allocated nand_info
> instead of needing to dynamically allocate an mtd_info instance.
> 
> Also, some pointer math was corrected in the initialization of
> the nand_chip pointer.
> 
> Signed-off-by: Kevin Smith <kevin.smith@elecsyscorp.com>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Luka Perkov <luka.perkov@sartura.hr>
> Cc: Scott Wood <scottwood@freescale.com>
> ---
>  drivers/mtd/nand/pxa3xx_nand.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> index 1565a9a..e5ea5c2 100644
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
> @@ -1486,8 +1486,8 @@ static int alloc_nand_resource(struct 
> pxa3xx_nand_info *info)
>       info->variant = pxa3xx_nand_get_variant();
>       for (cs = 0; cs < pdata->num_cs; cs++) {
>               mtd = &nand_info[cs];
> -             chip = (struct nand_chip *)info +
> -                     sizeof(struct pxa3xx_nand_host);
> +             chip = (struct nand_chip *)
> +                     ((u8 *)&info[1] + sizeof(*host) * cs);

Yuck.  Could you please rework this driver to not play games with pointers 
and one giant allocation?  Why can't this function allocate each region it 
needs separately?

-Scott

[U-Boot] [PATCH] mtd: pxa3xx_nand: Correct allocation and init bug

[Kevin Smith]

Hi Scott,

On 10/23/2015 01:20 PM, Scott Wood wrote:
>
> Yuck.  Could you please rework this driver to not play games with pointers
> and one giant allocation?  Why can't this function allocate each region it
> needs separately?
>
> -Scott
>
This driver is taken from Linux.  There are a few API modifications to 
make it work in U-Boot, but the main form and function of the driver is 
the same.  The single allocation method is used by Linux and is kept 
here in U-boot.

As for why Linux does this, it may be for cache coherency, avoiding 
memory fragmentation, speed (fewer calls to malloc), or something else.  
I agree it is kind of opaque, but is probably done for a good reason.  I 
didn't port the driver, and I don't know if the reason is applicable to 
U-Boot or if a rework is appropriate.  Maybe Stefan can comment?

Either way, I am not able to rework it right now.  I think my patch 
fixes a legitimate issue.  (It at least fixes the crashes I was 
experiencing).  I hope it can be accepted as-is.

Thanks,
Kevin

[U-Boot] [PATCH] mtd: pxa3xx_nand: Correct allocation and init bug

[Scott Wood]

On Fri, 2015-10-23 at 19:56 +0000, Kevin Smith wrote:
> Hi Scott,
> 
> On 10/23/2015 01:20 PM, Scott Wood wrote:
> > 
> > Yuck.  Could you please rework this driver to not play games with pointers
> > and one giant allocation?  Why can't this function allocate each region it
> > needs separately?
> > 
> > -Scott
> > 
> This driver is taken from Linux.  There are a few API modifications to 
> make it work in U-Boot, but the main form and function of the driver is 
> the same.  The single allocation method is used by Linux and is kept 
> here in U-boot.

Sigh... At least do this in alloc_nand_resources() the way Linux does, rather 
than taking it a step further and allocating an array of these blobs.

> As for why Linux does this, it may be for cache coherency, avoiding
> memory fragmentation, speed (fewer calls to malloc), or something else.  
> I agree it is kind of opaque, but is probably done for a good reason.

I'm sure there was a reason but that doesn't mean it was a good reason.  I 
don't understand how cache coherency would be relevant, nor do I agree that 
trying to optimize a boot path to have fewer malloc calls at the expense of 
making the code more complicated is a "good reason".

>   I didn't port the driver, and I don't know if the reason is applicable to
> U-Boot or if a rework is appropriate.  Maybe Stefan can comment?
> 
> Either way, I am not able to rework it right now.  I think my patch 
> fixes a legitimate issue.  (It at least fixes the crashes I was 
> experiencing).  I hope it can be accepted as-is.

Does Linux have this problem?  Assuming no, please fix this by making the 
driver look more like Linux.  At least then it would be the same ugliness.

Can you explain how the change in the calculation of "chip" and the 
allocation size is relevant to the NULL dereference?  Couldn't that be fixed 
by just removing the "info->host[0]->mtd" line?

-Scott

[U-Boot] [PATCH] mtd: pxa3xx_nand: Correct allocation and init bug

[Kevin Smith]

On 10/23/2015 03:34 PM, Scott Wood wrote:
> Does Linux have this problem?  Assuming no, please fix this by making the
> driver look more like Linux.  At least then it would be the same ugliness.
There are 2 problems and one improvement:
1) Invalid dereference.  This is U-Boot-only code not taken from Linux.  
Removed.
2) Bad pointer math.  This is different from Linux, and I have fixed it 
by making it more like Linux.
3) Unnecessary memory allocation.  I just noticed this while 
investigating my crashes caused by the other two issues.

> Can you explain how the change in the calculation of "chip" and the
> allocation size is relevant to the NULL dereference?  Couldn't that be fixed
> by just removing the "info->host[0]->mtd" line?
It's not, they are two separate bugs that crash when I try to load from 
NAND.  Perhaps I should submit a patch series for this?

- Kevin

[U-Boot] [PATCH] mtd: pxa3xx_nand: Correct allocation and init bug

[Scott Wood]

On Fri, 2015-10-23 at 20:57 +0000, Kevin Smith wrote:
> On 10/23/2015 03:34 PM, Scott Wood wrote:
> > Does Linux have this problem?  Assuming no, please fix this by making the
> > driver look more like Linux.  At least then it would be the same ugliness.
> There are 2 problems and one improvement:
> 1) Invalid dereference.  This is U-Boot-only code not taken from Linux.  
> Removed.
> 2) Bad pointer math.  This is different from Linux, and I have fixed it 
> by making it more like Linux.

It still doesn't look very much like Linux.  Linux has:
                mtd = (void *)&info[1] + (sizeof(*mtd) + sizeof(*host)) * cs;
                chip = (struct nand_chip *)(&mtd[1]);

> 3) Unnecessary memory allocation.  I just noticed this while 
> investigating my crashes caused by the other two issues.
> 
> > Can you explain how the change in the calculation of "chip" and the
> > allocation size is relevant to the NULL dereference?  Couldn't that be 
> > fixed
> > by just removing the "info->host[0]->mtd" line?
> It's not, they are two separate bugs that crash when I try to load from 
> NAND.  Perhaps I should submit a patch series for this?

The allocation size issue causes a crash, not just wasted memory?

-Scott

[U-Boot] [PATCH] mtd: pxa3xx_nand: Correct allocation and init bug

[Kevin Smith]

On 10/23/2015 04:14 PM, Scott Wood wrote:
> On Fri, 2015-10-23 at 20:57 +0000, Kevin Smith wrote:
>> On 10/23/2015 03:34 PM, Scott Wood wrote:
>>> Does Linux have this problem?  Assuming no, please fix this by making the
>>> driver look more like Linux.  At least then it would be the same ugliness.
>> There are 2 problems and one improvement:
>> 1) Invalid dereference.  This is U-Boot-only code not taken from Linux.
>> Removed.
>> 2) Bad pointer math.  This is different from Linux, and I have fixed it
>> by making it more like Linux.
> It still doesn't look very much like Linux.  Linux has:
>                  mtd = (void *)&info[1] + (sizeof(*mtd) + sizeof(*host)) * cs;
>                  chip = (struct nand_chip *)(&mtd[1]);
>
>> 3) Unnecessary memory allocation.  I just noticed this while
>> investigating my crashes caused by the other two issues.
>>
>>> Can you explain how the change in the calculation of "chip" and the
>>> allocation size is relevant to the NULL dereference?  Couldn't that be
>>> fixed
>>> by just removing the "info->host[0]->mtd" line?
>> It's not, they are two separate bugs that crash when I try to load from
>> NAND.  Perhaps I should submit a patch series for this?
> The allocation size issue causes a crash, not just wasted memory?
No, just wasted memory.  Only the invalid dereference and the bad "chip" 
pointer cause crashes.