[PATCH] mtd: nand: pxa3xx: Fix buffer overflow during raw reads

[PATCH] mtd: nand: pxa3xx: Fix buffer overflow during raw reads

[Pierre Bourdon]

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;
-- 
2.41.0

Re: [PATCH] mtd: nand: pxa3xx: Fix buffer overflow during raw reads

[Chris Packham]

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
>

Re: [PATCH] mtd: nand: pxa3xx: Fix buffer overflow during raw reads

[Michael Nazzareno Trimarchi]

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
> >



-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com

Re: [PATCH] mtd: nand: pxa3xx: Fix buffer overflow during raw reads

[Pierre Bourdon]

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,

-- 
Pierre Bourdon <delroth@gmail.com>
Software Engineer @ Zürich, Switzerland
https://delroth.net/

Re: [PATCH] mtd: nand: pxa3xx: Fix buffer overflow during raw reads

[Chris Packham]

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/
>

Re: [PATCH] mtd: nand: pxa3xx: Fix buffer overflow during raw reads

[Chris Packham]

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/