Hi Simon,

On Tue, May 15, 2018 at 12:28:14PM -0600, Simon Glass wrote:
> Hi Quentin,
> 
> On 15 May 2018 at 20:03, Quentin Schulz <quentin.sch...@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.sch...@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.
> 

I don't know about sandbox tests but I don't see why not.

> >
> > 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() ?
> 

Thanks for the discovery :)

> Also should check for NULL
> 

Indeed.

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

As told to Joe, I think I could use a goto and keep the appropriate
return value and that'd be cleaner indeed.

Thanks,
Quentin

Attachment: signature.asc
Description: PGP signature

_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Reply via email to