Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mtd: rawnand: sunxi: Missing bounds check on ecc->steps against max_ecc_steps
--
commit a2ae1cf95bb52a1850e6dc239c873b74d175805e
Author: James Hilliard <james.hilliard1@gmail.com>
mtd: rawnand: sunxi: support the Allwinner randomized OOB format
Normal hardware-ECC reads also decode protected OOB, including the
bad-block marker and metadata used in bad-block-table scans. Allwinner
firmware leaves the marker in the randomizer stream, but the existing
driver compensates it to keep a plain physical marker. On H6/H616,
firmware also packs protected user data before the first parity region
rather than maximizing its allocation across the page. Those mismatches
prevent normal ECC reads from interpreting firmware-formatted pages and
their markers.
> diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
> index 56594bb411933..0e2ff1c59120f 100644
> --- a/drivers/mtd/nand/raw/sunxi_nand.c
> +++ b/drivers/mtd/nand/raw/sunxi_nand.c
[ ... ]
> @@ -2342,6 +2353,299 @@ static const struct sunxi_nfc_oob_ops sunxi_nfc_scrambled_plain_bbm_oob_ops = {
> .finish_read = sunxi_nfc_oob_finish_read,
> };
>
> +/* Allwinner randomized-OOB format, selected only by allwinner,randomized-oob. */
> +#define SUNXI_NFC_H6_ALLWINNER_OOB_MAX_USER_DATA_SZ 16
> +
> +static void sunxi_nfc_randomize_buf(u16 state, u8 *buf, unsigned int len)
> +{
> + while (len--) {
> + *buf++ ^= state;
> + state = sunxi_nfc_randomizer_step(state, 8);
> + }
> +}
> +
> +static void
> +sunxi_nfc_allwinner_oob_record_status(struct nand_chip *nand,
> + struct sunxi_nfc_allwinner_oob_status *result,
> + int logical_step, int hw_step,
> + struct sunxi_nfc_ecc_snapshot *snapshot)
> +{
> + u32 count;
> +
> + if ((snapshot->pattern_found & BIT(hw_step)) &&
> + !(snapshot->pattern_id & BIT(hw_step)))
> + result->zero_steps |= BIT(logical_step);
[Severity: High]
Does this code invoke undefined behavior if logical_step exceeds the bit-width?
The number of ECC steps is derived directly from the NAND page size in
sunxi_nand_hw_ecc_ctrl_init():
nsectors = mtd->writesize / ecc->size;
ret = sunxi_nand->oob_ops->init_user_data(nand, nsectors);
It doesn't appear that nsectors is validated against the controller's
max_ecc_steps limit. If a NAND flash with a large page size is used,
ecc->steps (and thus logical_step) can exceed the hardware limit (e.g.,
16 or 32). On 32-bit platforms, this could cause the BIT(logical_step)
shift to overflow.
Additionally, this could lead to out-of-bounds MMIO reads in
sunxi_nfc_hw_ecc_get_prot_oob_bytes() when iterating over ecc->steps,
as reg_index might exceed the supported hardware registers for user data.
[ ... ]
> +static int sunxi_nfc_allwinner_oob_finish_read(struct nand_chip *nand,
> + struct sunxi_nfc_ecc_read *read,
> + int *cur_off)
> +{
> + struct sunxi_nfc_allwinner_oob_status *result = &read->allwinner_oob;
> + u8 *buf = read->buf;
> + bool oob_required = read->oob_required;
> + bool dma = read->dma;
> + int page = read->page;
> + struct mtd_info *mtd = nand_to_mtd(nand);
> + struct nand_ecc_ctrl *ecc = &nand->ecc;
> + int ret;
> +
> + /*
> + * Allwinner firmware treats an all-zero physical page as bad, even without
> + * ECC errors. This takes precedence over the spare-byte heuristic.
> + */
> + if (result->zero_steps == GENMASK(ecc->steps - 1, 0)) {
[Severity: High]
Can GENMASK() invoke undefined behavior here if ecc->steps exceeds the
architecture's bit-width?
Similar to the BIT() shift above, since ecc->steps isn't bounded by
max_ecc_steps, a large page size could cause this range to exceed 32 bits,
leading to an overflow.
> + memset(buf, 0, mtd->writesize);
> + memset(nand->oob_poi, 0, mtd->oobsize);
> + mtd->ecc_stats.failed += ecc->steps;
> + return 0;
> + }
@@ -349,6 +349,8 @@ struct sunxi_nfc_mdma_desc {
* @nuser_data_tab: Size of @user_data_len_tab
* @sram_size: Size of the NAND controller SRAM
* @timings: Controller timing characteristics
+ * @spare_is_erased: Allwinner randomized-OOB erased-page check on
+ * re-encoded protected OOB bytes
*/
struct sunxi_nfc_caps {
bool has_mdma;
@@ -377,6 +379,7 @@ struct sunxi_nfc_caps {
unsigned int max_ecc_steps;
int sram_size;
const struct sunxi_nfc_timings *timings;
+ bool (*spare_is_erased)(struct nand_chip *nand, const u8 *spare, int page);
};
/**
@@ -1160,7 +1163,15 @@ static int sunxi_nfc_hw_ecc_read_error(struct nand_chip *nand,
return ret >= 0;
}
+struct sunxi_nfc_allwinner_oob_status {
+ u32 error_steps;
+ u32 zero_steps;
+ unsigned int corrected;
+ unsigned int max_bitflips;
+};
+
struct sunxi_nfc_ecc_read {
+ struct sunxi_nfc_allwinner_oob_status allwinner_oob;
u8 *buf;
int page;
unsigned int max_bitflips;
@@ -2342,6 +2353,299 @@ static const struct sunxi_nfc_oob_ops sunxi_nfc_scrambled_plain_bbm_oob_ops = {
.finish_read = sunxi_nfc_oob_finish_read,
};
+/* Allwinner randomized-OOB format, selected only by allwinner,randomized-oob. */
+#define SUNXI_NFC_H6_ALLWINNER_OOB_MAX_USER_DATA_SZ 16
+
+static void sunxi_nfc_randomize_buf(u16 state, u8 *buf, unsigned int len)
+{
+ while (len--) {
+ *buf++ ^= state;
+ state = sunxi_nfc_randomizer_step(state, 8);
+ }
+}
+
+static void
+sunxi_nfc_allwinner_oob_record_status(struct nand_chip *nand,
+ struct sunxi_nfc_allwinner_oob_status *result,
+ int logical_step, int hw_step,
+ struct sunxi_nfc_ecc_snapshot *snapshot)
+{
+ u32 count;
+
+ if ((snapshot->pattern_found & BIT(hw_step)) &&
+ !(snapshot->pattern_id & BIT(hw_step)))
+ result->zero_steps |= BIT(logical_step);
+
+ /*
+ * Exception mode can suppress the ECC error for a physical pattern.
+ * Classify these steps with the Allwinner randomized-OOB page check too,
+ * and do not use their correction counters. Keep their decoded data until
+ * then.
+ */
+ if ((snapshot->status & NFC_ECC_ERR(hw_step)) ||
+ (snapshot->pattern_found & BIT(hw_step))) {
+ result->error_steps |= BIT(logical_step);
+ return;
+ }
+
+ count = sunxi_nfc_hw_ecc_read_count(nand, snapshot, hw_step);
+ result->corrected += count;
+ result->max_bitflips = max(result->max_bitflips, count);
+}
+
+static bool sun4i_a10_nfc_spare_is_erased(struct nand_chip *nand,
+ const u8 *spare, int page)
+{
+ struct mtd_info *mtd = nand_to_mtd(nand);
+ unsigned int block_page = page % mtd_div_by_ws(mtd->erasesize, mtd);
+
+ /*
+ * Older Allwinner firmware recognizes exact four-byte erased
+ * signatures on page zero and pages 127 modulo 128. With their
+ * 1 KiB ECC steps these correspond to four physical 0xff bytes.
+ * Other pages require all eight spare bytes to be 0xff.
+ */
+ if (nand->ecc.size == 1024 &&
+ (!block_page || block_page % 128 == 127) &&
+ !memchr_inv(spare, 0xff, USER_DATA_SZ))
+ return true;
+
+ return !memchr_inv(spare, 0xff, 8);
+}
+
+static bool sun50i_h6_nfc_spare_is_erased(struct nand_chip *nand,
+ const u8 *spare, int page)
+{
+ unsigned int erased = 0;
+ int i;
+
+ /* Byte zero and at least nine of ten bytes must be 0xff. */
+ for (i = 0; i < 10; i++)
+ erased += spare[i] == 0xff;
+
+ return spare[0] == 0xff && erased >= 9;
+}
+
+static bool sunxi_nfc_allwinner_oob_spare_is_erased(struct nand_chip *nand,
+ int page)
+{
+ struct sunxi_nand_chip *sunxi_nand = to_sunxi_nand(nand);
+ struct sunxi_nfc *nfc = to_sunxi_nfc(nand->controller);
+ struct nand_ecc_ctrl *ecc = &nand->ecc;
+ u16 state = sunxi_nfc_randomizer_state(nand, page, true);
+ u8 spare[10];
+ unsigned int len, pos = 0;
+ int i, off;
+
+ /*
+ * Re-encode ECC-decoded protected OOB for the firmware's spare check.
+ * This is not a raw reread: ECC-corrected bytes stay corrected. Each
+ * step restarts the OOB randomizer; H6/H616 pack the entire prefix in
+ * step zero. Pad unavailable bytes with 0xff.
+ */
+ memset(spare, 0xff, sizeof(spare));
+ for (i = 0; i < ecc->steps && pos < sizeof(spare); i++) {
+ len = min_t(unsigned int, sunxi_nfc_user_data_sz(sunxi_nand, i),
+ sizeof(spare) - pos);
+ off = sunxi_get_oob_offset(sunxi_nand, ecc, i);
+ memcpy(spare + pos, nand->oob_poi + off, len);
+ sunxi_nfc_randomize_buf(state, spare + pos, len);
+ pos += len;
+ }
+
+ return nfc->caps->spare_is_erased(nand, spare, page);
+}
+
+static int sunxi_nfc_allwinner_oob_read_unprotected(struct nand_chip *nand,
+ bool dma, int page)
+{
+ struct sunxi_nand_chip *sunxi_nand = to_sunxi_nand(nand);
+ struct mtd_info *mtd = nand_to_mtd(nand);
+ struct nand_ecc_ctrl *ecc = &nand->ecc;
+ unsigned int len, off;
+ u16 state;
+ int ret, i;
+
+ for (i = 0; i < ecc->steps; i++) {
+ off = sunxi_get_ecc_offset(sunxi_nand, ecc, i);
+ len = ecc->bytes;
+ /* Keep decoded user data, but combine adjacent parity regions. */
+ while (i + 1 < ecc->steps &&
+ !sunxi_nfc_user_data_sz(sunxi_nand, i + 1)) {
+ len += ecc->bytes;
+ i++;
+ }
+ if (i + 1 == ecc->steps)
+ len = mtd->oobsize - off;
+
+ ret = sunxi_nfc_read_column(nand, page, mtd->writesize + off,
+ nand->oob_poi + off, len);
+ if (ret)
+ return ret;
+ }
+
+ /* Preserve each path's normal representation of ECC bytes. */
+ if (!dma) {
+ for (i = 0; i < ecc->steps; i++) {
+ len = sunxi_nfc_user_data_sz(sunxi_nand, i);
+ off = sunxi_get_ecc_offset(sunxi_nand, ecc, i);
+ state = sunxi_nfc_randomizer_state(nand, page, true);
+ state = sunxi_nfc_randomizer_step(state, len * 8 + 15);
+ sunxi_nfc_randomize_buf(state, nand->oob_poi + off,
+ ecc->bytes);
+ }
+ }
+
+ off = sunxi_get_oob_offset(sunxi_nand, ecc, ecc->steps);
+ len = mtd->oobsize - off;
+ if (len) {
+ /* The unprotected tail uses the page seed and its 15-bit advance. */
+ state = sunxi_nfc_randomizer_state(nand, page, false);
+ state = sunxi_nfc_randomizer_step(state, 15);
+ sunxi_nfc_randomize_buf(state, nand->oob_poi + off, len);
+ }
+
+ return 0;
+}
+
+static int sunxi_nfc_allwinner_oob_read_batch(struct nand_chip *nand,
+ struct sunxi_nfc_ecc_read *read,
+ struct sunxi_nfc_ecc_snapshot *snapshot,
+ int first_step, int nsteps)
+{
+ struct sunxi_nand_chip *sunxi_nand = to_sunxi_nand(nand);
+ struct sunxi_nfc *nfc = to_sunxi_nfc(nand->controller);
+ struct nand_ecc_ctrl *ecc = &nand->ecc;
+ unsigned int reg_index = 0, user_data_sz;
+ int i;
+
+ /*
+ * The normal snapshot skips pattern IDs when every pattern has an
+ * ECC error. Allwinner randomized-OOB page classification needs those IDs
+ * as well.
+ */
+ if (snapshot->pattern_found &&
+ !(snapshot->pattern_found & ~snapshot->status))
+ snapshot->pattern_id = readl(nfc->regs + NFC_REG_PAT_ID(nfc));
+
+ for (i = 0; i < nsteps; i++, reg_index += user_data_sz / 4) {
+ int step = first_step + i;
+ int oob_off = sunxi_get_oob_offset(sunxi_nand, ecc, step);
+ u8 *oob = nand->oob_poi + oob_off;
+
+ user_data_sz = sunxi_nfc_user_data_sz(sunxi_nand, step);
+ sunxi_nfc_allwinner_oob_record_status(nand, &read->allwinner_oob,
+ step, i, snapshot);
+ if (!read->dma)
+ memcpy_fromio(read->buf + step * ecc->size,
+ nfc->regs + NFC_RAM0_BASE, ecc->size);
+ /* Page classification needs protected OOB even for data-only reads. */
+ sunxi_nfc_hw_ecc_get_prot_oob_bytes(nand, oob, reg_index, !step,
+ read->page, user_data_sz);
+ }
+
+ return 0;
+}
+
+static int sunxi_nfc_allwinner_oob_finish_read(struct nand_chip *nand,
+ struct sunxi_nfc_ecc_read *read,
+ int *cur_off)
+{
+ struct sunxi_nfc_allwinner_oob_status *result = &read->allwinner_oob;
+ u8 *buf = read->buf;
+ bool oob_required = read->oob_required;
+ bool dma = read->dma;
+ int page = read->page;
+ struct mtd_info *mtd = nand_to_mtd(nand);
+ struct nand_ecc_ctrl *ecc = &nand->ecc;
+ int ret;
+
+ /*
+ * Allwinner firmware treats an all-zero physical page as bad, even without
+ * ECC errors. This takes precedence over the spare-byte heuristic.
+ */
+ if (result->zero_steps == GENMASK(ecc->steps - 1, 0)) {
+ memset(buf, 0, mtd->writesize);
+ memset(nand->oob_poi, 0, mtd->oobsize);
+ mtd->ecc_stats.failed += ecc->steps;
+ return 0;
+ }
+
+ if (result->error_steps && sunxi_nfc_allwinner_oob_spare_is_erased(nand, page)) {
+ memset(buf, 0xff, mtd->writesize);
+ memset(nand->oob_poi, 0xff, mtd->oobsize);
+ return 0;
+ }
+
+ /*
+ * Keep the original decoded main and protected OOB bytes on ECC failure.
+ * BBT pattern scans inspect them even when an ECC error is reported.
+ * Read the remaining OOB only when requested, not to classify the page.
+ */
+ if (oob_required) {
+ ret = sunxi_nfc_allwinner_oob_read_unprotected(nand, dma, page);
+ if (ret)
+ return ret;
+ }
+
+ mtd->ecc_stats.corrected += result->corrected;
+ mtd->ecc_stats.failed += hweight32(result->error_steps);
+
+ return result->max_bitflips;
+}
+
+static int sunxi_nfc_allwinner_oob_read_subpage(struct nand_chip *nand,
+ u32 data_offs, u32 readlen,
+ u8 *buf, int page)
+{
+ return nand->ecc.read_page(nand, buf, false, page);
+}
+
+static unsigned int sunxi_nfc_h6_allwinner_oob_user_data_sz(int nsectors)
+{
+ return min(nsectors * USER_DATA_SZ,
+ SUNXI_NFC_H6_ALLWINNER_OOB_MAX_USER_DATA_SZ);
+}
+
+static unsigned int sunxi_nfc_allwinner_oob_min_user_data(struct nand_chip *nand,
+ int nsteps)
+{
+ struct sunxi_nfc *nfc = to_sunxi_nfc(nand->controller);
+
+ if (!nfc->caps->reg_user_data_len)
+ return sunxi_nfc_oob_min_user_data(nand, nsteps);
+
+ return sunxi_nfc_h6_allwinner_oob_user_data_sz(nsteps);
+}
+
+static int sunxi_nfc_allwinner_oob_init_user_data(struct nand_chip *nand, int nsteps)
+{
+ struct sunxi_nand_chip *sunxi_nand = to_sunxi_nand(nand);
+ struct sunxi_nfc *nfc = to_sunxi_nfc(nand->controller);
+
+ /* Do not let the core fall back to software ECC for this format. */
+ if (nsteps <= 0)
+ return -EINVAL;
+
+ if (!nfc->caps->reg_user_data_len)
+ return 0;
+
+ sunxi_nand->user_data_bytes = devm_kzalloc(nfc->dev, nsteps, GFP_KERNEL);
+ if (!sunxi_nand->user_data_bytes)
+ return -ENOMEM;
+
+ /* H6/H616 pack all protected user data before step-zero parity. */
+ sunxi_nand->user_data_bytes[0] = sunxi_nfc_h6_allwinner_oob_user_data_sz(nsteps);
+ return 0;
+}
+
+static const struct sunxi_nfc_oob_ops sunxi_nfc_allwinner_oob_ops = {
+ .min_user_data = sunxi_nfc_allwinner_oob_min_user_data,
+ .init_user_data = sunxi_nfc_allwinner_oob_init_user_data,
+ .read_batch = sunxi_nfc_allwinner_oob_read_batch,
+ .finish_read = sunxi_nfc_allwinner_oob_finish_read,
+};
+
static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
struct nand_ecc_ctrl *ecc,
struct device_node *np)
@@ -2473,6 +2777,28 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
return 0;
}
+static int sunxi_nfc_allwinner_oob_attach_chip(struct nand_chip *nand,
+ struct device_node *np)
+{
+ struct sunxi_nand_chip *sunxi_nand = to_sunxi_nand(nand);
+ struct sunxi_nfc *nfc = to_sunxi_nfc(nand->controller);
+ int ret;
+
+ if (nand->ecc.engine_type != NAND_ECC_ENGINE_TYPE_ON_HOST)
+ return dev_err_probe(nfc->dev, -EINVAL,
+ "Allwinner OOB format requires controller ECC\n");
+
+ sunxi_nand->oob_ops = &sunxi_nfc_allwinner_oob_ops;
+ nand->options |= NAND_NEED_SCRAMBLING | NAND_NO_SUBPAGE_WRITE;
+ ret = sunxi_nand_hw_ecc_ctrl_init(nand, &nand->ecc, np);
+ if (ret)
+ return ret;
+
+ /* This format's spare and all-zero checks classify the whole page. */
+ nand->ecc.read_subpage = sunxi_nfc_allwinner_oob_read_subpage;
+ return 0;
+}
+
static int sunxi_nand_attach_chip(struct nand_chip *nand)
{
struct sunxi_nand_chip *sunxi_nand = to_sunxi_nand(nand);
@@ -2498,6 +2824,9 @@ static int sunxi_nand_attach_chip(struct nand_chip *nand)
if (!ecc->size || !ecc->strength)
return -EINVAL;
+ if (of_property_read_bool(np, "allwinner,randomized-oob"))
+ return sunxi_nfc_allwinner_oob_attach_chip(nand, np);
+
sunxi_nand->oob_ops = nand->options & NAND_NEED_SCRAMBLING ?
&sunxi_nfc_scrambled_plain_bbm_oob_ops : &sunxi_nfc_plain_oob_ops;
@@ -2995,6 +3324,7 @@ static const struct sunxi_nfc_caps sunxi_nfc_a10_caps = {
.max_ecc_steps = 16,
.sram_size = 1024,
.timings = &sun4i_a10_nfc_timings,
+ .spare_is_erased = sun4i_a10_nfc_spare_is_erased,
};
static const struct sunxi_nfc_caps sunxi_nfc_a23_caps = {
@@ -3018,6 +3348,7 @@ static const struct sunxi_nfc_caps sunxi_nfc_a23_caps = {
.max_ecc_steps = 16,
.sram_size = 1024,
.timings = &sun4i_a10_nfc_timings,
+ .spare_is_erased = sun4i_a10_nfc_spare_is_erased,
};
static const struct sunxi_nfc_caps sunxi_nfc_h616_caps = {
@@ -3044,6 +3375,7 @@ static const struct sunxi_nfc_caps sunxi_nfc_h616_caps = {
.max_ecc_steps = 32,
.sram_size = 8192,
.timings = &sun50i_h616_nfc_timings,
+ .spare_is_erased = sun50i_h6_nfc_spare_is_erased,
};
static const struct of_device_id sunxi_nfc_ids[] = {