Patchwork Mtd alignment bug affecting 1.4.X including current git

login
register
mail settings
Submitter Artem Bityutskiy
Date April 1, 2011, 8:16 a.m.
Message ID <1301645767.2789.25.camel@localhost>
Download mbox | patch
Permalink /patch/89239/
State New
Headers show

Comments

Artem Bityutskiy - April 1, 2011, 8:16 a.m.
On Mon, 2011-03-28 at 07:48 -0600, Kelly Anderson wrote:
> If you create a patch I'll test it for you.

Kelly, would you please test the following patch:

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Subject: [PATCH] libmtd: fix OOB read and write interface

When reading and writing OOB we specify the address as absolute
offset from the beginning of the MTD device. This offset is
basically an absolute page offset plus the OOB offset. And it does
not have to be aligned to the min. I/O unit size (NAND page size).

So fix the 'do_oob_op()' function and remove incorrect checking
that the offset is page-aligned. This check leads to the following
errors:

libmtd: error!: unaligned address 2, mtd0 page size is 2048

But obviously, the intent was to write to offset 2 of the OOB area
of the very first NAND page.

Instead of that incorrect check, we should check that the OOB offset
we write to is within the OOB size and the length is withing the OOB
size. This patch adds such check.

Reported-by: Kelly Anderson <kelly@silka.with-linux.com>
Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 lib/libmtd.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)
Kelly Anderson - April 1, 2011, 9:02 a.m.
On 04/01/11 02:16, Artem Bityutskiy wrote:
> On Mon, 2011-03-28 at 07:48 -0600, Kelly Anderson wrote:
>> If you create a patch I'll test it for you.
> Kelly, would you please test the following patch:

Seems to work fine.

> From: Artem Bityutskiy<Artem.Bityutskiy@nokia.com>
> Subject: [PATCH] libmtd: fix OOB read and write interface
>
> When reading and writing OOB we specify the address as absolute
> offset from the beginning of the MTD device. This offset is
> basically an absolute page offset plus the OOB offset. And it does
> not have to be aligned to the min. I/O unit size (NAND page size).
>
> So fix the 'do_oob_op()' function and remove incorrect checking
> that the offset is page-aligned. This check leads to the following
> errors:
>
> libmtd: error!: unaligned address 2, mtd0 page size is 2048
>
> But obviously, the intent was to write to offset 2 of the OOB area
> of the very first NAND page.
>
> Instead of that incorrect check, we should check that the OOB offset
> we write to is within the OOB size and the length is withing the OOB
> size. This patch adds such check.
>
> Reported-by: Kelly Anderson<kelly@silka.with-linux.com>
> Signed-off-by: Artem Bityutskiy<Artem.Bityutskiy@nokia.com>
> ---
>   lib/libmtd.c |   12 ++++++++----
>   1 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/lib/libmtd.c b/lib/libmtd.c
> index e0c0934..e313fc3 100644
> --- a/lib/libmtd.c
> +++ b/lib/libmtd.c
> @@ -1083,6 +1083,7 @@ int do_oob_op(libmtd_t desc, const struct mtd_dev_info *mtd, int fd,
>   	struct mtd_oob_buf64 oob64;
>   	struct mtd_oob_buf oob;
>   	unsigned long long max_offs;
> +	unsigned int oob_offs;
>   	const char *cmd64_str, *cmd_str;
>   	struct libmtd *lib = (struct libmtd *)desc;
>
> @@ -1102,10 +1103,13 @@ int do_oob_op(libmtd_t desc, const struct mtd_dev_info *mtd, int fd,
>   		errno = EINVAL;
>   		return -1;
>   	}
> -	if (start % mtd->min_io_size) {
> -		errmsg("unaligned address %llu, mtd%d page size is %d",
> -		       (unsigned long long)start, mtd->mtd_num,
> -		       mtd->min_io_size);
> +
> +	oob_offs = start&  (mtd->min_io_size - 1);
> +	if (oob_offs + length>  mtd->oob_size) {
> +		errmsg("Cannot write %llu OOB bytes to address %llu "
> +		       "(OOB offset %u) - mtd%d OOB size is only %d bytes",
> +		       (unsigned long long)length, (unsigned long long)start,
> +		       oob_offs, mtd->mtd_num,  mtd->oob_size);
>   		errno = EINVAL;
>   		return -1;
>   	}
Artem Bityutskiy - April 1, 2011, 4:26 p.m.
On Fri, 2011-04-01 at 03:02 -0600, Kelly Anderson wrote:
> On 04/01/11 02:16, Artem Bityutskiy wrote:
> > On Mon, 2011-03-28 at 07:48 -0600, Kelly Anderson wrote:
> >> If you create a patch I'll test it for you.
> > Kelly, would you please test the following patch:
> 
> Seems to work fine.

Thanks, pushed to the mtd-utils tree.

Patch

diff --git a/lib/libmtd.c b/lib/libmtd.c
index e0c0934..e313fc3 100644
--- a/lib/libmtd.c
+++ b/lib/libmtd.c
@@ -1083,6 +1083,7 @@  int do_oob_op(libmtd_t desc, const struct mtd_dev_info *mtd, int fd,
 	struct mtd_oob_buf64 oob64;
 	struct mtd_oob_buf oob;
 	unsigned long long max_offs;
+	unsigned int oob_offs;
 	const char *cmd64_str, *cmd_str;
 	struct libmtd *lib = (struct libmtd *)desc;
 
@@ -1102,10 +1103,13 @@  int do_oob_op(libmtd_t desc, const struct mtd_dev_info *mtd, int fd,
 		errno = EINVAL;
 		return -1;
 	}
-	if (start % mtd->min_io_size) {
-		errmsg("unaligned address %llu, mtd%d page size is %d",
-		       (unsigned long long)start, mtd->mtd_num,
-		       mtd->min_io_size);
+
+	oob_offs = start & (mtd->min_io_size - 1);
+	if (oob_offs + length > mtd->oob_size) {
+		errmsg("Cannot write %llu OOB bytes to address %llu "
+		       "(OOB offset %u) - mtd%d OOB size is only %d bytes",
+		       (unsigned long long)length, (unsigned long long)start,
+		       oob_offs, mtd->mtd_num,  mtd->oob_size);
 		errno = EINVAL;
 		return -1;
 	}