Re: [PATCH] stm32mp: fix various array bounds checks
On 3/24/23 10:39, Patrice CHOTARD wrote:
> Hi Rasmus
>
> On 3/24/23 08:55, Rasmus Villemoes wrote:
>> In all these cases, the index on the LHS is immediately afterwards
>> used to access the array appearing in the ARRAY_SIZE() on the RHS - so
>> if that index is equal to the array size, we'll access
>> one-past-the-end of the array.
>>
>> Signed-off-by: Rasmus Villemoes
>> ---
>>
>> Patch generated with '-U5' to make that access obvious from the diff context.
>>
>> arch/arm/mach-stm32mp/cpu.c | 4 ++--
>> board/st/stm32mp1/stm32mp1.c| 2 +-
>> drivers/ram/stm32mp1/stm32mp1_interactive.c | 2 +-
>> 3 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/mach-stm32mp/cpu.c b/arch/arm/mach-stm32mp/cpu.c
>> index dc4112d5e6..e2f67fc423 100644
>> --- a/arch/arm/mach-stm32mp/cpu.c
>> +++ b/arch/arm/mach-stm32mp/cpu.c
>> @@ -188,11 +188,11 @@ static void setup_boot_mode(void)
>>
>> log_debug("%s: boot_ctx=0x%x => boot_mode=%x, instance=%d forced=%x\n",
>>__func__, boot_ctx, boot_mode, instance, forced_mode);
>> switch (boot_mode & TAMP_BOOT_DEVICE_MASK) {
>> case BOOT_SERIAL_UART:
>> -if (instance > ARRAY_SIZE(serial_addr))
>> +if (instance >= ARRAY_SIZE(serial_addr))
>> break;
>> /* serial : search associated node in devicetree */
>> sprintf(cmd, "serial@%x", serial_addr[instance]);
>> if (uclass_get_device_by_name(UCLASS_SERIAL, cmd, )) {
>> /* restore console on error */
>> @@ -218,11 +218,11 @@ static void setup_boot_mode(void)
>> env_set("boot_device", "usb");
>> env_set("boot_instance", "0");
>> break;
>> case BOOT_FLASH_SD:
>> case BOOT_FLASH_EMMC:
>> -if (instance > ARRAY_SIZE(sdmmc_addr))
>> +if (instance >= ARRAY_SIZE(sdmmc_addr))
>> break;
>> /* search associated sdmmc node in devicetree */
>> sprintf(cmd, "mmc@%x", sdmmc_addr[instance]);
>> if (uclass_get_device_by_name(UCLASS_MMC, cmd, )) {
>> printf("mmc%d = %s not found in device tree!\n",
>> diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c
>> index ca8f0255ae..1a1b1844c8 100644
>> --- a/board/st/stm32mp1/stm32mp1.c
>> +++ b/board/st/stm32mp1/stm32mp1.c
>> @@ -870,11 +870,11 @@ int mmc_get_boot(void)
>> STM32_SDMMC1_BASE,
>> STM32_SDMMC2_BASE,
>> STM32_SDMMC3_BASE
>> };
>>
>> -if (instance > ARRAY_SIZE(sdmmc_addr))
>> +if (instance >= ARRAY_SIZE(sdmmc_addr))
>> return 0;
>>
>> /* search associated sdmmc node in devicetree */
>> snprintf(cmd, sizeof(cmd), "mmc@%x", sdmmc_addr[instance]);
>> if (uclass_get_device_by_name(UCLASS_MMC, cmd, )) {
>> diff --git a/drivers/ram/stm32mp1/stm32mp1_interactive.c
>> b/drivers/ram/stm32mp1/stm32mp1_interactive.c
>> index f0fe7e61e3..2c19847c66 100644
>> --- a/drivers/ram/stm32mp1/stm32mp1_interactive.c
>> +++ b/drivers/ram/stm32mp1/stm32mp1_interactive.c
>> @@ -389,11 +389,11 @@ bool stm32mp1_ddr_interactive(void *priv,
>> log_debug("** step %d ** %s / %d\n", step, step_str[step], next_step);
>>
>> if (next_step < 0)
>> return false;
>>
>> -if (step < 0 || step > ARRAY_SIZE(step_str)) {
>> +if (step < 0 || step >= ARRAY_SIZE(step_str)) {
>> printf("** step %d ** INVALID\n", step);
>> return false;
>> }
>>
>> printf("%d:%s\n", step, step_str[step]);
>
> Good catch !
>
> Reviewed-by: Patrice Chotard
>
> Thanks
> Patrice
Applied on u-boot-stm/master, thanks
Patrice
Re: [PATCH] stm32mp: fix various array bounds checks
Hi, On 3/24/23 08:55, Rasmus Villemoes wrote: In all these cases, the index on the LHS is immediately afterwards used to access the array appearing in the ARRAY_SIZE() on the RHS - so if that index is equal to the array size, we'll access one-past-the-end of the array. Signed-off-by: Rasmus Villemoes --- Patch generated with '-U5' to make that access obvious from the diff context. arch/arm/mach-stm32mp/cpu.c | 4 ++-- board/st/stm32mp1/stm32mp1.c| 2 +- drivers/ram/stm32mp1/stm32mp1_interactive.c | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) Reviewed-by: Patrick Delaunay Thanks Patrick
Re: [PATCH] stm32mp: fix various array bounds checks
Hi Rasmus
On 3/24/23 08:55, Rasmus Villemoes wrote:
> In all these cases, the index on the LHS is immediately afterwards
> used to access the array appearing in the ARRAY_SIZE() on the RHS - so
> if that index is equal to the array size, we'll access
> one-past-the-end of the array.
>
> Signed-off-by: Rasmus Villemoes
> ---
>
> Patch generated with '-U5' to make that access obvious from the diff context.
>
> arch/arm/mach-stm32mp/cpu.c | 4 ++--
> board/st/stm32mp1/stm32mp1.c| 2 +-
> drivers/ram/stm32mp1/stm32mp1_interactive.c | 2 +-
> 3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/mach-stm32mp/cpu.c b/arch/arm/mach-stm32mp/cpu.c
> index dc4112d5e6..e2f67fc423 100644
> --- a/arch/arm/mach-stm32mp/cpu.c
> +++ b/arch/arm/mach-stm32mp/cpu.c
> @@ -188,11 +188,11 @@ static void setup_boot_mode(void)
>
> log_debug("%s: boot_ctx=0x%x => boot_mode=%x, instance=%d forced=%x\n",
> __func__, boot_ctx, boot_mode, instance, forced_mode);
> switch (boot_mode & TAMP_BOOT_DEVICE_MASK) {
> case BOOT_SERIAL_UART:
> - if (instance > ARRAY_SIZE(serial_addr))
> + if (instance >= ARRAY_SIZE(serial_addr))
> break;
> /* serial : search associated node in devicetree */
> sprintf(cmd, "serial@%x", serial_addr[instance]);
> if (uclass_get_device_by_name(UCLASS_SERIAL, cmd, )) {
> /* restore console on error */
> @@ -218,11 +218,11 @@ static void setup_boot_mode(void)
> env_set("boot_device", "usb");
> env_set("boot_instance", "0");
> break;
> case BOOT_FLASH_SD:
> case BOOT_FLASH_EMMC:
> - if (instance > ARRAY_SIZE(sdmmc_addr))
> + if (instance >= ARRAY_SIZE(sdmmc_addr))
> break;
> /* search associated sdmmc node in devicetree */
> sprintf(cmd, "mmc@%x", sdmmc_addr[instance]);
> if (uclass_get_device_by_name(UCLASS_MMC, cmd, )) {
> printf("mmc%d = %s not found in device tree!\n",
> diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c
> index ca8f0255ae..1a1b1844c8 100644
> --- a/board/st/stm32mp1/stm32mp1.c
> +++ b/board/st/stm32mp1/stm32mp1.c
> @@ -870,11 +870,11 @@ int mmc_get_boot(void)
> STM32_SDMMC1_BASE,
> STM32_SDMMC2_BASE,
> STM32_SDMMC3_BASE
> };
>
> - if (instance > ARRAY_SIZE(sdmmc_addr))
> + if (instance >= ARRAY_SIZE(sdmmc_addr))
> return 0;
>
> /* search associated sdmmc node in devicetree */
> snprintf(cmd, sizeof(cmd), "mmc@%x", sdmmc_addr[instance]);
> if (uclass_get_device_by_name(UCLASS_MMC, cmd, )) {
> diff --git a/drivers/ram/stm32mp1/stm32mp1_interactive.c
> b/drivers/ram/stm32mp1/stm32mp1_interactive.c
> index f0fe7e61e3..2c19847c66 100644
> --- a/drivers/ram/stm32mp1/stm32mp1_interactive.c
> +++ b/drivers/ram/stm32mp1/stm32mp1_interactive.c
> @@ -389,11 +389,11 @@ bool stm32mp1_ddr_interactive(void *priv,
> log_debug("** step %d ** %s / %d\n", step, step_str[step], next_step);
>
> if (next_step < 0)
> return false;
>
> - if (step < 0 || step > ARRAY_SIZE(step_str)) {
> + if (step < 0 || step >= ARRAY_SIZE(step_str)) {
> printf("** step %d ** INVALID\n", step);
> return false;
> }
>
> printf("%d:%s\n", step, step_str[step]);
Good catch !
Reviewed-by: Patrice Chotard
Thanks
Patrice
[PATCH] stm32mp: fix various array bounds checks
In all these cases, the index on the LHS is immediately afterwards
used to access the array appearing in the ARRAY_SIZE() on the RHS - so
if that index is equal to the array size, we'll access
one-past-the-end of the array.
Signed-off-by: Rasmus Villemoes
---
Patch generated with '-U5' to make that access obvious from the diff context.
arch/arm/mach-stm32mp/cpu.c | 4 ++--
board/st/stm32mp1/stm32mp1.c| 2 +-
drivers/ram/stm32mp1/stm32mp1_interactive.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/arm/mach-stm32mp/cpu.c b/arch/arm/mach-stm32mp/cpu.c
index dc4112d5e6..e2f67fc423 100644
--- a/arch/arm/mach-stm32mp/cpu.c
+++ b/arch/arm/mach-stm32mp/cpu.c
@@ -188,11 +188,11 @@ static void setup_boot_mode(void)
log_debug("%s: boot_ctx=0x%x => boot_mode=%x, instance=%d forced=%x\n",
__func__, boot_ctx, boot_mode, instance, forced_mode);
switch (boot_mode & TAMP_BOOT_DEVICE_MASK) {
case BOOT_SERIAL_UART:
- if (instance > ARRAY_SIZE(serial_addr))
+ if (instance >= ARRAY_SIZE(serial_addr))
break;
/* serial : search associated node in devicetree */
sprintf(cmd, "serial@%x", serial_addr[instance]);
if (uclass_get_device_by_name(UCLASS_SERIAL, cmd, )) {
/* restore console on error */
@@ -218,11 +218,11 @@ static void setup_boot_mode(void)
env_set("boot_device", "usb");
env_set("boot_instance", "0");
break;
case BOOT_FLASH_SD:
case BOOT_FLASH_EMMC:
- if (instance > ARRAY_SIZE(sdmmc_addr))
+ if (instance >= ARRAY_SIZE(sdmmc_addr))
break;
/* search associated sdmmc node in devicetree */
sprintf(cmd, "mmc@%x", sdmmc_addr[instance]);
if (uclass_get_device_by_name(UCLASS_MMC, cmd, )) {
printf("mmc%d = %s not found in device tree!\n",
diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c
index ca8f0255ae..1a1b1844c8 100644
--- a/board/st/stm32mp1/stm32mp1.c
+++ b/board/st/stm32mp1/stm32mp1.c
@@ -870,11 +870,11 @@ int mmc_get_boot(void)
STM32_SDMMC1_BASE,
STM32_SDMMC2_BASE,
STM32_SDMMC3_BASE
};
- if (instance > ARRAY_SIZE(sdmmc_addr))
+ if (instance >= ARRAY_SIZE(sdmmc_addr))
return 0;
/* search associated sdmmc node in devicetree */
snprintf(cmd, sizeof(cmd), "mmc@%x", sdmmc_addr[instance]);
if (uclass_get_device_by_name(UCLASS_MMC, cmd, )) {
diff --git a/drivers/ram/stm32mp1/stm32mp1_interactive.c
b/drivers/ram/stm32mp1/stm32mp1_interactive.c
index f0fe7e61e3..2c19847c66 100644
--- a/drivers/ram/stm32mp1/stm32mp1_interactive.c
+++ b/drivers/ram/stm32mp1/stm32mp1_interactive.c
@@ -389,11 +389,11 @@ bool stm32mp1_ddr_interactive(void *priv,
log_debug("** step %d ** %s / %d\n", step, step_str[step], next_step);
if (next_step < 0)
return false;
- if (step < 0 || step > ARRAY_SIZE(step_str)) {
+ if (step < 0 || step >= ARRAY_SIZE(step_str)) {
printf("** step %d ** INVALID\n", step);
return false;
}
printf("%d:%s\n", step, step_str[step]);
--
2.37.2
