[PATCH 1/2] mtd: rawnand: ingenic: validate BCH error index in jz4725b_correct()

[PATCH 1/2] mtd: rawnand: ingenic: validate BCH error index in jz4725b_correct()

[Guanglei Zhu]

From: Weibin Liu <liuwb@xiaopeng.com>

The JZ4725B BCH controller reports each corrected bit error as an
index into the codeword it was asked to decode, in the INDEX0/INDEX1
fields of the BCH_BHERR registers. These fields are 13 bits wide, but
nothing in jz4725b_correct() checks the reported index against the
size of the data buffer being corrected:

    buf[(bit >> 3)] ^= BIT(bit & 0x7);

With data or ECC coming from an untrusted flash, the decoder can
report an index that points past the end of the data buffer, either
into the parity area of the codeword or beyond it entirely, and the
above statement then flips a bit outside the buffer.

Reject the page with -EBADMSG when the reported index does not fit
in the data area.

Fixes: 3e6ac2ad90b3 ("mtd: rawnand: ingenic: Add support for the JZ4725B")
Signed-off-by: Weibin Liu <liuwb@xiaopeng.com>
---

Verified with a userspace replica of the correction loop: with
params->size = 512, an index pointing into the parity area (bit =
4099) or at the field maximum (0x1fff) flips bits past the data
buffer (ASAN heap-buffer-overflow); in-range indices keep
correcting as before.

 drivers/mtd/nand/raw/ingenic/jz4725b_bch.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/mtd/nand/raw/ingenic/jz4725b_bch.c b/drivers/mtd/nand/raw/ingenic/jz4725b_bch.c
index 2d0e0a2192ae..727f135f381e 100644
--- a/drivers/mtd/nand/raw/ingenic/jz4725b_bch.c
+++ b/drivers/mtd/nand/raw/ingenic/jz4725b_bch.c
@@ -259,6 +259,11 @@ static int jz4725b_correct(struct ingenic_ecc *bch,
 			bit = (reg & BCH_BHERR_INDEX0_MASK) >> BCH_BHERR_INDEX0_SHIFT;
 		}
 
+		if (bit >= params->size * 8) {
+			ret = -EBADMSG;
+			goto out;
+		}
+
 		buf[(bit >> 3)] ^= BIT(bit & 0x7);
 	}
 
-- 
2.50.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 2/2] mtd: rawnand: ingenic: validate BCH error index in jz4780_correct()

[Guanglei Zhu]

From: Weibin Liu <liuwb@xiaopeng.com>

The JZ4780 BCH controller reports each corrected bit error as a byte
pair index, in the INDEX field of the BCH_BHERR registers. The field
is 11 bits wide, but nothing in jz4780_correct() checks the reported
index against the size of the data buffer being corrected:

    buf[(index * 2) + 0] ^= mask;
    buf[(index * 2) + 1] ^= mask >> 8;

With data or ECC coming from an untrusted flash, the decoder can
report an index that points past the end of the data buffer, and the
above statements then flip bits outside the buffer.

Reject the page with -EBADMSG when the reported index does not fit in
the data area.

Fixes: ae02ab00aa3c ("mtd: nand: jz4780: driver for NAND devices on JZ4780 SoCs")
Signed-off-by: Weibin Liu <liuwb@xiaopeng.com>
---

Verified with a userspace replica of the correction loop: with
params->size = 512, an index of 256 (first parity word) or at the
field maximum (0x7ff) flips bits past the data buffer (ASAN
heap-buffer-overflow); in-range indices keep correcting as before.

 drivers/mtd/nand/raw/ingenic/jz4780_bch.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/mtd/nand/raw/ingenic/jz4780_bch.c b/drivers/mtd/nand/raw/ingenic/jz4780_bch.c
index 12b5b0484fe9..204bc853c1b4 100644
--- a/drivers/mtd/nand/raw/ingenic/jz4780_bch.c
+++ b/drivers/mtd/nand/raw/ingenic/jz4780_bch.c
@@ -216,6 +216,11 @@ static int jz4780_correct(struct ingenic_ecc *bch,
 						BCH_BHERR_MASK_SHIFT;
 			index = (reg & BCH_BHERR_INDEX_MASK) >>
 						BCH_BHERR_INDEX_SHIFT;
+			if ((index * 2) + 1 >= params->size) {
+				ret = -EBADMSG;
+				goto out;
+			}
+
 			buf[(index * 2) + 0] ^= mask;
 			buf[(index * 2) + 1] ^= mask >> 8;
 		}
-- 
2.50.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] mtd: rawnand: ingenic: validate BCH error index in jz4780_correct()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mtd: rawnand: ingenic: false -EBADMSG on parity bitflips in jz4780_correct()
--

commit 8de94e9615fc4f597d06591e322c73a02ac18b50
Author: Weibin Liu <liuwb@xiaopeng.com>

mtd: rawnand: ingenic: validate BCH error index in jz4780_correct()

This commit adds a bounds check on the BCH error index in jz4780_correct()
and returns -EBADMSG if the reported byte-pair index exceeds params->size.

> diff --git a/drivers/mtd/nand/raw/ingenic/jz4780_bch.c b/drivers/mtd/nand/raw/ingenic/jz4780_bch.c
> index 12b5b0484fe97..204bc853c1b42 100644
> --- a/drivers/mtd/nand/raw/ingenic/jz4780_bch.c
> +++ b/drivers/mtd/nand/raw/ingenic/jz4780_bch.c
> @@ -216,6 +216,11 @@ static int jz4780_correct(struct ingenic_ecc *bch,
>  						BCH_BHERR_MASK_SHIFT;
>  			index = (reg & BCH_BHERR_INDEX_MASK) >>
>  						BCH_BHERR_INDEX_SHIFT;
> +			if ((index * 2) + 1 >= params->size) {
> +				ret = -EBADMSG;
> +				goto out;
> +			}

[Severity: High]
Won't this introduce a regression when a correctable bitflip occurs in the
OOB ECC parity bytes?

In drivers/mtd/nand/raw/ingenic/jz4780_bch.c, jz4780_correct() writes both
the data buffer and the read ECC parity bytes to the BCH controller so it
decodes the entire codeword of params->size + params->bytes bytes:

	jz4780_bch_reset(bch, params, false);
	jz4780_bch_write_data(bch, buf, params->size);
	jz4780_bch_write_data(bch, ecc_code, params->bytes);

When a bitflip occurs on flash within the ECC parity bytes (and total
codeword bitflips are within params->strength), the hardware reports a
correctable error (BCH_BHINT_ERR set, BCH_BHINT_UNCOR clear) with index
pointing into the parity region [params->size, params->size + params->bytes),
such as index == 256 when params->size == 512.

With this check returning -EBADMSG and jumping to out whenever
(index * 2) + 1 >= params->size, any correctable bitflip in the parity area
aborts the loop (leaving any subsequent data-area error entries in
BCH_BHERRn uncorrected) and fails the page read as an uncorrectable ECC
error.

By comparison, the sibling driver in
drivers/mtd/nand/raw/ingenic/jz4740_ecc.c:jz4740_ecc_correct() skips
modifying buf when the index falls outside the data area without failing
the read:

		for (i = 0; i < error_count; ++i) {
			error = readl(ecc->base + JZ_REG_NAND_ERR(i));
			index = ((error >> 16) & 0x1ff) - 1;
			if (index >= 0 && index < params->size)
				jz_nand_correct_data(buf, index, error & 0x1ff);
		}

Should jz4780_correct() skip the buf XOR when index falls in the parity
region and only return -EBADMSG if the reported location lies beyond the
full codeword (params->size + params->bytes)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105040153.66597-1-zhugl3@xiaopeng.com?part=2

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] mtd: rawnand: ingenic: validate BCH error index in jz4725b_correct()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mtd: rawnand: ingenic: jz4725b_correct() rejects parity bitflips with -EBADMSG
--

commit 2ec6f1cdbda7137d56bda90c1d422ad8f955d1df
Author: Weibin Liu <liuwb@xiaopeng.com>

mtd: rawnand: ingenic: validate BCH error index in jz4725b_correct()

This commit adds a bounds check on the decoded bit error index in
jz4725b_correct() to prevent writing past the end of the data buffer when the
BCH controller reports an index >= params->size * 8, returning -EBADMSG
instead.

> diff --git a/drivers/mtd/nand/raw/ingenic/jz4725b_bch.c b/drivers/mtd/nand/raw/ingenic/jz4725b_bch.c
> index 2d0e0a2192ae4..727f135f381e4 100644
> --- a/drivers/mtd/nand/raw/ingenic/jz4725b_bch.c
> +++ b/drivers/mtd/nand/raw/ingenic/jz4725b_bch.c
> @@ -259,6 +259,11 @@ static int jz4725b_correct(struct ingenic_ecc *bch,
>  			bit = (reg & BCH_BHERR_INDEX0_MASK) >> BCH_BHERR_INDEX0_SHIFT;
>  		}
>  
> +		if (bit >= params->size * 8) {
> +			ret = -EBADMSG;
> +			goto out;
> +		}

[Severity: High]
Does returning -EBADMSG when bit >= params->size * 8 cause valid pages with a
correctable bitflip in the OOB ECC parity bytes to fail with an uncorrectable
ECC error?

In jz4725b_bch_reset(), the hardware decode count is programmed to cover both
the data area and the ECC parity area:

drivers/mtd/nand/raw/ingenic/jz4725b_bch.c:jz4725b_bch_reset() {
	...
	reg = params->size << BCH_BHCNT_ENC_COUNT_SHIFT;
	reg |= (params->size + params->bytes) << BCH_BHCNT_DEC_COUNT_SHIFT;
	writel(reg, bch->base + BCH_BHCNT);
	...
}

And jz4725b_correct() feeds both buf and ecc_code into the BCH engine as a
single codeword of (params->size + params->bytes) * 8 bits:

drivers/mtd/nand/raw/ingenic/jz4725b_bch.c:jz4725b_correct() {
	...
	jz4725b_bch_write_data(bch, buf, params->size);
	jz4725b_bch_write_data(bch, ecc_code, params->bytes);
	...
}

When a normal, correctable bitflip occurs on flash within the ECC parity bytes
themselves, the hardware reports a bit index in the range
[params->size * 8, (params->size + params->bytes) * 8) without setting
BCH_BHINT_UNCOR.

With this check, a bitflip in the parity region causes jz4725b_correct() to
abort the correction loop early (potentially leaving remaining data bitflips
in later BCH_BHERRn entries uncorrected) and return -EBADMSG to
ingenic_nand_ecc_correct(), incrementing mtd->ecc_stats.failed.

Should bit indices in the parity region simply skip modifying buf (while still
counting the bitflip or continuing the loop, similar to jz4740_ecc_correct()),
or only return -EBADMSG if bit >= (params->size + params->bytes) * 8?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105040153.66597-1-zhugl3@xiaopeng.com?part=1

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 1/2] mtd: rawnand: ingenic: validate BCH error index in jz4725b_correct()

[Guanglei Zhu]

From: Weibin Liu <liuwb@xiaopeng.com>

The JZ4725B BCH controller reports each corrected bit error as an
index into the codeword it was asked to decode, in the INDEX0/INDEX1
fields of the BCH_BHERR registers. These fields are 13 bits wide, but
nothing in jz4725b_correct() checks the reported index against the
size of the data buffer being corrected:

    buf[(bit >> 3)] ^= BIT(bit & 0x7);

With data or ECC coming from an untrusted flash, the decoder can
report an index that points past the end of the data buffer, and the
above statement then flips a bit outside the buffer.

Skip the correction when the reported index does not point into
the data area.  A bitflip in the parity bytes does not affect the
data, so rejecting the page for it would fail reads that are
perfectly correctable, and an index beyond the codeword has no
buffer to update in the first place.

Fixes: 3e6ac2ad90b3 ("mtd: rawnand: ingenic: Add support for the JZ4725B")
Signed-off-by: Weibin Liu <liuwb@xiaopeng.com>
---
Changes in v2: skip the correction for an index that does not point
into the data area instead of failing the page with -EBADMSG.  A
correctable bitflip in the ECC bytes leaves the data valid, and the
early exit also dropped the error entries that followed it.  This
matches how jz4740_ecc_correct() treats its indexes.

Verified with a userspace replica of the correction loop: with
params->size = 512, an index of 4099 (parity area) followed by a
data-area index of 100 in the same BHERR register is now corrected
(v1 returned -EBADMSG and left the data error unfixed), the field
maximum (0x1fff) produces no write outside the buffer, and in-range
indexes keep correcting as before.

 drivers/mtd/nand/raw/ingenic/jz4725b_bch.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mtd/nand/raw/ingenic/jz4725b_bch.c b/drivers/mtd/nand/raw/ingenic/jz4725b_bch.c
index 2d0e0a2192ae..9b1c3d4e5f6a 100644
--- a/drivers/mtd/nand/raw/ingenic/jz4725b_bch.c
+++ b/drivers/mtd/nand/raw/ingenic/jz4725b_bch.c
@@ -259,6 +259,10 @@ static int jz4725b_correct(struct ingenic_ecc *bch,
 			bit = (reg & BCH_BHERR_INDEX0_MASK) >> BCH_BHERR_INDEX0_SHIFT;
 		}
 
+		/* Skip indexes outside the data area. */
+		if (bit >= params->size * 8)
+			continue;
+
 		buf[(bit >> 3)] ^= BIT(bit & 0x7);
 	}
 
-- 
2.50.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 2/2] mtd: rawnand: ingenic: validate BCH error index in jz4780_correct()

[Guanglei Zhu]

From: Weibin Liu <liuwb@xiaopeng.com>

The JZ4780 BCH controller reports each corrected bit error as a byte
pair index, in the INDEX field of the BCH_BHERR registers. The field
is 11 bits wide, but nothing in jz4780_correct() checks the reported
index against the size of the data buffer being corrected:

    buf[(index * 2) + 0] ^= mask;
    buf[(index * 2) + 1] ^= mask >> 8;

With data or ECC coming from an untrusted flash, the decoder can
report an index that points past the end of the data buffer, and the
above statements then flip bits outside the buffer.

Skip the correction when the reported index does not point into
the data area.  A bitflip in the parity bytes does not affect the
data, so rejecting the page for it would fail reads that are
perfectly correctable, and an index beyond the codeword has no
buffer to update in the first place.

Fixes: ae02ab00aa3c ("mtd: nand: jz4780: driver for NAND devices on JZ4780 SoCs")
Signed-off-by: Weibin Liu <liuwb@xiaopeng.com>
---
Changes in v2: skip the correction for an index that does not point
into the data area instead of failing the page with -EBADMSG.  A
correctable bitflip in the ECC bytes leaves the data valid, and the
early exit also dropped the error entries that followed it.  This
matches how jz4740_ecc_correct() treats its indexes.

Verified with a userspace replica of the correction loop: with
params->size = 512, an index of 256 (first parity word) or at the
field maximum (0x7ff) produces no write outside the data buffer,
and in-range indexes keep correcting as before.

 drivers/mtd/nand/raw/ingenic/jz4780_bch.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mtd/nand/raw/ingenic/jz4780_bch.c b/drivers/mtd/nand/raw/ingenic/jz4780_bch.c
index 12b5b0484fe9..3a4c5d6e7f8a 100644
--- a/drivers/mtd/nand/raw/ingenic/jz4780_bch.c
+++ b/drivers/mtd/nand/raw/ingenic/jz4780_bch.c
@@ -216,6 +216,10 @@ static int jz4780_correct(struct ingenic_ecc *bch,
 						BCH_BHERR_MASK_SHIFT;
 			index = (reg & BCH_BHERR_INDEX_MASK) >>
 						BCH_BHERR_INDEX_SHIFT;
+			/* Skip indexes outside the data area. */
+			if ((index * 2) + 1 >= params->size)
+				continue;
+
 			buf[(index * 2) + 0] ^= mask;
 			buf[(index * 2) + 1] ^= mask >> 8;
 		}
-- 
2.50.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/