cmd: nvedit: add whitelist option for env import

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.

Quoted 15 lines · Quentin Schulz

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,

Quoted 1 line

Any variable not in whitelisted_vars should be left untouched.

Acked-by Stephen Warren <[email protected]>

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

Quoted 10 lines

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

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

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

---

added in v2

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

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.

Quoted 6 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 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,

Quoted 2 lines · Stephen Warren

Quoted 1 line · Quentin Schulz

While the env export can take as parameters variables to be

Quoted 4 lines

Quoted 4 lines

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

Quoted 3 lines

Quoted 1 line

that are whitelisted.

Would it be better for the -w option to accept a variable name rather

Quoted 2 lines

hard-coding it as whitelisted_vars? That way, people could import/export different sets of variables at different times, and also could choose a

Quoted 1 line

use-case-specific variable name than whitelisted_vars in order to

Quoted 1 line

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?

Quoted 5 lines · Quentin Schulz

Hi Stephen,

Quoted 2 lines · Stephen Warren

Quoted 1 line · Quentin Schulz

While the env export can take as parameters variables to be

exported,

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

that are whitelisted.

Would it be better for the -w option to accept a variable name rather

than

Quoted 2 lines

Quoted 2 lines

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

Quoted 1 line

Quoted 1 line

use-case-specific variable name than whitelisted_vars in order to

describe

Quoted 21 lines

Quoted 1 line

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,

Quoted 8 lines

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,

Quoted 1 line · Alex Kiernan

On Mon, May 21, 2018 at 9:02 AM Quentin Schulz <

Quoted 8 lines

Quoted 5 lines · wrote:

Hi Stephen,

Quoted 2 lines · Stephen Warren

Quoted 1 line · Quentin Schulz

While the env export can take as parameters variables to be

exported,

Quoted 8 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

that are whitelisted.

Would it be better for the -w option to accept a variable name

Quoted 2 lines

than

Quoted 1 line

Quoted 1 line

hard-coding it as whitelisted_vars? That way, people could

Quoted 1 line

Quoted 1 line

Quoted 1 line

different sets of variables at different times, and also could

Quoted 7 lines

more

Quoted 1 line

Quoted 1 line

use-case-specific variable name than whitelisted_vars in order to

describe

Quoted 4 lines

Quoted 1 line

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

Quoted 11 lines

Quoted 11 lines

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

Quoted 4 lines

Quoted 4 lines

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

Quoted 3 lines

Quoted 3 lines

import function,

Proposition 3: Have -w followed by the list of whitelisted env

Quoted 4 lines

Quoted 4 lines

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

Quoted 1 line

Quoted 1 line

optional => how to know if size was passed or not?), what char

Quoted 2 lines

Quoted 2 lines

used to separate env variables in the list? how does it perform with a very long list of whitelisted

Quoted 5 lines

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)

Quoted 1 line

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

Quoted 1 line

- use the remaining arguments approach and eat all the trailing

Quoted 1 line

as the names of env vars you import - needs a sentinel value for the

Quoted 2 lines

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

Quoted 4 lines · Quentin Schulz

Hi Alex,

Quoted 1 line · Alex Kiernan

On Mon, May 21, 2018 at 9:02 AM Quentin Schulz <

quentin.schulz at bootlin.com>

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines · wrote:

Hi Stephen,

Quoted 2 lines · Stephen Warren

Quoted 1 line · Quentin Schulz

While the env export can take as parameters variables to be

exported,

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

that are whitelisted.

Would it be better for the -w option to accept a variable name

rather

Quoted 2 lines

Quoted 2 lines

than

Quoted 1 line

Quoted 1 line

hard-coding it as whitelisted_vars? That way, people could

import/export

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

different sets of variables at different times, and also could

choose a

Quoted 7 lines

Quoted 7 lines

more

Quoted 1 line

Quoted 1 line

use-case-specific variable name than whitelisted_vars in order to

describe

Quoted 4 lines

Quoted 1 line

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

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

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

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

import function,

Proposition 3: Have -w followed by the list of whitelisted env

variable,

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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

Quoted 1 line

Quoted 1 line

Quoted 1 line

optional => how to know if size was passed or not?), what char

can be

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

used to separate env variables in the list? how does it perform with a very long list of whitelisted

variables?

Quoted 5 lines

Quoted 5 lines

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)

Quoted 9 lines

Quoted 1 line

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

Quoted 1 line

- use the remaining arguments approach and eat all the trailing

arguments

Quoted 1 line

Quoted 1 line

as the names of env vars you import - needs a sentinel value for the

size

Quoted 5 lines

Quoted 2 lines

argument

That can't work I think.

Quoted 7 lines

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

Quoted 1 line · Alex Kiernan

On Mon, May 21, 2018 at 1:06 PM Quentin Schulz <

Quoted 9 lines

Quoted 4 lines · wrote:

Hi Alex,

Quoted 1 line · Alex Kiernan

On Mon, May 21, 2018 at 9:02 AM Quentin Schulz <

quentin.schulz at bootlin.com>

Quoted 2 lines

Quoted 2 lines

wrote:

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

Hi Stephen,

Quoted 2 lines · Stephen Warren

Quoted 1 line · Quentin Schulz

While the env export can take as parameters variables to be

exported,

Quoted 1 line

Quoted 1 line

Quoted 1 line

env import does not have such a mechanism of variable

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

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

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

that are whitelisted.

Would it be better for the -w option to accept a variable name

Quoted 8 lines

rather

Quoted 2 lines

Quoted 2 lines

than

Quoted 1 line

Quoted 1 line

hard-coding it as whitelisted_vars? That way, people could

import/export

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

different sets of variables at different times, and also could

choose a

Quoted 2 lines

Quoted 2 lines

more

Quoted 1 line

Quoted 1 line

use-case-specific variable name than whitelisted_vars in order

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

describe

Quoted 3 lines

Quoted 1 line

why those variables are "whitelisted".

This has been raised in the previous version of the patch[1] (of

Quoted 3 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

you weren't in the mail recipients) and a similar patch[2] made by

Alex

Quoted 1 line

Quoted 1 line

Quoted 1 line

Kiernan (Cc of this patch series). I'd say it's an ongoing

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

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

Quoted 5 lines

of

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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

Quoted 7 lines

env

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

import function,

Proposition 3: Have -w followed by the list of whitelisted env

variable,

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

+: explicit list
-: the list cannot be separated by comma (valid character for an

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

variable) or a space (would not be able to distinguish the

Quoted 10 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

arguments of the commands which are address and size with size

being

Quoted 1 line

Quoted 1 line

Quoted 1 line

optional => how to know if size was passed or not?), what char

can be

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

used to separate env variables in the list? how does it perform with a very long list of whitelisted

variables?

Quoted 3 lines

Quoted 3 lines

Two more thoughts, both of which delegate the separator problem to

Quoted 1 line

Quoted 1 line

Quoted 1 line

caller (the second being the one I implemented as it's almost no

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

- specify multiple -w options each specifying a whitelisted env

Quoted 2 lines

Quoted 2 lines

You'll hit the maximum number of arguments/length of the command

Quoted 3 lines

Quoted 3 lines

with this method. Quicker than with the other propositions.

Moreover, this can make the command painfully long, painful to read

Quoted 22 lines

Quoted 3 lines

thus cumbersome to find the small typo in your command.

Quoted 1 line

- use the remaining arguments approach and eat all the trailing

arguments

Quoted 1 line

Quoted 1 line

as the names of env vars you import - needs a sentinel value for the

size

Quoted 5 lines

Quoted 2 lines

argument

That can't work I think.

Quoted 7 lines

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:

Quoted 8 lines

* 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

Quoted 3 lines

CRLF
* for line endings. Only effective in addition to -t.
* -b: assume binary format ('\0' separated, "\0\0"

Quoted 6 lines

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

Quoted 9 lines

* 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

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?

Hi Alex,

Quoted 1 line · Alex Kiernan

On Mon, May 21, 2018 at 9:02 AM Quentin Schulz <

Quoted 8 lines

Quoted 5 lines · wrote:

Hi Stephen,

Quoted 2 lines · Stephen Warren

Quoted 1 line · Quentin Schulz

While the env export can take as parameters variables to be

exported,

Quoted 8 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

that are whitelisted.

Would it be better for the -w option to accept a variable name

Quoted 2 lines

than

Quoted 1 line

Quoted 1 line

hard-coding it as whitelisted_vars? That way, people could

Quoted 1 line

Quoted 1 line

Quoted 1 line

different sets of variables at different times, and also could

Quoted 7 lines

more

Quoted 1 line

Quoted 1 line

use-case-specific variable name than whitelisted_vars in order to

describe

Quoted 4 lines

Quoted 1 line

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

Quoted 11 lines

Quoted 11 lines

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

Quoted 4 lines

Quoted 4 lines

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

Quoted 3 lines

Quoted 3 lines

import function,

Proposition 3: Have -w followed by the list of whitelisted env

Quoted 4 lines

Quoted 4 lines

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

Quoted 1 line

Quoted 1 line

optional => how to know if size was passed or not?), what char

Quoted 2 lines

Quoted 2 lines

used to separate env variables in the list? how does it perform with a very long list of whitelisted

Quoted 6 lines

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.

Quoted 4 lines · Quentin Schulz

Hi Alex,

Quoted 1 line · Alex Kiernan

On Mon, May 21, 2018 at 9:02 AM Quentin Schulz <

quentin.schulz at bootlin.com>

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines · wrote:

Hi Stephen,

Quoted 2 lines · Stephen Warren

Quoted 1 line · Quentin Schulz

While the env export can take as parameters variables to be

exported,

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

that are whitelisted.

Would it be better for the -w option to accept a variable name

rather

Quoted 2 lines

Quoted 2 lines

than

Quoted 1 line

Quoted 1 line

hard-coding it as whitelisted_vars? That way, people could

import/export

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

different sets of variables at different times, and also could

choose a

Quoted 7 lines

Quoted 7 lines

more

Quoted 1 line

Quoted 1 line

use-case-specific variable name than whitelisted_vars in order to

describe

Quoted 4 lines

Quoted 1 line

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

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

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

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

import function,

Proposition 3: Have -w followed by the list of whitelisted env

variable,

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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

Quoted 1 line

Quoted 1 line

Quoted 1 line

optional => how to know if size was passed or not?), what char

can be

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

used to separate env variables in the list? how does it perform with a very long list of whitelisted

variables?

Quoted 9 lines

Quoted 6 lines

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.

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

---

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

cmd/nvedit.c +24 -3

index 4e79d03856..1e33a26f4c 100644
@@ -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)

Hi,

Quoted 98 lines · 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 <[email protected]>

---

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

cmd/nvedit.c +27 -3

index 4e79d03856..1e33a26f4c 100644
@@ -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, ' ');

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

Quoted 7 lines

cmd/nvedit.c +6 -0

+				while (!token) {
+					wl_count++;
+					token = strchr(token + 1, ' ');
+				}
+
+				strcpy(tmp, str);
 

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

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

Quoted 3 lines

cmd/nvedit.c +2 -0

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

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

Quoted 6 lines

cmd/nvedit.c +5 -0

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