[U-Boot] [PATCH v3 1/2] cmd: nvedit: env import can now import only variables passed as parameters

[Lothar Waßmann]

Hi,

On Fri, 25 May 2018 11:52:17 +0100 Alex Kiernan wrote:
> On Fri, May 25, 2018 at 9:38 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 the ability to add parameters at the end of the command for
> > variables to be imported.
> 
> > Every env variable from the env to be imported passed by parameter to
> > this command will override the value of the variable in the current env.
> 
> > If a variable exists in the current env but not in the imported env, if
> > this variable is passed as a parameter to env import, the variable will
> > be unset.
> 
> > If a variable exists in the imported env, the variable in the current
> > env will be set to the value of the one from the imported env.
> 
> > All the remaining variables are left untouched.
> 
> > As the size parameter of env import is positional but optional, let's
> > add the possibility to use the sentinel '-' for when we don't want to
> > give the size parameter (when the env is '\0' terminated) but we pass a
> > list of variables at the end of the command.
> 
> > env import addr
> > env import addr -
> > env import addr size
> > env import addr - foo1 foo2
> > env import addr size foo1 foo2
> 
> > are all valid.
> 
> > env import -c addr
> > env import -c addr -
> > env import -c addr - foo1 foo2
> 
> > are all invalid because they don't pass the size parameter required for
> > checking, while the following are valid.
> 
> > env import addr size
> > env import addr size foo1 foo2
> 
> > Nothing's changed for the other parameters or the overall behaviour.
> 
> > 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>
> > ---
> 
> > v3:
> >    - migrate to env import addr size var... instead of env import -w addr
> >    size so that the list of variables to load is more explicit and the
> >    behaviour of env import is closer to the one of env export,
> 
> > v2:
> >    - use strdup instead of malloc + strcpy,
> >    - NULL-check the result of strdup,
> >    - add common exit path for freeing memory in one unique place,
> >    - store token pointer from strtok within the char** array instead of
> >    strdup-ing token within elements of array,
> 
> >   cmd/nvedit.c | 22 ++++++++++++++++------
> >   1 file changed, 16 insertions(+), 6 deletions(-)
> 
> > diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> > index 11489b0..18cc4db 100644
> > --- a/cmd/nvedit.c
> > +++ b/cmd/nvedit.c
> > @@ -1001,7 +1001,7 @@ sep_err:
> 
> >   #ifdef CONFIG_CMD_IMPORTENV
> >   /*
> > - * env import [-d] [-t [-r] | -b | -c] addr [size]
> > + * env import [-d] [-t [-r] | -b | -c] addr [size] [var ...]
> >    *     -d:     delete existing environment before importing;
> >    *             otherwise overwrite / append to existing definitions
> >    *     -t:     assume text format; either "size" must be given or the
> > @@ -1015,6 +1015,11 @@ sep_err:
> >    *     addr:   memory address to read from
> >    *     size:   length of input data; if missing, proper '\0'
> >    *             termination is mandatory
> > + *             if var is set and size should be missing (i.e. '\0'
> > + *             termination), set size to '-'
> > + *     var...  List of the names of the only variables that get imported
> from
> > + *             the environment at address 'addr'. Without arguments, the
> whole
> > + *             environment gets imported.
> >    */
> >   static int do_env_import(cmd_tbl_t *cmdtp, int flag,
> >                           int argc, char * const argv[])
> > @@ -1026,6 +1031,7 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
> >          int     fmt = 0;
> >          int     del = 0;
> >          int     crlf_is_lf = 0;
> > +       int     wl = 0;
> >          size_t  size;
> 
> >          cmd = *argv;
> > @@ -1074,9 +1080,9 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
> >          addr = simple_strtoul(argv[0], NULL, 16);
> >          ptr = map_sysmem(addr, 0);
> 
> > -       if (argc == 2) {
> > +       if (argc >= 2 && strcmp(argv[1], "-")) {
> >                  size = simple_strtoul(argv[1], NULL, 16);
> > -       } else if (argc == 1 && chk) {
> > +       } else if (chk) {
> >                  puts("## Error: external checksum format must pass
> size\n");
> >                  return CMD_RET_FAILURE;
> >          } else {
> > @@ -1098,6 +1104,9 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
> >                  printf("## Info: input data size = %zu = 0x%zX\n", size,
> size);
> >          }
> 
> > +       if (argc > 2)
> > +               wl = 1;
> > +
> >          if (chk) {
> >                  uint32_t crc;
> >                  env_t *ep = (env_t *)ptr;
> > @@ -1112,9 +1121,10 @@ static int do_env_import(cmd_tbl_t *cmdtp, int
> flag,
> >                  ptr = (char *)ep->data;
> >          }
> 
> > -       if (himport_r(&env_htab, ptr, size, sep, del ? 0 : H_NOCLEAR,
> > -                       crlf_is_lf, 0, NULL) == 0) {
> > +       if (!himport_r(&env_htab, ptr, size, sep, del ? 0 : H_NOCLEAR,
> 
> The addition of a '!' seems like an odd change, but looking at what
> himport_r() returns, I think you're correct and it's been broken forever.
>
No. It was if (h_import_r() == 0) before!
                            ^^^^


Lothar Waßmann