[v3,11/15] mtd: rawnand: sunxi: select the packed H6/H616 OOB layout
Commit Message
The H6/H616 controller can configure a separate protected user-data
length for every ECC step. Mainline fills all space left after ECC with
user data. This changes the physical ECC offsets from those used by
Allwinner NAND firmware, so pages written by one layout cannot be
decoded with the other.
The allwinner,randomized-oob property selects the firmware's normal-page
format for the configured controller hardware-ECC geometry. Use four
protected user-data bytes per 1 KiB ECC step, cap the total at 16 bytes,
and assign the entire total to ECC step zero. Reserve the same total before
maximizing ECC strength so the selected strength leaves enough room.
Keep the existing maximized mainline user-data layout when the property
is absent. Older controllers have fixed four-byte user-data registers
whose placement already matches the firmware and need no additional
change.
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
drivers/mtd/nand/raw/sunxi_nand.c | 31 +++++++++++++++++++++++++------
1 file changed, 25 insertions(+), 6 deletions(-)
Comments
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [High] The free OOB region incorrectly overlaps with the ECC OOB region for ECC steps > 0, causing silent ECC data corruption.
- [High] A division by zero kernel panic occurs when maximizing ECC strength on a NAND flash with 512-byte pages.
- [High] Unbounded accumulation of user data lengths leads to out-of-bounds MMIO writes, clobbering adjacent controller configuration registers.
--
commit 576ae0cb150295224ce149147695feb0bb49c1cf
Author: James Hilliard <james.hilliard1@gmail.com>
mtd: rawnand: sunxi: select the packed H6/H616 OOB layout
This patch adds a randomized OOB layout for the H6/H616 controller,
configuring a separate protected user-data length for ECC step zero to match
the firmware format. It keeps the maximized user-data layout when the
property is absent.
> diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
> index 138a141edbe4d..bd239d150730b 100644
> --- a/drivers/mtd/nand/raw/sunxi_nand.c
> +++ b/drivers/mtd/nand/raw/sunxi_nand.c
[ ... ]
> @@ -2365,6 +2374,12 @@ static int sunxi_nfc_maximize_user_data(struct nand_chip *nand, uint32_t oobsize
> if (!sunxi_nand->user_data_bytes)
> return -ENOMEM;
>
> + if (sunxi_nand->randomized_oob) {
> + sunxi_nand->user_data_bytes[0] =
> + sunxi_nfc_h6_user_data_sz(nsectors);
> + return 0;
> + }
> +
> for (step = 0; (step < nsectors) && (remaining_bytes > 0); step++) {
> for (i = 0; i < c->nuser_data_tab; i++) {
> if (c->user_data_len_tab[i] > remaining_bytes)
> break;
> sunxi_nand->user_data_bytes[step] = c->user_data_len_tab[i];
> }
[Severity: High]
This is a pre-existing issue, but does this loop lack an aggregate bounds
check against the hardware MMIO limit?
It appears this greedily assigns up to 32 bytes of user data per ECC step.
For a large page NAND flash, such as an 8KB page with 8 steps, it could
allocate up to 256 bytes in total.
When this total is later written out in sunxi_nfc_hw_ecc_set_prot_oob_bytes()
via a writel loop:
sunxi_nand.c:sunxi_nfc_hw_ecc_set_prot_oob_bytes() {
...
for (i = 0; i < user_data_sz / 4; i++, ptr += 4) {
writel(sunxi_nfc_buf_to_user_data(ptr),
nfc->regs + NFC_REG_USER_DATA(nfc, reg_index + i));
}
...
}
Will this step outside the available USER_DATA register bank and clobber
adjacent controller configuration registers like SPARE_AREA or PAT_ID,
potentially causing controller malfunction or a kernel panic during probe?
[ ... ]
> @@ -2422,6 +2437,10 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
> bytes -= 2;
>
> bytes -= total_user_data_sz;
> + } else if (sunxi_nand->randomized_oob) {
> + total_user_data_sz =
> + sunxi_nfc_h6_user_data_sz(nsectors);
> + bytes -= total_user_data_sz;
> } else {
[Severity: High]
This isn't a bug introduced by this patch, but earlier in this function
when the NAND_ECC_MAXIMIZE_STRENGTH flag is set, the code hardcodes
ecc->size to 1024.
If this is run on a NAND device with a 512-byte page size, will nsectors
evaluate to 0?
If nsectors is 0, the later operation to calculate the remaining ECC bytes
performs a division by zero:
sunxi_nand.c:sunxi_nand_hw_ecc_ctrl_init() {
...
/*
* Once all user data has been subtracted, the rest can be used
* for ECC bytes
*/
bytes /= nsectors;
...
}
Will this crash the kernel on boot for 512-byte page devices?
[ ... ]
[Severity: High]
This is also a pre-existing issue, but does sunxi_nand_ooblayout_free()
incorrectly overlap the free OOB region with the ECC OOB region?
Looking at sunxi_nand_ooblayout_free() for sections > 0:
sunxi_nand.c:sunxi_nand_ooblayout_free() {
...
oobregion->offset = sunxi_get_ecc_offset(sunxi_nand, ecc, section);
oobregion->length = user_data_sz;
return 0;
}
Should this be using sunxi_get_oob_offset() instead? By setting the free
region offset to the ECC region offset, it seems that if a filesystem utilizing
OOB writes to the free OOB area, it will write directly into the ECC parity
bytes, silently destroying the parity data and causing uncorrectable read
errors.
@@ -224,6 +224,9 @@
#define USER_DATA_SZ 4
#define SUNXI_NFC_MAX_USER_DATA_SZ 32
+/* The randomized H6/H616 layout packs at most 16 bytes before ECC step 0. */
+#define SUNXI_NFC_H6_MAX_USER_DATA_SZ 16
+
/**
* struct sunxi_nand_chip_sel - stores information related to NAND Chip Select
*
@@ -2348,8 +2351,14 @@ static void sunxi_nand_detach_chip(struct nand_chip *nand)
sunxi_nand->user_data_bytes = NULL;
}
-static int sunxi_nfc_maximize_user_data(struct nand_chip *nand, uint32_t oobsize,
- int ecc_bytes, int nsectors)
+static unsigned int sunxi_nfc_h6_user_data_sz(int nsectors)
+{
+ return min(nsectors * USER_DATA_SZ,
+ SUNXI_NFC_H6_MAX_USER_DATA_SZ);
+}
+
+static int sunxi_nfc_init_user_data(struct nand_chip *nand, uint32_t oobsize,
+ int ecc_bytes, int nsectors)
{
struct sunxi_nand_chip *sunxi_nand = to_sunxi_nand(nand);
struct sunxi_nfc *nfc = to_sunxi_nfc(nand->controller);
@@ -2365,6 +2374,12 @@ static int sunxi_nfc_maximize_user_data(struct nand_chip *nand, uint32_t oobsize
if (!sunxi_nand->user_data_bytes)
return -ENOMEM;
+ if (sunxi_nand->randomized_oob) {
+ sunxi_nand->user_data_bytes[0] =
+ sunxi_nfc_h6_user_data_sz(nsectors);
+ return 0;
+ }
+
for (step = 0; (step < nsectors) && (remaining_bytes > 0); step++) {
for (i = 0; i < c->nuser_data_tab; i++) {
if (c->user_data_len_tab[i] > remaining_bytes)
@@ -2425,6 +2440,10 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
bytes -= 2;
bytes -= total_user_data_sz;
+ } else if (sunxi_nand->randomized_oob) {
+ total_user_data_sz =
+ sunxi_nfc_h6_user_data_sz(nsectors);
+ bytes -= total_user_data_sz;
} else {
/*
* User-data lengths are encoded in four-byte units. Reserve
@@ -2495,12 +2514,12 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
nsectors = mtd->writesize / ecc->size;
/*
- * The rationale for variable data length is to prioritize maximum ECC
- * strength, and then use the remaining space for user data.
+ * The default variable-length layout prioritizes maximum ECC strength,
+ * then uses the remaining space for user data.
*/
if (nfc->caps->reg_user_data_len) {
- ret = sunxi_nfc_maximize_user_data(nand, mtd->oobsize,
- ecc->bytes, nsectors);
+ ret = sunxi_nfc_init_user_data(nand, mtd->oobsize,
+ ecc->bytes, nsectors);
if (ret)
return ret;
}