diff mbox series

mtd: spi_nor: Fixes out of bound shift

Message ID 20230126142612.1518046-1-lrannou@baylibre.com
State Superseded
Delegated to: Ambarus Tudor
Headers show
Series mtd: spi_nor: Fixes out of bound shift | expand

Commit Message

Louis Rannou Jan. 26, 2023, 2:26 p.m. UTC 
From: Louis Rannou <lrannou@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@baylibre.com>
---
 drivers/mtd/spi-nor/core.c | 19 +++++++++++++++++--
 drivers/mtd/spi-nor/sfdp.c |  4 ++--
 2 files changed, 19 insertions(+), 4 deletions(-)

Comments

Michael Walle Jan. 31, 2023, 8:17 a.m. UTC | #1 
Hi,

Am 2023-01-26 15:26, schrieb lrannou@baylibre.com:
> From: Louis Rannou <lrannou@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@baylibre.com>

There is also this thread with a pending patch:
https://lore.kernel.org/r/20211106075616.95401-1-tudor.ambarus@microchip.com/

-michael
Louis Rannou Jan. 31, 2023, 3:04 p.m. UTC | #2 
Hello,

On 31/01/2023 09:17, Michael Walle wrote:
> Hi,
> 
> Am 2023-01-26 15:26, schrieb lrannou@baylibre.com:
>> From: Louis Rannou <lrannou@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@baylibre.com>
> 
> There is also this thread with a pending patch:
> https://lore.kernel.org/r/20211106075616.95401-1-tudor.ambarus@microchip.com/

Indeed, I did not see that.

My patch also misses the core.h update. Let's see if we get news from 
this pending patch. And then perhaps we'll see if I push a v2.

Regards,
Louis
Tudor Ambarus Feb. 2, 2023, 7:18 p.m. UTC | #3 
Hi!

On 26.01.2023 16:26, lrannou@baylibre.com wrote:
> From: Louis Rannou <lrannou@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@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]);
>   	}
>   
>   	/*
Louis Rannou Feb. 3, 2023, 8:20 a.m. UTC | #4 
On 02/02/2023 20:18, Tudor Ambarus wrote:
> Hi!
> 
> On 26.01.2023 16:26, lrannou@baylibre.com wrote:
>> From: Louis Rannou <lrannou@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@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]);
>>       }
>>       /*
diff mbox series

Patch

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