[03/11] env: add support for whitelisting variables from secondary environments

Message ID 45316f927ce86b20958468367f7d642a76f32888.1513975247.git-series.quentin.schulz@free-electrons.com
State New
Headers show
Series
  • Introduce variables whitelisting in environment
Related show

Commit Message

Quentin Schulz Dec. 22, 2017, 9:13 p.m.
This patch makes it possible to have a first environment that will be
the base environment, secondary environment that will be pre-loaded and
have its variables passing through the whitelist "mask" to override
variables in the base environment.

The base environment will be the environment with the highest priority
(0 is highest) that can load and the next loadable environment will be
the secondary environment (with a lowest priority than the base
environment of course).

The whitelist "mask" is built with the content of space-separated list
of environment variables in ENV_VAR_WHITELIST_LIST in Kconfig.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
 env/Kconfig           |  17 +++++++-
 env/common.c          |   7 +++-
 env/env.c             | 100 +++++++++++++++++++++++++++++++++++++++----
 include/environment.h |   6 +++-
 4 files changed, 123 insertions(+), 7 deletions(-)

Comments

Lukasz Majewski Dec. 26, 2017, 11:21 a.m. | #1
Hi Quentin,

> +config ENV_VAR_WHITELIST

> +	bool "Enable overriding some variables from secondary

> environments"

> +	help

> +	  This allows overriding some variables from secondary

> environments.

> +	  After the first valid environment is loaded, a secondary

> environment

> +	  is pre-loaded and its variables that are present in the

> whitelist will

> +	  be added to the current environment or will override

> existing

> +	  variables.


This description seems to do what is already done in the "env import"
command.

It also allows for setting env variables from HUSH prompt.

Maybe it would be possible to re-use or extend it to suits your needs?


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Quentin Schulz Jan. 3, 2018, 11:07 a.m. | #2
Hi Lukasz,

On 26/12/2017 12:21, Lukasz Majewski wrote:
> Hi Quentin,

> 

>> +config ENV_VAR_WHITELIST

>> +	bool "Enable overriding some variables from secondary

>> environments"

>> +	help

>> +	  This allows overriding some variables from secondary

>> environments.

>> +	  After the first valid environment is loaded, a secondary

>> environment

>> +	  is pre-loaded and its variables that are present in the

>> whitelist will

>> +	  be added to the current environment or will override

>> existing

>> +	  variables.

> 

> This description seems to do what is already done in the "env import"

> command.

> 


Didn't know that one, thanks.

> It also allows for setting env variables from HUSH prompt.

> 

> Maybe it would be possible to re-use or extend it to suits your needs?

> 


It's pretty much everything I needed except the whitelisting of variables.

While it's possible for env export to take a list of variables to
export, for what I've seen, env import does not take a list of variables
to import.

It's needed since the env I'll import is unsafe. I'll have a look at
that and make an other RFC :)

Thanks for the review, very helpful,

Quentin
-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Patch

diff --git a/env/Kconfig b/env/Kconfig
index 1952463..d57594d 100644
--- a/env/Kconfig
+++ b/env/Kconfig
@@ -438,4 +438,21 @@  config ENV_UBI_VOLUME
 
 endif
 
+config ENV_VAR_WHITELIST
+	bool "Enable overriding some variables from secondary environments"
+	help
+	  This allows overriding some variables from secondary environments.
+	  After the first valid environment is loaded, a secondary environment
+	  is pre-loaded and its variables that are present in the whitelist will
+	  be added to the current environment or will override existing
+	  variables.
+
+config ENV_VAR_WHITELIST_LIST
+	depends on ENV_VAR_WHITELIST
+	string "Whitelist of variables from secondary environments"
+	help
+	  This defines the variables that are allowed to be overriden by
+	  ones from secondary environments.
+	  This is a list of environment variables that are space-separated.
+
 endmenu
diff --git a/env/common.c b/env/common.c
index 9d97541..00c454d 100644
--- a/env/common.c
+++ b/env/common.c
@@ -177,8 +177,15 @@  int env_import(const char *buf, int check)
 		return ret;
 	}
 
+#ifdef CONFIG_ENV_VAR_WHITELIST
+	if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0',
+		      whitelisting ? H_NOCLEAR : 0, 0,
+		      whitelisting ? whitelist_nvars : 0,
+		      whitelisting ? whitelist : NULL, whitelisting)) {
+#else
 	if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0, 0,
 			0, NULL, false)) {
+#endif
 		gd->flags |= GD_FLG_ENV_READY;
 		return 1;
 	}
diff --git a/env/env.c b/env/env.c
index 5e70ddf..43a62b8 100644
--- a/env/env.c
+++ b/env/env.c
@@ -7,9 +7,18 @@ 
 
 #include <common.h>
 #include <environment.h>
+#ifdef CONFIG_ENV_VAR_WHITELIST
+# include <malloc.h>
+#endif
 
 DECLARE_GLOBAL_DATA_PTR;
 
+#ifdef CONFIG_ENV_VAR_WHITELIST
+char *const *whitelist;
+bool whitelisting;
+unsigned int whitelist_nvars;
+#endif
+
 static struct env_driver *_env_driver_lookup(enum env_location loc)
 {
 	struct env_driver *drv;
@@ -62,6 +71,31 @@  static enum env_location env_locations[] = {
 
 static enum env_location env_load_location;
 
+#ifdef CONFIG_ENV_VAR_WHITELIST
+void env_create_whitelist(void)
+{
+	char **wl_vars;
+	char *wl = strchr(CONFIG_ENV_VAR_WHITELIST_LIST, ' ');
+	unsigned int nvars = 0;
+
+	while(wl) {
+		nvars++;
+		wl = strchr(wl + 1, ' ');
+	}
+
+	whitelist_nvars = nvars + 1;
+
+	wl_vars = malloc(sizeof(char *) * (nvars + 1));
+
+	wl_vars[nvars] = strtok(CONFIG_ENV_VAR_WHITELIST_LIST, " ");
+	while (nvars) {
+		wl_vars[--nvars] = strtok(NULL, " ");
+	}
+
+	whitelist = wl_vars;
+}
+#endif
+
 __weak enum env_location env_get_location(enum env_operation op, int prio)
 {
 	switch (op) {
@@ -74,7 +108,11 @@  __weak enum env_location env_get_location(enum env_operation op, int prio)
 		return env_load_location = env_locations[prio];
 
 	case ENVO_SAVE:
+#ifdef CONFIG_ENV_VAR_WHITELIST
+		return env_locations[prio];
+#else
 		return env_load_location;
+#endif
 	}
 
 	return ENVL_UNKNOWN;
@@ -130,10 +168,12 @@  int env_load(void)
 {
 	struct env_driver *drv;
 	int prio;
+	int ret = 1;
+#ifdef CONFIG_ENV_VAR_WHITELIST
+	bool found = false;
+#endif
 
 	for (prio = 0; (drv = env_driver_lookup(ENVO_LOAD, prio)); prio++) {
-		int ret;
-
 		if (!drv->load)
 			continue;
 
@@ -144,16 +184,52 @@  int env_load(void)
 		ret = drv->load();
 		printf("%s\n", ret ? "Failed" : "OK");
 		if (!ret)
-			return 0;
+			break;
 	}
 
-	return -ENODEV;
+	if (ret)
+		return -ENODEV;
+
+#ifdef CONFIG_ENV_VAR_WHITELIST
+	env_create_whitelist();
+
+	whitelisting = true;
+	/* When whitelisting, env from prio x+n will override env from prio x */
+	for (prio++; prio < ARRAY_SIZE(env_locations); prio++) {
+		drv = env_driver_lookup(ENVO_LOAD, prio);
+		if (!drv)
+			continue;
+
+		if (!drv->load)
+			continue;
+
+		printf("Overriding env variables with ones from %s env...",
+		      __func__, drv->name);
+		ret = drv->load();
+		printf("%s\n", ret ? "Failed" : "OK");
+		if (!ret) {
+			found = true;
+			continue;
+		}
+	}
+
+out:
+	if (!found)
+		debug("%s: Couldn't find other valid env for whitelisting\n",
+		      __func__);
+
+	whitelisting = false;
+#endif
+	return 0;
 }
 
 int env_save(void)
 {
 	struct env_driver *drv;
 	int prio;
+#ifdef CONFIG_ENV_VAR_WHITELIST
+	bool saved = false;
+#endif
 
 	for (prio = 0; (drv = env_driver_lookup(ENVO_SAVE, prio)); prio++) {
 		int ret;
@@ -167,13 +243,23 @@  int env_save(void)
 		printf("Saving Environment to %s... ", drv->name);
 		ret = drv->save();
 		printf("%s\n", ret ? "Failed" : "OK");
+#ifdef CONFIG_ENV_VAR_WHITELIST
+		/* When whitelisting, we want to save to all media available */
+		if (!ret) {
+			saved = true;
+			continue;
+		}
+#else
 		if (!ret)
 			return 0;
-
-		debug("%s: Environment %s failed to save (err=%d)\n", __func__,
-		      drv->name, ret);
+#endif
 	}
 
+#ifdef CONFIG_ENV_VAR_WHITELIST
+	if (saved)
+		return 0;
+#endif
+
 	return -ENODEV;
 }
 
diff --git a/include/environment.h b/include/environment.h
index 7fa8b98..33e47ba 100644
--- a/include/environment.h
+++ b/include/environment.h
@@ -267,6 +267,12 @@  struct env_driver {
 	int (*init)(void);
 };
 
+#ifdef CONFIG_ENV_VAR_WHITELIST
+extern char *const *whitelist;
+extern unsigned int whitelist_nvars;
+extern bool whitelisting;
+#endif
+
 /* Declare a new environment location driver */
 #define U_BOOT_ENV_LOCATION(__name)					\
 	ll_entry_declare(struct env_driver, __name, env_driver)