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

[Stefan Roese]

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=findComment&comment=138136
> 
> 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 at seaofdirac.org>
> Signed-off-by: Pali Rohár <pali at 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

-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de