Re: [U-Boot] [PATCH] cmd: nvedit: add whitelist option for env import

[Quentin Schulz]

Hi all,

On Thu, Jan 04, 2018 at 11:39:15AM +0100, Quentin Schulz wrote:
> While the `env export` can take as parameters variables to be exported,
> `env import` does not have such a mechanism of variable selection.
> 
> Let's add a `-w` option that asks `env import` to look for the
> `whitelisted_vars` env variable for a space-separated list of variables
> that are whitelisted.
> 
> Every env variable present in env at `addr` and in `whitelisted_vars`
> env variable will override the value of the variable in the current env.
> All the remaining variables are left untouched.
> 
> One of its use case could be to load a secure environment from the
> signed U-Boot binary and load only a handful of variables from an
> other, unsecure, environment without completely losing control of
> U-Boot.
> 
> Signed-off-by: Quentin Schulz 

It's been two months since I posted this patch and there hasn't been any
review.

Is there something to improve?
Is there an other approach to take?

I'm all ears.

Thanks,
Quentin

> ---
>  cmd/nvedit.c | 71 
> ++--
>  1 file changed, 64 insertions(+), 7 deletions(-)
> 
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index 4e79d03856..9f983b41a1 100644
> --- a/cmd/nvedit.c
> +++ b/cmd/nvedit.c
> @@ -971,7 +971,7 @@ sep_err:
>  
>  #ifdef CONFIG_CMD_IMPORTENV
>  /*
> - * env import [-d] [-t [-r] | -b | -c] addr [size]
> + * env import [-d] [-t [-r] | -b | -c] [-w] addr [size]
>   *   -d: delete existing environment before importing;
>   *   otherwise overwrite / append to existing definitions
>   *   -t: assume text format; either "size" must be given or the
> @@ -982,6 +982,10 @@ sep_err:
>   *   for line endings. Only effective in addition to -t.
>   *   -b: assume binary format ('\0' separated, "\0\0" terminated)
>   *   -c: assume checksum protected environment format
> + *   -w: specify that whitelisting of variables should be used when
> + *   importing environment. The space-separated list of variables
> + *   that should override the ones in current environment is stored
> + *   in `whitelisted_vars`.
>   *   addr:   memory address to read from
>   *   size:   length of input data; if missing, proper '\0'
>   *   termination is mandatory
> @@ -991,11 +995,16 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
>  {
>   ulong   addr;
>   char*cmd, *ptr;
> + char**array = NULL;
>   charsep = '\n';
>   int chk = 0;
>   int fmt = 0;
>   int del = 0;
>   int crlf_is_lf = 0;
> + int wl = 0;
> + int wl_count = 0;
> + int ret;
> + unsigned int i;
>   size_t  size;
>  
>   cmd = *argv;
> @@ -1026,6 +1035,9 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
>   case 'd':
>   del = 1;
>   break;
> + case 'w':
> + wl = 1;
> + break;
>   default:
>   return CMD_RET_USAGE;
>   }
> @@ -1082,14 +1094,59 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
>   ptr = (char *)ep->data;
>   }
>  
> - if (himport_r(_htab, ptr, size, sep, del ? 0 : H_NOCLEAR,
> - crlf_is_lf, 0, NULL) == 0) {
> + if(wl) {
> + char *str, *token, *tmp;
> + wl_count = 1;
> +
> + str = env_get("whitelisted_vars");
> + if (!str) {
> + puts("## Error: whitelisted_vars is not set.\n");
> + return CMD_RET_USAGE;
> + }
> +
> + tmp = malloc(sizeof(char) * (strlen(str) + 1));
> + strcpy(tmp, str);
> +
> + token = strchr(tmp, ' ');
> + while (!token) {
> + wl_count++;
> + token = strchr(token + 1, ' ');
> + }
> +
> + strcpy(tmp, str);
> +
> + array = malloc(sizeof(char *) * wl_count);
> + wl_count = 0;
> +
> + token = strtok(tmp, " ");
> + while (token) {
> + array[wl_count] = malloc(sizeof(char) *
> +  (strlen(token) + 1));
> + strcpy(array[wl_count], token);
> + wl_count++;
> + token = strtok(NULL, " ");
> + }
> +
> + free(tmp);
> + }
> +
> + ret = himport_r(_htab, ptr, size, sep, del ? 0 : H_NOCLEAR,
> + crlf_is_lf, wl ? wl_count : 0, wl ? array : NULL);
> + if (!ret) {
>   pr_err("Environment import failed: errno = %d\n", errno);
> - return 1;
> + ret = 1;
> + } else {
> + gd->flags |= GD_FLG_ENV_READY;
> + ret = 0;

[U-Boot] [PATCH] cmd: nvedit: add whitelist option for env import

[Quentin Schulz]

While the `env export` can take as parameters variables to be exported,
`env import` does not have such a mechanism of variable selection.

Let's add a `-w` option that asks `env import` to look for the
`whitelisted_vars` env variable for a space-separated list of variables
that are whitelisted.

Every env variable present in env at `addr` and in `whitelisted_vars`
env variable will override the value of the variable in the current env.
All the remaining variables are left untouched.

One of its use case could be to load a secure environment from the
signed U-Boot binary and load only a handful of variables from an
other, unsecure, environment without completely losing control of
U-Boot.

Signed-off-by: Quentin Schulz 
---
 cmd/nvedit.c | 71 ++--
 1 file changed, 64 insertions(+), 7 deletions(-)

diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 4e79d03856..9f983b41a1 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -971,7 +971,7 @@ sep_err:
 
 #ifdef CONFIG_CMD_IMPORTENV
 /*
- * env import [-d] [-t [-r] | -b | -c] addr [size]
+ * env import [-d] [-t [-r] | -b | -c] [-w] addr [size]
  * -d: delete existing environment before importing;
  * otherwise overwrite / append to existing definitions
  * -t: assume text format; either "size" must be given or the
@@ -982,6 +982,10 @@ sep_err:
  * for line endings. Only effective in addition to -t.
  * -b: assume binary format ('\0' separated, "\0\0" terminated)
  * -c: assume checksum protected environment format
+ * -w: specify that whitelisting of variables should be used when
+ * importing environment. The space-separated list of variables
+ * that should override the ones in current environment is stored
+ * in `whitelisted_vars`.
  * addr:   memory address to read from
  * size:   length of input data; if missing, proper '\0'
  * termination is mandatory
@@ -991,11 +995,16 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
 {
ulong   addr;
char*cmd, *ptr;
+   char**array = NULL;
charsep = '\n';
int chk = 0;
int fmt = 0;
int del = 0;
int crlf_is_lf = 0;
+   int wl = 0;
+   int wl_count = 0;
+   int ret;
+   unsigned int i;
size_t  size;
 
cmd = *argv;
@@ -1026,6 +1035,9 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
case 'd':
del = 1;
break;
+   case 'w':
+   wl = 1;
+   break;
default:
return CMD_RET_USAGE;
}
@@ -1082,14 +1094,59 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
ptr = (char *)ep->data;
}
 
-   if (himport_r(_htab, ptr, size, sep, del ? 0 : H_NOCLEAR,
-   crlf_is_lf, 0, NULL) == 0) {
+   if(wl) {
+   char *str, *token, *tmp;
+   wl_count = 1;
+
+   str = env_get("whitelisted_vars");
+   if (!str) {
+   puts("## Error: whitelisted_vars is not set.\n");
+   return CMD_RET_USAGE;
+   }
+
+   tmp = malloc(sizeof(char) * (strlen(str) + 1));
+   strcpy(tmp, str);
+
+   token = strchr(tmp, ' ');
+   while (!token) {
+   wl_count++;
+   token = strchr(token + 1, ' ');
+   }
+
+   strcpy(tmp, str);
+
+   array = malloc(sizeof(char *) * wl_count);
+   wl_count = 0;
+
+   token = strtok(tmp, " ");
+   while (token) {
+   array[wl_count] = malloc(sizeof(char) *
+(strlen(token) + 1));
+   strcpy(array[wl_count], token);
+   wl_count++;
+   token = strtok(NULL, " ");
+   }
+
+   free(tmp);
+   }
+
+   ret = himport_r(_htab, ptr, size, sep, del ? 0 : H_NOCLEAR,
+   crlf_is_lf, wl ? wl_count : 0, wl ? array : NULL);
+   if (!ret) {
pr_err("Environment import failed: errno = %d\n", errno);
-   return 1;
+   ret = 1;
+   } else {
+   gd->flags |= GD_FLG_ENV_READY;
+   ret = 0;
}
-   gd->flags |= GD_FLG_ENV_READY;
 
-   return 0;
+   if (wl) {
+   for (i = 0; i < wl_count; i++)
+   free(array[i]);
+   free(array);
+   }
+
+   return ret;
 
 sep_err:
printf("## %s: only one of \"-b\", \"-c\" or \"-t\" allowed\n",
@@ -1212,7 +1269,7 @@ static