From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 703AEC4332F for ; Mon, 12 Dec 2022 08:29:12 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id BC5E6853C9; Mon, 12 Dec 2022 09:29:09 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=denx.de Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=denx.de; s=phobos-20191101; t=1670833750; bh=H8cKYfJcLO7TA9xJ6iXzG3R9mTKcq8gCI7R27jyqr70=; h=Date:Subject:To:Cc:References:From:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=TSM3Fke7rb+0OiQJyEbw8+nlqONxEp+2nnRporXStyAxAB+NeYzos+JQEhS0v0pHr ps+nTfcU/QCtfGu4EUWKpXtQOvS3ChTLRpGSsb/LOLIT89Jy/OfGCA0qw1V+Blujpe oqbdb9Ybn4uIonLd9mhUSgXJzkP5S8HdNqdIj8EKjQahj+JGKRGpjqJfv5qo7KFBBw yIYu/fRX50NMebtf7HpVXYBCOp067MI6ARu3a6T/ct+m9pcVpTKybr3qRu0c25G0+9 32Eek7YwrSO/twokA7nGSQVU0gZoer10nE7eGj2gzks0yT6+/ac3R7AsyePoqGqioH 2F23OE8GEGI7A== Received: by phobos.denx.de (Postfix, from userid 109) id 22F7F853EE; Mon, 12 Dec 2022 09:29:08 +0100 (CET) Received: from mout-u-204.mailbox.org (mout-u-204.mailbox.org [IPv6:2001:67c:2050:101:465::204]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 1CCEE852E5 for ; Mon, 12 Dec 2022 09:29:05 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=denx.de Authentication-Results: phobos.denx.de; spf=fail smtp.mailfrom=sr@denx.de Received: from smtp102.mailbox.org (smtp102.mailbox.org [IPv6:2001:67c:2050:b231:465::102]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-384) server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by mout-u-204.mailbox.org (Postfix) with ESMTPS id 4NVvt44T7dz9sQc; Mon, 12 Dec 2022 09:29:00 +0100 (CET) Message-ID: Date: Mon, 12 Dec 2022 09:28:55 +0100 MIME-Version: 1.0 Subject: Re: [PATCH v3 1/1] arm: mvebu: Espressobin: Fix default env variables Content-Language: en-US To: Derek LaHousse , u-boot@lists.denx.de Cc: kostap@marvell.com, pali@kernel.org References: From: Stefan Roese In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: 4NVvt44T7dz9sQc X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean Hi Derek, On 12/9/22 23:04, 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 > Signed-off-by: Pali Rohár > --- > Changes in v3: > - End of line wrap fix > > 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++; > + } > > /* > * 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'; Still line-wrapped. I've fixed this by manually applying the changes. Applied to u-boot-marvell/master Thanks, Stefan