| Message ID | 1445622596-18764-1-git-send-email-kevin.smith@elecsyscorp.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Scott Wood |
| Headers | show |
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
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
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
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
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
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.
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;
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(-)