| Message ID | 20230729180732.4056248-1-delroth@gmail.com |
|---|---|
| State | New |
| Delegated to: | Dario Binacchi |
| Headers | show |
| Series | mtd: nand: pxa3xx: Fix buffer overflow during raw reads | expand |
Hi Pierre, On Sun, Jul 30, 2023 at 6:08 AM Pierre Bourdon <delroth@gmail.com> wrote: > > Chunked raw reads get accumulated to the data buffer, but in some > ECC configurations they can end up being larger than the originally > computed size (write page size + OOB size). For example: > > 4K page size, ECC strength 8: > - Normal reads: writesize (4096B) + oobsize (128B) = 4224 bytes. > - Chunked raw reads: 4 chunks of 1024B + 1 final spare area of 64B + 5 > ECC areas of 32B = 4320B. I'm not a NAND expert and I haven't sat down and fully grasped the math but I was curious to see what the Linux kernel did. It looks like it uses the same mtd->writesize + mtd->oobsize calculation (see nand_scan_tail() in nand_base.c). So either Linux has the same bug or maybe there's something off in u-boot's nfc_layouts[]. I'll see if I can get one of my boards to trigger a KASAN report (I'm not sure if any of the NAND chips we use will hit the cases you're pointing out). > > Fixes: 6293b0361d9 ("mtd: nand: pxa3xx: add raw read support") > Signed-off-by: Pierre Bourdon <delroth@gmail.com> > --- > > drivers/mtd/nand/raw/pxa3xx_nand.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/mtd/nand/raw/pxa3xx_nand.c b/drivers/mtd/nand/raw/pxa3xx_nand.c > index d502e967f9..2894ababbe 100644 > --- a/drivers/mtd/nand/raw/pxa3xx_nand.c > +++ b/drivers/mtd/nand/raw/pxa3xx_nand.c > @@ -1471,6 +1471,19 @@ static void pxa3xx_nand_detect_config(struct pxa3xx_nand_info *info) > > static int pxa3xx_nand_init_buff(struct pxa3xx_nand_info *info) > { > + unsigned int chunk_size; > + unsigned int last_chunk_size; > + > + /* > + * The data buffer needs to not only be large enough for normal + OOB > + * reads, but also for raw reads. The raw reads can end up taking more > + * space due to the chunking scheme. > + */ > + chunk_size = info->chunk_size + info->spare_size + info->ecc_size; > + last_chunk_size = > + info->last_chunk_size + info->last_spare_size + info->ecc_size; > + info->buf_size = info->nfullchunks * chunk_size + last_chunk_size; > + > info->data_buff = kmalloc(info->buf_size, GFP_KERNEL); > if (info->data_buff == NULL) > return -ENOMEM; > @@ -1661,7 +1674,6 @@ static int pxa3xx_nand_scan(struct mtd_info *mtd) > kfree(info->data_buff); > > /* allocate the real data + oob buffer */ > - info->buf_size = mtd->writesize + mtd->oobsize; > ret = pxa3xx_nand_init_buff(info); > if (ret) > return ret; > -- > 2.41.0 >
Hi Chris On Sun, Jul 30, 2023 at 11:21 PM Chris Packham <judge.packham@gmail.com> wrote: > > Hi Pierre, > > On Sun, Jul 30, 2023 at 6:08 AM Pierre Bourdon <delroth@gmail.com> wrote: > > > > Chunked raw reads get accumulated to the data buffer, but in some > > ECC configurations they can end up being larger than the originally > > computed size (write page size + OOB size). For example: > > > > 4K page size, ECC strength 8: > > - Normal reads: writesize (4096B) + oobsize (128B) = 4224 bytes. > > - Chunked raw reads: 4 chunks of 1024B + 1 final spare area of 64B + 5 > > ECC areas of 32B = 4320B. > > I'm not a NAND expert and I haven't sat down and fully grasped the > math but I was curious to see what the Linux kernel did. It looks like > it uses the same mtd->writesize + mtd->oobsize calculation (see > nand_scan_tail() in nand_base.c). So either Linux has the same bug or > maybe there's something off in u-boot's nfc_layouts[]. I'll see if I > can get one of my boards to trigger a KASAN report (I'm not sure if > any of the NAND chips we use will hit the cases you're pointing out). > I have already seen the code about it and this buffer is used only by uboot to get the result on fake interrupt implementation. The problem that I have seem is why buf_count is not affected on this change. Is possible to have a trace down on what happen with more info? MIchael > > > > Fixes: 6293b0361d9 ("mtd: nand: pxa3xx: add raw read support") > > Signed-off-by: Pierre Bourdon <delroth@gmail.com> > > --- > > > > drivers/mtd/nand/raw/pxa3xx_nand.c | 14 +++++++++++++- > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/mtd/nand/raw/pxa3xx_nand.c b/drivers/mtd/nand/raw/pxa3xx_nand.c > > index d502e967f9..2894ababbe 100644 > > --- a/drivers/mtd/nand/raw/pxa3xx_nand.c > > +++ b/drivers/mtd/nand/raw/pxa3xx_nand.c > > @@ -1471,6 +1471,19 @@ static void pxa3xx_nand_detect_config(struct pxa3xx_nand_info *info) > > > > static int pxa3xx_nand_init_buff(struct pxa3xx_nand_info *info) > > { > > + unsigned int chunk_size; > > + unsigned int last_chunk_size; > > + > > + /* > > + * The data buffer needs to not only be large enough for normal + OOB > > + * reads, but also for raw reads. The raw reads can end up taking more > > + * space due to the chunking scheme. > > + */ > > + chunk_size = info->chunk_size + info->spare_size + info->ecc_size; > > + last_chunk_size = > > + info->last_chunk_size + info->last_spare_size + info->ecc_size; > > + info->buf_size = info->nfullchunks * chunk_size + last_chunk_size; > > + > > info->data_buff = kmalloc(info->buf_size, GFP_KERNEL); > > if (info->data_buff == NULL) > > return -ENOMEM; > > @@ -1661,7 +1674,6 @@ static int pxa3xx_nand_scan(struct mtd_info *mtd) > > kfree(info->data_buff); > > > > /* allocate the real data + oob buffer */ > > - info->buf_size = mtd->writesize + mtd->oobsize; > > ret = pxa3xx_nand_init_buff(info); > > if (ret) > > return ret; > > -- > > 2.41.0 > >
On Sun, Jul 30, 2023 at 11:21 PM Chris Packham <judge.packham@gmail.com> wrote: > On Sun, Jul 30, 2023 at 6:08 AM Pierre Bourdon <delroth@gmail.com> wrote: > > > > Chunked raw reads get accumulated to the data buffer, but in some > > ECC configurations they can end up being larger than the originally > > computed size (write page size + OOB size). For example: > > > > 4K page size, ECC strength 8: > > - Normal reads: writesize (4096B) + oobsize (128B) = 4224 bytes. > > - Chunked raw reads: 4 chunks of 1024B + 1 final spare area of 64B + 5 > > ECC areas of 32B = 4320B. > > I'm not a NAND expert and I haven't sat down and fully grasped the > math but I was curious to see what the Linux kernel did. It looks like > it uses the same mtd->writesize + mtd->oobsize calculation (see > nand_scan_tail() in nand_base.c). So either Linux has the same bug or > maybe there's something off in u-boot's nfc_layouts[]. I'll see if I > can get one of my boards to trigger a KASAN report (I'm not sure if > any of the NAND chips we use will hit the cases you're pointing out). Sure, please let me know. I'm not 100% convinced that this is the correct fix - I know very little about this driver or NANDs in general. On the board I'm playing with (Marvell AC3-based) this patch prevents the driver from corrupting dlmalloc's data structures and causing u-boot to hang. But it could be that this is just papering over another root cause. The NAND chip is a Micron MT29F4G08ABBEAH4, so nothing too unusual. Thanks! Best,
On Mon, 31 Jul 2023, 9:29 am Pierre Bourdon, <delroth@gmail.com> wrote: > On Sun, Jul 30, 2023 at 11:21 PM Chris Packham <judge.packham@gmail.com> > wrote: > > On Sun, Jul 30, 2023 at 6:08 AM Pierre Bourdon <delroth@gmail.com> > wrote: > > > > > > Chunked raw reads get accumulated to the data buffer, but in some > > > ECC configurations they can end up being larger than the originally > > > computed size (write page size + OOB size). For example: > > > > > > 4K page size, ECC strength 8: > > > - Normal reads: writesize (4096B) + oobsize (128B) = 4224 bytes. > > > - Chunked raw reads: 4 chunks of 1024B + 1 final spare area of 64B + 5 > > > ECC areas of 32B = 4320B. > > > > I'm not a NAND expert and I haven't sat down and fully grasped the > > math but I was curious to see what the Linux kernel did. It looks like > > it uses the same mtd->writesize + mtd->oobsize calculation (see > > nand_scan_tail() in nand_base.c). So either Linux has the same bug or > > maybe there's something off in u-boot's nfc_layouts[]. I'll see if I > > can get one of my boards to trigger a KASAN report (I'm not sure if > > any of the NAND chips we use will hit the cases you're pointing out). > > Sure, please let me know. I'm not 100% convinced that this is the > correct fix - I know very little about this driver or NANDs in > general. On the board I'm playing with (Marvell AC3-based) this patch > prevents the driver from corrupting dlmalloc's data structures and > causing u-boot to hang. But it could be that this is just papering > over another root cause. > > The NAND chip is a Micron MT29F4G08ABBEAH4, so nothing too unusual. > Hmm. Both boards I tried had sufficient space in writesize+oobsize. I'll see if I can find others with different nand chips. > Thanks! > Best, > > -- > Pierre Bourdon <delroth@gmail.com> > Software Engineer @ Zürich, Switzerland > https://delroth.net/ >
On Mon, Jul 31, 2023 at 9:29 AM Pierre Bourdon <delroth@gmail.com> wrote: > > On Sun, Jul 30, 2023 at 11:21 PM Chris Packham <judge.packham@gmail.com> wrote: > > On Sun, Jul 30, 2023 at 6:08 AM Pierre Bourdon <delroth@gmail.com> wrote: > > > > > > Chunked raw reads get accumulated to the data buffer, but in some > > > ECC configurations they can end up being larger than the originally > > > computed size (write page size + OOB size). For example: > > > > > > 4K page size, ECC strength 8: > > > - Normal reads: writesize (4096B) + oobsize (128B) = 4224 bytes. > > > - Chunked raw reads: 4 chunks of 1024B + 1 final spare area of 64B + 5 > > > ECC areas of 32B = 4320B. > > > > I'm not a NAND expert and I haven't sat down and fully grasped the > > math but I was curious to see what the Linux kernel did. It looks like > > it uses the same mtd->writesize + mtd->oobsize calculation (see > > nand_scan_tail() in nand_base.c). So either Linux has the same bug or > > maybe there's something off in u-boot's nfc_layouts[]. I'll see if I > > can get one of my boards to trigger a KASAN report (I'm not sure if > > any of the NAND chips we use will hit the cases you're pointing out). > > Sure, please let me know. I'm not 100% convinced that this is the > correct fix - I know very little about this driver or NANDs in > general. On the board I'm playing with (Marvell AC3-based) this patch > prevents the driver from corrupting dlmalloc's data structures and > causing u-boot to hang. But it could be that this is just papering > over another root cause. > > The NAND chip is a Micron MT29F4G08ABBEAH4, so nothing too unusual. At least according to the datasheet I can find MT29F4G08ABBEAH4 has a page size of 4320 bytes (4096 + 224 bytes). Perhaps the problem is actually related to the fact that somehow you've ended up with an oobsize of 128. > > Thanks! > Best, > > -- > Pierre Bourdon <delroth@gmail.com> > Software Engineer @ Zürich, Switzerland > https://delroth.net/
diff --git a/drivers/mtd/nand/raw/pxa3xx_nand.c b/drivers/mtd/nand/raw/pxa3xx_nand.c index d502e967f9..2894ababbe 100644 --- a/drivers/mtd/nand/raw/pxa3xx_nand.c +++ b/drivers/mtd/nand/raw/pxa3xx_nand.c @@ -1471,6 +1471,19 @@ static void pxa3xx_nand_detect_config(struct pxa3xx_nand_info *info) static int pxa3xx_nand_init_buff(struct pxa3xx_nand_info *info) { + unsigned int chunk_size; + unsigned int last_chunk_size; + + /* + * The data buffer needs to not only be large enough for normal + OOB + * reads, but also for raw reads. The raw reads can end up taking more + * space due to the chunking scheme. + */ + chunk_size = info->chunk_size + info->spare_size + info->ecc_size; + last_chunk_size = + info->last_chunk_size + info->last_spare_size + info->ecc_size; + info->buf_size = info->nfullchunks * chunk_size + last_chunk_size; + info->data_buff = kmalloc(info->buf_size, GFP_KERNEL); if (info->data_buff == NULL) return -ENOMEM; @@ -1661,7 +1674,6 @@ static int pxa3xx_nand_scan(struct mtd_info *mtd) kfree(info->data_buff); /* allocate the real data + oob buffer */ - info->buf_size = mtd->writesize + mtd->oobsize; ret = pxa3xx_nand_init_buff(info); if (ret) return ret;
Chunked raw reads get accumulated to the data buffer, but in some ECC configurations they can end up being larger than the originally computed size (write page size + OOB size). For example: 4K page size, ECC strength 8: - Normal reads: writesize (4096B) + oobsize (128B) = 4224 bytes. - Chunked raw reads: 4 chunks of 1024B + 1 final spare area of 64B + 5 ECC areas of 32B = 4320B. Fixes: 6293b0361d9 ("mtd: nand: pxa3xx: add raw read support") Signed-off-by: Pierre Bourdon <delroth@gmail.com> --- drivers/mtd/nand/raw/pxa3xx_nand.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)