All of lore.kernel.org
 help / color / mirror / Atom feed
* Locking down U-Boot env with ENV_WRITEABLE_LIST
@ 2021-03-26 18:15 Tim Harvey
  2021-03-26 18:34 ` Marek Vasut
  0 siblings, 1 reply; 14+ messages in thread
From: Tim Harvey @ 2021-03-26 18:15 UTC (permalink / raw)
  To: u-boot

Greetings,

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.

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?

What is the best way to set the list of vars that you wish to be
allowed to be imported from a FLASH env?

Best regards,

Tim

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Locking down U-Boot env with ENV_WRITEABLE_LIST
  2021-03-26 18:15 Locking down U-Boot env with ENV_WRITEABLE_LIST Tim Harvey
@ 2021-03-26 18:34 ` Marek Vasut
  2021-03-26 20:41   ` Stefano Babic
  2021-04-03  2:21   ` Tim Harvey
  0 siblings, 2 replies; 14+ messages in thread
From: Marek Vasut @ 2021-03-26 18:34 UTC (permalink / raw)
  To: u-boot

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"

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Locking down U-Boot env with ENV_WRITEABLE_LIST
  2021-03-26 18:34 ` Marek Vasut
@ 2021-03-26 20:41   ` Stefano Babic
  2021-04-03  2:21   ` Tim Harvey
  1 sibling, 0 replies; 14+ messages in thread
From: Stefano Babic @ 2021-03-26 20:41 UTC (permalink / raw)
  To: u-boot

Hi Tim,

On 26.03.21 19:34, Marek Vasut 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
> 

 From host:
	make u-boot-initial-env
	cat u-boot-initial-env

Best regards,
Stefano

>> 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"
> 
> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Locking down U-Boot env with ENV_WRITEABLE_LIST
  2021-03-26 18:34 ` Marek Vasut
  2021-03-26 20:41   ` Stefano Babic
@ 2021-04-03  2:21   ` Tim Harvey
  2021-04-03 10:24     ` Marek Vasut
  1 sibling, 1 reply; 14+ messages in thread
From: Tim Harvey @ 2021-04-03  2:21 UTC (permalink / raw)
  To: u-boot

On Fri, Mar 26, 2021 at 11:34 AM Marek Vasut <marex@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}

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.

Best regards,

Tim

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Locking down U-Boot env with ENV_WRITEABLE_LIST
  2021-04-03  2:21   ` Tim Harvey
@ 2021-04-03 10:24     ` Marek Vasut
  2021-04-03 16:43       ` Tim Harvey
  0 siblings, 1 reply; 14+ messages in thread
From: Marek Vasut @ 2021-04-03 10:24 UTC (permalink / raw)
  To: u-boot

On 4/3/21 4:21 AM, Tim Harvey wrote:
> On Fri, Mar 26, 2021 at 11:34 AM Marek Vasut <marex@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.

> 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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Locking down U-Boot env with ENV_WRITEABLE_LIST
  2021-04-03 10:24     ` Marek Vasut
@ 2021-04-03 16:43       ` Tim Harvey
  2021-04-03 19:09         ` Marek Vasut
  0 siblings, 1 reply; 14+ messages in thread
From: Tim Harvey @ 2021-04-03 16:43 UTC (permalink / raw)
  To: u-boot

On Sat, Apr 3, 2021 at 3:25 AM Marek Vasut <marex@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@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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Locking down U-Boot env with ENV_WRITEABLE_LIST
  2021-04-03 16:43       ` Tim Harvey
@ 2021-04-03 19:09         ` Marek Vasut
  2021-04-05 16:27           ` Tim Harvey
  0 siblings, 1 reply; 14+ messages in thread
From: Marek Vasut @ 2021-04-03 19:09 UTC (permalink / raw)
  To: u-boot

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 ?

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Locking down U-Boot env with ENV_WRITEABLE_LIST
  2021-04-03 19:09         ` Marek Vasut
@ 2021-04-05 16:27           ` Tim Harvey
  2021-04-05 17:36             ` Marek Vasut
  0 siblings, 1 reply; 14+ messages in thread
From: Tim Harvey @ 2021-04-05 16:27 UTC (permalink / raw)
  To: u-boot

On Sat, Apr 3, 2021 at 12:09 PM Marek Vasut <marex@denx.de> 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.

Tim

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Locking down U-Boot env with ENV_WRITEABLE_LIST
  2021-04-05 16:27           ` Tim Harvey
@ 2021-04-05 17:36             ` Marek Vasut
  2021-04-06 19:52               ` Tim Harvey
  0 siblings, 1 reply; 14+ messages in thread
From: Marek Vasut @ 2021-04-05 17:36 UTC (permalink / raw)
  To: u-boot

On 4/5/21 6:27 PM, Tim Harvey wrote:
> On Sat, Apr 3, 2021 at 12:09 PM Marek Vasut <marex@denx.de> 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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Locking down U-Boot env with ENV_WRITEABLE_LIST
  2021-04-05 17:36             ` Marek Vasut
@ 2021-04-06 19:52               ` Tim Harvey
  2021-04-06 20:10                 ` Marek Vasut
  0 siblings, 1 reply; 14+ messages in thread
From: Tim Harvey @ 2021-04-06 19:52 UTC (permalink / raw)
  To: u-boot

On Mon, Apr 5, 2021 at 10:36 AM Marek Vasut <marex@denx.de> wrote:
>
> On 4/5/21 6:27 PM, Tim Harvey wrote:
> > On Sat, Apr 3, 2021 at 12:09 PM Marek Vasut <marex@denx.de> 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.

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?

Best regards,

Tim

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Locking down U-Boot env with ENV_WRITEABLE_LIST
  2021-04-06 19:52               ` Tim Harvey
@ 2021-04-06 20:10                 ` Marek Vasut
  2021-04-06 21:54                   ` Sean Anderson
  0 siblings, 1 reply; 14+ messages in thread
From: Marek Vasut @ 2021-04-06 20:10 UTC (permalink / raw)
  To: u-boot

On 4/6/21 9:52 PM, Tim Harvey wrote:
> On Mon, Apr 5, 2021 at 10:36 AM Marek Vasut <marex@denx.de> wrote:
>>
>> On 4/5/21 6:27 PM, Tim Harvey wrote:
>>> On Sat, Apr 3, 2021 at 12:09 PM Marek Vasut <marex@denx.de> 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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Locking down U-Boot env with ENV_WRITEABLE_LIST
  2021-04-06 20:10                 ` Marek Vasut
@ 2021-04-06 21:54                   ` Sean Anderson
  2021-04-06 22:39                     ` Joel Peshkin
  2021-04-06 23:23                     ` Tim Harvey
  0 siblings, 2 replies; 14+ messages in thread
From: Sean Anderson @ 2021-04-06 21:54 UTC (permalink / raw)
  To: u-boot



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 <marex@denx.de> wrote:
 >>>
 >>> On 4/5/21 6:27 PM, Tim Harvey wrote:
 >>>> On Sat, Apr 3, 2021 at 12:09 PM Marek Vasut <marex@denx.de> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Locking down U-Boot env with ENV_WRITEABLE_LIST
  2021-04-06 21:54                   ` Sean Anderson
@ 2021-04-06 22:39                     ` Joel Peshkin
  2021-04-06 23:23                     ` Tim Harvey
  1 sibling, 0 replies; 14+ messages in thread
From: Joel Peshkin @ 2021-04-06 22:39 UTC (permalink / raw)
  To: u-boot

I've been using bootstopkeysha256 and, if I want to make it completely
impossible to enter, just setting it to an impossible value.   To do this,
I do have an additional patch to common/autoboot.c that calls the password
mechanism one last time after a bootcmd fails and loops until reset if it
isn't satisfied (never allowing it to fall through to the CLI)


On Tue, Apr 6, 2021 at 2:54 PM Sean Anderson <sean.anderson@seco.com> 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 <marex@denx.de> wrote:
>  >>>
>  >>> On 4/5/21 6:27 PM, Tim Harvey wrote:
>  >>>> On Sat, Apr 3, 2021 at 12:09 PM Marek Vasut <marex@denx.de> 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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4209 bytes
Desc: S/MIME Cryptographic Signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210406/f268ae59/attachment-0001.bin>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Locking down U-Boot env with ENV_WRITEABLE_LIST
  2021-04-06 21:54                   ` Sean Anderson
  2021-04-06 22:39                     ` Joel Peshkin
@ 2021-04-06 23:23                     ` Tim Harvey
  1 sibling, 0 replies; 14+ messages in thread
From: Tim Harvey @ 2021-04-06 23:23 UTC (permalink / raw)
  To: u-boot

On Tue, Apr 6, 2021 at 2:54 PM Sean Anderson <sean.anderson@seco.com> 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 <marex@denx.de> wrote:
>  >>>
>  >>> On 4/5/21 6:27 PM, Tim Harvey wrote:
>  >>>> On Sat, Apr 3, 2021 at 12:09 PM Marek Vasut <marex@denx.de> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2021-04-06 23:23 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-26 18:15 Locking down U-Boot env with ENV_WRITEABLE_LIST Tim Harvey
2021-03-26 18:34 ` Marek Vasut
2021-03-26 20:41   ` Stefano Babic
2021-04-03  2:21   ` Tim Harvey
2021-04-03 10:24     ` Marek Vasut
2021-04-03 16:43       ` Tim Harvey
2021-04-03 19:09         ` Marek Vasut
2021-04-05 16:27           ` Tim Harvey
2021-04-05 17:36             ` Marek Vasut
2021-04-06 19:52               ` Tim Harvey
2021-04-06 20:10                 ` Marek Vasut
2021-04-06 21:54                   ` Sean Anderson
2021-04-06 22:39                     ` Joel Peshkin
2021-04-06 23:23                     ` Tim Harvey

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.