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

["Lothar Waßmann" <LW@KARO-electronics.de>]

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.

> +				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!

> +				if (!array) {
> +					free(tmp);
> +					return CMD_RET_FAILURE;
> +				}
> +
>
wl_count should probably be zeroed here...
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.


Lothar Waßmann