[PATCH] Revert "mtd: spinand: Fix OOB read"

[PATCH] Revert "mtd: spinand: Fix OOB read"

[Felix Fietkau]

This reverts stable commit baad618d078c857f99cc286ea249e9629159901f.

This commit is adding lines to spinand_write_to_cache_op, wheras the upstream
commit 868cbe2a6dcee451bd8f87cbbb2a73cf463b57e5 that this was supposed to
backport was touching spinand_read_from_cache_op.
It causes a crash on writing OOB data by attempting to write to read-only
kernel memory.

Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 drivers/mtd/nand/spi/core.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 7900571fc85b..c35221794645 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -318,10 +318,6 @@ static int spinand_write_to_cache_op(struct spinand_device *spinand,
 		buf += ret;
 	}
 
-	if (req->ooblen)
-		memcpy(req->oobbuf.in, spinand->oobbuf + req->ooboffs,
-		       req->ooblen);
-
 	return 0;
 }
 
-- 
2.28.0

Re: [PATCH] Revert "mtd: spinand: Fix OOB read"

[Greg KH]

On Tue, Jan 05, 2021 at 11:18:21AM +0100, Felix Fietkau wrote:
> This reverts stable commit baad618d078c857f99cc286ea249e9629159901f.
> 
> This commit is adding lines to spinand_write_to_cache_op, wheras the upstream
> commit 868cbe2a6dcee451bd8f87cbbb2a73cf463b57e5 that this was supposed to
> backport was touching spinand_read_from_cache_op.
> It causes a crash on writing OOB data by attempting to write to read-only
> kernel memory.
> 
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> ---
>  drivers/mtd/nand/spi/core.c | 4 ----
>  1 file changed, 4 deletions(-)

So the backport to 5.10.y broke, but not the backport to 4.19.y or
5.4.y?  Can you provide a "correct" backport for this instead of just
removing this fix?

thanks,

greg k-h

Re: [PATCH] Revert "mtd: spinand: Fix OOB read"

[Felix Fietkau]

On 2021-01-05 11:31, Greg KH wrote:
> On Tue, Jan 05, 2021 at 11:18:21AM +0100, Felix Fietkau wrote:
>> This reverts stable commit baad618d078c857f99cc286ea249e9629159901f.
>> 
>> This commit is adding lines to spinand_write_to_cache_op, wheras the upstream
>> commit 868cbe2a6dcee451bd8f87cbbb2a73cf463b57e5 that this was supposed to
>> backport was touching spinand_read_from_cache_op.
>> It causes a crash on writing OOB data by attempting to write to read-only
>> kernel memory.
>> 
>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>> ---
>>  drivers/mtd/nand/spi/core.c | 4 ----
>>  1 file changed, 4 deletions(-)
> 
> So the backport to 5.10.y broke, but not the backport to 4.19.y or
> 5.4.y?  Can you provide a "correct" backport for this instead of just
> removing this fix?
I just checked, it seems that 4.19.y and 5.4.y are broken in exactly the
same way.

On a first glance, it seems that the upstream commit has a wrong Fixes
line and is fixing 3d1f08b032dc ("mtd: spinand: Use the external ECC
engine logic") instead, which is not in 5.10.
If that is correct, then we don't need any stable backport at all.

In my opinion it's best to just revert the broken commit in all the
stable trees as quickly as possible and let Miquel sort out the mess
afterwards, if needed.

- Felix

Re: [PATCH] Revert "mtd: spinand: Fix OOB read"

[Miquel Raynal]

Hello,

Greg KH <gregkh@linuxfoundation.org> wrote on Tue, 5 Jan 2021 11:31:26
+0100:

> On Tue, Jan 05, 2021 at 11:18:21AM +0100, Felix Fietkau wrote:
> > This reverts stable commit baad618d078c857f99cc286ea249e9629159901f.
> > 
> > This commit is adding lines to spinand_write_to_cache_op, wheras the upstream
> > commit 868cbe2a6dcee451bd8f87cbbb2a73cf463b57e5 that this was supposed to
> > backport was touching spinand_read_from_cache_op.
> > It causes a crash on writing OOB data by attempting to write to read-only
> > kernel memory.
> > 
> > Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> > Signed-off-by: Felix Fietkau <nbd@nbd.name>
> > ---
> >  drivers/mtd/nand/spi/core.c | 4 ----
> >  1 file changed, 4 deletions(-)  
> 
> So the backport to 5.10.y broke, but not the backport to 4.19.y or
> 5.4.y?  Can you provide a "correct" backport for this instead of just
> removing this fix?

Agreed, I think the proper way to handle the situation would be to move
these three lines to spinand_read_from_cache_op() instead of just
getting rid of them.

Thanks,
Miquèl

Re: [PATCH] Revert "mtd: spinand: Fix OOB read"

[Felix Fietkau]

On 2021-01-05 11:40, Miquel Raynal wrote:
> Hello,
> 
> Greg KH <gregkh@linuxfoundation.org> wrote on Tue, 5 Jan 2021 11:31:26
> +0100:
> 
>> On Tue, Jan 05, 2021 at 11:18:21AM +0100, Felix Fietkau wrote:
>> > This reverts stable commit baad618d078c857f99cc286ea249e9629159901f.
>> > 
>> > This commit is adding lines to spinand_write_to_cache_op, wheras the upstream
>> > commit 868cbe2a6dcee451bd8f87cbbb2a73cf463b57e5 that this was supposed to
>> > backport was touching spinand_read_from_cache_op.
>> > It causes a crash on writing OOB data by attempting to write to read-only
>> > kernel memory.
>> > 
>> > Cc: Miquel Raynal <miquel.raynal@bootlin.com>
>> > Signed-off-by: Felix Fietkau <nbd@nbd.name>
>> > ---
>> >  drivers/mtd/nand/spi/core.c | 4 ----
>> >  1 file changed, 4 deletions(-)  
>> 
>> So the backport to 5.10.y broke, but not the backport to 4.19.y or
>> 5.4.y?  Can you provide a "correct" backport for this instead of just
>> removing this fix?
> 
> Agreed, I think the proper way to handle the situation would be to move
> these three lines to spinand_read_from_cache_op() instead of just
> getting rid of them.
But they have a similar line in spinand_read_from_cache_op already (in
addition to some extra code for dealing with MTD_OPS_AUTO_OOB).

Please take another look at your commit 3d1f08b032dc, it really looks to
me like you were just adding back a part of what you removed there.
Maybe the proper solution is to add back the rest of it as well (the
part that deals with MTD_OPS_AUTO_OOB) and leave the stable kernels alone.

- Felix

Re: [PATCH] Revert "mtd: spinand: Fix OOB read"

[Greg KH]

On Tue, Jan 05, 2021 at 11:40:11AM +0100, Felix Fietkau wrote:
> 
> On 2021-01-05 11:31, Greg KH wrote:
> > On Tue, Jan 05, 2021 at 11:18:21AM +0100, Felix Fietkau wrote:
> >> This reverts stable commit baad618d078c857f99cc286ea249e9629159901f.
> >> 
> >> This commit is adding lines to spinand_write_to_cache_op, wheras the upstream
> >> commit 868cbe2a6dcee451bd8f87cbbb2a73cf463b57e5 that this was supposed to
> >> backport was touching spinand_read_from_cache_op.
> >> It causes a crash on writing OOB data by attempting to write to read-only
> >> kernel memory.
> >> 
> >> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> >> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> >> ---
> >>  drivers/mtd/nand/spi/core.c | 4 ----
> >>  1 file changed, 4 deletions(-)
> > 
> > So the backport to 5.10.y broke, but not the backport to 4.19.y or
> > 5.4.y?  Can you provide a "correct" backport for this instead of just
> > removing this fix?
> I just checked, it seems that 4.19.y and 5.4.y are broken in exactly the
> same way.
> 
> On a first glance, it seems that the upstream commit has a wrong Fixes
> line and is fixing 3d1f08b032dc ("mtd: spinand: Use the external ECC
> engine logic") instead, which is not in 5.10.
> If that is correct, then we don't need any stable backport at all.

Ah, then yes, this should be reverted everywhere.  If that is the case,
otherwise, I went off of the fixes: value in the original commit, which
is in the 4.19.0 release.

> In my opinion it's best to just revert the broken commit in all the
> stable trees as quickly as possible and let Miquel sort out the mess
> afterwards, if needed.

I can do that, but need an ack from the maintainer...

thanks,

greg k-h

Re: [PATCH] Revert "mtd: spinand: Fix OOB read"

[Miquel Raynal]

Hi Felix,

Felix Fietkau <nbd@nbd.name> wrote on Tue, 5 Jan 2021 11:47:27 +0100:

> On 2021-01-05 11:40, Miquel Raynal wrote:
> > Hello,
> > 
> > Greg KH <gregkh@linuxfoundation.org> wrote on Tue, 5 Jan 2021 11:31:26
> > +0100:
> >   
> >> On Tue, Jan 05, 2021 at 11:18:21AM +0100, Felix Fietkau wrote:  
> >> > This reverts stable commit baad618d078c857f99cc286ea249e9629159901f.
> >> > 
> >> > This commit is adding lines to spinand_write_to_cache_op, wheras the upstream
> >> > commit 868cbe2a6dcee451bd8f87cbbb2a73cf463b57e5 that this was supposed to
> >> > backport was touching spinand_read_from_cache_op.
> >> > It causes a crash on writing OOB data by attempting to write to read-only
> >> > kernel memory.
> >> > 
> >> > Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> >> > Signed-off-by: Felix Fietkau <nbd@nbd.name>
> >> > ---
> >> >  drivers/mtd/nand/spi/core.c | 4 ----
> >> >  1 file changed, 4 deletions(-)    
> >> 
> >> So the backport to 5.10.y broke, but not the backport to 4.19.y or
> >> 5.4.y?  Can you provide a "correct" backport for this instead of just
> >> removing this fix?  
> > 
> > Agreed, I think the proper way to handle the situation would be to move
> > these three lines to spinand_read_from_cache_op() instead of just
> > getting rid of them.  
> But they have a similar line in spinand_read_from_cache_op already (in
> addition to some extra code for dealing with MTD_OPS_AUTO_OOB).
> 
> Please take another look at your commit 3d1f08b032dc, it really looks to
> me like you were just adding back a part of what you removed there.
> Maybe the proper solution is to add back the rest of it as well (the
> part that deals with MTD_OPS_AUTO_OOB) and leave the stable kernels alone.

You are actually right, I've got confused by my own previous change.

The right thing to do is indeed to get rid of this patch in the stable
kernels.

Thanks for reporting,
Miquèl