Re: [PATCH] mtd: rawnand: validate ONFI extended parameter page sections

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Pengpeng,

On 15/07/2026 at 16:42:26 +08, Pengpeng Hou <pengpeng@iscas.ac.cn> wrote:

> nand_flash_detect_ext_param_page() allocates a length declared by the ONFI
> parameter page.  It treats the returned data as a fixed header followed by
> variable-length sections.  It reads that header and advances over sections
> without first proving that the page and each current section fit in the
> allocated buffer.
>
> Reject pages shorter than the fixed header.  Track remaining bytes while
> walking sections.  Require the ECC section to cover every field read from
> struct onfi_ext_ecc_info.
>
> Fixes: 6dcbe0cdd83f ("mtd: get the ECC info from the Extended Parameter Page")
> Cc: stable@vger.kernel.org
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
>  drivers/mtd/nand/raw/nand_onfi.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/nand_onfi.c b/drivers/mtd/nand/raw/nand_onfi.c
> index cd3ad373883e..40b7db10bd73 100644
> --- a/drivers/mtd/nand/raw/nand_onfi.c
> +++ b/drivers/mtd/nand/raw/nand_onfi.c
> @@ -40,11 +40,16 @@ static int nand_flash_detect_ext_param_page(struct nand_chip *chip,
>  	struct onfi_ext_section *s;
>  	struct onfi_ext_ecc_info *ecc;
>  	uint8_t *cursor;
> +	size_t section_len;
> +	size_t remaining;
>  	int ret;
>  	int len;
>  	int i;
>  
>  	len = le16_to_cpu(p->ext_param_page_length) * 16;
> +	if (len < sizeof(*ep))
> +		return -EINVAL;
> +
>  	ep = kmalloc(len, GFP_KERNEL);
>  	if (!ep)
>  		return -ENOMEM;
> @@ -77,11 +82,24 @@ static int nand_flash_detect_ext_param_page(struct nand_chip *chip,
>  
>  	/* find the ECC section. */
>  	cursor = (uint8_t *)(ep + 1);
> +	remaining = len - sizeof(*ep);
>  	for (i = 0; i < ONFI_EXT_SECTION_MAX; i++) {
>  		s = ep->sections + i;
> -		if (s->type == ONFI_SECTION_TYPE_2)
> +		section_len = s->length * 16;
> +		if (section_len > remaining) {
> +			pr_debug("The section is invalid.\n");

This debug message is not useful, it could mention the section, and at
least contain "ONFI" in it. But since you have access to a nand_chip, it
is also probably better to use dev_dbg here.

> +			goto ext_out;
> +		}
> +
> +		if (s->type == ONFI_SECTION_TYPE_2) {
> +			if (section_len < sizeof(*ecc)) {
> +				pr_debug("The ECC section is
> invalid.\n");

Same.

> +				goto ext_out;
> +			}
>  			break;
> -		cursor += s->length * 16;
> +		}

\n

> +		cursor += section_len;
> +		remaining -= section_len;
>  	}
>  	if (i == ONFI_EXT_SECTION_MAX) {
>  		pr_debug("We can not find the ECC section.\n");

Chances are this will never happen, because it is hardware hard to
tamper with, covered by an initial CRC, and if the manufacturers mess up
so badly their tables the chips won't be manufactured anyway.

But, heh, the cost of this is tolerable. Nevertheless, a v2 is expected.

Thanks,
Miquèl

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