[PATCH 1/1] mvebu: fix end-of-array check

[Stefan Roese]

Hi Pali,

On 12/5/22 19:18, Pali Rohár wrote:
> On Monday 05 December 2022 12:42:44 Stefan Roese wrote:
>> Hi!
>>
>> On 12/4/22 11:39, Pali Rohár wrote:
>>> Hello!
>>>
>>> I would suggest to change description of the patch from
>>>
>>>     "mvebu: fix end-of-array check"
>>>
>>> to something like:
>>>
>>>     "arm: mvebu: Espressobin: fix end-of-array check in env"
>>>
>>> as it affects only Espressobin boards (not other mvebu).
>>
>> Yes, please update the commit subject here.
>>
>>> Stefan, please look below as this issue/fix is important.
>>
>> Yes.
>>
>>> On Wednesday 30 November 2022 13:33:40 Derek LaHousse wrote:
>>>> Properly seek the end of default_environment variables.
>>>>
>>>> The current algorithm overwrites from the second variable.  This
>>>> replacement finds the end of the array of strings.
>>>>
>>>> Stomped variables include "board", "soc", "loadaddr".  These can be
>>>> seen on a "env default -a" after patch, but they are not seen with a
>>>> version before the patch.
>>>
>>> This is a real issue which I introduced in the past. So it some fix for
>>> this issue should be pulled into the v2023.01 release.
>>
>> Understood.
>>
>>>> Signed-off-by: Derek LaHousse <derek at seaofdirac.org>
>>>> ---
>>>>    board/Marvell/mvebu_armada-37xx/board.c | 7 +++++--
>>>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/board/Marvell/mvebu_armada-37xx/board.c
>>>> b/board/Marvell/mvebu_armada-37xx/board.c
>>>> index c6ecc323bb..ac29ac5b95 100644
>>>> --- a/board/Marvell/mvebu_armada-37xx/board.c
>>>> +++ b/board/Marvell/mvebu_armada-37xx/board.c
>>>> @@ -100,8 +100,11 @@ int board_late_init(void)
>>>>    		return 0;
>>>>    	/* Find free buffer in default_environment[] for new variables
>>>> */
>>>> -	while (*ptr != '\0' && *(ptr+1) != '\0') ptr++;
>>>> -	ptr += 2;
>>>> +	if (*ptr != '\0') { // Defending against empty default env
>>>> +		while ((i = strlen(ptr)) != 0) {
>>>> +			ptr += i + 1;
>>>> +		}
>>>> +	}
>>>
>>> If I'm looking at the outer-if condition correctly then it is not
>>> needed. strlen("") returns zero and so inner-while loop stops
>>> immediately.
>>>
>>> My proposed fix for this issue was just changing correct while loop
>>> check to ensure that ptr is set after the _last_ variable.
>>>
>>> -	while (*ptr != '\0' && *(ptr+1) != '\0') ptr++;
>>> -	ptr += 2;
>>> +	while (*ptr != '\0' || *(ptr+1) != '\0') ptr++;
>>> +	ptr++;
>>>
>>> Both changes should be equivalent, but I'm not sure which one is more
>>> readable. The original issue was introduced really by non-readable
>>> code...
>>>
>>> Stefan, do you have a preference which one fix is better / more
>>> readable?
>>
>> I would prefer to get Pali's corrected version included. Could you
>> please prepare a v2 patch with this update and also with the added
>> or changed patch subject.
> 
> Originally this issue was reported half year ago on the armbian forum:
> https://forum.armbian.com/topic/19564-making-espressobin-v7-work-in-2022/?do=findComment&comment=138136
> 
> U-Boot "changed" variable name scriptaddr to criptaddr (without leading
> s) and broke booting.
> 
> Details are later in comments:
> https://forum.armbian.com/topic/19564-making-espressobin-v7-work-in-2022/?do=findComment&comment=154062
> https://forum.armbian.com/topic/19564-making-espressobin-v7-work-in-2022/?do=findComment&comment=154235
> 
> If you prefer my variant, I can prepare a v2 patch.

Yes, please do. That's easiest for me. I'll push this quickly then.

Thanks,
Stefan