[U-Boot] [PATCH v2 1/2] cmd: nvedit: add whitelist option for env import
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); + + wl_count = 0; + array = malloc(sizeof(char *) * wl_count); + if (!array) { + free(tmp); + return CMD_RET_FAILURE; + } + + token = strtok(tmp, " "); + while (token) { + array[wl_count] = token; + wl_count++; + token = strtok(NULL, " "); + } + break; default: return CMD_RET_USAGE; @@ -1032,8 +1077,10 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag, } }
- if (argc < 1) - return CMD_RET_USAGE; + if (argc < 1) { + ret = CMD_RET_USAGE; + goto exit; + }
if (!fmt) printf("## Warning: defaulting to text format\n"); @@ -1048,7 +1095,8 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag, size = simple_strtoul(argv[1], NULL, 16); } else if (argc == 1 && chk) { puts("## Error: external checksum format must pass size\n"); - return CMD_RET_FAILURE; + ret = CMD_RET_FAILURE; + goto exit; } else { char *s = ptr;
@@ -1077,19 +1125,28 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
if (crc32(0, ep->data, size) != crc) { puts("## Error: bad CRC, import failed\n"); - return 1; + ret = 1; + goto exit; } ptr = (char *)ep->data; }
if (himport_r(&env_htab, ptr, size, sep, del ? 0 : H_NOCLEAR, - crlf_is_lf, 0, NULL) == 0) { + crlf_is_lf, wl ? wl_count : 0, wl ? array : NULL) == 0) { pr_err("Environment import failed: errno = %d\n", errno); - return 1; + + ret = 1; + goto exit; } gd->flags |= GD_FLG_ENV_READY;
- return 0; +exit: + if (wl) { + free(tmp); + free(array); + } + + return ret;
sep_err: printf("## %s: only one of "-b", "-c" or "-t" allowed\n", @@ -1212,7 +1269,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] [-w] addr [size] - import environment\n" #endif "env print [-a | name ...] - print environment\n" #if defined(CONFIG_CMD_RUN)
This tests that the importing of an environment with a specified whitelist works as intended.
If the variable whitelisted_vars is not set, the env importing should fail with a given message.
For each variable separated by spaces in whitelisted_vars, 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 in the whitelisted_vars), after importing exported env, foo shall be empty,
Any variable not in whitelisted_vars should be left untouched.
Signed-off-by: Quentin Schulz quentin.schulz@bootlin.com ---
added in v2
test/py/tests/test_env.py | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)
diff --git a/test/py/tests/test_env.py b/test/py/tests/test_env.py index b7f960c755..d0a2e36876 100644 --- a/test/py/tests/test_env.py +++ b/test/py/tests/test_env.py @@ -6,6 +6,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? @@ -231,3 +232,42 @@ 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_whitelist_empty(state_test_env): + """Test trying to import 0 variables from an environment""" + 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") + + state_test_env.u_boot_console.run_command('env export %s' % addr) + + c = state_test_env.u_boot_console + with c.disable_check('error_notification'): + response = c.run_command('env import -w %s' % addr) + assert(response.startswith('## Error: whitelisted_vars is not set')) + +def test_env_import_whitelist(state_test_env): + """Test importing only a handful of env variables from an environment""" + ram_base = u_boot_utils.find_ram_base(state_test_env.u_boot_console) + addr = '%08x' % ram_base + + #no foo1 in current env, foo2 overridden, foo3 should be of the value + #before exporting and foo4 should be deleted + set_var(state_test_env, "whitelisted_vars", "foo1 foo2 foo4") + set_var(state_test_env, "foo1", "bar1") + set_var(state_test_env, "foo2", "bar2") + set_var(state_test_env, "foo3", "bar3") + + state_test_env.u_boot_console.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") + + state_test_env.u_boot_console.run_command('env import -w %s' % 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")
On 05/18/2018 08:45 AM, Quentin Schulz wrote:
This tests that the importing of an environment with a specified whitelist works as intended.
If the variable whitelisted_vars is not set, the env importing should fail with a given message.
For each variable separated by spaces in whitelisted_vars, 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
in the whitelisted_vars), after importing exported env, foo shall be empty,
Any variable not in whitelisted_vars should be left untouched.
Acked-by: Stephen Warren swarren@nvidia.com
diff --git a/test/py/tests/test_env.py b/test/py/tests/test_env.py
+def test_env_import_whitelist(state_test_env):
- state_test_env.u_boot_console.run_command('env import -w %s' % 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")
It might make sense to iterate over *all* variables and make sure that they have the same value before/after the import. But, the current code seems to cover the basic cases, so it's probably not strictly necessary to do that.
Hi Stephen,
On Fri, May 18, 2018 at 10:04:05AM -0600, Stephen Warren wrote:
On 05/18/2018 08:45 AM, Quentin Schulz wrote:
This tests that the importing of an environment with a specified whitelist works as intended.
If the variable whitelisted_vars is not set, the env importing should fail with a given message.
For each variable separated by spaces in whitelisted_vars, 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
in the whitelisted_vars), after importing exported env, foo shall be empty,
Any variable not in whitelisted_vars should be left untouched.
Acked-by: Stephen Warren swarren@nvidia.com
diff --git a/test/py/tests/test_env.py b/test/py/tests/test_env.py
+def test_env_import_whitelist(state_test_env):
- state_test_env.u_boot_console.run_command('env import -w %s' % 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")
It might make sense to iterate over *all* variables and make sure that they have the same value before/after the import. But, the current code seems to cover the basic cases, so it's probably not strictly necessary to do that.
I would advocate for as many test sets as possible, each one testing a very specific scenario. That way, it's easy to know which scenario failed instead of having to find within a big scenario what failed and why. Here, I just want to test that importing some env variables work.
It would make sense to have a small test that tests that setting an environment variable works (that what's done with test_env_set* test functions I think) and others to test the exporting of variables and the importing of some others with all the possible/impossible arguments of these cli commands. I wish I could test this importing without manually exporting an environnement before to reduce "dependencies" of the test and thus keep the test scenario to the actual command we're testing.
However the more the arguments of the commands, the more functions to write ("exponentially") as we have to test each and every case. The best would be to also have tests for expected and wanted failures. I think it's a good start for now but more needs to be done in the env testing regarding (at least) env exporting and importing.
Quentin
On 18 May 2018 at 08:45, Quentin Schulz quentin.schulz@bootlin.com wrote:
This tests that the importing of an environment with a specified whitelist works as intended.
If the variable whitelisted_vars is not set, the env importing should fail with a given message.
For each variable separated by spaces in whitelisted_vars, 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
in the whitelisted_vars), after importing exported env, foo shall be empty,
Any variable not in whitelisted_vars should be left untouched.
Signed-off-by: Quentin Schulz quentin.schulz@bootlin.com
added in v2
test/py/tests/test_env.py | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
On 05/18/2018 08:44 AM, 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.
Would it be better for the -w option to accept a variable name rather than hard-coding it as whitelisted_vars? That way, people could import/export different sets of variables at different times, and also could choose a more use-case-specific variable name than whitelisted_vars in order to describe why those variables are "whitelisted".
Hi Stephen,
On Fri, May 18, 2018 at 10:00:27AM -0600, Stephen Warren wrote:
On 05/18/2018 08:44 AM, 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.
Would it be better for the -w option to accept a variable name rather than hard-coding it as whitelisted_vars? That way, people could import/export different sets of variables at different times, and also could choose a more use-case-specific variable name than whitelisted_vars in order to describe why those variables are "whitelisted".
This has been raised in the previous version of the patch[1] (of which you weren't in the mail recipients) and a similar patch[2] made by Alex Kiernan (Cc of this patch series). I'd say it's an ongoing discussion, though I should have mentioned it in the comments of the patch?
TL;DR: Proposition 1: Have -w only which "hardcodedly" checks for "whitelisting_vars", +: straightforward implementation of the argument parsing, -: implicit declaration of the list: you have to know to set whitelisted_vars in the environnement,
Proposition 2: Have -w followed by one string-word which is the name of the env variable where to look for the list of whitelisted env variables, +: explicit var to check where whitelist is looked for, -: a bit of complexity added to the parsing of the parameters of the env import function,
Proposition 3: Have -w followed by the list of whitelisted env variable, +: explicit list -: the list cannot be separated by comma (valid character for an env variable) or a space (would not be able to distinguish the last arguments of the commands which are address and size with size being optional => how to know if size was passed or not?), what char can be used to separate env variables in the list? how does it perform with a very long list of whitelisted variables?
I don't know how many features and their complexity we can add to the env import/export functions, but keeping their arguments simple is a way to keep features hopefully easy to implement in the future.
I'm not convinced by any of the propositions but proposition 1 is the one I implemented for now. I just want a consensus towards a solution (be it among the given 3 or another one) and I'm good with it.
Thanks, Quentin
[1] https://www.mail-archive.com/u-boot@lists.denx.de/msg286266.html [2] http://patchwork.ozlabs.org/patch/891422/
On Mon, May 21, 2018 at 9:02 AM Quentin Schulz quentin.schulz@bootlin.com wrote:
Hi Stephen,
On Fri, May 18, 2018 at 10:00:27AM -0600, Stephen Warren wrote:
On 05/18/2018 08:44 AM, 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.
Would it be better for the -w option to accept a variable name rather
than
hard-coding it as whitelisted_vars? That way, people could import/export different sets of variables at different times, and also could choose a
more
use-case-specific variable name than whitelisted_vars in order to
describe
why those variables are "whitelisted".
This has been raised in the previous version of the patch[1] (of which you weren't in the mail recipients) and a similar patch[2] made by Alex Kiernan (Cc of this patch series). I'd say it's an ongoing discussion, though I should have mentioned it in the comments of the patch?
TL;DR: Proposition 1: Have -w only which "hardcodedly" checks for "whitelisting_vars", +: straightforward implementation of the argument parsing, -: implicit declaration of the list: you have to know to set whitelisted_vars in the environnement,
Proposition 2: Have -w followed by one string-word which is the name of the env variable where to look for the list of whitelisted env variables, +: explicit var to check where whitelist is looked for, -: a bit of complexity added to the parsing of the parameters of the env import function,
Proposition 3: Have -w followed by the list of whitelisted env variable, +: explicit list -: the list cannot be separated by comma (valid character for an env variable) or a space (would not be able to distinguish the last arguments of the commands which are address and size with size being optional => how to know if size was passed or not?), what char can be used to separate env variables in the list? how does it perform with a very long list of whitelisted variables?
Two more thoughts, both of which delegate the separator problem to the caller (the second being the one I implemented as it's almost no code)
- specify multiple -w options each specifying a whitelisted env variable - use the remaining arguments approach and eat all the trailing arguments as the names of env vars you import - needs a sentinel value for the size argument
Hi Alex,
On Mon, May 21, 2018 at 12:56:04PM +0100, Alex Kiernan wrote:
On Mon, May 21, 2018 at 9:02 AM Quentin Schulz quentin.schulz@bootlin.com wrote:
Hi Stephen,
On Fri, May 18, 2018 at 10:00:27AM -0600, Stephen Warren wrote:
On 05/18/2018 08:44 AM, 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.
Would it be better for the -w option to accept a variable name rather
than
hard-coding it as whitelisted_vars? That way, people could import/export different sets of variables at different times, and also could choose a
more
use-case-specific variable name than whitelisted_vars in order to
describe
why those variables are "whitelisted".
This has been raised in the previous version of the patch[1] (of which you weren't in the mail recipients) and a similar patch[2] made by Alex Kiernan (Cc of this patch series). I'd say it's an ongoing discussion, though I should have mentioned it in the comments of the patch?
TL;DR: Proposition 1: Have -w only which "hardcodedly" checks for "whitelisting_vars", +: straightforward implementation of the argument parsing, -: implicit declaration of the list: you have to know to set whitelisted_vars in the environnement,
Proposition 2: Have -w followed by one string-word which is the name of the env variable where to look for the list of whitelisted env variables, +: explicit var to check where whitelist is looked for, -: a bit of complexity added to the parsing of the parameters of the env import function,
Proposition 3: Have -w followed by the list of whitelisted env variable, +: explicit list -: the list cannot be separated by comma (valid character for an env variable) or a space (would not be able to distinguish the last arguments of the commands which are address and size with size being optional => how to know if size was passed or not?), what char can be used to separate env variables in the list? how does it perform with a very long list of whitelisted variables?
Two more thoughts, both of which delegate the separator problem to the caller (the second being the one I implemented as it's almost no code)
- specify multiple -w options each specifying a whitelisted env variable
You'll hit the maximum number of arguments/length of the command quickly with this method. Quicker than with the other propositions.
Moreover, this can make the command painfully long, painful to read and thus cumbersome to find the small typo in your command.
- use the remaining arguments approach and eat all the trailing arguments
as the names of env vars you import - needs a sentinel value for the size argument
That can't work I think.
How do you know if the size argument was passed or not? How'd you know what string is addr, size or the whitelist (if there is even any)?
env import foo1 foo2 foo3 foo4 addr size env import foo1 foo2 foo3 addr env import addr size env import addr
Quentin
On Mon, May 21, 2018 at 1:06 PM Quentin Schulz quentin.schulz@bootlin.com wrote:
Hi Alex,
On Mon, May 21, 2018 at 12:56:04PM +0100, Alex Kiernan wrote:
On Mon, May 21, 2018 at 9:02 AM Quentin Schulz <
quentin.schulz@bootlin.com>
wrote:
Hi Stephen,
On Fri, May 18, 2018 at 10:00:27AM -0600, Stephen Warren wrote:
On 05/18/2018 08:44 AM, 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.
Would it be better for the -w option to accept a variable name
rather
than
hard-coding it as whitelisted_vars? That way, people could
import/export
different sets of variables at different times, and also could
choose a
more
use-case-specific variable name than whitelisted_vars in order to
describe
why those variables are "whitelisted".
This has been raised in the previous version of the patch[1] (of which you weren't in the mail recipients) and a similar patch[2] made by
Alex
Kiernan (Cc of this patch series). I'd say it's an ongoing discussion, though I should have mentioned it in the comments of the patch?
TL;DR: Proposition 1: Have -w only which "hardcodedly" checks for "whitelisting_vars", +: straightforward implementation of the argument parsing, -: implicit declaration of the list: you have to know to set whitelisted_vars in the environnement,
Proposition 2: Have -w followed by one string-word which is the name
of
the env variable where to look for the list of whitelisted env variables, +: explicit var to check where whitelist is looked for, -: a bit of complexity added to the parsing of the parameters of the
env
import function,Proposition 3: Have -w followed by the list of whitelisted env
variable,
+: explicit list -: the list cannot be separated by comma (valid character for an env variable) or a space (would not be able to distinguish the last arguments of the commands which are address and size with size
being
optional => how to know if size was passed or not?), what char
can be
used to separate env variables in the list? how does it perform with a very long list of whitelisted
variables?
Two more thoughts, both of which delegate the separator problem to the caller (the second being the one I implemented as it's almost no code)
- specify multiple -w options each specifying a whitelisted env variable
You'll hit the maximum number of arguments/length of the command quickly with this method. Quicker than with the other propositions.
Moreover, this can make the command painfully long, painful to read and thus cumbersome to find the small typo in your command.
- use the remaining arguments approach and eat all the trailing
arguments
as the names of env vars you import - needs a sentinel value for the
size
argument
That can't work I think.
How do you know if the size argument was passed or not? How'd you know what string is addr, size or the whitelist (if there is even any)?
env import foo1 foo2 foo3 foo4 addr size env import foo1 foo2 foo3 addr env import addr size env import addr
That's why you need a sentinel for the 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 * text data must be '\0' terminated * -r: handle CRLF like LF, that means exported variables with * a content which ends with \r won't get imported. Used * to import text files created with editors which are using CRLF * for line endings. Only effective in addition to -t. * -b: assume binary format ('\0' separated, "\0\0" terminated) * -c: assume checksum protected environment format * addr: memory address to read from * size: length of input data; if missing, proper '\0' * termination is mandatory. If not required and passing * variables to import use '-' * var...: List of variable names that get imported. Without arguments, * all variables are imported
Which for your examples translates to:
env import addr size foo1 foo2 foo3 foo4 env import addr - foo1 foo2 foo3 env import addr size env import addr
On Mon, May 21, 2018 at 01:26:11PM +0100, Alex Kiernan wrote:
On Mon, May 21, 2018 at 1:06 PM Quentin Schulz quentin.schulz@bootlin.com wrote:
Hi Alex,
On Mon, May 21, 2018 at 12:56:04PM +0100, Alex Kiernan wrote:
On Mon, May 21, 2018 at 9:02 AM Quentin Schulz <
quentin.schulz@bootlin.com>
wrote:
Hi Stephen,
On Fri, May 18, 2018 at 10:00:27AM -0600, Stephen Warren wrote:
On 05/18/2018 08:44 AM, 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.
Would it be better for the -w option to accept a variable name
rather
than
hard-coding it as whitelisted_vars? That way, people could
import/export
different sets of variables at different times, and also could
choose a
more
use-case-specific variable name than whitelisted_vars in order to
describe
why those variables are "whitelisted".
This has been raised in the previous version of the patch[1] (of which you weren't in the mail recipients) and a similar patch[2] made by
Alex
Kiernan (Cc of this patch series). I'd say it's an ongoing discussion, though I should have mentioned it in the comments of the patch?
TL;DR: Proposition 1: Have -w only which "hardcodedly" checks for "whitelisting_vars", +: straightforward implementation of the argument parsing, -: implicit declaration of the list: you have to know to set whitelisted_vars in the environnement,
Proposition 2: Have -w followed by one string-word which is the name
of
the env variable where to look for the list of whitelisted env variables, +: explicit var to check where whitelist is looked for, -: a bit of complexity added to the parsing of the parameters of the
env
import function,Proposition 3: Have -w followed by the list of whitelisted env
variable,
+: explicit list -: the list cannot be separated by comma (valid character for an env variable) or a space (would not be able to distinguish the last arguments of the commands which are address and size with size
being
optional => how to know if size was passed or not?), what charcan be
used to separate env variables in the list? how does it perform with a very long list of whitelistedvariables?
Two more thoughts, both of which delegate the separator problem to the caller (the second being the one I implemented as it's almost no code)
- specify multiple -w options each specifying a whitelisted env variable
You'll hit the maximum number of arguments/length of the command quickly with this method. Quicker than with the other propositions.
Moreover, this can make the command painfully long, painful to read and thus cumbersome to find the small typo in your command.
- use the remaining arguments approach and eat all the trailing
arguments
as the names of env vars you import - needs a sentinel value for the
size
argument
That can't work I think.
How do you know if the size argument was passed or not? How'd you know what string is addr, size or the whitelist (if there is even any)?
env import foo1 foo2 foo3 foo4 addr size env import foo1 foo2 foo3 addr env import addr size env import addr
That's why you need a sentinel for the 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 thetext data must be '\0' terminated-r: handle CRLF like LF, that means exported variables witha content which ends with \r won't get imported. Usedto import text files created with editors which are usingCRLF
for line endings. Only effective in addition to -t.-b: assume binary format ('\0' separated, "\0\0" terminated)-c: assume checksum protected environment formataddr: memory address to read fromsize: length of input data; if missing, proper '\0'termination is mandatory. If not required and passingvariables to import use '-'var...: List of variable names that get imported. Without arguments,all variables are importedWhich for your examples translates to:
env import addr size foo1 foo2 foo3 foo4 env import addr - foo1 foo2 foo3 env import addr size env import addr
Ah, I misunderstood the word sentinel then :)
Would have been an even better idea if we had some consistency between env export and env import. For specifying the env export size, we have to use the -s argument but it's a positional argument for env export. We then have a list of variables to export, so it would make sense to have the same for env import I guess?
Quentin
On Mon, May 21, 2018 at 1:34 PM Quentin Schulz quentin.schulz@bootlin.com wrote:
On Mon, May 21, 2018 at 01:26:11PM +0100, Alex Kiernan wrote:
On Mon, May 21, 2018 at 1:06 PM Quentin Schulz <
quentin.schulz@bootlin.com>
wrote:
Hi Alex,
On Mon, May 21, 2018 at 12:56:04PM +0100, Alex Kiernan wrote:
On Mon, May 21, 2018 at 9:02 AM Quentin Schulz <
quentin.schulz@bootlin.com>
wrote:
Hi Stephen,
On Fri, May 18, 2018 at 10:00:27AM -0600, Stephen Warren wrote:
On 05/18/2018 08:44 AM, 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.
Would it be better for the -w option to accept a variable name
rather
than
hard-coding it as whitelisted_vars? That way, people could
import/export
different sets of variables at different times, and also could
choose a
more
use-case-specific variable name than whitelisted_vars in order
to
describe
why those variables are "whitelisted".
This has been raised in the previous version of the patch[1] (of
which
you weren't in the mail recipients) and a similar patch[2] made by
Alex
Kiernan (Cc of this patch series). I'd say it's an ongoing
discussion,
though I should have mentioned it in the comments of the patch?
TL;DR: Proposition 1: Have -w only which "hardcodedly" checks for "whitelisting_vars", +: straightforward implementation of the argument parsing, -: implicit declaration of the list: you have to know to set whitelisted_vars in the environnement,
Proposition 2: Have -w followed by one string-word which is the
name
of
the env variable where to look for the list of whitelisted env variables, +: explicit var to check where whitelist is looked for, -: a bit of complexity added to the parsing of the parameters of
the
env
import function,Proposition 3: Have -w followed by the list of whitelisted env
variable,
+: explicit list -: the list cannot be separated by comma (valid character for an
env
variable) or a space (would not be able to distinguish the
last
arguments of the commands which are address and size with sizebeing
optional => how to know if size was passed or not?), what charcan be
used to separate env variables in the list? how does it perform with a very long list of whitelistedvariables?
Two more thoughts, both of which delegate the separator problem to
the
caller (the second being the one I implemented as it's almost no
code)
- specify multiple -w options each specifying a whitelisted env
variable
You'll hit the maximum number of arguments/length of the command
quickly
with this method. Quicker than with the other propositions.
Moreover, this can make the command painfully long, painful to read
and
thus cumbersome to find the small typo in your command.
- use the remaining arguments approach and eat all the trailing
arguments
as the names of env vars you import - needs a sentinel value for the
size
argument
That can't work I think.
How do you know if the size argument was passed or not? How'd you know what string is addr, size or the whitelist (if there is even any)?
env import foo1 foo2 foo3 foo4 addr size env import foo1 foo2 foo3 addr env import addr size env import addr
That's why you need a sentinel for the 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 thetext data must be '\0' terminated-r: handle CRLF like LF, that means exported variables witha content which ends with \r won't get imported. Usedto import text files created with editors which are
using
CRLF
for line endings. Only effective in addition to -t.-b: assume binary format ('\0' separated, "\0\0"
terminated)
-c: assume checksum protected environment formataddr: memory address to read fromsize: length of input data; if missing, proper '\0'termination is mandatory. If not required and passingvariables to import use '-'var...: List of variable names that get imported. Without
arguments,
all variables are importedWhich for your examples translates to:
env import addr size foo1 foo2 foo3 foo4 env import addr - foo1 foo2 foo3 env import addr size env import addr
Ah, I misunderstood the word sentinel then :)
Would have been an even better idea if we had some consistency between env export and env import. For specifying the env export size, we have to use the -s argument but it's a positional argument for env export. We then have a list of variables to export, so it would make sense to have the same for env import I guess?
I agree, but I can't immediately think of a good way of doing that which is backward compatible.
On Mon, May 21, 2018 at 1:06 PM Quentin Schulz quentin.schulz@bootlin.com wrote:
Hi Alex,
On Mon, May 21, 2018 at 12:56:04PM +0100, Alex Kiernan wrote:
On Mon, May 21, 2018 at 9:02 AM Quentin Schulz <
quentin.schulz@bootlin.com>
wrote:
Hi Stephen,
On Fri, May 18, 2018 at 10:00:27AM -0600, Stephen Warren wrote:
On 05/18/2018 08:44 AM, 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.
Would it be better for the -w option to accept a variable name
rather
than
hard-coding it as whitelisted_vars? That way, people could
import/export
different sets of variables at different times, and also could
choose a
more
use-case-specific variable name than whitelisted_vars in order to
describe
why those variables are "whitelisted".
This has been raised in the previous version of the patch[1] (of which you weren't in the mail recipients) and a similar patch[2] made by
Alex
Kiernan (Cc of this patch series). I'd say it's an ongoing discussion, though I should have mentioned it in the comments of the patch?
TL;DR: Proposition 1: Have -w only which "hardcodedly" checks for "whitelisting_vars", +: straightforward implementation of the argument parsing, -: implicit declaration of the list: you have to know to set whitelisted_vars in the environnement,
Proposition 2: Have -w followed by one string-word which is the name
of
the env variable where to look for the list of whitelisted env variables, +: explicit var to check where whitelist is looked for, -: a bit of complexity added to the parsing of the parameters of the
env
import function,Proposition 3: Have -w followed by the list of whitelisted env
variable,
+: explicit list -: the list cannot be separated by comma (valid character for an env variable) or a space (would not be able to distinguish the last arguments of the commands which are address and size with size
being
optional => how to know if size was passed or not?), what char
can be
used to separate env variables in the list? how does it perform with a very long list of whitelisted
variables?
Two more thoughts, both of which delegate the separator problem to the caller (the second being the one I implemented as it's almost no code)
- specify multiple -w options each specifying a whitelisted env variable
You'll hit the maximum number of arguments/length of the command quickly with this method. Quicker than with the other propositions.
So you just made me go and read the definition for CONFIG_SYS_MAXARGS, the default's clearly pretty small (16). I guess I don't have a feeling for how many args you want to import - my use case is for 4, so even with max args at 16 I'm within that limit.
-- Alex Kiernan
On Mon, May 21, 2018 at 01:36:49PM +0100, Alex Kiernan wrote:
On Mon, May 21, 2018 at 1:06 PM Quentin Schulz quentin.schulz@bootlin.com wrote:
Hi Alex,
On Mon, May 21, 2018 at 12:56:04PM +0100, Alex Kiernan wrote:
On Mon, May 21, 2018 at 9:02 AM Quentin Schulz <
quentin.schulz@bootlin.com>
wrote:
Hi Stephen,
On Fri, May 18, 2018 at 10:00:27AM -0600, Stephen Warren wrote:
On 05/18/2018 08:44 AM, 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.
Would it be better for the -w option to accept a variable name
rather
than
hard-coding it as whitelisted_vars? That way, people could
import/export
different sets of variables at different times, and also could
choose a
more
use-case-specific variable name than whitelisted_vars in order to
describe
why those variables are "whitelisted".
This has been raised in the previous version of the patch[1] (of which you weren't in the mail recipients) and a similar patch[2] made by
Alex
Kiernan (Cc of this patch series). I'd say it's an ongoing discussion, though I should have mentioned it in the comments of the patch?
TL;DR: Proposition 1: Have -w only which "hardcodedly" checks for "whitelisting_vars", +: straightforward implementation of the argument parsing, -: implicit declaration of the list: you have to know to set whitelisted_vars in the environnement,
Proposition 2: Have -w followed by one string-word which is the name
of
the env variable where to look for the list of whitelisted env variables, +: explicit var to check where whitelist is looked for, -: a bit of complexity added to the parsing of the parameters of the
env
import function,Proposition 3: Have -w followed by the list of whitelisted env
variable,
+: explicit list -: the list cannot be separated by comma (valid character for an env variable) or a space (would not be able to distinguish the last arguments of the commands which are address and size with size
being
optional => how to know if size was passed or not?), what charcan be
used to separate env variables in the list? how does it perform with a very long list of whitelistedvariables?
Two more thoughts, both of which delegate the separator problem to the caller (the second being the one I implemented as it's almost no code)
- specify multiple -w options each specifying a whitelisted env variable
You'll hit the maximum number of arguments/length of the command quickly with this method. Quicker than with the other propositions.
So you just made me go and read the definition for CONFIG_SYS_MAXARGS, the default's clearly pretty small (16). I guess I don't have a feeling for how many args you want to import - my use case is for 4, so even with max args at 16 I'm within that limit.
Same for me, a few variables and that's it. I'm trying not to have a solution that's working only for me/us with a pretty low limitation though. Once we've settled for a solution, IMHO, we've to stick with it and thus if the solution can't be used by others, that's bad design from us.
I guess at worst, we could call multiple times the env import command with different whitelists but that's not really user-friendly.
Quentin
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;@@ -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
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;@@ -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
participants (5)
-
Alex Kiernan -
Lothar Waßmann -
Quentin Schulz -
Simon Glass -
Stephen Warren