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

[Alex Kiernan <alex.kiernan@gmail.com>]

On Fri, May 25, 2018 at 12:26 PM Lothar Waßmann <LW@karo-electronics.de>
wrote:

> 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@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@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!
>                              ^^^^


I should learn to read properly... thanks and sorry for the noise.

-- 
Alex Kiernan