[PATCH 1/6] env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set

[PATCH 1/6] env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set

[Marek Vasut]

If the ENV_ACCESS_IGNORE_FORCE is set, inform user that the variable
cannot be force-set if such attempt happens.

Signed-off-by: Marek Vasut <marex@denx.de>
---
 env/flags.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/env/flags.c b/env/flags.c
index b88fe7ba9c..f7a53775c4 100644
--- a/env/flags.c
+++ b/env/flags.c
@@ -524,8 +524,10 @@ int env_flags_validate(const struct env_entry *item, const char *newval,
 
 	/* check for access permission */
 #ifndef CONFIG_ENV_ACCESS_IGNORE_FORCE
-	if (flag & H_FORCE)
+	if (flag & H_FORCE) {
+		printf("## Error: Can't force access to \"%s\"\n", name);
 		return 0;
+	}
 #endif
 	switch (op) {
 	case env_op_delete:
-- 
2.25.1

[PATCH 2/6] env: Add H_DEFAULT flag

[Marek Vasut]

Add another internal environment flag which indicates that the operation
is resetting the environment to the default one. This allows the env code
to discern between import of external environment and reset to default.

Signed-off-by: Marek Vasut <marex@denx.de>
---
 env/common.c     | 3 ++-
 include/search.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/env/common.c b/env/common.c
index 088b2aebb4..0db56e610a 100644
--- a/env/common.c
+++ b/env/common.c
@@ -81,6 +81,7 @@ void env_set_default(const char *s, int flags)
 		debug("Using default environment\n");
 	}
 
+	flags |= H_DEFAULT;
 	if (himport_r(&env_htab, (char *)default_environment,
 			sizeof(default_environment), '\0', flags, 0,
 			0, NULL) == 0)
@@ -99,7 +100,7 @@ int env_set_default_vars(int nvars, char * const vars[], int flags)
 	 * Special use-case: import from default environment
 	 * (and use \0 as a separator)
 	 */
-	flags |= H_NOCLEAR;
+	flags |= H_NOCLEAR | H_DEFAULT;
 	return himport_r(&env_htab, (const char *)default_environment,
 				sizeof(default_environment), '\0',
 				flags, 0, nvars, vars);
diff --git a/include/search.h b/include/search.h
index bca36d3abc..c4b50c9630 100644
--- a/include/search.h
+++ b/include/search.h
@@ -112,5 +112,6 @@ int hwalk_r(struct hsearch_data *htab,
 #define H_MATCH_METHOD	(H_MATCH_IDENT | H_MATCH_SUBSTR | H_MATCH_REGEX)
 #define H_PROGRAMMATIC	(1 << 9) /* indicate that an import is from env_set() */
 #define H_ORIGIN_FLAGS	(H_INTERACTIVE | H_PROGRAMMATIC)
+#define H_DEFAULT	(1 << 10) /* indicate that an import is default env */
 
 #endif /* _SEARCH_H_ */
-- 
2.25.1

[PATCH 3/6] env: Fix invalid env handling in env_init()

[Marek Vasut]

In case the env storage driver marks environment as ENV_INVALID, we must
reset the $ret return value to -ENOENT to let the env init code reset the
environment to the default one a bit further down.

Signed-off-by: Marek Vasut <marex@denx.de>
---
 env/env.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/env/env.c b/env/env.c
index dcc25c030b..024d36fdbe 100644
--- a/env/env.c
+++ b/env/env.c
@@ -300,6 +300,9 @@ int env_init(void)
 
 		debug("%s: Environment %s init done (ret=%d)\n", __func__,
 		      drv->name, ret);
+
+		if (gd->env_valid == ENV_INVALID)
+			ret = -ENOENT;
 	}
 
 	if (!prio)
-- 
2.25.1

[PATCH 4/6] env: nowhere: Implement .load callback

[Marek Vasut]

Add .load callback for the 'nowhere' environment driver. This is useful
for when the 'nowhere' driver is used in combination with other drivers
and should be used to load the default environment.

Signed-off-by: Marek Vasut <marex@denx.de>
---
 env/nowhere.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/env/nowhere.c b/env/nowhere.c
index f5b0a17652..417a636f83 100644
--- a/env/nowhere.c
+++ b/env/nowhere.c
@@ -15,6 +15,12 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
+static int env_nowhere_load(void)
+{
+	env_set_default(NULL, 0);
+
+	return 0;
+}
 /*
  * Because we only ever have the default environment available we must mark
  * it as invalid.
@@ -30,5 +36,6 @@ static int env_nowhere_init(void)
 U_BOOT_ENV_LOCATION(nowhere) = {
 	.location	= ENVL_NOWHERE,
 	.init		= env_nowhere_init,
+	.load		= env_nowhere_load,
 	ENV_NAME("nowhere")
 };
-- 
2.25.1

[PATCH 5/6] env: Add option to only ever append environment

[Marek Vasut]

Add configuration option which prevents the environment hash table to be
ever cleared and reloaded with different content. This is useful in case
the first environment loaded into the hash table contains e.g. sensitive
content which must not be dropped or reloaded.

Signed-off-by: Marek Vasut <marex@denx.de>
---
 env/Kconfig     | 9 +++++++++
 env/env.c       | 2 ++
 lib/hashtable.c | 4 ++++
 3 files changed, 15 insertions(+)

diff --git a/env/Kconfig b/env/Kconfig
index ca7fef682b..8166e5df91 100644
--- a/env/Kconfig
+++ b/env/Kconfig
@@ -604,6 +604,15 @@ config DELAY_ENVIRONMENT
 	  later by U-Boot code. With CONFIG_OF_CONTROL this is instead
 	  controlled by the value of /config/load-environment.
 
+config ENV_APPEND
+	bool "Always append the environment with new data"
+	default n
+	help
+	  If defined, the environment hash table is only ever appended with new
+	  data, but the existing hash table can never be dropped and reloaded
+	  with newly imported data. This may be used in combination with static
+	  flags to e.g. to protect variables which must not be modified.
+
 config ENV_ACCESS_IGNORE_FORCE
 	bool "Block forced environment operations"
 	default n
diff --git a/env/env.c b/env/env.c
index 024d36fdbe..d85f925bcb 100644
--- a/env/env.c
+++ b/env/env.c
@@ -204,7 +204,9 @@ int env_load(void)
 		ret = drv->load();
 		if (!ret) {
 			printf("OK\n");
+#ifdef CONFIG_ENV_APPEND
 			return 0;
+#endif
 		} else if (ret == -ENOMSG) {
 			/* Handle "bad CRC" case */
 			if (best_prio == -1)
diff --git a/lib/hashtable.c b/lib/hashtable.c
index b96dbe19be..24aef5a085 100644
--- a/lib/hashtable.c
+++ b/lib/hashtable.c
@@ -822,6 +822,10 @@ int himport_r(struct hsearch_data *htab,
 	if (nvars)
 		memcpy(localvars, vars, sizeof(vars[0]) * nvars);
 
+#ifdef CONFIG_ENV_APPEND
+	flag |= H_NOCLEAR;
+#endif
+
 	if ((flag & H_NOCLEAR) == 0 && !nvars) {
 		/* Destroy old hash table if one exists */
 		debug("Destroy Hash Table: %p table = %p\n", htab,
-- 
2.25.1

[PATCH 6/6] env: Add support for explicit write access list

[Marek Vasut]

This option marks any U-Boot variable which does not have explicit 'w'
writeable flag set as read-only. This way the environment can be locked
down and only variables explicitly configured to be writeable can ever
be changed by either 'env import', 'env set' or loading user environment
from environment storage.

Signed-off-by: Marek Vasut <marex@denx.de>
---
 env/Kconfig         |  8 ++++
 env/flags.c         | 92 +++++++++++++++++++++++++++++++++++++++------
 include/env_flags.h |  6 ++-
 lib/hashtable.c     |  5 ++-
 4 files changed, 98 insertions(+), 13 deletions(-)

diff --git a/env/Kconfig b/env/Kconfig
index 8166e5df91..f53a1457fb 100644
--- a/env/Kconfig
+++ b/env/Kconfig
@@ -613,6 +613,14 @@ config ENV_APPEND
 	  with newly imported data. This may be used in combination with static
 	  flags to e.g. to protect variables which must not be modified.
 
+config ENV_WRITEABLE_LIST
+	bool "Permit write access only to listed variables"
+	default n
+	help
+	  If defined, only environment variables which explicitly set the 'w'
+	  writeable flag can be written and modified at runtime. No variables
+	  can be otherwise created, written or imported into the environment.
+
 config ENV_ACCESS_IGNORE_FORCE
 	bool "Block forced environment operations"
 	default n
diff --git a/env/flags.c b/env/flags.c
index f7a53775c4..a2f6c1a3ec 100644
--- a/env/flags.c
+++ b/env/flags.c
@@ -28,8 +28,15 @@
 #define ENV_FLAGS_NET_VARTYPE_REPS ""
 #endif
 
+#if CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)
+#define ENV_FLAGS_WRITEABLE_VARACCESS_REPS "w"
+#else
+#define ENV_FLAGS_WRITEABLE_VARACCESS_REPS ""
+#endif
+
 static const char env_flags_vartype_rep[] = "sdxb" ENV_FLAGS_NET_VARTYPE_REPS;
-static const char env_flags_varaccess_rep[] = "aroc";
+static const char env_flags_varaccess_rep[] =
+	"aroc" ENV_FLAGS_WRITEABLE_VARACCESS_REPS;
 static const int env_flags_varaccess_mask[] = {
 	0,
 	ENV_FLAGS_VARACCESS_PREVENT_DELETE |
@@ -38,7 +45,11 @@ static const int env_flags_varaccess_mask[] = {
 	ENV_FLAGS_VARACCESS_PREVENT_DELETE |
 		ENV_FLAGS_VARACCESS_PREVENT_OVERWR,
 	ENV_FLAGS_VARACCESS_PREVENT_DELETE |
-		ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR};
+		ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR,
+#if CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)
+	ENV_FLAGS_VARACCESS_WRITEABLE,
+#endif
+	};
 
 #ifdef CONFIG_CMD_ENV_FLAGS
 static const char * const env_flags_vartype_names[] = {
@@ -56,6 +67,9 @@ static const char * const env_flags_varaccess_names[] = {
 	"read-only",
 	"write-once",
 	"change-default",
+#if CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)
+	"writeable",
+#endif
 };
 
 /*
@@ -130,21 +144,33 @@ enum env_flags_vartype env_flags_parse_vartype(const char *flags)
  */
 enum env_flags_varaccess env_flags_parse_varaccess(const char *flags)
 {
+#if CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)
+	enum env_flags_varaccess va_default = env_flags_varaccess_readonly;
+#else
+	enum env_flags_varaccess va_default = env_flags_varaccess_any;
+#endif
+	enum env_flags_varaccess va;
 	char *access;
 
 	if (strlen(flags) <= ENV_FLAGS_VARACCESS_LOC)
-		return env_flags_varaccess_any;
+		return va_default;
 
 	access = strchr(env_flags_varaccess_rep,
 		flags[ENV_FLAGS_VARACCESS_LOC]);
 
-	if (access != NULL)
-		return (enum env_flags_varaccess)
+	if (access != NULL) {
+		va = (enum env_flags_varaccess)
 			(access - &env_flags_varaccess_rep[0]);
+#if CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)
+		if (va != env_flags_varaccess_writeable)
+			return env_flags_varaccess_readonly;
+#endif
+		return va;
+	}
 
 	printf("## Warning: Unknown environment variable access method '%c'\n",
 		flags[ENV_FLAGS_VARACCESS_LOC]);
-	return env_flags_varaccess_any;
+	return va_default;
 }
 
 /*
@@ -152,17 +178,29 @@ enum env_flags_varaccess env_flags_parse_varaccess(const char *flags)
  */
 enum env_flags_varaccess env_flags_parse_varaccess_from_binflags(int binflags)
 {
+#if CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)
+	enum env_flags_varaccess va_default = env_flags_varaccess_readonly;
+#else
+	enum env_flags_varaccess va_default = env_flags_varaccess_any;
+#endif
+	enum env_flags_varaccess va;
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(env_flags_varaccess_mask); i++)
 		if (env_flags_varaccess_mask[i] ==
-		    (binflags & ENV_FLAGS_VARACCESS_BIN_MASK))
-			return (enum env_flags_varaccess)i;
+		    (binflags & ENV_FLAGS_VARACCESS_BIN_MASK)) {
+			va = (enum env_flags_varaccess)i;
+#if CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)
+			if (va != env_flags_varaccess_writeable)
+				return env_flags_varaccess_readonly;
+#endif
+			return va;
+	}
 
 	printf("Warning: Non-standard access flags. (0x%x)\n",
 		binflags & ENV_FLAGS_VARACCESS_BIN_MASK);
 
-	return env_flags_varaccess_any;
+	return va_default;
 }
 
 static inline int is_hex_prefix(const char *value)
@@ -325,14 +363,20 @@ enum env_flags_vartype env_flags_get_type(const char *name)
  */
 enum env_flags_varaccess env_flags_get_varaccess(const char *name)
 {
+#if CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)
+	const char *flags_list = NULL;
+	enum env_flags_varaccess va_default = env_flags_varaccess_readonly;
+#else
 	const char *flags_list = env_get(ENV_FLAGS_VAR);
+	enum env_flags_varaccess va_default = env_flags_varaccess_any;
+#endif
 	char flags[ENV_FLAGS_ATTR_MAX_LEN + 1];
 
 	if (env_flags_lookup(flags_list, name, flags))
-		return env_flags_varaccess_any;
+		return va_default;
 
 	if (strlen(flags) <= ENV_FLAGS_VARACCESS_LOC)
-		return env_flags_varaccess_any;
+		return va_default;
 
 	return env_flags_parse_varaccess(flags);
 }
@@ -426,7 +470,11 @@ void env_flags_init(struct env_entry *var_entry)
 	int ret = 1;
 
 	if (first_call) {
+#if CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)
+		flags_list = ENV_FLAGS_LIST_STATIC;
+#else
 		flags_list = env_get(ENV_FLAGS_VAR);
+#endif
 		first_call = 0;
 	}
 	/* look in the ".flags" and static for a reference to this variable */
@@ -435,6 +483,16 @@ void env_flags_init(struct env_entry *var_entry)
 	/* if any flags were found, set the binary form to the entry */
 	if (!ret && strlen(flags))
 		var_entry->flags = env_parse_flags_to_bin(flags);
+
+#if CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)
+	/* Anything which is not explicitly writeable is read-only */
+	if (!(var_entry->flags & ENV_FLAGS_VARACCESS_WRITEABLE)) {
+		var_entry->flags &= ~ENV_FLAGS_VARACCESS_BIN_MASK;
+		var_entry->flags |= ENV_FLAGS_VARACCESS_PREVENT_DELETE |
+				ENV_FLAGS_VARACCESS_PREVENT_CREATE |
+				ENV_FLAGS_VARACCESS_PREVENT_OVERWR;
+	}
+#endif
 }
 
 /*
@@ -523,6 +581,18 @@ int env_flags_validate(const struct env_entry *item, const char *newval,
 	}
 
 	/* check for access permission */
+#if CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)
+	if (flag & H_DEFAULT)
+		return 0;	/* Default env is always OK */
+
+	/* Writeable variables can be overwritten, anything else can not */
+	if (op == env_op_overwrite &&
+	    item->flags & ENV_FLAGS_VARACCESS_WRITEABLE)
+		return 0;
+
+	return 1;
+#endif
+
 #ifndef CONFIG_ENV_ACCESS_IGNORE_FORCE
 	if (flag & H_FORCE) {
 		printf("## Error: Can't force access to \"%s\"\n", name);
diff --git a/include/env_flags.h b/include/env_flags.h
index 725841a891..62c3dd9fa0 100644
--- a/include/env_flags.h
+++ b/include/env_flags.h
@@ -24,6 +24,9 @@ enum env_flags_varaccess {
 	env_flags_varaccess_readonly,
 	env_flags_varaccess_writeonce,
 	env_flags_varaccess_changedefault,
+#if CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)
+	env_flags_varaccess_writeable,
+#endif
 	env_flags_varaccess_end
 };
 
@@ -173,6 +176,7 @@ int env_flags_validate(const struct env_entry *item, const char *newval,
 #define ENV_FLAGS_VARACCESS_PREVENT_CREATE		0x00000010
 #define ENV_FLAGS_VARACCESS_PREVENT_OVERWR		0x00000020
 #define ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR	0x00000040
-#define ENV_FLAGS_VARACCESS_BIN_MASK			0x00000078
+#define ENV_FLAGS_VARACCESS_WRITEABLE			0x00000080
+#define ENV_FLAGS_VARACCESS_BIN_MASK			0x000000f8
 
 #endif /* __ENV_FLAGS_H__ */
diff --git a/lib/hashtable.c b/lib/hashtable.c
index 24aef5a085..bebad9d382 100644
--- a/lib/hashtable.c
+++ b/lib/hashtable.c
@@ -946,9 +946,12 @@ int himport_r(struct hsearch_data *htab,
 		e.data = value;
 
 		hsearch_r(e, ENV_ENTER, &rv, htab, flag);
-		if (rv == NULL)
+#if !CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)
+		if (rv == NULL) {
 			printf("himport_r: can't insert \"%s=%s\" into hash table\n",
 				name, value);
+		}
+#endif
 
 		debug("INSERT: table %p, filled %d/%d rv %p ==> name=\"%s\" value=\"%s\"\n",
 			htab, htab->filled, htab->size,
-- 
2.25.1

[PATCH 1/6] env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set

[Tom Rini]

On Wed, Jun 03, 2020 at 02:01:06AM +0200, Marek Vasut wrote:

> If the ENV_ACCESS_IGNORE_FORCE is set, inform user that the variable
> cannot be force-set if such attempt happens.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

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

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200605/d9e039fe/attachment.sig>

[PATCH 2/6] env: Add H_DEFAULT flag

[Tom Rini]

On Wed, Jun 03, 2020 at 02:01:07AM +0200, Marek Vasut wrote:

> Add another internal environment flag which indicates that the operation
> is resetting the environment to the default one. This allows the env code
> to discern between import of external environment and reset to default.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

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

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200605/15c25e32/attachment.sig>

[PATCH 3/6] env: Fix invalid env handling in env_init()

[Tom Rini]

On Wed, Jun 03, 2020 at 02:01:08AM +0200, Marek Vasut wrote:

> In case the env storage driver marks environment as ENV_INVALID, we must
> reset the $ret return value to -ENOENT to let the env init code reset the
> environment to the default one a bit further down.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
>  env/env.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/env/env.c b/env/env.c
> index dcc25c030b..024d36fdbe 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -300,6 +300,9 @@ int env_init(void)
>  
>  		debug("%s: Environment %s init done (ret=%d)\n", __func__,
>  		      drv->name, ret);
> +
> +		if (gd->env_valid == ENV_INVALID)
> +			ret = -ENOENT;
>  	}
>  
>  	if (!prio)

Is the storage driver marking the environment as invalid but not
returning ENOENT valid?

How does all of this work in the case of multiple configured storage
drivers?

Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200605/3fb5ce1f/attachment.sig>

[PATCH 4/6] env: nowhere: Implement .load callback

[Tom Rini]

On Wed, Jun 03, 2020 at 02:01:09AM +0200, Marek Vasut wrote:

> Add .load callback for the 'nowhere' environment driver. This is useful
> for when the 'nowhere' driver is used in combination with other drivers
> and should be used to load the default environment.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

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

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200605/fe1128bf/attachment.sig>

[PATCH 5/6] env: Add option to only ever append environment

[Tom Rini]

On Wed, Jun 03, 2020 at 02:01:10AM +0200, Marek Vasut wrote:

> Add configuration option which prevents the environment hash table to be
> ever cleared and reloaded with different content. This is useful in case
> the first environment loaded into the hash table contains e.g. sensitive
> content which must not be dropped or reloaded.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

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

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200605/245abf84/attachment.sig>

[PATCH 6/6] env: Add support for explicit write access list

[Tom Rini]

On Wed, Jun 03, 2020 at 02:01:11AM +0200, Marek Vasut wrote:

> This option marks any U-Boot variable which does not have explicit 'w'
> writeable flag set as read-only. This way the environment can be locked
> down and only variables explicitly configured to be writeable can ever
> be changed by either 'env import', 'env set' or loading user environment
> from environment storage.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
[snip]
>  static const char env_flags_vartype_rep[] = "sdxb" ENV_FLAGS_NET_VARTYPE_REPS;
> -static const char env_flags_varaccess_rep[] = "aroc";
> +static const char env_flags_varaccess_rep[] =
> +	"aroc" ENV_FLAGS_WRITEABLE_VARACCESS_REPS;

nit: I think this is an example of why going a tiny bit past 80 chars
wide improves readability.  If inclined and someone else asks for a v2,
go ahead.

I think all of the code itself is fine, but it's complex enough in a
complex area I'm not adding my own Reviewed-by.

What I would ask for is to add some tests to test/py/tests/test_env.py
for what we can test for.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200605/41fbcaf0/attachment.sig>

[PATCH 3/6] env: Fix invalid env handling in env_init()

[Marek Vasut]

On 6/5/20 9:07 PM, Tom Rini wrote:
> On Wed, Jun 03, 2020 at 02:01:08AM +0200, Marek Vasut wrote:
> 
>> In case the env storage driver marks environment as ENV_INVALID, we must
>> reset the $ret return value to -ENOENT to let the env init code reset the
>> environment to the default one a bit further down.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> ---
>>  env/env.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/env/env.c b/env/env.c
>> index dcc25c030b..024d36fdbe 100644
>> --- a/env/env.c
>> +++ b/env/env.c
>> @@ -300,6 +300,9 @@ int env_init(void)
>>  
>>  		debug("%s: Environment %s init done (ret=%d)\n", __func__,
>>  		      drv->name, ret);
>> +
>> +		if (gd->env_valid == ENV_INVALID)
>> +			ret = -ENOENT;
>>  	}
>>  
>>  	if (!prio)
> 
> Is the storage driver marking the environment as invalid but not
> returning ENOENT valid?

Yes, some are doing that.

> How does all of this work in the case of multiple configured storage
> drivers?

If the env is invalid, then we report it as invalid.

[PATCH 6/6] env: Add support for explicit write access list

[Marek Vasut]

On 6/5/20 9:07 PM, Tom Rini wrote:
> On Wed, Jun 03, 2020 at 02:01:11AM +0200, Marek Vasut wrote:
> 
>> This option marks any U-Boot variable which does not have explicit 'w'
>> writeable flag set as read-only. This way the environment can be locked
>> down and only variables explicitly configured to be writeable can ever
>> be changed by either 'env import', 'env set' or loading user environment
>> from environment storage.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
> [snip]
>>  static const char env_flags_vartype_rep[] = "sdxb" ENV_FLAGS_NET_VARTYPE_REPS;
>> -static const char env_flags_varaccess_rep[] = "aroc";
>> +static const char env_flags_varaccess_rep[] =
>> +	"aroc" ENV_FLAGS_WRITEABLE_VARACCESS_REPS;
> 
> nit: I think this is an example of why going a tiny bit past 80 chars
> wide improves readability.  If inclined and someone else asks for a v2,
> go ahead.
> 
> I think all of the code itself is fine, but it's complex enough in a
> complex area I'm not adding my own Reviewed-by.
> 
> What I would ask for is to add some tests to test/py/tests/test_env.py
> for what we can test for.  Thanks!

I was hoping someone could properly review this.

[PATCH 3/6] env: Fix invalid env handling in env_init()

[Tom Rini]

On Fri, Jun 05, 2020 at 10:47:24PM +0200, Marek Vasut wrote:
> On 6/5/20 9:07 PM, Tom Rini wrote:
> > On Wed, Jun 03, 2020 at 02:01:08AM +0200, Marek Vasut wrote:
> > 
> >> In case the env storage driver marks environment as ENV_INVALID, we must
> >> reset the $ret return value to -ENOENT to let the env init code reset the
> >> environment to the default one a bit further down.
> >>
> >> Signed-off-by: Marek Vasut <marex@denx.de>
> >> ---
> >>  env/env.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/env/env.c b/env/env.c
> >> index dcc25c030b..024d36fdbe 100644
> >> --- a/env/env.c
> >> +++ b/env/env.c
> >> @@ -300,6 +300,9 @@ int env_init(void)
> >>  
> >>  		debug("%s: Environment %s init done (ret=%d)\n", __func__,
> >>  		      drv->name, ret);
> >> +
> >> +		if (gd->env_valid == ENV_INVALID)
> >> +			ret = -ENOENT;
> >>  	}
> >>  
> >>  	if (!prio)
> > 
> > Is the storage driver marking the environment as invalid but not
> > returning ENOENT valid?
> 
> Yes, some are doing that.

Why?  Is that correct or incorrect?  It doesn't seem like this is
something that should be inconsistent from storage driver to storage
driver and needs fixing.

> > How does all of this work in the case of multiple configured storage
> > drivers?
> 
> If the env is invalid, then we report it as invalid.

Right.  And what change, if any, does your proposed change have in this
case?  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200605/77f349a1/attachment.sig>

[PATCH 3/6] env: Fix invalid env handling in env_init()

[Marek Vasut]

On 6/5/20 11:11 PM, Tom Rini wrote:
> On Fri, Jun 05, 2020 at 10:47:24PM +0200, Marek Vasut wrote:
>> On 6/5/20 9:07 PM, Tom Rini wrote:
>>> On Wed, Jun 03, 2020 at 02:01:08AM +0200, Marek Vasut wrote:
>>>
>>>> In case the env storage driver marks environment as ENV_INVALID, we must
>>>> reset the $ret return value to -ENOENT to let the env init code reset the
>>>> environment to the default one a bit further down.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>> ---
>>>>  env/env.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/env/env.c b/env/env.c
>>>> index dcc25c030b..024d36fdbe 100644
>>>> --- a/env/env.c
>>>> +++ b/env/env.c
>>>> @@ -300,6 +300,9 @@ int env_init(void)
>>>>  
>>>>  		debug("%s: Environment %s init done (ret=%d)\n", __func__,
>>>>  		      drv->name, ret);
>>>> +
>>>> +		if (gd->env_valid == ENV_INVALID)
>>>> +			ret = -ENOENT;
>>>>  	}
>>>>  
>>>>  	if (!prio)
>>>
>>> Is the storage driver marking the environment as invalid but not
>>> returning ENOENT valid?
>>
>> Yes, some are doing that.
> 
> Why?  Is that correct or incorrect?  It doesn't seem like this is
> something that should be inconsistent from storage driver to storage
> driver and needs fixing.

The default env driver is doing it, whether it's a workaround or correct
behavior, I really don't know. Maybe Joe does ?

>>> How does all of this work in the case of multiple configured storage
>>> drivers?
>>
>> If the env is invalid, then we report it as invalid.
> 
> Right.  And what change, if any, does your proposed change have in this
> case?  Thanks!

Before this patch, that check was missing and the result was random,
depending on which env order you had.

[PATCH 3/6] env: Fix invalid env handling in env_init()

[Tom Rini]

On Sat, Jun 06, 2020 at 04:54:52PM +0200, Marek Vasut wrote:
> On 6/5/20 11:11 PM, Tom Rini wrote:
> > On Fri, Jun 05, 2020 at 10:47:24PM +0200, Marek Vasut wrote:
> >> On 6/5/20 9:07 PM, Tom Rini wrote:
> >>> On Wed, Jun 03, 2020 at 02:01:08AM +0200, Marek Vasut wrote:
> >>>
> >>>> In case the env storage driver marks environment as ENV_INVALID, we must
> >>>> reset the $ret return value to -ENOENT to let the env init code reset the
> >>>> environment to the default one a bit further down.
> >>>>
> >>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>> ---
> >>>>  env/env.c | 3 +++
> >>>>  1 file changed, 3 insertions(+)
> >>>>
> >>>> diff --git a/env/env.c b/env/env.c
> >>>> index dcc25c030b..024d36fdbe 100644
> >>>> --- a/env/env.c
> >>>> +++ b/env/env.c
> >>>> @@ -300,6 +300,9 @@ int env_init(void)
> >>>>  
> >>>>  		debug("%s: Environment %s init done (ret=%d)\n", __func__,
> >>>>  		      drv->name, ret);
> >>>> +
> >>>> +		if (gd->env_valid == ENV_INVALID)
> >>>> +			ret = -ENOENT;
> >>>>  	}
> >>>>  
> >>>>  	if (!prio)
> >>>
> >>> Is the storage driver marking the environment as invalid but not
> >>> returning ENOENT valid?
> >>
> >> Yes, some are doing that.
> > 
> > Why?  Is that correct or incorrect?  It doesn't seem like this is
> > something that should be inconsistent from storage driver to storage
> > driver and needs fixing.
> 
> The default env driver is doing it, whether it's a workaround or correct
> behavior, I really don't know. Maybe Joe does ?
> 
> >>> How does all of this work in the case of multiple configured storage
> >>> drivers?
> >>
> >> If the env is invalid, then we report it as invalid.
> > 
> > Right.  And what change, if any, does your proposed change have in this
> > case?  Thanks!
> 
> Before this patch, that check was missing and the result was random,
> depending on which env order you had.

So have you changed the behavior of multiple environments then?  Today
it's indeed link order based, which is not optimal but used.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200606/4c53cc6f/attachment.sig>

[PATCH 3/6] env: Fix invalid env handling in env_init()

[Marek Vasut]

On 6/6/20 6:24 PM, Tom Rini wrote:
> On Sat, Jun 06, 2020 at 04:54:52PM +0200, Marek Vasut wrote:
>> On 6/5/20 11:11 PM, Tom Rini wrote:
>>> On Fri, Jun 05, 2020 at 10:47:24PM +0200, Marek Vasut wrote:
>>>> On 6/5/20 9:07 PM, Tom Rini wrote:
>>>>> On Wed, Jun 03, 2020 at 02:01:08AM +0200, Marek Vasut wrote:
>>>>>
>>>>>> In case the env storage driver marks environment as ENV_INVALID, we must
>>>>>> reset the $ret return value to -ENOENT to let the env init code reset the
>>>>>> environment to the default one a bit further down.
>>>>>>
>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>> ---
>>>>>>  env/env.c | 3 +++
>>>>>>  1 file changed, 3 insertions(+)
>>>>>>
>>>>>> diff --git a/env/env.c b/env/env.c
>>>>>> index dcc25c030b..024d36fdbe 100644
>>>>>> --- a/env/env.c
>>>>>> +++ b/env/env.c
>>>>>> @@ -300,6 +300,9 @@ int env_init(void)
>>>>>>  
>>>>>>  		debug("%s: Environment %s init done (ret=%d)\n", __func__,
>>>>>>  		      drv->name, ret);
>>>>>> +
>>>>>> +		if (gd->env_valid == ENV_INVALID)
>>>>>> +			ret = -ENOENT;
>>>>>>  	}
>>>>>>  
>>>>>>  	if (!prio)
>>>>>
>>>>> Is the storage driver marking the environment as invalid but not
>>>>> returning ENOENT valid?
>>>>
>>>> Yes, some are doing that.
>>>
>>> Why?  Is that correct or incorrect?  It doesn't seem like this is
>>> something that should be inconsistent from storage driver to storage
>>> driver and needs fixing.
>>
>> The default env driver is doing it, whether it's a workaround or correct
>> behavior, I really don't know. Maybe Joe does ?
>>
>>>>> How does all of this work in the case of multiple configured storage
>>>>> drivers?
>>>>
>>>> If the env is invalid, then we report it as invalid.
>>>
>>> Right.  And what change, if any, does your proposed change have in this
>>> case?  Thanks!
>>
>> Before this patch, that check was missing and the result was random,
>> depending on which env order you had.
> 
> So have you changed the behavior of multiple environments then?  Today
> it's indeed link order based, which is not optimal but used.

Yes, I believe this patch makes it work as intended.

[PATCH 3/6] env: Fix invalid env handling in env_init()

[Tom Rini]

On Mon, Jun 08, 2020 at 11:45:18PM +0200, Marek Vasut wrote:
> On 6/6/20 6:24 PM, Tom Rini wrote:
> > On Sat, Jun 06, 2020 at 04:54:52PM +0200, Marek Vasut wrote:
> >> On 6/5/20 11:11 PM, Tom Rini wrote:
> >>> On Fri, Jun 05, 2020 at 10:47:24PM +0200, Marek Vasut wrote:
> >>>> On 6/5/20 9:07 PM, Tom Rini wrote:
> >>>>> On Wed, Jun 03, 2020 at 02:01:08AM +0200, Marek Vasut wrote:
> >>>>>
> >>>>>> In case the env storage driver marks environment as ENV_INVALID, we must
> >>>>>> reset the $ret return value to -ENOENT to let the env init code reset the
> >>>>>> environment to the default one a bit further down.
> >>>>>>
> >>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>>>> ---
> >>>>>>  env/env.c | 3 +++
> >>>>>>  1 file changed, 3 insertions(+)
> >>>>>>
> >>>>>> diff --git a/env/env.c b/env/env.c
> >>>>>> index dcc25c030b..024d36fdbe 100644
> >>>>>> --- a/env/env.c
> >>>>>> +++ b/env/env.c
> >>>>>> @@ -300,6 +300,9 @@ int env_init(void)
> >>>>>>  
> >>>>>>  		debug("%s: Environment %s init done (ret=%d)\n", __func__,
> >>>>>>  		      drv->name, ret);
> >>>>>> +
> >>>>>> +		if (gd->env_valid == ENV_INVALID)
> >>>>>> +			ret = -ENOENT;
> >>>>>>  	}
> >>>>>>  
> >>>>>>  	if (!prio)
> >>>>>
> >>>>> Is the storage driver marking the environment as invalid but not
> >>>>> returning ENOENT valid?
> >>>>
> >>>> Yes, some are doing that.
> >>>
> >>> Why?  Is that correct or incorrect?  It doesn't seem like this is
> >>> something that should be inconsistent from storage driver to storage
> >>> driver and needs fixing.
> >>
> >> The default env driver is doing it, whether it's a workaround or correct
> >> behavior, I really don't know. Maybe Joe does ?
> >>
> >>>>> How does all of this work in the case of multiple configured storage
> >>>>> drivers?
> >>>>
> >>>> If the env is invalid, then we report it as invalid.
> >>>
> >>> Right.  And what change, if any, does your proposed change have in this
> >>> case?  Thanks!
> >>
> >> Before this patch, that check was missing and the result was random,
> >> depending on which env order you had.
> > 
> > So have you changed the behavior of multiple environments then?  Today
> > it's indeed link order based, which is not optimal but used.
> 
> Yes, I believe this patch makes it work as intended.

Which is what?  Since I believe it works as intended today.  If there's
some behavior change / correction here in this case you need to spell it
out in the commit message.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200608/0f248bbf/attachment.sig>

[PATCH 3/6] env: Fix invalid env handling in env_init()

[Marek Vasut]

On 6/9/20 12:57 AM, Tom Rini wrote:
> On Mon, Jun 08, 2020 at 11:45:18PM +0200, Marek Vasut wrote:
>> On 6/6/20 6:24 PM, Tom Rini wrote:
>>> On Sat, Jun 06, 2020 at 04:54:52PM +0200, Marek Vasut wrote:
>>>> On 6/5/20 11:11 PM, Tom Rini wrote:
>>>>> On Fri, Jun 05, 2020 at 10:47:24PM +0200, Marek Vasut wrote:
>>>>>> On 6/5/20 9:07 PM, Tom Rini wrote:
>>>>>>> On Wed, Jun 03, 2020 at 02:01:08AM +0200, Marek Vasut wrote:
>>>>>>>
>>>>>>>> In case the env storage driver marks environment as ENV_INVALID, we must
>>>>>>>> reset the $ret return value to -ENOENT to let the env init code reset the
>>>>>>>> environment to the default one a bit further down.
>>>>>>>>
>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>>>> ---
>>>>>>>>  env/env.c | 3 +++
>>>>>>>>  1 file changed, 3 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/env/env.c b/env/env.c
>>>>>>>> index dcc25c030b..024d36fdbe 100644
>>>>>>>> --- a/env/env.c
>>>>>>>> +++ b/env/env.c
>>>>>>>> @@ -300,6 +300,9 @@ int env_init(void)
>>>>>>>>  
>>>>>>>>  		debug("%s: Environment %s init done (ret=%d)\n", __func__,
>>>>>>>>  		      drv->name, ret);
>>>>>>>> +
>>>>>>>> +		if (gd->env_valid == ENV_INVALID)
>>>>>>>> +			ret = -ENOENT;
>>>>>>>>  	}
>>>>>>>>  
>>>>>>>>  	if (!prio)
>>>>>>>
>>>>>>> Is the storage driver marking the environment as invalid but not
>>>>>>> returning ENOENT valid?
>>>>>>
>>>>>> Yes, some are doing that.
>>>>>
>>>>> Why?  Is that correct or incorrect?  It doesn't seem like this is
>>>>> something that should be inconsistent from storage driver to storage
>>>>> driver and needs fixing.
>>>>
>>>> The default env driver is doing it, whether it's a workaround or correct
>>>> behavior, I really don't know. Maybe Joe does ?
>>>>
>>>>>>> How does all of this work in the case of multiple configured storage
>>>>>>> drivers?
>>>>>>
>>>>>> If the env is invalid, then we report it as invalid.
>>>>>
>>>>> Right.  And what change, if any, does your proposed change have in this
>>>>> case?  Thanks!
>>>>
>>>> Before this patch, that check was missing and the result was random,
>>>> depending on which env order you had.
>>>
>>> So have you changed the behavior of multiple environments then?  Today
>>> it's indeed link order based, which is not optimal but used.
>>
>> Yes, I believe this patch makes it work as intended.
> 
> Which is what?  Since I believe it works as intended today.  If there's
> some behavior change / correction here in this case you need to spell it
> out in the commit message.  Thanks!

I think the commit message is quite clear that if the env is ENV_INVALID
, then we need to set the return value to -ENOENT, not random.

[PATCH 3/6] env: Fix invalid env handling in env_init()

[Tom Rini]

On Tue, Jun 09, 2020 at 03:50:09AM +0200, Marek Vasut wrote:
> On 6/9/20 12:57 AM, Tom Rini wrote:
> > On Mon, Jun 08, 2020 at 11:45:18PM +0200, Marek Vasut wrote:
> >> On 6/6/20 6:24 PM, Tom Rini wrote:
> >>> On Sat, Jun 06, 2020 at 04:54:52PM +0200, Marek Vasut wrote:
> >>>> On 6/5/20 11:11 PM, Tom Rini wrote:
> >>>>> On Fri, Jun 05, 2020 at 10:47:24PM +0200, Marek Vasut wrote:
> >>>>>> On 6/5/20 9:07 PM, Tom Rini wrote:
> >>>>>>> On Wed, Jun 03, 2020 at 02:01:08AM +0200, Marek Vasut wrote:
> >>>>>>>
> >>>>>>>> In case the env storage driver marks environment as ENV_INVALID, we must
> >>>>>>>> reset the $ret return value to -ENOENT to let the env init code reset the
> >>>>>>>> environment to the default one a bit further down.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>>>>>> ---
> >>>>>>>>  env/env.c | 3 +++
> >>>>>>>>  1 file changed, 3 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/env/env.c b/env/env.c
> >>>>>>>> index dcc25c030b..024d36fdbe 100644
> >>>>>>>> --- a/env/env.c
> >>>>>>>> +++ b/env/env.c
> >>>>>>>> @@ -300,6 +300,9 @@ int env_init(void)
> >>>>>>>>  
> >>>>>>>>  		debug("%s: Environment %s init done (ret=%d)\n", __func__,
> >>>>>>>>  		      drv->name, ret);
> >>>>>>>> +
> >>>>>>>> +		if (gd->env_valid == ENV_INVALID)
> >>>>>>>> +			ret = -ENOENT;
> >>>>>>>>  	}
> >>>>>>>>  
> >>>>>>>>  	if (!prio)
> >>>>>>>
> >>>>>>> Is the storage driver marking the environment as invalid but not
> >>>>>>> returning ENOENT valid?
> >>>>>>
> >>>>>> Yes, some are doing that.
> >>>>>
> >>>>> Why?  Is that correct or incorrect?  It doesn't seem like this is
> >>>>> something that should be inconsistent from storage driver to storage
> >>>>> driver and needs fixing.
> >>>>
> >>>> The default env driver is doing it, whether it's a workaround or correct
> >>>> behavior, I really don't know. Maybe Joe does ?
> >>>>
> >>>>>>> How does all of this work in the case of multiple configured storage
> >>>>>>> drivers?
> >>>>>>
> >>>>>> If the env is invalid, then we report it as invalid.
> >>>>>
> >>>>> Right.  And what change, if any, does your proposed change have in this
> >>>>> case?  Thanks!
> >>>>
> >>>> Before this patch, that check was missing and the result was random,
> >>>> depending on which env order you had.
> >>>
> >>> So have you changed the behavior of multiple environments then?  Today
> >>> it's indeed link order based, which is not optimal but used.
> >>
> >> Yes, I believe this patch makes it work as intended.
> > 
> > Which is what?  Since I believe it works as intended today.  If there's
> > some behavior change / correction here in this case you need to spell it
> > out in the commit message.  Thanks!
> 
> I think the commit message is quite clear that if the env is ENV_INVALID
> , then we need to set the return value to -ENOENT, not random.

If it was clear that you were fixing a problem, or changing the behavior
of how multiple configured environments work we wouldn't be this far in
to an email thread where I'm asking questions about what changed.  Given
the complex use cases of the area we're talking about here, an overly
verbose commit message is far preferred to a terse one.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200609/c1d56d2f/attachment.sig>

[PATCH 6/6] env: Add support for explicit write access list

[Harald Seiler]

Hi Marek,

On Wed, 2020-06-03 at 02:01 +0200, Marek Vasut wrote:
> This option marks any U-Boot variable which does not have explicit 'w'
> writeable flag set as read-only. This way the environment can be locked
> down and only variables explicitly configured to be writeable can ever
> be changed by either 'env import', 'env set' or loading user environment
> from environment storage.

I haven't yet been able to get to the bottom of it but this patch seems to
regress the `envtools` build for me.  Here is the rather weird error message:

       HOSTCC  tools/env/fw_env_main.o
     In file included from tools/env/fw_env.c:15:
     include/env_flags.h:27:22: error: missing binary operator before token "("
      #if CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)
                           ^

Any idea what could be the cause for this?

-- 
Harald

> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
>  env/Kconfig         |  8 ++++
>  env/flags.c         | 92 +++++++++++++++++++++++++++++++++++++++------
>  include/env_flags.h |  6 ++-
>  lib/hashtable.c     |  5 ++-
>  4 files changed, 98 insertions(+), 13 deletions(-)
> 
> diff --git a/env/Kconfig b/env/Kconfig
> index 8166e5df91..f53a1457fb 100644
> --- a/env/Kconfig
> +++ b/env/Kconfig
> @@ -613,6 +613,14 @@ config ENV_APPEND
>  	  with newly imported data. This may be used in combination with static
>  	  flags to e.g. to protect variables which must not be modified.
>  
> +config ENV_WRITEABLE_LIST
> +	bool "Permit write access only to listed variables"
> +	default n
> +	help
> +	  If defined, only environment variables which explicitly set the 'w'
> +	  writeable flag can be written and modified at runtime. No variables
> +	  can be otherwise created, written or imported into the environment.
> +
>  config ENV_ACCESS_IGNORE_FORCE
>  	bool "Block forced environment operations"
>  	default n
> diff --git a/env/flags.c b/env/flags.c
> index f7a53775c4..a2f6c1a3ec 100644
> --- a/env/flags.c
> +++ b/env/flags.c
> @@ -28,8 +28,15 @@
>  #define ENV_FLAGS_NET_VARTYPE_REPS ""
>  #endif
>  
> +#if CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)
> +#define ENV_FLAGS_WRITEABLE_VARACCESS_REPS "w"
> +#else
> +#define ENV_FLAGS_WRITEABLE_VARACCESS_REPS ""
> +#endif
> +
>  static const char env_flags_vartype_rep[] = "sdxb" ENV_FLAGS_NET_VARTYPE_REPS;
> -static const char env_flags_varaccess_rep[] = "aroc";
> +static const char env_flags_varaccess_rep[] =
> +	"aroc" ENV_FLAGS_WRITEABLE_VARACCESS_REPS;
>  static const int env_flags_varaccess_mask[] = {
>  	0,
>  	ENV_FLAGS_VARACCESS_PREVENT_DELETE |
> @@ -38,7 +45,11 @@ static const int env_flags_varaccess_mask[] = {
>  	ENV_FLAGS_VARACCESS_PREVENT_DELETE |
>  		ENV_FLAGS_VARACCESS_PREVENT_OVERWR,
>  	ENV_FLAGS_VARACCESS_PREVENT_DELETE |
> -		ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR};
> +		ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR,
> +#if CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)
> +	ENV_FLAGS_VARACCESS_WRITEABLE,
> +#endif
> +	};
>  
>  #ifdef CONFIG_CMD_ENV_FLAGS
>  static const char * const env_flags_vartype_names[] = {
> @@ -56,6 +67,9 @@ static const char * const env_flags_varaccess_names[] = {
>  	"read-only",
>  	"write-once",
>  	"change-default",
> +#if CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)
> +	"writeable",
> +#endif
>  };
>  
>  /*
> @@ -130,21 +144,33 @@ enum env_flags_vartype env_flags_parse_vartype(const char *flags)
>   */
>  enum env_flags_varaccess env_flags_parse_varaccess(const char *flags)
>  {
> +#if CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)
> +	enum env_flags_varaccess va_default = env_flags_varaccess_readonly;
> +#else
> +	enum env_flags_varaccess va_default = env_flags_varaccess_any;
> +#endif
> +	enum env_flags_varaccess va;
>  	char *access;
>  
>  	if (strlen(flags) <= ENV_FLAGS_VARACCESS_LOC)
> -		return env_flags_varaccess_any;
> +		return va_default;
>  
>  	access = strchr(env_flags_varaccess_rep,
>  		flags[ENV_FLAGS_VARACCESS_LOC]);
>  
> -	if (access != NULL)
> -		return (enum env_flags_varaccess)
> +	if (access != NULL) {
> +		va = (enum env_flags_varaccess)
>  			(access - &env_flags_varaccess_rep[0]);
> +#if CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)
> +		if (va != env_flags_varaccess_writeable)
> +			return env_flags_varaccess_readonly;
> +#endif
> +		return va;
> +	}
>  
>  	printf("## Warning: Unknown environment variable access method '%c'\n",
>  		flags[ENV_FLAGS_VARACCESS_LOC]);
> -	return env_flags_varaccess_any;
> +	return va_default;
>  }
>  
>  /*
> @@ -152,17 +178,29 @@ enum env_flags_varaccess env_flags_parse_varaccess(const char *flags)
>   */
>  enum env_flags_varaccess env_flags_parse_varaccess_from_binflags(int binflags)
>  {
> +#if CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)
> +	enum env_flags_varaccess va_default = env_flags_varaccess_readonly;
> +#else
> +	enum env_flags_varaccess va_default = env_flags_varaccess_any;
> +#endif
> +	enum env_flags_varaccess va;
>  	int i;
>  
>  	for (i = 0; i < ARRAY_SIZE(env_flags_varaccess_mask); i++)
>  		if (env_flags_varaccess_mask[i] ==
> -		    (binflags & ENV_FLAGS_VARACCESS_BIN_MASK))
> -			return (enum env_flags_varaccess)i;
> +		    (binflags & ENV_FLAGS_VARACCESS_BIN_MASK)) {
> +			va = (enum env_flags_varaccess)i;
> +#if CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)
> +			if (va != env_flags_varaccess_writeable)
> +				return env_flags_varaccess_readonly;
> +#endif
> +			return va;
> +	}
>  
>  	printf("Warning: Non-standard access flags. (0x%x)\n",
>  		binflags & ENV_FLAGS_VARACCESS_BIN_MASK);
>  
> -	return env_flags_varaccess_any;
> +	return va_default;
>  }
>  
>  static inline int is_hex_prefix(const char *value)
> @@ -325,14 +363,20 @@ enum env_flags_vartype env_flags_get_type(const char *name)
>   */
>  enum env_flags_varaccess env_flags_get_varaccess(const char *name)
>  {
> +#if CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)
> +	const char *flags_list = NULL;
> +	enum env_flags_varaccess va_default = env_flags_varaccess_readonly;
> +#else
>  	const char *flags_list = env_get(ENV_FLAGS_VAR);
> +	enum env_flags_varaccess va_default = env_flags_varaccess_any;
> +#endif
>  	char flags[ENV_FLAGS_ATTR_MAX_LEN + 1];
>  
>  	if (env_flags_lookup(flags_list, name, flags))
> -		return env_flags_varaccess_any;
> +		return va_default;
>  
>  	if (strlen(flags) <= ENV_FLAGS_VARACCESS_LOC)
> -		return env_flags_varaccess_any;
> +		return va_default;
>  
>  	return env_flags_parse_varaccess(flags);
>  }
> @@ -426,7 +470,11 @@ void env_flags_init(struct env_entry *var_entry)
>  	int ret = 1;
>  
>  	if (first_call) {
> +#if CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)
> +		flags_list = ENV_FLAGS_LIST_STATIC;
> +#else
>  		flags_list = env_get(ENV_FLAGS_VAR);
> +#endif
>  		first_call = 0;
>  	}
>  	/* look in the ".flags" and static for a reference to this variable */
> @@ -435,6 +483,16 @@ void env_flags_init(struct env_entry *var_entry)
>  	/* if any flags were found, set the binary form to the entry */
>  	if (!ret && strlen(flags))
>  		var_entry->flags = env_parse_flags_to_bin(flags);
> +
> +#if CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)
> +	/* Anything which is not explicitly writeable is read-only */
> +	if (!(var_entry->flags & ENV_FLAGS_VARACCESS_WRITEABLE)) {
> +		var_entry->flags &= ~ENV_FLAGS_VARACCESS_BIN_MASK;
> +		var_entry->flags |= ENV_FLAGS_VARACCESS_PREVENT_DELETE |
> +				ENV_FLAGS_VARACCESS_PREVENT_CREATE |
> +				ENV_FLAGS_VARACCESS_PREVENT_OVERWR;
> +	}
> +#endif
>  }
>  
>  /*
> @@ -523,6 +581,18 @@ int env_flags_validate(const struct env_entry *item, const char *newval,
>  	}
>  
>  	/* check for access permission */
> +#if CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)
> +	if (flag & H_DEFAULT)
> +		return 0;	/* Default env is always OK */
> +
> +	/* Writeable variables can be overwritten, anything else can not */
> +	if (op == env_op_overwrite &&
> +	    item->flags & ENV_FLAGS_VARACCESS_WRITEABLE)
> +		return 0;
> +
> +	return 1;
> +#endif
> +
>  #ifndef CONFIG_ENV_ACCESS_IGNORE_FORCE
>  	if (flag & H_FORCE) {
>  		printf("## Error: Can't force access to \"%s\"\n", name);
> diff --git a/include/env_flags.h b/include/env_flags.h
> index 725841a891..62c3dd9fa0 100644
> --- a/include/env_flags.h
> +++ b/include/env_flags.h
> @@ -24,6 +24,9 @@ enum env_flags_varaccess {
>  	env_flags_varaccess_readonly,
>  	env_flags_varaccess_writeonce,
>  	env_flags_varaccess_changedefault,
> +#if CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)
> +	env_flags_varaccess_writeable,
> +#endif
>  	env_flags_varaccess_end
>  };
>  
> @@ -173,6 +176,7 @@ int env_flags_validate(const struct env_entry *item, const char *newval,
>  #define ENV_FLAGS_VARACCESS_PREVENT_CREATE		0x00000010
>  #define ENV_FLAGS_VARACCESS_PREVENT_OVERWR		0x00000020
>  #define ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR	0x00000040
> -#define ENV_FLAGS_VARACCESS_BIN_MASK			0x00000078
> +#define ENV_FLAGS_VARACCESS_WRITEABLE			0x00000080
> +#define ENV_FLAGS_VARACCESS_BIN_MASK			0x000000f8
>  
>  #endif /* __ENV_FLAGS_H__ */
> diff --git a/lib/hashtable.c b/lib/hashtable.c
> index 24aef5a085..bebad9d382 100644
> --- a/lib/hashtable.c
> +++ b/lib/hashtable.c
> @@ -946,9 +946,12 @@ int himport_r(struct hsearch_data *htab,
>  		e.data = value;
>  
>  		hsearch_r(e, ENV_ENTER, &rv, htab, flag);
> -		if (rv == NULL)
> +#if !CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)
> +		if (rv == NULL) {
>  			printf("himport_r: can't insert \"%s=%s\" into hash table\n",
>  				name, value);
> +		}
> +#endif
>  
>  		debug("INSERT: table %p, filled %d/%d rv %p ==> name=\"%s\" value=\"%s\"\n",
>  			htab, htab->filled, htab->size,

[PATCH 6/6] env: Add support for explicit write access list

[Tom Rini]

On Thu, Jun 25, 2020 at 08:12:57PM +0200, Harald Seiler wrote:
> Hi Marek,
> 
> On Wed, 2020-06-03 at 02:01 +0200, Marek Vasut wrote:
> > This option marks any U-Boot variable which does not have explicit 'w'
> > writeable flag set as read-only. This way the environment can be locked
> > down and only variables explicitly configured to be writeable can ever
> > be changed by either 'env import', 'env set' or loading user environment
> > from environment storage.
> 
> I haven't yet been able to get to the bottom of it but this patch seems to
> regress the `envtools` build for me.  Here is the rather weird error message:
> 
>        HOSTCC  tools/env/fw_env_main.o
>      In file included from tools/env/fw_env.c:15:
>      include/env_flags.h:27:22: error: missing binary operator before token "("
>       #if CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)
>                            ^
> 
> Any idea what could be the cause for this?

We can't use CONFIG_IS_ENABLED()/etc in host-tool code, the macros
aren't exposed.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200625/84457d5e/attachment.sig>