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

Hi Quentin,

On 15 May 2018 at 20:03, Quentin Schulz quentin.schulz@bootlin.com 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 quentin.schulz@bootlin.com

cmd/nvedit.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 62 insertions(+), 4 deletions(-)

Is this something you could write a sandbox test for? I think it would be useful.

diff --git a/cmd/nvedit.c b/cmd/nvedit.c index 4e79d03856..4637ec656c 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,17 +995,21 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag, { ulong addr; char *cmd, *ptr;

•   char    **array = NULL;
    char    sep = '\n';
    int     chk = 0;
    int     fmt = 0;
    int     del = 0;
    int     crlf_is_lf = 0;
  

•   int     wl = 0;
  

•   int     wl_count = 0;
  

•   unsigned int i;
    size_t  size;
  
    cmd = *argv;
  
    while (--argc > 0 && **++argv == '-') {
  

•           char *arg = *argv;
  

•           char *arg = *argv, *str, *token, *tmp;
            while (*++arg) {
                    switch (*arg) {
                    case 'b':               /* raw binary format */
  

@@ -1026,6 +1034,43 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag, case 'd': del = 1; break;

•                   case 'w':
  

•                           wl = 1;
  

•                           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));
  

strdup() ?

Also should check for NULL

•                           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) {
  

•                                   unsigned int size = strlen(token);
  

•                                   array[wl_count] = malloc(sizeof(char) *
  

•                                                            (size + 1));
  

Check for NULL

•                                   strcpy(array[wl_count], token);
  

•                                   wl_count++;
  

•                                   token = strtok(NULL, " ");
  

•                           }
  

•                           free(tmp);
  

•                           break;
                    default:
                            return CMD_RET_USAGE;
                    }
  

@@ -1083,12 +1128,25 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag, }

    if (himport_r(&env_htab, ptr, size, sep, del ? 0 : H_NOCLEAR,

•                   crlf_is_lf, 0, NULL) == 0) {
  

•                 crlf_is_lf, wl ? wl_count : 0, wl ? array : NULL) == 0) {
            pr_err("Environment import failed: errno = %d\n", errno);
  

•           if (wl) {
  

•                   for (i = 0; i < wl_count; i++)
  

•                           free(array[i]);
  

•                   free(array);
  

•           }
  

•           return 1;
    }
    gd->flags |= GD_FLG_ENV_READY;
  
  

•   if (wl) {
  

•           for (i = 0; i < wl_count; i++)
  

•                   free(array[i]);
  

•           free(array);
  

•   }
  

Is there a way to avoid repeating that code twice, perhaps goto or function call.

    return 0;

sep_err: @@ -1212,7 +1270,7 @@ static char env_help_text[] = #endif #endif #if defined(CONFIG_CMD_IMPORTENV)

•   "env import [-d] [-t [-r] | -b | -c] addr [size] - import environment\n"
  

•   "env import [-d] [-t [-r] | -b | -c] [-w] addr [size] - import environment\n"
  

#endif "env print [-a | name ...] - print environment\n"

#if defined(CONFIG_CMD_RUN)

2.14.1

Regards, Simon