[PATCH 1/1] mtd: nand: add check for out of page read

[PATCH 1/1] mtd: nand: add check for out of page read

[Jason Liu]

When run mtd_oobtest case, there will be one error for step(4),
which turned out it need add one check for out of page read in
nand_do_read_oob just like mtd_do_write_oob did it already.
This commit also fix one typo error for comments in mtd_do_write_oob

Signed-off-by: Jason Liu <r64343@freescale.com>
---
 drivers/mtd/nand/nand_base.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 1f75a1b..75d199e 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1782,6 +1782,13 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
 	else
 		len = mtd->oobsize;
 
+	/* Do not allow read past end of page */
+	if ((ops->ooboffs + readlen) > len) {
+		DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt to read "
+				"past end of page\n", __func__);
+		return -EINVAL;
+	}
+
 	if (unlikely(ops->ooboffs >= len)) {
 		DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt to start read "
 					"outside oob\n", __func__);
@@ -2377,7 +2384,7 @@ static int nand_do_write_oob(struct mtd_info *mtd, loff_t to,
 		return -EINVAL;
 	}
 
-	/* Do not allow reads past end of device */
+	/* Do not allow write past end of device */
 	if (unlikely(to >= mtd->size ||
 		     ops->ooboffs + ops->ooblen >
 			((mtd->size >> chip->page_shift) -
-- 
1.7.0.4

Re: [PATCH 1/1] mtd: nand: add check for out of page read

[Artem Bityutskiy]

On Fri, 2010-11-19 at 16:40 +0800, Jason Liu wrote:
> When run mtd_oobtest case, there will be one error for step(4),
> which turned out it need add one check for out of page read in
> nand_do_read_oob just like mtd_do_write_oob did it already.
> This commit also fix one typo error for comments in mtd_do_write_oob
> 
> Signed-off-by: Jason Liu <r64343@freescale.com>
> ---
>  drivers/mtd/nand/nand_base.c |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)

Pushed to l2-mtd-2.6.git, thanks!

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 1/1] mtd: nand: add check for out of page read

[Artem Bityutskiy]

On Fri, 2010-11-19 at 16:40 +0800, Jason Liu wrote:
> When run mtd_oobtest case, there will be one error for step(4),
> which turned out it need add one check for out of page read in
> nand_do_read_oob just like mtd_do_write_oob did it already.
> This commit also fix one typo error for comments in mtd_do_write_oob
> 
> Signed-off-by: Jason Liu <r64343@freescale.com>
> ---
>  drivers/mtd/nand/nand_base.c |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 1f75a1b..75d199e 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -1782,6 +1782,13 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
>  	else
>  		len = mtd->oobsize;
>  
> +	/* Do not allow read past end of page */
> +	if ((ops->ooboffs + readlen) > len) {
> +		DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt to read "
> +				"past end of page\n", __func__);
> +		return -EINVAL;
> +	}

As you reported to me in a private e-mail (although I prefer to always
have a public ML in CC when dealing with open source stuff) this patch
is wrong. Indeed, it limits the maximum amount of bytes which can be
read at one go to the OOB size, which is incorrect, because
mtd->read_oob() allows reading multiple pages at a time, see comment
near "struct mtd_oob_ops" at include/linux/mtd/mtd.h. So this patch
breaks ABI and hence, has to be reverted.

>  	if (unlikely(ops->ooboffs >= len)) {
>  		DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt to start read "
>  					"outside oob\n", __func__);

Side note: nand_base.c has a bunch of senseless unlikely() hints, would
be nice to clean that up some day.

> -	/* Do not allow reads past end of device */
> +	/* Do not allow write past end of device */

Care to make this a separate clean-up patch meanwhile?

Thank!

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 1/1] mtd: nand: add check for out of page read

[Artem Bityutskiy]

On Fri, 2010-11-19 at 16:40 +0800, Jason Liu wrote:
> When run mtd_oobtest case, there will be one error for step(4),
> which turned out it need add one check for out of page read in
> nand_do_read_oob just like mtd_do_write_oob did it already.
> This commit also fix one typo error for comments in mtd_do_write_oob
> 
> Signed-off-by: Jason Liu <r64343@freescale.com>

I cannot reproduce this with nandsim, can you?

I tried:

1. sudo modprobe nandsim
   sudo modprobe mtd_oobtest dev=0

2. sudo ./load_nandsim.sh 128 128 2048
   sudo modprobe mtd_oobtest dev=0

In both cases the test passes: "mtd_oobtest: finished with 0 errors"

And in 'nand_do_read_oob()' I see the following piece of code:

        /* Do not allow reads past end of device */
        if (unlikely(from >= mtd->size ||
                     ops->ooboffs + readlen > ((mtd->size >> chip->page_shift) -
                                        (from >> chip->page_shift)) * len)) {
                DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt read beyond end "
                                        "of device\n", __func__);
                return -EINVAL;
        }

which should handle the case AFAICS.

So, although I did think hard about this, it looks like there is no
problem. What is the kernel version you are looking at? I checked it
with my l2-mtd-2.6.git, which is the latest mtd-2.6.git plus a revert
commit of your patch:

http://git.infradead.org/users/dedekind/l2-mtd-2.6.git

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 1/1] mtd: nand: add check for out of page read

[Jason Liu]

Hi, Artem,

2010/12/14 Artem Bityutskiy <dedekind1@gmail.com>:
> On Fri, 2010-11-19 at 16:40 +0800, Jason Liu wrote:
>> When run mtd_oobtest case, there will be one error for step(4),
>> which turned out it need add one check for out of page read in
>> nand_do_read_oob just like mtd_do_write_oob did it already.
>> This commit also fix one typo error for comments in mtd_do_write_oob
>>
>> Signed-off-by: Jason Liu <r64343@freescale.com>
>
> I cannot reproduce this with nandsim, can you?
>
> I tried:
>
> 1. sudo modprobe nandsim
>   sudo modprobe mtd_oobtest dev=0
>
> 2. sudo ./load_nandsim.sh 128 128 2048
>   sudo modprobe mtd_oobtest dev=0
>
> In both cases the test passes: "mtd_oobtest: finished with 0 errors"
>
> And in 'nand_do_read_oob()' I see the following piece of code:
>
>        /* Do not allow reads past end of device */
>        if (unlikely(from >= mtd->size ||
>                     ops->ooboffs + readlen > ((mtd->size >> chip->page_shift) -
>                                        (from >> chip->page_shift)) * len)) {
>                DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt read beyond end "
>                                        "of device\n", __func__);
>                return -EINVAL;
>        }

Here the mtd->size in nand_base.c should be the NAND flash chip size,
while in nand_oobtest,

              /* Attempt to read off end of device */
                ops.mode      = MTD_OOB_AUTO;
                ops.len       = 0;
                ops.retlen    = 0;
                ops.ooblen    = mtd->ecclayout->oobavail;
                ops.oobretlen = 0;
                ops.ooboffs   = 1;
                ops.datbuf    = NULL;
                ops.oobbuf    = readbuf;
                printk(PRINT_PREF "attempting to read past end of device\n");
                printk(PRINT_PREF "an error is expected...\n");
                err = mtd->read_oob(mtd, mtd->size - mtd->writesize, &ops);
                if (err) {
                        printk(PRINT_PREF "error occurred as expected\n");
                        err = 0;
                } else {
                        printk(PRINT_PREF "error: read past end of device\n");
                        errcnt += 1;
                }
        }

here, mtd->size is mtd partition size right? when the mtd partition is
not the last MTD partition, the error will happen.

I have tested on real NAND, while not on nandsim.

>
> which should handle the case AFAICS.
>
> So, although I did think hard about this, it looks like there is no
> problem. What is the kernel version you are looking at? I checked it
> with my l2-mtd-2.6.git, which is the latest mtd-2.6.git plus a revert
> commit of your patch:
>
> http://git.infradead.org/users/dedekind/l2-mtd-2.6.git

Yes, as I have written the email to you before in a private email to
tell that this patch has issue and please drop it.

Thanks,

>
> --
> Best Regards,
> Artem Bityutskiy (Артём Битюцкий)
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH 1/1] mtd: nand: add check for out of page read

[Artem Bityutskiy]

On Wed, 2010-12-15 at 09:55 +0800, Jason Liu wrote:
> >        /* Do not allow reads past end of device */
> >        if (unlikely(from >= mtd->size ||
> >                     ops->ooboffs + readlen > ((mtd->size >> chip->page_shift) -
> >                                        (from >> chip->page_shift)) * len)) {
> >                DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt read beyond end "
> >                                        "of device\n", __func__);
> >                return -EINVAL;
> >        }
> 
> Here the mtd->size in nand_base.c should be the NAND flash chip size,

I think this is partition size as well.

> while in nand_oobtest,
> 
>               /* Attempt to read off end of device */
>                 ops.mode      = MTD_OOB_AUTO;
>                 ops.len       = 0;
>                 ops.retlen    = 0;
>                 ops.ooblen    = mtd->ecclayout->oobavail;
>                 ops.oobretlen = 0;
>                 ops.ooboffs   = 1;
>                 ops.datbuf    = NULL;
>                 ops.oobbuf    = readbuf;
>                 printk(PRINT_PREF "attempting to read past end of device\n");
>                 printk(PRINT_PREF "an error is expected...\n");
>                 err = mtd->read_oob(mtd, mtd->size - mtd->writesize, &ops);
>                 if (err) {
>                         printk(PRINT_PREF "error occurred as expected\n");
>                         err = 0;
>                 } else {
>                         printk(PRINT_PREF "error: read past end of device\n");
>                         errcnt += 1;
>                 }
>         }
> 
> here, mtd->size is mtd partition size right? when the mtd partition is
> not the last MTD partition, the error will happen.

I think this is the same in both. Try to debug this with printks().

> 
> I have tested on real NAND, while not on nandsim.

Please, try to reproduce this with nandsim as well, and if it works, try
to find out why (using printk debugging)

-- 
Best Regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [PATCH 1/1] mtd: nand: add check for out of page read

[Jason Liu]

Hi, Artem,

2010/12/20 Artem Bityutskiy <dedekind1@gmail.com>:
> On Wed, 2010-12-15 at 09:55 +0800, Jason Liu wrote:
>> >        /* Do not allow reads past end of device */
>> >        if (unlikely(from >= mtd->size ||
>> >                     ops->ooboffs + readlen > ((mtd->size >> chip->page_shift) -
>> >                                        (from >> chip->page_shift)) * len)) {
>> >                DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt read beyond end "
>> >                                        "of device\n", __func__);
>> >                return -EINVAL;
>> >        }
>>
>> Here the mtd->size in nand_base.c should be the NAND flash chip size,
>
> I think this is partition size as well.

I think you are wrong. This should be the NAND flash chip size not the
partition size.

>
>> while in nand_oobtest,
>>
>>               /* Attempt to read off end of device */
>>                 ops.mode      = MTD_OOB_AUTO;
>>                 ops.len       = 0;
>>                 ops.retlen    = 0;
>>                 ops.ooblen    = mtd->ecclayout->oobavail;
>>                 ops.oobretlen = 0;
>>                 ops.ooboffs   = 1;
>>                 ops.datbuf    = NULL;
>>                 ops.oobbuf    = readbuf;
>>                 printk(PRINT_PREF "attempting to read past end of device\n");
>>                 printk(PRINT_PREF "an error is expected...\n");
>>                 err = mtd->read_oob(mtd, mtd->size - mtd->writesize, &ops);
>>                 if (err) {
>>                         printk(PRINT_PREF "error occurred as expected\n");
>>                         err = 0;
>>                 } else {
>>                         printk(PRINT_PREF "error: read past end of device\n");
>>                         errcnt += 1;
>>                 }
>>         }
>>
>> here, mtd->size is mtd partition size right? when the mtd partition is
>> not the last MTD partition, the error will happen.
>
> I think this is the same in both. Try to debug this with printks().
>
>>
>> I have tested on real NAND, while not on nandsim.
>
> Please, try to reproduce this with nandsim as well, and if it works, try
> to find out why (using printk debugging)

Yes, I have reproduce this issue with nandsim, please check the following log,

root@freescale ~$ insmod nandsim.ko first_id_byte=0x20
second_id_byte=0xaa third_id_byte=0x00 fourth_id_byte=0x15
parts=0x400,0x400
[nandsim] warning: read_byte: unexpected data output cycle, state is
STATE_READY return 0x0
[nandsim] warning: read_byte: unexpected data output cycle, state is
STATE_READY return 0x0
[nandsim] warning: read_byte: unexpected data output cycle, state is
STATE_READY return 0x0
[nandsim] warning: read_byte: unexpected data output cycle, state is
STATE_READY return 0x0
NAND device: Manufacturer ID: 0x20, Chip ID: 0xaa (ST Micro NAND
256MiB 1,8V 8-bit)
flash size: 256 MiB
page size: 2048 bytes
OOB area size: 64 bytes
sector size: 128 KiB
pages number: 131072
pages per sector: 64
bus width: 8
bits in sector size: 17
bits in page size: 11
bits in OOB size: 6
flash size with OOB: 270336 KiB
page address bytes: 5
sector address bytes: 3
options: 0x8
Scanning device for bad blocks
Creating 2 MTD partitions on "NAND 256MiB 1,8V 8-bit":
0x000000000000-0x000008000000 : "NAND simulator partition 0"
0x000008000000-0x000010000000 : "NAND simulator partition 1"
root@freescale ~$ cat /proc/mtd
dev:    size   erasesize  name
mtd0: 00300000 00080000 "bootloader"
mtd1: 00500000 00080000 "nand.kernel"
mtd2: 10000000 00080000 "nand.rootfs"
mtd3: 10000000 00080000 "nand.userfs1"
mtd4: df800000 00080000 "nand.userfs2"
mtd5: 08000000 00020000 "NAND simulator partition 0"
mtd6: 08000000 00020000 "NAND simulator partition 1"
root@freescale ~$ insmod mtd_oobtest.ko dev=5
=================================================
mtd_oobtest: MTD device: 5
mtd_oobtest: MTD device size 134217728, eraseblock size 131072, page
size 2048, count of eraseblocks 1024, pages per eraseblock
64, OOB size 64
mtd_oobtest: scanning for bad eraseblocks
mtd_oobtest: scanned 1024 eraseblocks, 0 are bad
mtd_oobtest: test 1 of 5
mtd_oobtest: erasing whole device
mtd_oobtest: erased 1024 eraseblocks
mtd_oobtest: writing OOBs of whole device
mtd_oobtest: written up to eraseblock 0
mtd_oobtest: written up to eraseblock 256
mtd_oobtest: written up to eraseblock 512
mtd_oobtest: written up to eraseblock 768
mtd_oobtest: written 1024 eraseblocks
mtd_oobtest: verifying all eraseblocks
mtd_oobtest: verified up to eraseblock 0
mtd_oobtest: verified up to eraseblock 256
mtd_oobtest: verified up to eraseblock 512
mtd_oobtest: verified up to eraseblock 768
mtd_oobtest: verified 1024 eraseblocks
mtd_oobtest: test 2 of 5
mtd_oobtest: erasing whole device
mtd_oobtest: erased 1024 eraseblocks
mtd_oobtest: writing OOBs of whole device
mtd_oobtest: written up to eraseblock 0
mtd_oobtest: written up to eraseblock 256
mtd_oobtest: written up to eraseblock 512
mtd_oobtest: written up to eraseblock 768
mtd_oobtest: written 1024 eraseblocks
mtd_oobtest: verifying all eraseblocks
mtd_oobtest: verified up to eraseblock 0
mtd_oobtest: verified up to eraseblock 256
mtd_oobtest: verified up to eraseblock 512
mtd_oobtest: verified up to eraseblock 768
mtd_oobtest: verified 1024 eraseblocks
mtd_oobtest: test 3 of 5
mtd_oobtest: erasing whole device
mtd_oobtest: erased 1024 eraseblocks
mtd_oobtest: writing OOBs of whole device
mtd_oobtest: written up to eraseblock 0
mtd_oobtest: written up to eraseblock 256
mtd_oobtest: written up to eraseblock 512
mtd_oobtest: written up to eraseblock 768
mtd_oobtest: written 1024 eraseblocks
mtd_oobtest: verifying all eraseblocks
mtd_oobtest: verified up to eraseblock 0
mtd_oobtest: verified up to eraseblock 256
mtd_oobtest: verified up to eraseblock 512
mtd_oobtest: verified up to eraseblock 768
mtd_oobtest: verified 1024 eraseblocks
mtd_oobtest: test 4 of 5
mtd_oobtest: erasing whole device
mtd_oobtest: erased 1024 eraseblocks
mtd_oobtest: attempting to start write past end of OOB
mtd_oobtest: an error is expected...
mtd_oobtest: error occurred as expected
mtd_oobtest: attempting to start read past end of OOB
mtd_oobtest: an error is expected...
mtd_oobtest: error occurred as expected
mtd_oobtest: attempting to write past end of device
mtd_oobtest: an error is expected...
mtd_oobtest: error occurred as expected
mtd_oobtest: attempting to read past end of device
mtd_oobtest: an error is expected...
mtd_oobtest: error: read past end of device
mtd_oobtest: attempting to write past end of device
mtd_oobtest: an error is expected...
mtd_oobtest: error occurred as expected
mtd_oobtest: attempting to read past end of device
mtd_oobtest: an error is expected...
mtd_oobtest: error: read past end of device
mtd_oobtest: test 5 of 5
mtd_oobtest: erasing whole device
mtd_oobtest: erased 1024 eraseblocks
mtd_oobtest: writing OOBs of whole device
mtd_oobtest: written up to eraseblock 0
mtd_oobtest: written up to eraseblock 0
mtd_oobtest: written up to eraseblock 256
mtd_oobtest: written up to eraseblock 256
mtd_oobtest: written up to eraseblock 512
mtd_oobtest: written up to eraseblock 512
mtd_oobtest: written up to eraseblock 768
mtd_oobtest: written up to eraseblock 768
mtd_oobtest: written 1023 eraseblocks
mtd_oobtest: verifying all eraseblocks
mtd_oobtest: verified up to eraseblock 0
mtd_oobtest: verified up to eraseblock 256
mtd_oobtest: verified up to eraseblock 512
mtd_oobtest: verified up to eraseblock 768
mtd_oobtest: verified 1023 eraseblocks
mtd_oobtest: finished with 2 errors
=================================================

>
> --
> Best Regards,
> Artem Bityutskiy (Битюцкий Артём)
>
>

Re: [PATCH 1/1] mtd: nand: add check for out of page read

[Artem Bityutskiy]

On Thu, 2010-12-23 at 14:06 +0800, Jason Liu wrote:
> Hi, Artem,
> 
> 2010/12/20 Artem Bityutskiy <dedekind1@gmail.com>:
> > On Wed, 2010-12-15 at 09:55 +0800, Jason Liu wrote:
> >> >        /* Do not allow reads past end of device */
> >> >        if (unlikely(from >= mtd->size ||
> >> >                     ops->ooboffs + readlen > ((mtd->size >> chip->page_shift) -
> >> >                                        (from >> chip->page_shift)) * len)) {
> >> >                DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt read beyond end "
> >> >                                        "of device\n", __func__);
> >> >                return -EINVAL;
> >> >        }
> >>
> >> Here the mtd->size in nand_base.c should be the NAND flash chip size,
> >
> > I think this is partition size as well.
> 
> I think you are wrong. This should be the NAND flash chip size not the
> partition size.

Yes, thanks for pointing this.

The patch below fixes the issue you found. I've also pushed this patch
to my l2-mtd-2.6 tree. If you can test it, please, do, but it fixes the
issue when nandsim is used.


>From 2b6dc448005afdfe48ac2096d24694b8ea3ac88b Mon Sep 17 00:00:00 2001
From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Date: Sun, 16 Jan 2011 17:50:54 +0200
Subject: [PATCH] mtd: mtdpart: disallow reading OOB past the end of the partition

This patch fixes the mtdpart bug which allows users reading OOB past the
end of the partition. This happens because 'part_read_oob()' allows reading
multiple OOB areas in one go, and mtdparts does not validate the OOB
length in the request.

Although there is such check in 'nand_do_read_oob()' in nand_base.c, but
it checks that we do not read past the flash chip, not the partition,
because in nand_base.c we work with the whole chip (e.g., mtd->size
in nand_base.c is the size of the whole chip). So this check cannot
be done correctly in nand_base.c and should be instead done in mtdparts.c.

This problem was reported by Jason Liu <r64343@freescale.com> and reproduced
with nandsim:

$ modprobe nandsim first_id_byte=0x20 second_id_byte=0xaa third_id_byte=0x00 \
                   fourth_id_byte=0x15 parts=0x400,0x400
$ modprobe nandsim mtd_oobtest.ko dev=0
$ dmesg
= snip =
mtd_oobtest: attempting to read past end of device
mtd_oobtest: an error is expected...
mtd_oobtest: error: read past end of device
= snip =
mtd_oobtest: finished with 2 errors

Reported-by: Jason Liu <liu.h.jason@gmail.com>
Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 drivers/mtd/mtdpart.c |   19 ++++++++++++++++++-
 1 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 79e3689..d2b932f 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -120,8 +120,25 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from,
 		return -EINVAL;
 	if (ops->datbuf && from + ops->len > mtd->size)
 		return -EINVAL;
-	res = part->master->read_oob(part->master, from + part->offset, ops);
 
+	/*
+	 * If OOB is also requested, make sure that we do not read past the end
+	 * of this partition.
+	 */
+	if (ops->oobbuf) {
+		size_t len, pages;
+
+		if (ops->mode == MTD_OOB_AUTO)
+			len = mtd->oobavail;
+		else
+			len = mtd->oobsize;
+		pages = mtd_div_by_ws(mtd->size, mtd);
+		pages -= mtd_div_by_ws(from, mtd);
+		if (ops->ooboffs + ops->ooblen > pages * len)
+			return -EINVAL;
+	}
+
+	res = part->master->read_oob(part->master, from + part->offset, ops);
 	if (unlikely(res)) {
 		if (res == -EUCLEAN)
 			mtd->ecc_stats.corrected++;
-- 
1.7.3.4

-- 
Best Regards,
Artem Bityutskiy (Битюцкий Артём)