* [U-Boot] [RESEND][PATCH] cmd: nvedit: add whitelist option for env import
@ 2018-05-15 10:03 Quentin Schulz
2018-05-15 18:25 ` Joe Hershberger
2018-05-15 18:28 ` Simon Glass
0 siblings, 2 replies; 7+ messages in thread
From: Quentin Schulz @ 2018-05-15 10:03 UTC (permalink / raw)
To: u-boot
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>
---
cmd/nvedit.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 62 insertions(+), 4 deletions(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 4e79d03856..4637ec656c 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
@@ -991,17 +995,21 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
{
ulong addr;
char *cmd, *ptr;
+ 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;
+ unsigned int i;
size_t size;
cmd = *argv;
while (--argc > 0 && **++argv == '-') {
- char *arg = *argv;
+ char *arg = *argv, *str, *token, *tmp;
while (*++arg) {
switch (*arg) {
case 'b': /* raw binary format */
@@ -1026,6 +1034,43 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
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 = malloc(sizeof(char) * (strlen(str) + 1));
+ strcpy(tmp, str);
+
+ token = strchr(tmp, ' ');
+ while (!token) {
+ wl_count++;
+ token = strchr(token + 1, ' ');
+ }
+
+ strcpy(tmp, str);
+
+ array = malloc(sizeof(char *) * wl_count);
+ wl_count = 0;
+
+ token = strtok(tmp, " ");
+ while (token) {
+ unsigned int size = strlen(token);
+
+ array[wl_count] = malloc(sizeof(char) *
+ (size + 1));
+ strcpy(array[wl_count], token);
+ wl_count++;
+ token = strtok(NULL, " ");
+ }
+
+ free(tmp);
+ break;
default:
return CMD_RET_USAGE;
}
@@ -1083,12 +1128,25 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
}
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);
+
+ if (wl) {
+ for (i = 0; i < wl_count; i++)
+ free(array[i]);
+ free(array);
+ }
+
return 1;
}
gd->flags |= GD_FLG_ENV_READY;
+ if (wl) {
+ for (i = 0; i < wl_count; i++)
+ free(array[i]);
+ free(array);
+ }
+
return 0;
sep_err:
@@ -1212,7 +1270,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)
--
2.14.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [U-Boot] [RESEND][PATCH] cmd: nvedit: add whitelist option for env import
2018-05-15 10:03 [U-Boot] [RESEND][PATCH] cmd: nvedit: add whitelist option for env import Quentin Schulz
@ 2018-05-15 18:25 ` Joe Hershberger
2018-05-15 18:43 ` Alex Kiernan
2018-05-16 7:32 ` Quentin Schulz
2018-05-15 18:28 ` Simon Glass
1 sibling, 2 replies; 7+ messages in thread
From: Joe Hershberger @ 2018-05-15 18:25 UTC (permalink / raw)
To: u-boot
On Tue, May 15, 2018 at 5:03 AM, Quentin Schulz
<quentin.schulz@bootlin.com> 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>
> ---
> cmd/nvedit.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 62 insertions(+), 4 deletions(-)
>
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index 4e79d03856..4637ec656c 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
> @@ -991,17 +995,21 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
> {
> ulong addr;
> char *cmd, *ptr;
> + 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;
> + unsigned int i;
> size_t size;
>
> cmd = *argv;
>
> while (--argc > 0 && **++argv == '-') {
> - char *arg = *argv;
> + char *arg = *argv, *str, *token, *tmp;
> while (*++arg) {
> switch (*arg) {
> case 'b': /* raw binary format */
> @@ -1026,6 +1034,43 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
> case 'd':
> del = 1;
> break;
> + case 'w':
> + wl = 1;
> + wl_count = 1;
> +
> + str = env_get("whitelisted_vars");
I don't like how this is grabbing the list from the env. I think a
comma-separated list should be a parameter following the '-w'.
> + if (!str) {
> + puts("## Error: whitelisted_vars is not set.\n");
> + return CMD_RET_USAGE;
> + }
> +
> + tmp = malloc(sizeof(char) * (strlen(str) + 1));
> + strcpy(tmp, str);
> +
> + token = strchr(tmp, ' ');
> + while (!token) {
> + wl_count++;
> + token = strchr(token + 1, ' ');
> + }
> +
> + strcpy(tmp, str);
> +
> + array = malloc(sizeof(char *) * wl_count);
> + wl_count = 0;
> +
> + token = strtok(tmp, " ");
> + while (token) {
> + unsigned int size = strlen(token);
> +
> + array[wl_count] = malloc(sizeof(char) *
> + (size + 1));
Why malloc again for every string? Just use the buffer we already have.
> + strcpy(array[wl_count], token);
> + wl_count++;
> + token = strtok(NULL, " ");
> + }
> +
> + free(tmp);
> + break;
> default:
> return CMD_RET_USAGE;
> }
> @@ -1083,12 +1128,25 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
> }
>
> 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);
> +
> + if (wl) {
> + for (i = 0; i < wl_count; i++)
> + free(array[i]);
> + free(array);
> + }
It would be better to have a consolidated cleanup at the end of the function.
> +
> return 1;
> }
> gd->flags |= GD_FLG_ENV_READY;
>
> + if (wl) {
> + for (i = 0; i < wl_count; i++)
> + free(array[i]);
> + free(array);
> + }
> +
> return 0;
>
> sep_err:
> @@ -1212,7 +1270,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)
> --
> 2.14.1
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [RESEND][PATCH] cmd: nvedit: add whitelist option for env import
2018-05-15 10:03 [U-Boot] [RESEND][PATCH] cmd: nvedit: add whitelist option for env import Quentin Schulz
2018-05-15 18:25 ` Joe Hershberger
@ 2018-05-15 18:28 ` Simon Glass
2018-05-16 7:37 ` Quentin Schulz
1 sibling, 1 reply; 7+ messages in thread
From: Simon Glass @ 2018-05-15 18:28 UTC (permalink / raw)
To: u-boot
Hi Quentin,
On 15 May 2018 at 20:03, Quentin Schulz <quentin.schulz@bootlin.com> 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>
> ---
> cmd/nvedit.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 62 insertions(+), 4 deletions(-)
Is this something you could write a sandbox test for? I think it would
be useful.
>
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index 4e79d03856..4637ec656c 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
> @@ -991,17 +995,21 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
> {
> ulong addr;
> char *cmd, *ptr;
> + 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;
> + unsigned int i;
> size_t size;
>
> cmd = *argv;
>
> while (--argc > 0 && **++argv == '-') {
> - char *arg = *argv;
> + char *arg = *argv, *str, *token, *tmp;
> while (*++arg) {
> switch (*arg) {
> case 'b': /* raw binary format */
> @@ -1026,6 +1034,43 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
> 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 = malloc(sizeof(char) * (strlen(str) + 1));
strdup() ?
Also should check for NULL
> + strcpy(tmp, str);
> +
> + token = strchr(tmp, ' ');
> + while (!token) {
> + wl_count++;
> + token = strchr(token + 1, ' ');
> + }
> +
> + strcpy(tmp, str);
> +
> + array = malloc(sizeof(char *) * wl_count);
> + wl_count = 0;
> +
> + token = strtok(tmp, " ");
> + while (token) {
> + unsigned int size = strlen(token);
> +
> + array[wl_count] = malloc(sizeof(char) *
> + (size + 1));
Check for NULL
> + strcpy(array[wl_count], token);
> + wl_count++;
> + token = strtok(NULL, " ");
> + }
> +
> + free(tmp);
> + break;
> default:
> return CMD_RET_USAGE;
> }
> @@ -1083,12 +1128,25 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
> }
>
> 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);
> +
> + if (wl) {
> + for (i = 0; i < wl_count; i++)
> + free(array[i]);
> + free(array);
> + }
> +
> return 1;
> }
> gd->flags |= GD_FLG_ENV_READY;
>
> + if (wl) {
> + for (i = 0; i < wl_count; i++)
> + free(array[i]);
> + free(array);
> + }
> +
Is there a way to avoid repeating that code twice, perhaps goto or
function call.
> return 0;
>
> sep_err:
> @@ -1212,7 +1270,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)
> --
> 2.14.1
>
Regards,
Simon
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [RESEND][PATCH] cmd: nvedit: add whitelist option for env import
2018-05-15 18:25 ` Joe Hershberger
@ 2018-05-15 18:43 ` Alex Kiernan
2018-05-15 20:34 ` Simon Glass
2018-05-16 7:32 ` Quentin Schulz
1 sibling, 1 reply; 7+ messages in thread
From: Alex Kiernan @ 2018-05-15 18:43 UTC (permalink / raw)
To: u-boot
On Tue, May 15, 2018 at 7:25 PM Joe Hershberger <joe.hershberger@gmail.com>
wrote:
> On Tue, May 15, 2018 at 5:03 AM, Quentin Schulz
> <quentin.schulz@bootlin.com> 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>
> > ---
> > cmd/nvedit.c | 66
++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
> > 1 file changed, 62 insertions(+), 4 deletions(-)
> >
> > diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> > index 4e79d03856..4637ec656c 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
> > @@ -991,17 +995,21 @@ static int do_env_import(cmd_tbl_t *cmdtp, int
flag,
> > {
> > ulong addr;
> > char *cmd, *ptr;
> > + 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;
> > + unsigned int i;
> > size_t size;
> >
> > cmd = *argv;
> >
> > while (--argc > 0 && **++argv == '-') {
> > - char *arg = *argv;
> > + char *arg = *argv, *str, *token, *tmp;
> > while (*++arg) {
> > switch (*arg) {
> > case 'b': /* raw binary format */
> > @@ -1026,6 +1034,43 @@ static int do_env_import(cmd_tbl_t *cmdtp, int
flag,
> > case 'd':
> > del = 1;
> > break;
> > + case 'w':
> > + wl = 1;
> > + wl_count = 1;
> > +
> > + str = env_get("whitelisted_vars");
> I don't like how this is grabbing the list from the env. I think a
> comma-separated list should be a parameter following the '-w'.
I posted a patch, independently in that vein as I hadn't spotted Quentin's
patch:
http://patchwork.ozlabs.org/patch/891422/
I could clean it up to take parameters in an option.
--
Alex Kiernan
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [RESEND][PATCH] cmd: nvedit: add whitelist option for env import
2018-05-15 18:43 ` Alex Kiernan
@ 2018-05-15 20:34 ` Simon Glass
0 siblings, 0 replies; 7+ messages in thread
From: Simon Glass @ 2018-05-15 20:34 UTC (permalink / raw)
To: u-boot
Hi Alex,
On 15 May 2018 at 12:43, Alex Kiernan <alex.kiernan@gmail.com> wrote:
> On Tue, May 15, 2018 at 7:25 PM Joe Hershberger <joe.hershberger@gmail.com>
> wrote:
>
>> On Tue, May 15, 2018 at 5:03 AM, Quentin Schulz
>> <quentin.schulz@bootlin.com> 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>
>> > ---
>> > cmd/nvedit.c | 66
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
>> > 1 file changed, 62 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/cmd/nvedit.c b/cmd/nvedit.c
>> > index 4e79d03856..4637ec656c 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
>> > @@ -991,17 +995,21 @@ static int do_env_import(cmd_tbl_t *cmdtp, int
> flag,
>> > {
>> > ulong addr;
>> > char *cmd, *ptr;
>> > + 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;
>> > + unsigned int i;
>> > size_t size;
>> >
>> > cmd = *argv;
>> >
>> > while (--argc > 0 && **++argv == '-') {
>> > - char *arg = *argv;
>> > + char *arg = *argv, *str, *token, *tmp;
>> > while (*++arg) {
>> > switch (*arg) {
>> > case 'b': /* raw binary format */
>> > @@ -1026,6 +1034,43 @@ static int do_env_import(cmd_tbl_t *cmdtp, int
> flag,
>> > case 'd':
>> > del = 1;
>> > break;
>> > + case 'w':
>> > + wl = 1;
>> > + wl_count = 1;
>> > +
>> > + str = env_get("whitelisted_vars");
>
>> I don't like how this is grabbing the list from the env. I think a
>> comma-separated list should be a parameter following the '-w'.
>
>
> I posted a patch, independently in that vein as I hadn't spotted Quentin's
> patch:
>
> http://patchwork.ozlabs.org/patch/891422/
>
> I could clean it up to take parameters in an option.
>
> --
> Alex Kiernan
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [RESEND][PATCH] cmd: nvedit: add whitelist option for env import
2018-05-15 18:25 ` Joe Hershberger
2018-05-15 18:43 ` Alex Kiernan
@ 2018-05-16 7:32 ` Quentin Schulz
1 sibling, 0 replies; 7+ messages in thread
From: Quentin Schulz @ 2018-05-16 7:32 UTC (permalink / raw)
To: u-boot
Hi Joe,
On Tue, May 15, 2018 at 01:25:09PM -0500, Joe Hershberger wrote:
> On Tue, May 15, 2018 at 5:03 AM, Quentin Schulz
> <quentin.schulz@bootlin.com> 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>
> > ---
> > cmd/nvedit.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
> > 1 file changed, 62 insertions(+), 4 deletions(-)
> >
> > diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> > index 4e79d03856..4637ec656c 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
> > @@ -991,17 +995,21 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
> > {
> > ulong addr;
> > char *cmd, *ptr;
> > + 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;
> > + unsigned int i;
> > size_t size;
> >
> > cmd = *argv;
> >
> > while (--argc > 0 && **++argv == '-') {
> > - char *arg = *argv;
> > + char *arg = *argv, *str, *token, *tmp;
> > while (*++arg) {
> > switch (*arg) {
> > case 'b': /* raw binary format */
> > @@ -1026,6 +1034,43 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
> > case 'd':
> > del = 1;
> > break;
> > + case 'w':
> > + wl = 1;
> > + wl_count = 1;
> > +
> > + str = env_get("whitelisted_vars");
>
> I don't like how this is grabbing the list from the env. I think a
> comma-separated list should be a parameter following the '-w'.
>
Comma are valids in the name of environment variables if I'm not
mistaken. (Did a quick setenv test,123 test and it saved test in
test,123). So that's not an option.
Using a space separated list after "-w" is also not easy as I don't see
from a quick thought how to differentiate the list of vars passed to -w
from the addr and size parameters at the end of the function (how to
know if size was passed or not?).
Is there any other character aside from a space that can be used to
separate the list of variables?
> > + if (!str) {
> > + puts("## Error: whitelisted_vars is not set.\n");
> > + return CMD_RET_USAGE;
> > + }
> > +
> > + tmp = malloc(sizeof(char) * (strlen(str) + 1));
> > + strcpy(tmp, str);
> > +
> > + token = strchr(tmp, ' ');
> > + while (!token) {
> > + wl_count++;
> > + token = strchr(token + 1, ' ');
> > + }
> > +
> > + strcpy(tmp, str);
> > +
> > + array = malloc(sizeof(char *) * wl_count);
> > + wl_count = 0;
> > +
> > + token = strtok(tmp, " ");
> > + while (token) {
> > + unsigned int size = strlen(token);
> > +
> > + array[wl_count] = malloc(sizeof(char) *
> > + (size + 1));
>
> Why malloc again for every string? Just use the buffer we already have.
>
Do an array[wl_count] = token instead?
I'm not sure this will work with this patch's code.
From what I understand from strtok[1], token is a pointer to somewhere
in the string tmp. So we're screwed once tmp is freed aren't we?
I could move the free(tmp) to the final goto I'm talking about below
though and get away with it.
> > + strcpy(array[wl_count], token);
> > + wl_count++;
> > + token = strtok(NULL, " ");
> > + }
> > +
> > + free(tmp);
> > + break;
> > default:
> > return CMD_RET_USAGE;
> > }
> > @@ -1083,12 +1128,25 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
> > }
> >
> > 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);
> > +
> > + if (wl) {
> > + for (i = 0; i < wl_count; i++)
> > + free(array[i]);
> > + free(array);
> > + }
>
> It would be better to have a consolidated cleanup at the end of the function.
>
Sure. I'll add a goto with appropriate return value.
Thanks,
Quentin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180516/7c9283a0/attachment.sig>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [RESEND][PATCH] cmd: nvedit: add whitelist option for env import
2018-05-15 18:28 ` Simon Glass
@ 2018-05-16 7:37 ` Quentin Schulz
0 siblings, 0 replies; 7+ messages in thread
From: Quentin Schulz @ 2018-05-16 7:37 UTC (permalink / raw)
To: u-boot
Hi Simon,
On Tue, May 15, 2018 at 12:28:14PM -0600, Simon Glass wrote:
> Hi Quentin,
>
> On 15 May 2018 at 20:03, Quentin Schulz <quentin.schulz@bootlin.com> 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>
> > ---
> > cmd/nvedit.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
> > 1 file changed, 62 insertions(+), 4 deletions(-)
>
> Is this something you could write a sandbox test for? I think it would
> be useful.
>
I don't know about sandbox tests but I don't see why not.
> >
> > diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> > index 4e79d03856..4637ec656c 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
> > @@ -991,17 +995,21 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
> > {
> > ulong addr;
> > char *cmd, *ptr;
> > + 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;
> > + unsigned int i;
> > size_t size;
> >
> > cmd = *argv;
> >
> > while (--argc > 0 && **++argv == '-') {
> > - char *arg = *argv;
> > + char *arg = *argv, *str, *token, *tmp;
> > while (*++arg) {
> > switch (*arg) {
> > case 'b': /* raw binary format */
> > @@ -1026,6 +1034,43 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
> > 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 = malloc(sizeof(char) * (strlen(str) + 1));
>
> strdup() ?
>
Thanks for the discovery :)
> Also should check for NULL
>
Indeed.
> > + strcpy(tmp, str);
> > +
> > + token = strchr(tmp, ' ');
> > + while (!token) {
> > + wl_count++;
> > + token = strchr(token + 1, ' ');
> > + }
> > +
> > + strcpy(tmp, str);
> > +
> > + array = malloc(sizeof(char *) * wl_count);
> > + wl_count = 0;
> > +
> > + token = strtok(tmp, " ");
> > + while (token) {
> > + unsigned int size = strlen(token);
> > +
> > + array[wl_count] = malloc(sizeof(char) *
> > + (size + 1));
>
> Check for NULL
>
> > + strcpy(array[wl_count], token);
> > + wl_count++;
> > + token = strtok(NULL, " ");
> > + }
> > +
> > + free(tmp);
> > + break;
> > default:
> > return CMD_RET_USAGE;
> > }
> > @@ -1083,12 +1128,25 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
> > }
> >
> > 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);
> > +
> > + if (wl) {
> > + for (i = 0; i < wl_count; i++)
> > + free(array[i]);
> > + free(array);
> > + }
> > +
> > return 1;
> > }
> > gd->flags |= GD_FLG_ENV_READY;
> >
> > + if (wl) {
> > + for (i = 0; i < wl_count; i++)
> > + free(array[i]);
> > + free(array);
> > + }
> > +
>
> Is there a way to avoid repeating that code twice, perhaps goto or
> function call.
>
As told to Joe, I think I could use a goto and keep the appropriate
return value and that'd be cleaner indeed.
Thanks,
Quentin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180516/cfd2fe0e/attachment.sig>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-05-16 7:37 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-15 10:03 [U-Boot] [RESEND][PATCH] cmd: nvedit: add whitelist option for env import Quentin Schulz
2018-05-15 18:25 ` Joe Hershberger
2018-05-15 18:43 ` Alex Kiernan
2018-05-15 20:34 ` Simon Glass
2018-05-16 7:32 ` Quentin Schulz
2018-05-15 18:28 ` Simon Glass
2018-05-16 7:37 ` Quentin Schulz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.