Re: [PATCH v8 17/17] mtd: rawnand: sunxi: support the Allwinner randomized OOB format

[sashiko-bot]

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@xxxxxxxxx>

    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;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-submit-sunxi-nand-vendor-oob-layout-v1-v8-0-8ba4881fbee8@xxxxxxxxx?part=17