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

[Louis Rannou]

On 02/02/2023 20:18, Tudor Ambarus wrote:
> 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

Of course it's fine for me. I also agree to the v3.

Thanks !
Louis

> 
> 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]);
>>       }
>>       /*