diff mbox series

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

Message ID 20200603000111.7919-6-marex@denx.de
State Superseded
Delegated to: Joe Hershberger
Headers show
Series [1/6] env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set | expand

Commit Message

Marek Vasut June 3, 2020, 12:01 a.m. UTC 
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(-)

Comments

Tom Rini June 5, 2020, 7:07 p.m. UTC | #1 
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!
Marek Vasut June 5, 2020, 8:48 p.m. UTC | #2 
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.
Harald Seiler June 25, 2020, 6:12 p.m. UTC | #3 
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?
Tom Rini June 25, 2020, 9:42 p.m. UTC | #4 
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.
diff mbox series

Patch

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,