Re: [U-Boot] [PATCH v2 1/2] cmd: nvedit: add whitelist option for env import

Hi Lothar,

On Sun, May 20, 2018 at 03:01:22PM +0200, Lothar Waßmann wrote:

Hi,

On Fri, 18 May 2018 16:44:59 +0200 Quentin Schulz 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

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 | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 68 insertions(+), 11 deletions(-)

diff --git a/cmd/nvedit.c b/cmd/nvedit.c index 4e79d03856..1e33a26f4c 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
    

@@ -990,18 +994,22 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { ulong addr;

• char *cmd, *ptr;

• char *cmd, *ptr, *tmp;

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

• int ret = 0; size_t size;

  cmd = *argv;

  while (--argc > 0 && **++argv == '-') {

• char *arg = *argv;
  

• char *arg = *argv, *str, *token;
  

  while (*++arg) { switch (*arg) { case 'b': /* raw binary format */

@@ -1025,6 +1033,43 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag, break; 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 = strdup(str);
  

• 		if (!tmp)
  

• 			return CMD_RET_FAILURE;
  

• 		token = strchr(tmp, ' ');
  

• 		while (!token) {
  

• 			wl_count++;
  

• 			token = strchr(token + 1, ' ');
  

• 		}
  

• 		strcpy(tmp, str);
  
  

This is redundant. strchr() doesn't modify the array.

ACK.

...

• 		wl_count = 0;
  

• 		array = malloc(sizeof(char *) * wl_count);
  
  

You have juste before reset wl_count to 0 are mallocing a zero sized array here!

Don't know how I could get the sandbox test to pass with this error, thanks.

...

• 		if (!array) {
  

• 			free(tmp);
  

• 			return CMD_RET_FAILURE;
  

• 		}
  

wl_count should probably be zeroed here...

Indeed.

But I would keep the predetermined vlaue of wl_count and check against it in the following loop to be sure not to overflow the allocated array, even in the improbable case, that strtok() should happen to return more tokens, than you tested before with the strchr() loop.

Good point, never too safe to double check.

Thanks, Quentin