Re: [PATCH] mtd: nand: warn if hamming layout is used with too large ECC

[Boris Brezillon <boris.brezillon@bootlin.com>]

On Fri, 09 Feb 2018 10:20:37 +0100
Stefan Agner <stefan@agner.ch> wrote:

> On 09.02.2018 09:50, Boris Brezillon wrote:
> > On Fri,  9 Feb 2018 00:33:05 +0100
> > Stefan Agner <stefan@agner.ch> wrote:
> >   
> >> Warn in case a driver uses too large ECC with hamming layout.
> >> This is especially helpful since hamming layout is the default
> >> layout when using hardware ECC and no specific OOB layout is
> >> specified.
> >>
> >> Signed-off-by: Stefan Agner <stefan@agner.ch>
> >> ---
> >>  drivers/mtd/nand/nand_base.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> >> index 96c97588e1ba..2f3f43d0e288 100644
> >> --- a/drivers/mtd/nand/nand_base.c
> >> +++ b/drivers/mtd/nand/nand_base.c
> >> @@ -197,6 +197,8 @@ static int nand_ooblayout_free_lp_hamming(struct mtd_info *mtd, int section,
> >>  		return -EINVAL;
> >>  	}
> >>
> >> +	WARN_ON(mtd->oobsize - ecc_offset < ecc->total);
> >> +  
> > 
> > Did you hit this problem? Anyway, if there's a case where the number of
> > ECC bytes does not fit in the space reserved for ECC, there's a bug
> > before this point, and this should be checked at init/probe time.
> >   
> 
> Yes, I realized that vf610_nfc.c, which is currently is using the
> hamming ooblayout. This probably crept in with commit 3cf32d180227
> ("mtd: nand: vf610: switch to mtd_ooblayout_ops").
> 
> When using 32-bit ECC mode the driver uses 60 bytes out of 64 bytes OOB,
> so it actually fits into the OOB.
> 
> The layout is just bogus for that case. Surprisingly the oobavail
> calculation ends up being correct, but only because the calculation
> overflows twice:
> 
> mtd_ooblayout_count_bytes calls first with section 0, which results in
> 38. the second call leads to an overflow ("-36").
> mtd_ooblayout_count_bytes then adds 38 to that overflow, which then
> overflows again to the correct value of overall free bytes of 2... I did
> not try actually using the free OOB area, I guess this would fail....
> 
> Of course the underlying issue that ooblayout for vf610_nfc.c is bogus
> needs to be fixed, I will send a patch for that.
> 
> But some kind of sanity check somewhere might be worthwhile, I was a bit
> surprised that this overflowing happens on a driver in operational use
> and goes unnoticed. I realize that this patch is not ideal. Maybe making
> length signed, then we could sanity check in
> mtd_ooblayout_count_bytes...

My point was not that we don't need the sanity check, but we probably
want that check to happen earlier, for example when we select this
layout. Maybe that's not possible though (I didn't look closely at the
init sequence).

> 
> --
> Stefan
> 
> >>  	if (section == 0) {
> >>  		oobregion->offset = 2;
> >>  		oobregion->length = ecc_offset - 2;  



-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com