Re: [PATCH 1/2] mtd: spi-nor: sfdp: check the length of the xSPI Profile 1.0 table

[Michael Walle]

On Sun Jul 19, 2026 at 3:08 AM CEST, HyeongJun An wrote:
> spi_nor_parse_profile1() sizes its bounce buffer from the length the
> flash reports in the SFDP parameter header, then indexes that buffer at
> fixed offsets:
>
>     len = profile1_header->length * sizeof(*dwords);
>     dwords = kmalloc(len, GFP_KERNEL);
>     ...
>     dummy = FIELD_GET(PROFILE1_DWORD5_DUMMY_166MHZ, dwords[SFDP_DWORD(5)]);
>
> The parser reads up to DWORD5, but never checks that the table is that
> long.  The length is a u8 the flash supplies, so a device advertising
> the table with a shorter length makes the read run past the allocation:
> with a length of one the buffer is four bytes and dwords[SFDP_DWORD(5)]
> reads sixteen bytes beyond it.  A length of zero is worse, as kmalloc()
> then returns ZERO_SIZE_PTR rather than an error and the first access
> dereferences it.
>
> The bytes read out of bounds are not just discarded either, they end up
> as the dummy cycle count programmed for 8D-8D-8D fast reads.
>
> SFDP tables that do not match what the flash actually implements are
> common enough that this file already carries fixup hooks for them, and
> malformed SFDP has caused memory-safety bugs here before.  See
> commit f0f0cfdc3a02 ("mtd: spi-nor: Fix shift-out-of-bounds in
> spi_nor_set_erase_type").
>
> Reject a table shorter than the highest DWORD the parser reads, the way
> spi_nor_parse_4bait() already does with SFDP_4BAIT_DWORD_MAX.  Failing
> an optional parameter table is not fatal: spi_nor_parse_sfdp() warns and
> carries on with the data gathered so far.

Please be more precise. Best would be to describe the issue in your
own words.

> Fixes: fb27f198971a ("mtd: spi-nor: sfdp: parse xSPI Profile 1.0 table")
> Cc: stable@xxxxxxxxxxxxxxx
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: HyeongJun An <sammiee5311@xxxxxxxxx>
> ---
>  drivers/mtd/spi-nor/sfdp.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index 4600983cb579..d3d85b5d1b4a 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -1175,6 +1175,7 @@ static int spi_nor_parse_4bait(struct spi_nor *nor,
>  #define PROFILE1_DWORD5_DUMMY_166MHZ		GENMASK(31, 27)
>  #define PROFILE1_DWORD5_DUMMY_133MHZ		GENMASK(21, 17)
>  #define PROFILE1_DWORD5_DUMMY_100MHZ		GENMASK(11, 7)
> +#define SFDP_PROFILE1_DWORD_MAX			5
>  
>  /**
>   * spi_nor_parse_profile1() - parse the xSPI Profile 1.0 table
> @@ -1192,6 +1193,9 @@ static int spi_nor_parse_profile1(struct spi_nor *nor,
>  	int ret;
>  	u8 dummy, opcode;
>  
> +	if (profile1_header->length < SFDP_PROFILE1_DWORD_MAX)
> +		return -EINVAL;
> +

I see that it's also use in the 4bait parsing, but I find it very
confusing to read. IMHO it should be SFDP_PROFILE1_DWORD_MIN.

-michael

>  	len = profile1_header->length * sizeof(*dwords);
>  	dwords = kmalloc(len, GFP_KERNEL);
>  	if (!dwords)

Attachment: signature.asc
Description: PGP signature