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

[Joe Hershberger]

On Tue, May 15, 2018 at 5:03 AM, Quentin Schulz
<quentin.schulz at 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 at 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'.

> +                               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.

> +                                       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.

> +
>                 return 1;
>         }
>         gd->flags |= GD_FLG_ENV_READY;
>
> +       if (wl) {
> +               for (i = 0; i < wl_count; i++)
> +                       free(array[i]);
> +               free(array);
> +       }
> +
>         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
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot