| Message ID | ff107c95e9900ced38abd5e9996a9f79c7ea6d9a.1675426972.git.jan.kiszka@siemens.com |
|---|---|
| State | Accepted |
| Commit | 5ab81058364b5e49bdc6f530368d49e94dfcb960 |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | env: Enhancements for establishing variable protection | expand |
Hi Jan, On Fri, 3 Feb 2023 at 05:23, Jan Kiszka <jan.kiszka@siemens.com> wrote: > > From: Jan Kiszka <jan.kiszka@siemens.com> > > This completes what 890feecaab72 started by selecting ENV_APPEND and > loading the default env before any other sources. This ensures that load > operations pick up all non-writable vars from the default env and only > permitted parts from other locations according to the regular > priorities. > > With this change, boards only need to define the list of writable > variables but no longer have to provide a custom env_get_location > implementation. > > CC: Joe Hershberger <joe.hershberger@ni.com> > CC: Marek Vasut <marex@denx.de> > CC: Stefan Herbrechtsmeier <stefan.herbrechtsmeier-oss@weidmueller.com> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > Reviewed-by: Marek Vasut <marex@denx.de> > --- > env/Kconfig | 1 + > env/env.c | 8 ++++++++ > 2 files changed, 9 insertions(+) > > diff --git a/env/Kconfig b/env/Kconfig > index c409ea71fe5..6e24eee55f2 100644 > --- a/env/Kconfig > +++ b/env/Kconfig > @@ -733,6 +733,7 @@ config ENV_APPEND > > config ENV_WRITEABLE_LIST > bool "Permit write access only to listed variables" > + select ENV_APPEND > help > If defined, only environment variables which explicitly set the 'w' > writeable flag can be written and modified at runtime. No variables > diff --git a/env/env.c b/env/env.c > index 06078c7f374..45e638fcd1f 100644 > --- a/env/env.c > +++ b/env/env.c > @@ -195,6 +195,14 @@ int env_load(void) > int best_prio = -1; > int prio; > > + if (CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)) { > + /* > + * When using a list of writeable variables, the baseline comes > + * from the built-in default env. So load this first. > + */ > + env_set_default(NULL, 0); > + } > + > for (prio = 0; (drv = env_driver_lookup(ENVOP_LOAD, prio)); prio++) { > int ret; > > -- > 2.35.3 > This looks OK, but please can you write some tests in test/env ? Regards, SImon
On 07.02.23 05:02, Simon Glass wrote: > Hi Jan, > > On Fri, 3 Feb 2023 at 05:23, Jan Kiszka <jan.kiszka@siemens.com> wrote: >> >> From: Jan Kiszka <jan.kiszka@siemens.com> >> >> This completes what 890feecaab72 started by selecting ENV_APPEND and >> loading the default env before any other sources. This ensures that load >> operations pick up all non-writable vars from the default env and only >> permitted parts from other locations according to the regular >> priorities. >> >> With this change, boards only need to define the list of writable >> variables but no longer have to provide a custom env_get_location >> implementation. >> >> CC: Joe Hershberger <joe.hershberger@ni.com> >> CC: Marek Vasut <marex@denx.de> >> CC: Stefan Herbrechtsmeier <stefan.herbrechtsmeier-oss@weidmueller.com> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >> Reviewed-by: Marek Vasut <marex@denx.de> >> --- >> env/Kconfig | 1 + >> env/env.c | 8 ++++++++ >> 2 files changed, 9 insertions(+) >> >> diff --git a/env/Kconfig b/env/Kconfig >> index c409ea71fe5..6e24eee55f2 100644 >> --- a/env/Kconfig >> +++ b/env/Kconfig >> @@ -733,6 +733,7 @@ config ENV_APPEND >> >> config ENV_WRITEABLE_LIST >> bool "Permit write access only to listed variables" >> + select ENV_APPEND >> help >> If defined, only environment variables which explicitly set the 'w' >> writeable flag can be written and modified at runtime. No variables >> diff --git a/env/env.c b/env/env.c >> index 06078c7f374..45e638fcd1f 100644 >> --- a/env/env.c >> +++ b/env/env.c >> @@ -195,6 +195,14 @@ int env_load(void) >> int best_prio = -1; >> int prio; >> >> + if (CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)) { >> + /* >> + * When using a list of writeable variables, the baseline comes >> + * from the built-in default env. So load this first. >> + */ >> + env_set_default(NULL, 0); >> + } >> + >> for (prio = 0; (drv = env_driver_lookup(ENVOP_LOAD, prio)); prio++) { >> int ret; >> >> -- >> 2.35.3 >> > > This looks OK, but please can you write some tests in test/env ? Looking at those, it seems there is nothing at all regarding access flags yet. Any suggestions how to first of all build up that infrastructure are welcome. Then I could add this aspect here on top. Jan
()Hi Jan, On Mon, 6 Feb 2023 at 22:49, Jan Kiszka <jan.kiszka@siemens.com> wrote: > > On 07.02.23 05:02, Simon Glass wrote: > > Hi Jan, > > > > On Fri, 3 Feb 2023 at 05:23, Jan Kiszka <jan.kiszka@siemens.com> wrote: > >> > >> From: Jan Kiszka <jan.kiszka@siemens.com> > >> > >> This completes what 890feecaab72 started by selecting ENV_APPEND and > >> loading the default env before any other sources. This ensures that load > >> operations pick up all non-writable vars from the default env and only > >> permitted parts from other locations according to the regular > >> priorities. > >> > >> With this change, boards only need to define the list of writable > >> variables but no longer have to provide a custom env_get_location > >> implementation. > >> > >> CC: Joe Hershberger <joe.hershberger@ni.com> > >> CC: Marek Vasut <marex@denx.de> > >> CC: Stefan Herbrechtsmeier <stefan.herbrechtsmeier-oss@weidmueller.com> > >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > >> Reviewed-by: Marek Vasut <marex@denx.de> > >> --- > >> env/Kconfig | 1 + > >> env/env.c | 8 ++++++++ > >> 2 files changed, 9 insertions(+) > >> > >> diff --git a/env/Kconfig b/env/Kconfig > >> index c409ea71fe5..6e24eee55f2 100644 > >> --- a/env/Kconfig > >> +++ b/env/Kconfig > >> @@ -733,6 +733,7 @@ config ENV_APPEND > >> > >> config ENV_WRITEABLE_LIST > >> bool "Permit write access only to listed variables" > >> + select ENV_APPEND > >> help > >> If defined, only environment variables which explicitly set the 'w' > >> writeable flag can be written and modified at runtime. No variables > >> diff --git a/env/env.c b/env/env.c > >> index 06078c7f374..45e638fcd1f 100644 > >> --- a/env/env.c > >> +++ b/env/env.c > >> @@ -195,6 +195,14 @@ int env_load(void) > >> int best_prio = -1; > >> int prio; > >> > >> + if (CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)) { > >> + /* > >> + * When using a list of writeable variables, the baseline comes > >> + * from the built-in default env. So load this first. > >> + */ > >> + env_set_default(NULL, 0); > >> + } > >> + > >> for (prio = 0; (drv = env_driver_lookup(ENVOP_LOAD, prio)); prio++) { > >> int ret; > >> > >> -- > >> 2.35.3 > >> > > > > This looks OK, but please can you write some tests in test/env ? > > Looking at those, it seems there is nothing at all regarding access > flags yet. Any suggestions how to first of all build up that > infrastructure are welcome. Then I could add this aspect here on top. Yes, starting something a bit new is always harder. My approach is normally to just start small. Anything is better than nothing, and it provides something for others to build on. There are a few tests for hashtable which could be a way to start this. I suggest starting a new env.c file in there with a few env_get() and env_set() calls. You could also use himport_r() to test out different flags. I'm not sure how easy it would be to call env_load() in a test, but it might work. The trickier thing (perhaps to worry about later) is that you are (correctly) using a CONFIG option to enable the feature. For sandbox tests we typically want all features to be enabled at build time (e.g. default y if SANDBOX), then select them at runtime, so we can test them. See test_eth_enabled() for an example of how this is done. Regards, Simon
On Fri, Feb 03, 2023 at 01:22:51PM +0100, Jan Kiszka wrote: > From: Jan Kiszka <jan.kiszka@siemens.com> > > This completes what 890feecaab72 started by selecting ENV_APPEND and > loading the default env before any other sources. This ensures that load > operations pick up all non-writable vars from the default env and only > permitted parts from other locations according to the regular > priorities. > > With this change, boards only need to define the list of writable > variables but no longer have to provide a custom env_get_location > implementation. > > CC: Joe Hershberger <joe.hershberger@ni.com> > CC: Marek Vasut <marex@denx.de> > CC: Stefan Herbrechtsmeier <stefan.herbrechtsmeier-oss@weidmueller.com> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > Reviewed-by: Marek Vasut <marex@denx.de> Applied to u-boot/master, thanks!
diff --git a/env/Kconfig b/env/Kconfig index c409ea71fe5..6e24eee55f2 100644 --- a/env/Kconfig +++ b/env/Kconfig @@ -733,6 +733,7 @@ config ENV_APPEND config ENV_WRITEABLE_LIST bool "Permit write access only to listed variables" + select ENV_APPEND help If defined, only environment variables which explicitly set the 'w' writeable flag can be written and modified at runtime. No variables diff --git a/env/env.c b/env/env.c index 06078c7f374..45e638fcd1f 100644 --- a/env/env.c +++ b/env/env.c @@ -195,6 +195,14 @@ int env_load(void) int best_prio = -1; int prio; + if (CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)) { + /* + * When using a list of writeable variables, the baseline comes + * from the built-in default env. So load this first. + */ + env_set_default(NULL, 0); + } + for (prio = 0; (drv = env_driver_lookup(ENVOP_LOAD, prio)); prio++) { int ret;