pwrite() to nand in raw mode fails with EINVAL

pwrite() to nand in raw mode fails with EINVAL

[Maxime Bizon]

Hi,

Since commit 782ce79a45b3b850b108896fcf7da26754061c8f ("cleanup the
nand_do_write_ops"), a pwrite() to a NAND device fails with EINVAL.

The EINVAL comes from here:

        /* Don't allow multipage oob writes with offset */
        if (ops->ooboffs && (ops->ooboffs + ops->ooblen > oobmaxlen))
                return -EINVAL;


ops->ooboffs seems uninitialized.


I think the following memset() are missing:

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index e4def84..731ff36 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -202,6 +202,7 @@ static ssize_t mtd_read(struct file *file, char __user *buf, size_t count,loff_t
 		{
 			struct mtd_oob_ops ops;
 
+			memset(&ops, 0, sizeof (ops));
 			ops.mode = MTD_OOB_RAW;
 			ops.datbuf = kbuf;
 			ops.oobbuf = NULL;
@@ -305,6 +306,7 @@ static ssize_t mtd_write(struct file *file, const char __user *buf, size_t count
 		{
 			struct mtd_oob_ops ops;
 
+			memset(&ops, 0, sizeof (ops));
 			ops.mode = MTD_OOB_RAW;
 			ops.datbuf = kbuf;
 			ops.oobbuf = NULL;



-- 
Maxime

Re: pwrite() to nand in raw mode fails with EINVAL

[Artem Bityutskiy]

On Tue, 2010-09-28 at 20:47 +0200, Maxime Bizon wrote:
> Hi,
> 
> Since commit 782ce79a45b3b850b108896fcf7da26754061c8f ("cleanup the
> nand_do_write_ops"), a pwrite() to a NAND device fails with EINVAL.
> 
> The EINVAL comes from here:
> 
>         /* Don't allow multipage oob writes with offset */
>         if (ops->ooboffs && (ops->ooboffs + ops->ooblen > oobmaxlen))
>                 return -EINVAL;
> 
> 
> ops->ooboffs seems uninitialized.
> 
> 
> I think the following memset() are missing:

This looks like a regression fix and this also needs to be sent to
stable, right?


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

Re: pwrite() to nand in raw mode fails with EINVAL

[Artem Bityutskiy]

Maxime, I've cleaned up comments, added your signed-off-by and cc
stable. Is this OK?

It would also be nice to get an ack from Maxim Levitsky.

From: Maxime Bizon <mbizon@freebox.fr>
Subject: [PATCH] mtd: fix NAND pwrite in raw mode

Since commit 782ce79a45b3b850b108896fcf7da26754061c8f ("cleanup the
nand_do_write_ops") 'pwrite()' to a NAND device fails with '-EINVAL'
which comes from:

        /* Don't allow multipage oob writes with offset */
        if (ops->ooboffs && (ops->ooboffs + ops->ooblen > oobmaxlen))
                return -EINVAL;

because 'ops->ooboffs' was uninitialized. This patch fixes the issue.

Artem: clean-up comment, add stable.

Signed-off-by: Maxime Bizon <mbizon@freebox.fr>
Cc: stable <stable@kernel.org>
Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 drivers/mtd/mtdchar.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index a825002..39dcb57 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -216,6 +216,7 @@ static ssize_t mtd_read(struct file *file, char __user *buf, size_t count,loff_t
 		{
 			struct mtd_oob_ops ops;
 
+			memset(&ops, 0, sizeof (ops));
 			ops.mode = MTD_OOB_RAW;
 			ops.datbuf = kbuf;
 			ops.oobbuf = NULL;
@@ -319,6 +320,7 @@ static ssize_t mtd_write(struct file *file, const char __user *buf, size_t count
 		{
 			struct mtd_oob_ops ops;
 
+			memset(&ops, 0, sizeof (ops));
 			ops.mode = MTD_OOB_RAW;
 			ops.datbuf = kbuf;
 			ops.oobbuf = NULL;
-- 
1.7.2.3


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

Re: pwrite() to nand in raw mode fails with EINVAL

[Maxime Bizon]

On Wed, 2010-09-29 at 15:52 +0300, Artem Bityutskiy wrote:

> Maxime, I've cleaned up comments, added your signed-off-by and cc
> stable. Is this OK?

Yep

> It would also be nice to get an ack from Maxim Levitsky.

The patch fixes the problem, but I let Maxim decides if that's the
correct way to fix it.

-- 
Maxime

Re: pwrite() to nand in raw mode fails with EINVAL

[Maxim Levitsky]

On Wed, 2010-09-29 at 15:58 +0200, Maxime Bizon wrote:
> On Wed, 2010-09-29 at 15:52 +0300, Artem Bityutskiy wrote:
> 
> > Maxime, I've cleaned up comments, added your signed-off-by and cc
> > stable. Is this OK?
> 
> Yep
> 
> > It would also be nice to get an ack from Maxim Levitsky.
> 
> The patch fixes the problem, but I let Maxim decides if that's the
> correct way to fix it.

Very sorry for not noticing this.
I will look very carefully at this tomorrow.

Best regards,
	Maxim Levitsky

Re: pwrite() to nand in raw mode fails with EINVAL

[Maxim Levitsky]

On Fri, 2010-10-22 at 00:54 +0200, Maxim Levitsky wrote:
> On Wed, 2010-09-29 at 15:58 +0200, Maxime Bizon wrote:
> > On Wed, 2010-09-29 at 15:52 +0300, Artem Bityutskiy wrote:
> > 
> > > Maxime, I've cleaned up comments, added your signed-off-by and cc
> > > stable. Is this OK?
> > 
> > Yep
> > 
> > > It would also be nice to get an ack from Maxim Levitsky.
> > 
> > The patch fixes the problem, but I let Maxim decides if that's the
> > correct way to fix it.
> 
> Very sorry for not noticing this.
> I will look very carefully at this tomorrow.
> 
> Best regards,
> 	Maxim Levitsky
> 
I don't see any reason for that patch to be wrong.
I can't test it on my xD card, because like I said before its OOB is
fake, and it probably just emulates the NAND interface.

So of course:

Acked-by: Maxim Levitsky <maximlevitsky@gmail.com>

Best regards,
	Maxim Levitsky

Re: pwrite() to nand in raw mode fails with EINVAL

[David Woodhouse]

On Tue, 2010-09-28 at 20:47 +0200, Maxime Bizon wrote:
> Since commit 782ce79a45b3b850b108896fcf7da26754061c8f ("cleanup the
> nand_do_write_ops"), a pwrite() to a NAND device fails with EINVAL.
> 
> The EINVAL comes from here:
> 
>         /* Don't allow multipage oob writes with offset */
>         if (ops->ooboffs && (ops->ooboffs + ops->ooblen > oobmaxlen))
>                 return -EINVAL;
> 
> 
> ops->ooboffs seems uninitialized.
> 
> 
> I think the following memset() are missing: 

Alternatively, perhaps the sanity check should treat the ->oob{offs,len}
fields as undefined if ->oobbuf is NULL (which is set explicitly in the
two functions you patched). Otherwise, we've effectively changed the API
and we need to make more of an effort to audit *all* users.

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index d551ddd..5d0dc74 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2200,7 +2200,8 @@ static int nand_do_write_ops(struct mtd_info *mtd, loff_t to,
 		memset(chip->oob_poi, 0xff, mtd->oobsize);
 
 	/* Don't allow multipage oob writes with offset */
-	if (ops->ooboffs && (ops->ooboffs + ops->ooblen > oobmaxlen))
+	if (ops->oobbuf && ops->ooboffs &&
+	    (ops->ooboffs + ops->ooblen > oobmaxlen))
 		return -EINVAL;
 
 	while(1) {


-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation