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

Hi Joe,

On Tue, May 15, 2018 at 01:25:09PM -0500, Joe Hershberger wrote:

On Tue, May 15, 2018 at 5:03 AM, 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(-)

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

I don't like how this is grabbing the list from the env. I think a comma-separated list should be a parameter following the '-w'.

Comma are valids in the name of environment variables if I'm not mistaken. (Did a quick setenv test,123 test and it saved test in test,123). So that's not an option.

Using a space separated list after "-w" is also not easy as I don't see from a quick thought how to differentiate the list of vars passed to -w from the addr and size parameters at the end of the function (how to know if size was passed or not?).

Is there any other character aside from a space that can be used to separate the list of variables?

...

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

•                                   unsigned int size = strlen(token);
  

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

•                                                            (size + 1));
  

Why malloc again for every string? Just use the buffer we already have.

Do an array[wl_count] = token instead?

I'm not sure this will work with this patch's code.

From what I understand from strtok[1], token is a pointer to somewhere in the string tmp. So we're screwed once tmp is freed aren't we?

I could move the free(tmp) to the final goto I'm talking about below though and get away with it.

...

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

•           }
  

It would be better to have a consolidated cleanup at the end of the function.

Sure. I'll add a goto with appropriate return value.

Thanks, Quentin