[PATCH 0/2] env: Enhancements for establishing variable protection

[PATCH 0/2] env: Enhancements for establishing variable protection

[Jan Kiszka]

This was factored out of [1] upon request in the hope of easing the
merge.

Jan

[1] https://lore.kernel.org/u-boot/cover.1675325279.git.jan.kiszka@siemens.com


CC: Joe Hershberger <joe.hershberger@ni.com>
CC: Marek Vasut <marex@denx.de>
CC: Stefan Herbrechtsmeier <stefan.herbrechtsmeier-oss@weidmueller.com>

Jan Kiszka (2):
  env: Complete generic support for writable list
  env: Couple networking-related variable flags to CONFIG_NET

 env/Kconfig         |  1 +
 env/env.c           |  8 ++++++++
 env/flags.c         | 10 +++++-----
 include/env_flags.h |  4 ++--
 4 files changed, 16 insertions(+), 7 deletions(-)

-- 
2.35.3

[PATCH 1/2] env: Complete generic support for writable list

[Jan Kiszka]

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

[PATCH 2/2] env: Couple networking-related variable flags to CONFIG_NET

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Boards may set networking variables programmatically, thus may have
CONFIG_NET on but CONFIG_CMD_NET off. The IOT2050 is an example.

CC: Joe Hershberger <joe.hershberger@ni.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 env/flags.c         | 10 +++++-----
 include/env_flags.h |  4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/env/flags.c b/env/flags.c
index e3e833c4333..e2866361dfe 100644
--- a/env/flags.c
+++ b/env/flags.c
@@ -22,7 +22,7 @@
 #include <env_internal.h>
 #endif
 
-#ifdef CONFIG_CMD_NET
+#ifdef CONFIG_NET
 #define ENV_FLAGS_NET_VARTYPE_REPS "im"
 #else
 #define ENV_FLAGS_NET_VARTYPE_REPS ""
@@ -57,7 +57,7 @@ static const char * const env_flags_vartype_names[] = {
 	"decimal",
 	"hexadecimal",
 	"boolean",
-#ifdef CONFIG_CMD_NET
+#ifdef CONFIG_NET
 	"IP address",
 	"MAC address",
 #endif
@@ -211,7 +211,7 @@ static void skip_num(int hex, const char *value, const char **end,
 		*end = value;
 }
 
-#ifdef CONFIG_CMD_NET
+#ifdef CONFIG_NET
 int eth_validate_ethaddr_str(const char *addr)
 {
 	const char *end;
@@ -244,7 +244,7 @@ static int _env_flags_validate_type(const char *value,
 	enum env_flags_vartype type)
 {
 	const char *end;
-#ifdef CONFIG_CMD_NET
+#ifdef CONFIG_NET
 	const char *cur;
 	int i;
 #endif
@@ -273,7 +273,7 @@ static int _env_flags_validate_type(const char *value,
 		if (value[1] != '\0')
 			return -1;
 		break;
-#ifdef CONFIG_CMD_NET
+#ifdef CONFIG_NET
 	case env_flags_vartype_ipaddr:
 		cur = value;
 		for (i = 0; i < 4; i++) {
diff --git a/include/env_flags.h b/include/env_flags.h
index 6bd574c2bdb..7de58cc57c3 100644
--- a/include/env_flags.h
+++ b/include/env_flags.h
@@ -12,7 +12,7 @@ enum env_flags_vartype {
 	env_flags_vartype_decimal,
 	env_flags_vartype_hex,
 	env_flags_vartype_bool,
-#ifdef CONFIG_CMD_NET
+#ifdef CONFIG_NET
 	env_flags_vartype_ipaddr,
 	env_flags_vartype_macaddr,
 #endif
@@ -121,7 +121,7 @@ enum env_flags_varaccess env_flags_parse_varaccess(const char *flags);
  */
 enum env_flags_varaccess env_flags_parse_varaccess_from_binflags(int binflags);
 
-#ifdef CONFIG_CMD_NET
+#ifdef CONFIG_NET
 /*
  * Check if a string has the format of an Ethernet MAC address
  */
-- 
2.35.3

Re: [PATCH 2/2] env: Couple networking-related variable flags to CONFIG_NET

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 415 bytes --]

On Fri, Feb 03, 2023 at 01:22:52PM +0100, Jan Kiszka wrote:

> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Boards may set networking variables programmatically, thus may have
> CONFIG_NET on but CONFIG_CMD_NET off. The IOT2050 is an example.
> 
> CC: Joe Hershberger <joe.hershberger@ni.com>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/2] env: Complete generic support for writable list

[Simon Glass]

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

Re: [PATCH 1/2] env: Complete generic support for writable list

[Jan Kiszka]

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

-- 
Siemens AG, Technology
Competence Center Embedded Linux

Re: [PATCH 1/2] env: Complete generic support for writable list

[Simon Glass]

()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

Re: [PATCH 1/2] env: Complete generic support for writable list

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 884 bytes --]

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!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/2] env: Couple networking-related variable flags to CONFIG_NET

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 452 bytes --]

On Fri, Feb 03, 2023 at 01:22:52PM +0100, Jan Kiszka wrote:

> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Boards may set networking variables programmatically, thus may have
> CONFIG_NET on but CONFIG_CMD_NET off. The IOT2050 is an example.
> 
> CC: Joe Hershberger <joe.hershberger@ni.com>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> Reviewed-by: Tom Rini <trini@konsulko.com>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]