[PATCH] mtd: spi_nor: Fixes out of bound shift

[Tudor Ambarus]

Hi!

On 26.01.2023 16:26, lrannou at baylibre.com wrote:
> From: Louis Rannou <lrannou at baylibre.com>
> 
> spi_nor_set_erase_type is called twice in sfdp.c with a null size. The
> return from ffs is 0 as well and the shift size becomes (2^32 - 1) which is
> out of bound when applied to the << operator.
> 
> This considers as illegal a call to this function with null size. It
> creates a replacement spi_nor_mask_erase_type for explicit calls to mask
> the erase type.
> 
> Signed-off-by: Louis Rannou <lrannou at baylibre.com>
> ---
>   drivers/mtd/spi-nor/core.c | 19 +++++++++++++++++--
>   drivers/mtd/spi-nor/sfdp.c |  4 ++--
>   2 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index d67c926bca8b..140a8fa81f03 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2015,15 +2015,30 @@ spi_nor_spimem_adjust_hwcaps(struct spi_nor *nor, u32 *hwcaps)
>    * @erase:	pointer to a structure that describes a SPI NOR erase type
>    * @size:	the size of the sector/block erased by the erase type
>    * @opcode:	the SPI command op code to erase the sector/block
> + *
> + * Return: 0 on success, -errno otherwise
>    */
> -void spi_nor_set_erase_type(struct spi_nor_erase_type *erase, u32 size,
> -			    u8 opcode)
> +int spi_nor_set_erase_type(struct spi_nor_erase_type *erase, u32 size,
> +			   u8 opcode)
>   {
>   	erase->size = size;
>   	erase->opcode = opcode;
>   	/* JEDEC JESD216B Standard imposes erase sizes to be power of 2. */
> +	if (size == 0)
> +		return -EINVAL;

we should check the return value on each call then.

>   	erase->size_shift = ffs(erase->size) - 1;
>   	erase->size_mask = (1 << erase->size_shift) - 1;
> +	return 0;
> +}
> +
> +/**
> + * spi_nor_mask_erase_type() - mask out a SPI NOR erase type
> + * @erase:	pointer to a structure that describes a SPI NOR erase type
> + */
> +void spi_nor_mask_erase_type(struct spi_nor_erase_type *erase)
> +{
> +	erase->size = 0;

but I like the idea of introducing a mask out method.

I sent a slightly different version where I added your Suggested-by tag
for the mask out idea. Let me know if it's fine by you:
https://lore.kernel.org/linux-mtd/20230202191451.35142-1-tudor.ambarus@linaro.org/T/#u

Cheers,
ta
> +	erase->opcode = 0xFF;
>   }
>   
>   /**
> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index 8434f654eca1..8d158243fe49 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -875,7 +875,7 @@ static int spi_nor_init_non_uniform_erase_map(struct spi_nor *nor,
>   	 */
>   	for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++)
>   		if (!(regions_erase_type & BIT(erase[i].idx)))
> -			spi_nor_set_erase_type(&erase[i], 0, 0xFF);
> +			spi_nor_mask_erase_type(&erase[i]);
>   
>   	return 0;
>   }
> @@ -1089,7 +1089,7 @@ static int spi_nor_parse_4bait(struct spi_nor *nor,
>   			erase_type[i].opcode = (dwords[1] >>
>   						erase_type[i].idx * 8) & 0xFF;
>   		else
> -			spi_nor_set_erase_type(&erase_type[i], 0u, 0xFF);
> +			spi_nor_mask_erase_type(&erase_type[i]);
>   	}
>   
>   	/*