[PATCH] [MTD] OneNAND: Do not allow oob write past end of page

[PATCH] [MTD] OneNAND: Do not allow oob write past end of page

[Adrian Hunter]

>From ba01b89188f21ca9a982e33452217e5c6874fd0b Mon Sep 17 00:00:00 2001
From: Adrian Hunter <ext-adrian.hunter@nokia.com>
Date: Thu, 1 Feb 2007 16:04:50 +0200
Subject: [MTD] OneNAND: Do not allow oob write past end of page

OneNAND can write oob to successive pages, but NAND
does not do that.  For compatibility, disallow OneNAND
from writing past the end of the page.

Signed-off-by: Adrian Hunter <ext-adrian.hunter@nokia.com>
---
 drivers/mtd/onenand/onenand_base.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
index 9ec28bb..b7da84a 100644
--- a/drivers/mtd/onenand/onenand_base.c
+++ b/drivers/mtd/onenand/onenand_base.c
@@ -1191,6 +1191,13 @@ static int onenand_do_write_oob(struct m
 		return -EINVAL;
 	}
 
+	/* For compatibility with NAND: Do not allow write past end of page */
+	if (column + len > oobsize) {
+		DEBUG(MTD_DEBUG_LEVEL0, "onenand_write_oob: "
+		      "Attempt to write past end of page\n");
+		return -EINVAL;
+	}
+
 	/* Do not allow reads past end of device */
 	if (unlikely(to >= mtd->size ||
 		     column + len > ((mtd->size >> this->page_shift) -
-- 
1.4.3

RE: [PATCH] [MTD] OneNAND: Do not allow oob write past end of page

[Kyungmin Park]

Hi Adrian

Umm Why is it required in OneNAND? Even though OneNAND is based on NAND. We
regard it as NOR
So we don't need this patch.

Thank you,
Kyungmin Park

> -----Original Message-----
> From: linux-mtd-bounces@lists.infradead.org 
> [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf Of 
> Adrian Hunter
> Sent: Friday, February 02, 2007 4:55 PM
> To: linux-mtd@lists.infradead.org
> Subject: [PATCH] [MTD] OneNAND: Do not allow oob write past 
> end of page
> 
> >From ba01b89188f21ca9a982e33452217e5c6874fd0b Mon Sep 17 
> 00:00:00 2001
> From: Adrian Hunter <ext-adrian.hunter@nokia.com>
> Date: Thu, 1 Feb 2007 16:04:50 +0200
> Subject: [MTD] OneNAND: Do not allow oob write past end of page
> 
> OneNAND can write oob to successive pages, but NAND does not 
> do that.  For compatibility, disallow OneNAND from writing 
> past the end of the page.
> 
> Signed-off-by: Adrian Hunter <ext-adrian.hunter@nokia.com>
> ---
>  drivers/mtd/onenand/onenand_base.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mtd/onenand/onenand_base.c 
> b/drivers/mtd/onenand/onenand_base.c
> index 9ec28bb..b7da84a 100644
> --- a/drivers/mtd/onenand/onenand_base.c
> +++ b/drivers/mtd/onenand/onenand_base.c
> @@ -1191,6 +1191,13 @@ static int onenand_do_write_oob(struct m
>  		return -EINVAL;
>  	}
>  
> +	/* For compatibility with NAND: Do not allow write past 
> end of page */
> +	if (column + len > oobsize) {
> +		DEBUG(MTD_DEBUG_LEVEL0, "onenand_write_oob: "
> +		      "Attempt to write past end of page\n");
> +		return -EINVAL;
> +	}
> +
>  	/* Do not allow reads past end of device */
>  	if (unlikely(to >= mtd->size ||
>  		     column + len > ((mtd->size >> this->page_shift) -
> --
> 1.4.3
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 
> 

Re: [PATCH] [MTD] OneNAND: Do not allow oob write past end of page

[Adrian Hunter]

ext Kyungmin Park wrote:
> Umm Why is it required in OneNAND? Even though OneNAND is based on NAND. We
> regard it as NOR

I am not sure what you mean.  In onenand_base.c it has:

	mtd->type = MTD_NANDFLASH;

So mtd sees it as NAND.

RE: [PATCH] [MTD] OneNAND: Do not allow oob write past end of page

[Kyungmin Park]

> ext Kyungmin Park wrote:
> > Umm Why is it required in OneNAND? Even though OneNAND is based on 
> > NAND. We regard it as NOR
> 
> I am not sure what you mean.  In onenand_base.c it has:
> 
> 	mtd->type = MTD_NANDFLASH;
> 
> So mtd sees it as NAND.

I mean that in the view point of software OnenNAND is NAND, but the one of
hardware it is NOR.
So we don't need to work it as NAND. I think we don't need to restrict the
OneNAND

Thank you,
Kyungmin Park

RE: [PATCH] [MTD] OneNAND: Do not allow oob write past end of page

[Artem Bityutskiy]

On Mon, 2007-02-05 at 16:44 +0900, Kyungmin Park wrote:
> I mean that in the view point of software OnenNAND is NAND, but the one of
> hardware it is NOR.
> So we don't need to work it as NAND. I think we don't need to restrict the
> OneNAND

OneNAND stuff which we have now is _part of MTD_. MTD has its interface,
its model of the device. As long as we are part of this infrastructure,
we have to obey MTD's interface, rules and restrictions.

The whole idea of MTD is to make upper layers to access flashes in a
uniform layer. MTD achieves this at certain extent. There are flaws, but
anyway. So basically OneNAND implements more then NAND in this OOB
handling stuff. And it is bad. It encourages other people to write
OneNAND-only software, incompatible with NAND, which is totally
unreasonable in this case.

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

RE: [PATCH] [MTD] OneNAND: Do not allow oob write past end of page

[Kyungmin Park]

Hi Adrian,

> 
> OneNAND stuff which we have now is _part of MTD_. MTD has its 
> interface, its model of the device. As long as we are part of 
> this infrastructure, we have to obey MTD's interface, rules 
> and restrictions.
> 
> The whole idea of MTD is to make upper layers to access 
> flashes in a uniform layer. MTD achieves this at certain 
> extent. There are flaws, but anyway. So basically OneNAND 
> implements more then NAND in this OOB handling stuff. And it 
> is bad. It encourages other people to write OneNAND-only 
> software, incompatible with NAND, which is totally 
> unreasonable in this case.
> 

I will push it.

BTW, after patch, we got dead code.
I downloaded oobtest again, re-run it and see that it trigger exception
code in onenand_do_write_oob
But it don't touch following codes whether include your patch or not.

        /* Do not allow write past end of device */
        if (unlikely(to >= mtd->size ||
            column + len > ((mtd->size >> this->page_shift) -
                                (to >> this->page_shift)) * oobsize)) {
                DEBUG(MTD_DEBUG_LEVEL0, "onenand_write_oob: Attempted to
write past end of device\n");
                return -EINVAL;
        }

Please check oobtest program.

Thank you,
Kyungmin Park

--

oobtest: Test 4 of 5
oobtest: erasing
oobtest: erased 0
oobtest: erased 128
oobtest: Attempting to start write past end of oob
oobtest: An error is expected...
oobtest: Error occurred as expected
oobtest: Attempting to start read past end of oob
oobtest: An error is expected...
oobtest: Error occurred as expected
oobtest: Attempting to write past end of device
oobtest: An error is expected...
-> onenand_write_oob: Attempt to write past end of page
oobtest: Error occurred as expected
oobtest: Attempting to read past end of device
oobtest: An error is expected...
oobtest: error: read past end of device
oobtest: Attempting to write past end of device
oobtest: An error is expected...
-> onenand_write_oob: Attempt to write past end of page
oobtest: Error occurred as expected
oobtest: Attempting to read past end of device
oobtest: An error is expected...
oobtest: error: read past end of device

Re: [PATCH] [MTD] OneNAND: Do not allow oob write past end of page

[Adrian Hunter]

ext Kyungmin Park wrote:
> BTW, after patch, we got dead code.
> I downloaded oobtest again, re-run it and see that it trigger exception
> code in onenand_do_write_oob
> But it don't touch following codes whether include your patch or not.
> 
>         /* Do not allow write past end of device */
>         if (unlikely(to >= mtd->size ||
>             column + len > ((mtd->size >> this->page_shift) -
>                                 (to >> this->page_shift)) * oobsize)) {
>                 DEBUG(MTD_DEBUG_LEVEL0, "onenand_write_oob: Attempted to
> write past end of device\n");
>                 return -EINVAL;
>         }
> 
> Please check oobtest program.
> 
> Thank you,
> Kyungmin Park
> 
> --
> 
> oobtest: Test 4 of 5
> oobtest: erasing
> oobtest: erased 0
> oobtest: erased 128
> oobtest: Attempting to start write past end of oob
> oobtest: An error is expected...
> oobtest: Error occurred as expected
> oobtest: Attempting to start read past end of oob
> oobtest: An error is expected...
> oobtest: Error occurred as expected
> oobtest: Attempting to write past end of device
> oobtest: An error is expected...
> -> onenand_write_oob: Attempt to write past end of page
> oobtest: Error occurred as expected
> oobtest: Attempting to read past end of device
> oobtest: An error is expected...
> oobtest: error: read past end of device
> oobtest: Attempting to write past end of device
> oobtest: An error is expected...
> -> onenand_write_oob: Attempt to write past end of page
> oobtest: Error occurred as expected
> oobtest: Attempting to read past end of device
> oobtest: An error is expected...
> oobtest: error: read past end of device
> 

Seems ok for me:

oobtest: Attempting to start write past end of oob
oobtest: An error is expected...
onenand_write_oob: Attempted to start write outside oob
oobtest: Error occurred as expected
oobtest: Attempting to start read past end of oob
oobtest: An error is expected...
onenand_read_oob: Attempted to start read outside oob
oobtest: Error occurred as expected
oobtest: Attempting to write past end of device
oobtest: An error is expected...
onenand_write_oob: Attempted to write past end of device
oobtest: Error occurred as expected
oobtest: Attempting to read past end of device
oobtest: An error is expected...
onenand_read_oob: Attempted to read beyond end of device
oobtest: Error occurred as expected
oobtest: Attempting to write past end of device
oobtest: An error is expected...
onenand_write_oob: Attempted to write past end of device
oobtest: Error occurred as expected
oobtest: Attempting to read past end of device
oobtest: An error is expected...
onenand_read_oob: Attempted to read beyond end of device
oobtest: Error occurred as expected

RE: [PATCH] [MTD] OneNAND: Do not allow oob write past end of page

[Kyungmin Park]

With new oobtest it's passed.
It's same as you

Thank you,
Kyungmin Park

> 
> Seems ok for me:
> 
> oobtest: Attempting to start write past end of oob
> oobtest: An error is expected...
> onenand_write_oob: Attempted to start write outside oob
> oobtest: Error occurred as expected
> oobtest: Attempting to start read past end of oob
> oobtest: An error is expected...
> onenand_read_oob: Attempted to start read outside oob
> oobtest: Error occurred as expected
> oobtest: Attempting to write past end of device
> oobtest: An error is expected...
> onenand_write_oob: Attempted to write past end of device
> oobtest: Error occurred as expected
> oobtest: Attempting to read past end of device
> oobtest: An error is expected...
> onenand_read_oob: Attempted to read beyond end of device
> oobtest: Error occurred as expected
> oobtest: Attempting to write past end of device
> oobtest: An error is expected...
> onenand_write_oob: Attempted to write past end of device
> oobtest: Error occurred as expected
> oobtest: Attempting to read past end of device
> oobtest: An error is expected...
> onenand_read_oob: Attempted to read beyond end of device
> oobtest: Error occurred as expected
>