Re: [PATCH v2 1/1] arm: mvebu: Espressobin: Fix default env variables

Hi Derek,

On 12/7/22 21:56, Derek LaHousse wrote:

Default env variables on Espressobin boards are broken since commit c4df0f6f315c ("arm: mvebu: Espressobin: Set default value for $fdtfile env variable") as well as the 'env default -a' command.

The algorithm to find free space in the default_environment[] array returns after the first env variable instead of the correct position of the last variable, where there is allocated free space.

This causes that U-Boot board_late_init() function to overwrite a portion of the default environment with $ethXaddr and $fdtfile variables immediately after the first env variable and so it is overwriting other variables.

This patch also adds an additional null byte to terminate the environment array.

But U-Boot board_late_init() function do not fill this nul byte explicitly. And because of that, U-Boot is later trying to interpret remaining buffer as a continuation of variable list. Normally buffer should be empty but due to the above issue, it contains garbage from remaining env variables.

For example 'env default -a' command results in damaging variable names. It was observed that scritaddr variable name was changed to criptaddr (without leading 's').

This bug was reported and discussed on the Armbian forum: https://forum.armbian.com/topic/19564-making-espressobin-v7-work-in-2022/?do...

Fix these issues in two steps:

1. Change code which finds free space for dynamic env variables in default_environment[] array by jumping to the end of the variable

list instead of jumping after the first defined variable. [By Derek]

2. Add code which appends terminating nul byte as indication of the end

of the env list, after the last nul term env string. [By Pali]

Fixes: c4df0f6f315c ("arm: mvebu: Espressobin: Set default value for $fdtfile env variable") Signed-off-by: Derek LaHousse derek@seaofdirac.org Signed-off-by: Pali Rohár pali@kernel.org

---

Changes in v2:

• Include Pali's end-of-array null
• Better tags
• More documentation of what is fixed

diff --git a/board/Marvell/mvebu_armada-37xx/board.c b/board/Marvell/mvebu_armada-37xx/board.c index c6ecc323bb99..44c72344e8be 100644 --- a/board/Marvell/mvebu_armada-37xx/board.c +++ b/board/Marvell/mvebu_armada-37xx/board.c @@ -99,9 +99,16 @@ int board_late_init(void) if (!of_machine_is_compatible("globalscale,espressobin")) return 0;

• /* Find free buffer in default_environment[] for new variables

*/

• while (*ptr != '\0' && *(ptr+1) != '\0') ptr++;
• ptr += 2;

• /*

• * Find free space for new variables in default_environment[]
  

array.

• * Free space is after the last variable, each variable is
  

termined

• * by nul byte and after the last variable is additional nul
  

byte.

• * Move ptr to the position where new variable can be filled.
  

• */
  

• while (*ptr != '\0') {

• do { ptr++; } while (*ptr != '\0');
  

• ptr++;
  

• }

Thanks for working on this so quickly. But as you can see above, there are multiple lines wrapped. Please use "git send-email" or some other means to send the patch correctly.

Thanks, Stefan

/* * Ensure that 'env default -a' does not erase permanent MAC addresses @@ -145,6 +152,13 @@ int board_late_init(void) strcpy(ptr, "fdtfile=marvell/armada-3720-espressobin- emmc.dtb"); else strcpy(ptr, "fdtfile=marvell/armada-3720- espressobin.dtb");

• ptr += strlen(ptr) + 1;
• /*

• * After the last variable (which is nul term string) append
  

another nul

• * byte which terminates the list. So everything after ptr is
  

ignored.

• */
  

• *ptr = '\0';

  return 0; }

Viele Grüße, Stefan Roese