[PATCH] mtd: nand: pxa3xx: Fix buffer overflow during raw reads
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(-)
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;
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/
participants (3)
-
Chris Packham -
Michael Nazzareno Trimarchi -
Pierre Bourdon