Re: [PATCH v3 11/15] mtd: rawnand: sunxi: select the packed H6/H616 OOB layout

[sashiko-bot@kernel.org]

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.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-submit-sunxi-nand-vendor-oob-layout-v1-v3-0-838cb0ba1547@gmail.com?part=11