cmd: nvedit: env import can now import only variables passed as parameters

PATCH Superseded Quentin Schulz 4 participants 7 messages May 25, 2018 – May 26, 2018

Share & tools

Quentin Schulz

author

2018-05-25 08:38 UTC #0 lore raw

PATCH v3 1/2 cmd: nvedit: env import can now import only variables passed as parameters

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 <[email protected]>

---

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

cmd/nvedit.c +16 -6

index 11489b0..18cc4db 100644
@@ -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,
+		       crlf_is_lf, wl ? argc - 2 : 0, wl ? &argv[2] : NULL)) {
 		pr_err("Environment import failed: errno = %d\n", errno);
+
 		return 1;
 	}
 	gd->flags |= GD_FLG_ENV_READY;

@@ -1242,7 +1252,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] addr [size] [var ...] - import environment\n"
 #endif
 	"env print [-a | name ...] - print environment\n"
 #if defined(CONFIG_CMD_RUN)

base-commit: 8a9dc16e4d07d29fff08b7caca36f0865065f7f7 --
git-series 0.9.1

Quentin Schulz

author

2018-05-25 08:38 UTC #1 lore raw

PATCH v3 2/2 test/py: add test for whitelist of variables while importing environment

This tests that the importing of an environment with a specified whitelist works as intended.

If there are variables passed as parameter to the env import command,
those only should be imported in the current environment.

For each variable passed as parameter, if
- foo is bar in current env and bar2 in exported env, after importing exported env, foo shall be bar2,
- foo does not exist in current env and foo is bar2 in exported env, after importing exported env, foo shall be bar2,
- foo is bar in current env and does not exist in exported env (but is passed as parameter), after importing exported env, foo shall be empty,

Any variable not passed as parameter should be left untouched.

Two other tests are made to test that size cannot be '-' if the checksum protection is enabled.

Signed-off-by Quentin Schulz <[email protected]>

---

v3:
- update whitelist test to reflect changes made in patch 1,
- add two tests for no size parameter passed but checksum protection is enabled because I added the possibility to have a sentinel in place of size parameter,
- I intentionally didn't put the Reviewed and Acked-by of Simon and Stephen since the code changed since v2,

added in v2

 test/py/tests/test_env.py | 50 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+)

test/py/tests/test_env.py +50 -0

index bfb5fc0..b83d2b1 100644
@@ -5,6 +5,7 @@
 # Test operation of shell commands relating to environment variables.
 
 import pytest
+import u_boot_utils
 
 # FIXME: This might be useful for other tests;
 # perhaps refactor it into ConsoleBase or some other state object?

@@ -239,3 +240,52 @@ def test_env_expansion_spaces(state_test_env):
             unset_var(state_test_env, var_space)
         if var_test:
             unset_var(state_test_env, var_test)
+
+def test_env_import_checksum_no_size(state_test_env):
+    """Test that omitted ('-') size parameter with checksum validation fails the
+       env import function.
+    """
+    c = state_test_env.u_boot_console
+    ram_base = u_boot_utils.find_ram_base(state_test_env.u_boot_console)
+    addr = '%08x' % ram_base
+
+    with c.disable_check('error_notification'):
+        response = c.run_command('env import -c %s -' % addr)
+    assert(response == '## Error: external checksum format must pass size')
+
+def test_env_import_whitelist_checksum_no_size(state_test_env):
+    """Test that omitted ('-') size parameter with checksum validation fails the
+       env import function when variables are passed as parameters.
+    """
+    c = state_test_env.u_boot_console
+    ram_base = u_boot_utils.find_ram_base(state_test_env.u_boot_console)
+    addr = '%08x' % ram_base
+
+    with c.disable_check('error_notification'):
+        response = c.run_command('env import -c %s - foo1 foo2 foo4' % addr)
+    assert(response == '## Error: external checksum format must pass size')
+
+def test_env_import_whitelist(state_test_env):
+    """Test importing only a handful of env variables from an environment."""
+    c = state_test_env.u_boot_console
+    ram_base = u_boot_utils.find_ram_base(state_test_env.u_boot_console)
+    addr = '%08x' % ram_base
+
+    set_var(state_test_env, "foo1", "bar1")
+    set_var(state_test_env, "foo2", "bar2")
+    set_var(state_test_env, "foo3", "bar3")
+
+    c.run_command('env export %s' % addr)
+
+    unset_var(state_test_env, "foo1")
+    set_var(state_test_env, "foo2", "test2")
+    set_var(state_test_env, "foo4", "bar4")
+
+    #no foo1 in current env, foo2 overridden, foo3 should be of the value
+    #before exporting and foo4 should be deleted
+    c.run_command('env import %s - foo1 foo2 foo4' % addr)
+
+    validate_set(state_test_env, "foo1", "bar1")
+    validate_set(state_test_env, "foo2", "bar2")
+    validate_set(state_test_env, "foo3", "bar3")
+    validate_empty(state_test_env, "foo4")

Signature · 1 line

git-series 0.9.1

Simon Glass

2018-05-26 02:07 UTC #2 lore raw

PATCH v3 2/2 test/py: add test for whitelist of variables while importing environment

Hi,

Quoted 30 lines · Quentin Schulz

This tests that the importing of an environment with a specified whitelist works as intended.
If there are variables passed as parameter to the env import command,
those only should be imported in the current environment.
For each variable passed as parameter, if
- foo is bar in current env and bar2 in exported env, after importing exported env, foo shall be bar2,
- foo does not exist in current env and foo is bar2 in exported env, after importing exported env, foo shall be bar2,
- foo is bar in current env and does not exist in exported env (but is passed as parameter), after importing exported env, foo shall be empty,
Any variable not passed as parameter should be left untouched.
Two other tests are made to test that size cannot be '-' if the checksum protection is enabled.
Signed-off-by Quentin Schulz <[email protected]>
---
v3:
- update whitelist test to reflect changes made in patch 1,
- add two tests for no size parameter passed but checksum protection is enabled because I added the possibility to have a sentinel in place of size parameter,
- I intentionally didn't put the Reviewed and Acked-by of Simon and Stephen since the code changed since v2,

Quoted 4 lines

added in v2

 test/py/tests/test_env.py | 50 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+)

Reviewed-by Simon Glass <[email protected]>

Some nits below

Quoted 39 lines

test/py/tests/test_env.py +23 -0

index bfb5fc0..b83d2b1 100644
@@ -5,6 +5,7 @@
 # Test operation of shell commands relating to environment variables.
 
 import pytest
+import u_boot_utils
 
 # FIXME: This might be useful for other tests;
 # perhaps refactor it into ConsoleBase or some other state object?

@@ -239,3 +240,52 @@ def test_env_expansion_spaces(state_test_env):
             unset_var(state_test_env, var_space)
         if var_test:
             unset_var(state_test_env, var_test)
+
+def test_env_import_checksum_no_size(state_test_env):
+    """Test that omitted ('-') size parameter with checksum validation fails the
+       env import function.
+    """
+    c = state_test_env.u_boot_console
+    ram_base = u_boot_utils.find_ram_base(state_test_env.u_boot_console)
+    addr = '%08x' % ram_base
+
+    with c.disable_check('error_notification'):
+        response = c.run_command('env import -c %s -' % addr)
+    assert(response == '## Error: external checksum format must pass size')
+
+def test_env_import_whitelist_checksum_no_size(state_test_env):
+    """Test that omitted ('-') size parameter with checksum validation fails the
+       env import function when variables are passed as parameters.
+    """
+    c = state_test_env.u_boot_console
+    ram_base = u_boot_utils.find_ram_base(state_test_env.u_boot_console)
+    addr = '%08x' % ram_base
+
+    with c.disable_check('error_notification'):

test/py/tests/test_env.py def test_env_expansion_spaces(state_test_env): · line 274

Quoted 10 lines

test/py/tests/test_env.py +10 -0

+        response = c.run_command('env import -c %s - foo1 foo2 foo4' % addr)
+    assert(response == '## Error: external checksum format must pass size')
+
+def test_env_import_whitelist(state_test_env):
+    """Test importing only a handful of env variables from an environment."""
+    c = state_test_env.u_boot_console
+    ram_base = u_boot_utils.find_ram_base(state_test_env.u_boot_console)
+    addr = '%08x' % ram_base
+
+    set_var(state_test_env, "foo1", "bar1")

Please use ' for strings rather than ", except for the function/class comments

test/py/tests/test_env.py def test_env_expansion_spaces(state_test_env): · line 285

Quoted 11 lines

test/py/tests/test_env.py +11 -0

+    set_var(state_test_env, "foo2", "bar2")
+    set_var(state_test_env, "foo3", "bar3")
+
+    c.run_command('env export %s' % addr)
+
+    unset_var(state_test_env, "foo1")
+    set_var(state_test_env, "foo2", "test2")
+    set_var(state_test_env, "foo4", "bar4")
+
+    #no foo1 in current env, foo2 overridden, foo3 should be of the value
+    #before exporting and foo4 should be deleted

Spaces after each #

Quoted 8 lines

test/py/tests/test_env.py +6 -1

+    c.run_command('env import %s - foo1 foo2 foo4' % addr)
+
+    validate_set(state_test_env, "foo1", "bar1")
+    validate_set(state_test_env, "foo2", "bar2")
+    validate_set(state_test_env, "foo3", "bar3")
+    validate_empty(state_test_env, "foo4")
--

git-series 0.9.1

Alex Kiernan

2018-05-25 10:52 UTC #3 lore raw

PATCH v3 1/2 cmd: nvedit: env import can now import only variables passed as parameters

In reply to Quentin Schulz

Quoted 80 lines · Quentin Schulz

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 <[email protected]>
---
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(-)
cmd/nvedit.c +1 -1
index 11489b0..18cc4db 100644
@@ -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

cmd/nvedit.c sep_err: · line 1020

Quoted 6 lines

cmd/nvedit.c +3 -0

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

cmd/nvedit.c sep_err: · line 1021

Quoted 1 line

cmd/nvedit.c +1 -0

+ *             the environment at address 'addr'. Without arguments, the

whole

Quoted 13 lines

cmd/nvedit.c +2 -0

+ *             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;

cmd/nvedit.c static int do_env_import(cmd_tbl_t *cmdtp, int flag, · line 1086

Quoted 9 lines

cmd/nvedit.c +2 -2

@@ -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");

cmd/nvedit.c static int do_env_import(cmd_tbl_t *cmdtp, int flag, · line 1104

Quoted 4 lines

cmd/nvedit.c +0 -0

                 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);

cmd/nvedit.c static int do_env_import(cmd_tbl_t *cmdtp, int

Quoted 9 lines

cmd/nvedit.c +3 -0

         }
 
+       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,

cmd/nvedit.c static int do_env_import(cmd_tbl_t *cmdtp, int flag, · line 1124

Quoted 6 lines

cmd/nvedit.c +1 -2

                 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.

cmd/nvedit.c static int do_env_import(cmd_tbl_t *cmdtp, int · line 1125

Quoted 1 line

cmd/nvedit.c +1 -0

+                      crlf_is_lf, wl ? argc - 2 : 0, wl ? &argv[2] :

NULL)) {

cmd/nvedit.c static char env_help_text[] = · line 1245

Quoted 10 lines

cmd/nvedit.c +1 -1

                 pr_err("Environment import failed: errno = %d\n", errno);
+
                 return 1;
         }
         gd->flags |= GD_FLG_ENV_READY;

@@ -1242,7 +1252,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"

cmd/nvedit.c static char env_help_text[] = · line 1255

Quoted 1 line

cmd/nvedit.c +1 -0

+       "env import [-d] [-t [-r] | -b | -c] addr [size] [var ...] -

import environment\n"

Quoted 7 lines

cmd/nvedit.c +0 -0
  #endif
         "env print [-a | name ...] - print environment\n"
  #if defined(CONFIG_CMD_RUN)
base-commit: 8a9dc16e4d07d29fff08b7caca36f0865065f7f7 --
git-series 0.9.1

Tested-by Alex Kiernan <[email protected]>

Signature · 1 line

Alex Kiernan

Lothar Waßmann

2018-05-25 11:26 UTC #4 lore raw

PATCH v3 1/2 cmd: nvedit: env import can now import only variables passed as parameters

Hi,

Quoted 133 lines · On Fri, 25 May 2018 11:52:17 +0100 Alex Kiernan wrote:

Quoted 80 lines · Quentin Schulz
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 <[email protected]>
---
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(-)
cmd/nvedit.c +1 -1
index 11489b0..18cc4db 100644
@@ -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
cmd/nvedit.c sep_err: · line 1020
Quoted 6 lines
cmd/nvedit.c +3 -0
@@ -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
cmd/nvedit.c sep_err: · line 1021
Quoted 1 line
cmd/nvedit.c +1 -0
+ *             the environment at address 'addr'. Without arguments, the
whole
Quoted 13 lines
cmd/nvedit.c +2 -0
+ *             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;
cmd/nvedit.c static int do_env_import(cmd_tbl_t *cmdtp, int flag, · line 1086
Quoted 9 lines
cmd/nvedit.c +2 -2
@@ -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");
cmd/nvedit.c static int do_env_import(cmd_tbl_t *cmdtp, int flag, · line 1104
Quoted 4 lines
cmd/nvedit.c +0 -0
                 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);
cmd/nvedit.c static int do_env_import(cmd_tbl_t *cmdtp, int
Quoted 9 lines
cmd/nvedit.c +3 -0
         }
 
+       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,
Quoted 2 lines
cmd/nvedit.c +0 -0
                 ptr = (char *)ep->data;
         }

Quoted 7 lines

cmd/nvedit.c static int do_env_import(cmd_tbl_t *cmdtp, int flag, · line 1124
Quoted 3 lines
cmd/nvedit.c +1 -2
-       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

Quentin Schulz

author

2018-05-25 12:12 UTC #5 lore raw

PATCH v3 1/2 cmd: nvedit: env import can now import only variables passed as parameters

Hi Lothar and Alex,

Quoted 140 lines · Lothar WaÃŸmann

Hi,
Quoted 137 lines · On Fri, 25 May 2018 11:52:17 +0100 Alex Kiernan wrote:
Quoted 80 lines · Quentin Schulz
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 <[email protected]>
---
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(-)
cmd/nvedit.c +1 -1
index 11489b0..18cc4db 100644
@@ -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
cmd/nvedit.c sep_err: · line 1020
Quoted 6 lines
cmd/nvedit.c +3 -0
@@ -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
cmd/nvedit.c sep_err: · line 1021
Quoted 1 line
cmd/nvedit.c +1 -0
+ *             the environment at address 'addr'. Without arguments, the
whole
Quoted 13 lines
cmd/nvedit.c +2 -0
+ *             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;
cmd/nvedit.c static int do_env_import(cmd_tbl_t *cmdtp, int flag, · line 1086
Quoted 9 lines
cmd/nvedit.c +2 -2
@@ -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");
cmd/nvedit.c static int do_env_import(cmd_tbl_t *cmdtp, int flag, · line 1104
Quoted 4 lines
cmd/nvedit.c +0 -0
                 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);
cmd/nvedit.c static int do_env_import(cmd_tbl_t *cmdtp, int
Quoted 9 lines
cmd/nvedit.c +3 -0
         }
 
+       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,
Quoted 6 lines
cmd/nvedit.c +1 -2
                 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,

Quoted 5 lines

Quoted 3 lines
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!
                            ^^^^

Exactly, with the added ternary conditions, the line exceeds the 80 characters just for the opening bracket. I just replaced the == 0 with a ! to gain a few precious characters and avoid having to do a:

ret = himport_r();
if (!ret)

But I could do it if wanted.

If I had changed the behaviour of the function, I would have made a separate commit for it so it's not hidden in some unrelated code, so, that's just a cosmetic fix here :)

Quentin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180525/89cdd355/attachment.sig>

Alex Kiernan

2018-05-25 12:17 UTC #6 lore raw

PATCH v3 1/2 cmd: nvedit: env import can now import only variables passed as parameters

In reply to Lothar Waßmann

Quoted 4 lines · Lothar WaÃŸmann

Hi,
Quoted 1 line · On Fri, 25 May 2018 11:52:17 +0100 Alex Kiernan wrote:
On Fri, May 25, 2018 at 9:38 AM Quentin Schulz <

[email protected]>

Quoted 3 lines

Quoted 3 lines
Quoted 1 line · wrote:
While the env export can take as parameters variables to be

exported,

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
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.

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
If a variable exists in the current env but not in the imported env,

Quoted 1 line

Quoted 1 line
Quoted 1 line
this variable is passed as a parameter to env import, the variable

will

Quoted 10 lines

Quoted 10 lines
Quoted 10 lines
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

Quoted 8 lines

Quoted 8 lines
Quoted 7 lines
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

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
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

Quoted 12 lines

Quoted 12 lines
Quoted 11 lines
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.

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
Signed-off-by Quentin Schulz <[email protected]>
---
v3:
- migrate to env import addr size var... instead of env import -w

addr

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
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

Quoted 11 lines

Quoted 11 lines
Quoted 10 lines
strdup-ing token within elements of array,
  cmd/nvedit.c | 22 ++++++++++++++++------
  1 file changed, 16 insertions(+), 6 deletions(-)
cmd/nvedit.c +0 -0
index 11489b0..18cc4db 100644
@@ -1001,7 +1001,7 @@ sep_err:

Quoted 7 lines

cmd/nvedit.c sep_err:

Quoted 7 lines

cmd/nvedit.c +1 -1

  #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

Quoted 7 lines

cmd/nvedit.c sep_err: · line 1020

Quoted 7 lines

cmd/nvedit.c +3 -0

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

Quoted 2 lines

Quoted 2 lines
from
Quoted 1 line
cmd/nvedit.c +1 -0
+ *             the environment at address 'addr'. Without arguments,

the

Quoted 6 lines

whole

cmd/nvedit.c static int do_env_import(cmd_tbl_t *cmdtp, int

Quoted 5 lines

cmd/nvedit.c +1 -0

+ *             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,

Quoted 8 lines

cmd/nvedit.c static int do_env_import(cmd_tbl_t *cmdtp, int

Quoted 8 lines

cmd/nvedit.c +1 -0

         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,

Quoted 9 lines

cmd/nvedit.c +2 -2

         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

Quoted 4 lines

Quoted 4 lines
size\n");
cmd/nvedit.c static int do_env_import(cmd_tbl_t *cmdtp, int
Quoted 3 lines
cmd/nvedit.c +0 -0
                 return CMD_RET_FAILURE;
         } else {
@@ -1098,6 +1104,9 @@ static int do_env_import(cmd_tbl_t *cmdtp, int

flag,

Quoted 1 line

Quoted 1 line
cmd/nvedit.c static int do_env_import(cmd_tbl_t *cmdtp, int · line 1104
Quoted 1 line
cmd/nvedit.c +0 -0
                 printf("## Info: input data size = %zu = 0x%zX\n",

size,

Quoted 14 lines

size);

cmd/nvedit.c static int do_env_import(cmd_tbl_t *cmdtp, int

Quoted 9 lines

cmd/nvedit.c +3 -0

         }
 
+       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,

Quoted 2 lines

cmd/nvedit.c +0 -0

                 ptr = (char *)ep->data;
         }

Quoted 6 lines

Quoted 6 lines
cmd/nvedit.c static int do_env_import(cmd_tbl_t *cmdtp, int flag, · line 1124
Quoted 3 lines
cmd/nvedit.c +1 -2
-       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.

Quoted 3 lines

Quoted 1 line
No. It was if (h_import_r() == 0) before!
                             ^^^^

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

Signature · 1 line

Alex Kiernan