From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tim Harvey Date: Tue, 6 Apr 2021 16:23:05 -0700 Subject: Locking down U-Boot env with ENV_WRITEABLE_LIST In-Reply-To: References: <541706f0-9baa-0c28-7b0a-122286120849@denx.de> <58235e91-8c21-1438-9de1-6eed4e77eeab@denx.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Tue, Apr 6, 2021 at 2:54 PM Sean Anderson wrote: > > > > On 4/6/21 4:10 PM, Marek Vasut wrote: > > On 4/6/21 9:52 PM, Tim Harvey wrote: > >> On Mon, Apr 5, 2021 at 10:36 AM Marek Vasut wrote: > >>> > >>> On 4/5/21 6:27 PM, Tim Harvey wrote: > >>>> On Sat, Apr 3, 2021 at 12:09 PM Marek Vasut wrote: > >>>>> > >>>>> On 4/3/21 6:43 PM, Tim Harvey wrote: > >>>>> > >>>>> Hi, > >>>>> > >>>>> [...] > >>>>> > >>>>>>>>> And those config options I had enabled in u-boot defconfig: > >>>>>>>>> > >>>>>>>>> CONFIG_CMD_ENV_CALLBACK=y > >>>>>>>>> CONFIG_CMD_ENV_FLAGS=y > >>>>>>>>> CONFIG_ENV_IS_NOWHERE=y > >>>>>>>>> CONFIG_ENV_IS_IN_MMC=y > >>>>>>>>> CONFIG_ENV_APPEND=y > >>>>>>>>> CONFIG_ENV_WRITEABLE_LIST=y > >>>>>>>>> CONFIG_ENV_ACCESS_IGNORE_FORCE=y > >>>>>>>> > >>>>>>>> Do you really define both ENV_IS_NOWHERE and ENV_IS_IN_MMC? From what > >>>>>>>> I see if you define ENV_IS_NOWHERE none of the others will be used. > >>>>>>> > >>>>>>> Yes, having two ENV drivers is mandatory. One provides the base env (the > >>>>>>> nowhere) and the other is used to import the filtered extras from > >>>>>>> external env. > >>>>>> > >>>>>> Enabling ENV_IS_NOWHERE does not work as you describe in my testing. > >>>>>> I'm testing this with imx8mm_venice_defconfig on U-Boot 2021.01 and as > >>>>>> soon as I define ENV_IS_NOWHERE then env_load is never called because > >>>>>> when env_relocate is called env is not valid yet so env_set_default is > >>>>>> called and env_load is 'never' called, thus mmc env is never loaded. > >>>>>> This is all from > >>>>>> https://elixir.bootlin.com/u-boot/v2021.01/source/env/common.c#L259 > >>>>> > >>>>> Maybe this [PATCH V3] env: Fix invalid env handling in env_init() patch > >>>>> is missing ? > >>>>> > >>>>> [...] > >>>>> > >>>>>> /* Link Definitions */ > >>>>>> #define CONFIG_LOADADDR 0x40480000 > >>>>>> #define CONFIG_SYS_LOAD_ADDR CONFIG_LOADADDR'' > >>>>>> > >>>>>> With this configuration a blank env in flash results in the entire > >>>>>> default env showing in an 'env print' (ie all the stuff from > >>>>>> include/env_default.h 'default_environment') but as soon as I put an > >>>>>> env in flash (ie saveenv) and reset now the only env is what was set > >>>>>> via running code (ethaddr's fdtcontroladdr stdin/err/out). The only > >>>>>> different I expected is that I expected the default env from > >>>>>> include/env_default.h to be there on an initial boot with no valid mmc > >>>>>> env. > >>>>> > >>>>> Maybe the aforementioned patch is missing ? > >>>> > >>>> Marek, > >>>> > >>>> Yes, that patch fixes the issue I'm seeing of mmc not initializing and > >>>> now the default env shows up as expected when MMC env is empty or > >>>> populated. > >>>> > >>>> Thanks - hopefully that patch gets merged soon... I did respond with a > >>>> Tested-By. > >>> > >>> Oh, good. You might want to notify Tom about that, so it would get picked. > >> > >> Marek, > >> > >> One additional thing I did get stuck on is that if your writable > >> var(s) appears in the default environment the default will take > >> precedence over the FLASH env. This does make sense, but requires that > >> you create a default FLASH env (ie mkenvimage) and put it in the > >> appropriate place. I figured I would mention this for anyone else that > >> ends up reading this thread for help. > > > > Can you maybe write some better env documentation and submit a patch, now that you got it all figured out ? > > > >> One last thing that I have not yet figured out how to work-around is > >> how to best disable the shell completely so that if there is any > >> failure in your bootcmd you don't simply drop into a completely > >> insecure U-Boot shell. What is your recommendation there? > > > > set bootdelay=-2 disables the prompt access. > > This is insufficient. If the end of the boot command is reached, U-Boot > will fall back to the shell. One must ensure that the bootcmd never > exists. This can be done either by placing the whole command in a loop, > recursively calling another command, or by resetting the board if no > boot commands work. See e.g. [1] for a good writeup. > > --Sean > > [1] https://labs.f-secure.com/assets/BlogFiles/2020-05-u-booting-securely-wp-final.pdf Right, even though bootdelay=-2 if for whatever reason the bootcmd fails it will drop you in a shell. I like the idea of adding a reset to the end to handle failures. Thanks for the reference - its full of great info! Tim