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

[Pali Rohár]

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.

Please let me know.

> > It would be really a good idea if you try to check if any of those
> > proposed fixes/patches are now really correct.
> 
> Yes, please do.
> 
> Thanks,
> Stefan