mtd: pxa3xx_nand: Correct allocation and init bug
Subsystems (2)
PATCH 1/1 mtd: pxa3xx_nand: Correct allocation and init bug
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.
--- drivers/mtd/nand/pxa3xx_nand.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)
drivers/mtd/nand/pxa3xx_nand.c +5 -12
@@ -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 ;
Signature · 1 line
2.4.6
PATCH 1/1 mtd: pxa3xx_nand: Correct allocation and init bug
Quoted 25 lines · 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 inalloc_nand_resources.The allocation size is reduced from what was introduced from the Linux kernel, as U-boot uses the statically allocated
nand_infoinstead of needing to dynamically allocate anmtd_infoinstance.Also, some pointer math was corrected in the initialization of the
nand_chippointer.Signed-off-by Kevin Smith <[email protected]>Cc Stefan Roese <[email protected]>Cc Luka Perkov <[email protected]>Cc Scott Wood <[email protected]>--- drivers/mtd/nand/pxa3xx_nand.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)drivers/mtd/nand/pxa3xx_nand.c +0
-0
drivers/mtd/nand/pxa3xx_nand.c static int alloc_nand_resource(struct · line 1491
Quoted 8 lines
drivers/mtd/nand/pxa3xx_nand.c +2
-2@@ -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
PATCH 1/1 mtd: pxa3xx_nand: Correct allocation and init bug
Hi Scott,
Quoted 7 lines · Scott Wood
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
PATCH 1/1 mtd: pxa3xx_nand: Correct allocation and init bug
Quoted 8 lines · Kevin Smith
Hi Scott,
Quoted 5 lines · Scott Wood
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?
Quoted 6 lines
Quoted 2 lines
-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.
Quoted 3 lines
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".
Quoted 6 lines
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
PATCH 1/1 mtd: pxa3xx_nand: Correct allocation and init bug
Quoted 2 lines · Scott Wood
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.
Quoted 3 lines
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
PATCH 1/1 mtd: pxa3xx_nand: Correct allocation and init bug
Quoted 8 lines · Kevin Smith
Quoted 2 lines · Scott Wood
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]);Quoted 9 lines
3) Unnecessary memory allocation. I just noticed this while investigating my crashes caused by the other two issues.
Quoted 4 lines
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
PATCH 1/1 mtd: pxa3xx_nand: Correct allocation and init bug
Quoted 16 lines · Scott Wood
Quoted 8 lines · Kevin Smith
Quoted 2 lines · Scott Wood
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]);Quoted 3 lines
3) Unnecessary memory allocation. I just noticed this while investigating my crashes caused by the other two issues.
Quoted 7 lines
Quoted 6 lines
Quoted 4 lines
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.