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

15 May 2018

On Tue, May 15, 2018 at 7:25 PM Joe Hershberger joe.hershberger@gmail.com
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'.
I posted a patch, independently in that vein as I hadn't spotted Quentin's
patch:
http://patchwork.ozlabs.org/patch/891422/
I could clean it up to take parameters in an option.
-- 
Alex Kiernan

    