Locking down U-Boot env with ENV_WRITEABLE_LIST

[Tim Harvey]

On Sat, Apr 3, 2021 at 3:25 AM Marek Vasut <marex at denx.de> wrote:
>
> On 4/3/21 4:21 AM, Tim Harvey wrote:
> > On Fri, Mar 26, 2021 at 11:34 AM Marek Vasut <marex at denx.de> wrote:
> >>
> >> On 3/26/21 7:15 PM, Tim Harvey wrote:
> >>> Greetings,
> >>
> >> Hi,
> >>
> >>> I'm trying to understand best how to lock down a U-Boot environment
> >>> using ENV_WRITEABLE_LIST=y.
> >>>
> >>> My understanding is that I should define all vars that I wish to be
> >>> able to be loaded from a FLASH env in CONFIG_ENV_FLAGS_LIST_DEFAULT. I
> >>> would think this would be something in Kconfig but it's not so I
> >>> wonder if I'm misunderstanding something or if I truly need to patch a
> >>> config.h when using this feature.
> >>
> >> You do need to patch board config in include/configs/ , since the flags
> >> were note converted to Kconfig. And make sure you only use integer or
> >> bool vars, since strings might contain scripts, which you want to avoid.
> >>
> >>> What is the best way to actively see your static U-Boot env that gets
> >>> linked into U-Boot? I can see it with a hexdump but there must be a
> >>> better way by looking at an include file?
> >>
> >>   From running u-boot, => env print
> >>
> >>> What is the best way to set the list of vars that you wish to be
> >>> allowed to be imported from a FLASH env?
> >>
> >> Ideally none, and if you really want to make sure something can be
> >> pulled in from external env, then:
> >> #define CONFIG_ENV_FLAGS_LIST_STATIC "var1:dw,var2:dw"
> >
> > Marek,
> >
> > I can't seem to understand CONFIG_ENV_FLAGS_LIST_STATIC vs
> > CONFIG_ENF_FLAGS_LIST_DEFAULT. The code seems convoluted and
> > experimentally I am just as confused.
> >
> > It seems that as soon as you define CONFIG_ENV_WRITEABLE_LIST=y then
> > all variables defined elsewhere (ie CONFIG_EXTRA_ENV_SETTINGS
> > CONFIG_BOOTCOMMAND) can no longer be imported from an env (they are
> > present if you clobber your flash env but not if anything is written
> > to it).
> >
> > I quite simply want only the following environment:
> > kernel_addr_r=0x02000000
> > mmcbootpart=4
> > ustate=1
> > bootcmd setenv bootargs root=/dev/mmcblk0p${mmcbootpart} rootwait rw;
> > load mmc 0:${mmcbootpart} ${kernel_addr_r} boot/kernel.itb && bootm
> > ${kernel_addr_r} - ${fdtcontroladdr}
>
> This script is gonna be a problem, since it is something some external
> entity can overwrite and implant random script into your env. That's why
> I wrote you want minimal set of vars imported from external env and they
> should be boolean or integer.

I want the bootscript static, the only vars I want changeable are
ustate/mmcbootpart which are integers.

In my case U-Boot is a signed/authenticated image so I'm not worried
about anyone changing bytes on the flash to hack bootcmd.

I'm also using a signed FIT to secure the kernel/conf/initramfs so
really all I'm trying to do is get to booting the FIT with a secure
environment. The only reason I need mmcbootpart writable is because
I'm using SWUpdate with a A/B rootfs so it needs to alter mmcbootpart
to pick the rootfs partition after an update.

>
> > and the only variables with flags I want to be able to be overridden
> > from MMC_ENV are:
> > mmcbootpart:dw
> > usate:dw
> >
> > It is too bad this can't be done via defconfig - perhaps when I
> > finally understand it I can submit a patch to move it to Kconfig.
> >
> >>
> >> 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

My diff currently looks like the following to define
CONFIG_ENV_WRITEABLE_LIST, CONFIG_ENV_FLAGS_LIST_DEFAULT, and
CONFIG_ENV_FLAGS_LIST_STATIC

diff --git a/configs/imx8mm_venice_defconfig b/configs/imx8mm_venice_defconfig
index 3be5c48701..84bea671ed 100644
--- a/configs/imx8mm_venice_defconfig
+++ b/configs/imx8mm_venice_defconfig
@@ -41,6 +41,8 @@ CONFIG_SPL_WATCHDOG_SUPPORT=y
 CONFIG_SYS_PROMPT="u-boot=> "
 # CONFIG_CMD_EXPORTENV is not set
 # CONFIG_CMD_IMPORTENV is not set
+CONFIG_CMD_ENV_CALLBACK=y
+CONFIG_CMD_ENV_FLAGS=y
 # CONFIG_CMD_CRC32 is not set
 CONFIG_CMD_MEMTEST=y
 CONFIG_CMD_CLK=y
@@ -64,6 +66,9 @@ CONFIG_OF_LIST="imx8mm-venice-gw71xx-0x
imx8mm-venice-gw72xx-0x imx8mm-venice-gw
 CONFIG_ENV_IS_IN_MMC=y
 CONFIG_SYS_REDUNDAND_ENVIRONMENT=y
 CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG=y
+CONFIG_ENV_APPEND=y
+CONFIG_ENV_WRITEABLE_LIST=y
+CONFIG_ENV_ACCESS_IGNORE_FORCE=y
 CONFIG_NET_RANDOM_ETHADDR=y
 CONFIG_IP_DEFRAG=y
 CONFIG_TFTP_BLOCKSIZE=4096
diff --git a/include/configs/imx8mm_venice.h b/include/configs/imx8mm_venice.h
index 13437d7694..e63eceac49 100644
--- a/include/configs/imx8mm_venice.h
+++ b/include/configs/imx8mm_venice.h
@@ -36,6 +36,9 @@
        "ramdisk_addr_r=0x46400000\0" \
        "scriptaddr=0x46000000\0"

+#define CONFIG_ENV_FLAGS_LIST_DEFAULT "ustate:dw,mmcbootpart:dw"
+#define CONFIG_ENV_FLAGS_LIST_STATIC "ustate:dw,mmcbootpart:dw"
+
 /* 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.

Tim