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 > <[email protected]> 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]> > > --- > > 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
signature.asc
Description: PGP signature
_______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
