* Re: [PATCH] mtd: nand: warn if hamming layout is used with too large ECC
2018-02-09 9:20 ` Stefan Agner
@ 2018-02-09 9:34 ` Boris Brezillon
2018-02-09 9:54 ` Boris Brezillon
2018-02-09 9:58 ` Boris Brezillon
2 siblings, 0 replies; 9+ messages in thread
From: Boris Brezillon @ 2018-02-09 9:34 UTC (permalink / raw)
To: Stefan Agner
Cc: boris.brezillon, richard, dwmw2, computersforpeace, linux-mtd
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] mtd: nand: warn if hamming layout is used with too large ECC
2018-02-09 9:20 ` Stefan Agner
2018-02-09 9:34 ` Boris Brezillon
@ 2018-02-09 9:54 ` Boris Brezillon
2018-02-09 11:51 ` Stefan Agner
2018-02-09 9:58 ` Boris Brezillon
2 siblings, 1 reply; 9+ messages in thread
From: Boris Brezillon @ 2018-02-09 9:54 UTC (permalink / raw)
To: Stefan Agner
Cc: boris.brezillon, richard, dwmw2, computersforpeace, linux-mtd
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").
Actually, it's bogus since 6a623e076944 ("mtd: nand: add ooblayout for
old hamming layout") which was fixing a bug I had introduced with my
mtd_ooblayout_ops series :-).
>
> 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.
Yep, you should use nand_ooblayout_lp_ops, which was the one used by
default before 6a623e076944 ("mtd: nand: add ooblayout for old hamming
layout").
> 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....
That should be fixed to be more robust, indeed.
>
> 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...
>
> --
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] mtd: nand: warn if hamming layout is used with too large ECC
2018-02-09 9:54 ` Boris Brezillon
@ 2018-02-09 11:51 ` Stefan Agner
0 siblings, 0 replies; 9+ messages in thread
From: Stefan Agner @ 2018-02-09 11:51 UTC (permalink / raw)
To: Boris Brezillon
Cc: boris.brezillon, richard, dwmw2, computersforpeace, linux-mtd
On 09.02.2018 10:54, Boris Brezillon wrote:
> 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").
>
> Actually, it's bogus since 6a623e076944 ("mtd: nand: add ooblayout for
> old hamming layout") which was fixing a bug I had introduced with my
> mtd_ooblayout_ops series :-).
>
>>
>> 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.
>
> Yep, you should use nand_ooblayout_lp_ops, which was the one used by
> default before 6a623e076944 ("mtd: nand: add ooblayout for old hamming
> layout").
>
Ok, will send a patch for that.
--
Stefan
>> 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....
>
> That should be fixed to be more robust, indeed.
>
>>
>> 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...
>>
>> --
>> Stefan
>>
>> >> if (section == 0) {
>> >> oobregion->offset = 2;
>> >> oobregion->length = ecc_offset - 2;
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mtd: nand: warn if hamming layout is used with too large ECC
2018-02-09 9:20 ` Stefan Agner
2018-02-09 9:34 ` Boris Brezillon
2018-02-09 9:54 ` Boris Brezillon
@ 2018-02-09 9:58 ` Boris Brezillon
2018-02-09 11:55 ` Stefan Agner
2 siblings, 1 reply; 9+ messages in thread
From: Boris Brezillon @ 2018-02-09 9:58 UTC (permalink / raw)
To: Stefan Agner
Cc: boris.brezillon, richard, dwmw2, computersforpeace, linux-mtd
On Fri, 09 Feb 2018 10:20:37 +0100
Stefan Agner <stefan@agner.ch> wrote:
>
> 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...
Something like that should help us detect this unexpected case:
--->8---
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 66b67014508f..ada2e709743f 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -201,6 +201,9 @@ static int nand_ooblayout_free_lp_hamming(struct
mtd_info *mtd, int section, oobregion->offset = 2;
oobregion->length = ecc_offset - 2;
} else {
+ if (ecc_offset + ecc->total > mtd->oobsize)
+ return -EINVAL;
+
oobregion->offset = ecc_offset + ecc->total;
oobregion->length = mtd->oobsize - oobregion->offset;
}
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] mtd: nand: warn if hamming layout is used with too large ECC
2018-02-09 9:58 ` Boris Brezillon
@ 2018-02-09 11:55 ` Stefan Agner
2018-02-12 9:34 ` Boris Brezillon
0 siblings, 1 reply; 9+ messages in thread
From: Stefan Agner @ 2018-02-09 11:55 UTC (permalink / raw)
To: Boris Brezillon
Cc: boris.brezillon, richard, dwmw2, computersforpeace, linux-mtd
On 09.02.2018 10:58, Boris Brezillon wrote:
> On Fri, 09 Feb 2018 10:20:37 +0100
> Stefan Agner <stefan@agner.ch> wrote:
>
>>
>> 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...
>
> Something like that should help us detect this unexpected case:
>
It is less generic than making length signed and checking for positive
length, but fine for me.
--
Stefan
> --->8---
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 66b67014508f..ada2e709743f 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -201,6 +201,9 @@ static int nand_ooblayout_free_lp_hamming(struct
> mtd_info *mtd, int section, oobregion->offset = 2;
> oobregion->length = ecc_offset - 2;
> } else {
> + if (ecc_offset + ecc->total > mtd->oobsize)
> + return -EINVAL;
> +
> oobregion->offset = ecc_offset + ecc->total;
> oobregion->length = mtd->oobsize - oobregion->offset;
> }
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] mtd: nand: warn if hamming layout is used with too large ECC
2018-02-09 11:55 ` Stefan Agner
@ 2018-02-12 9:34 ` Boris Brezillon
0 siblings, 0 replies; 9+ messages in thread
From: Boris Brezillon @ 2018-02-12 9:34 UTC (permalink / raw)
To: Stefan Agner
Cc: boris.brezillon, richard, dwmw2, computersforpeace, linux-mtd
On Fri, 09 Feb 2018 12:55:49 +0100
Stefan Agner <stefan@agner.ch> wrote:
> On 09.02.2018 10:58, Boris Brezillon wrote:
> > On Fri, 09 Feb 2018 10:20:37 +0100
> > Stefan Agner <stefan@agner.ch> wrote:
> >
> >>
> >> 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...
> >
> > Something like that should help us detect this unexpected case:
> >
>
> It is less generic than making length signed and checking for positive
> length, but fine for me.
It's just easier for callers if they only have to check the return
code instead of having to check both the return code and the oobregion
fields.
Would you mind modifying your patch accordingly and sending a v2?
Thanks,
Boris
>
> --
> Stefan
>
> > --->8---
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index 66b67014508f..ada2e709743f 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -201,6 +201,9 @@ static int nand_ooblayout_free_lp_hamming(struct
> > mtd_info *mtd, int section, oobregion->offset = 2;
> > oobregion->length = ecc_offset - 2;
> > } else {
> > + if (ecc_offset + ecc->total > mtd->oobsize)
> > + return -EINVAL;
> > +
> > oobregion->offset = ecc_offset + ecc->total;
> > oobregion->length = mtd->oobsize - oobregion->offset;
> > }
--
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
^ permalink raw reply [flat|nested] 9+ messages in thread