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 at bootlin.com>
> > ---
> >
> > v2:
> > - use strdup instead of malloc + strcpy,
> > - NULL-check the result of strdup,
> > - add common exit path for freeing memory in one unique place,
> > - store token pointer from strtok within the char** array instead of
> > strdup-ing token within elements of array,
> >
> > cmd/nvedit.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++---------
> > 1 file changed, 68 insertions(+), 11 deletions(-)
> >
> > diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> > index 4e79d03856..1e33a26f4c 100644
> > --- a/cmd/nvedit.c
> > +++ b/cmd/nvedit.c
> > @@ -971,7 +971,7 @@ sep_err:
> >
> > #ifdef CONFIG_CMD_IMPORTENV
> > /*
> > - * env import [-d] [-t [-r] | -b | -c] addr [size]
> > + * env import [-d] [-t [-r] | -b | -c] [-w] addr [size]
> > * -d: delete existing environment before importing;
> > * otherwise overwrite / append to existing definitions
> > * -t: assume text format; either "size" must be given or the
> > @@ -982,6 +982,10 @@ sep_err:
> > * for line endings. Only effective in addition to -t.
> > * -b: assume binary format ('\0' separated, "\0\0" terminated)
> > * -c: assume checksum protected environment format
> > + * -w: specify that whitelisting of variables should be used when
> > + * importing environment. The space-separated list of variables
> > + * that should override the ones in current environment is stored
> > + * in `whitelisted_vars`.
> > * addr: memory address to read from
> > * size: length of input data; if missing, proper '\0'
> > * termination is mandatory
> > @@ -990,18 +994,22 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
> > int argc, char * const argv[])
> > {
> > ulong addr;
> > - char *cmd, *ptr;
> > + char *cmd, *ptr, *tmp;
> > + char **array = NULL;
> > char sep = '\n';
> > int chk = 0;
> > int fmt = 0;
> > int del = 0;
> > int crlf_is_lf = 0;
> > + int wl = 0;
> > + int wl_count = 0;
> > + int ret = 0;
> > size_t size;
> >
> > cmd = *argv;
> >
> > while (--argc > 0 && **++argv == '-') {
> > - char *arg = *argv;
> > + char *arg = *argv, *str, *token;
> > while (*++arg) {
> > switch (*arg) {
> > case 'b': /* raw binary format */
> > @@ -1025,6 +1033,43 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
> > break;
> > case 'd':
> > del = 1;
> > + break;
> > + case 'w':
> > + wl = 1;
> > + wl_count = 1;
> > +
> > + str = env_get("whitelisted_vars");
> > + if (!str) {
> > + puts("## Error: whitelisted_vars is not set.\n");
> > + return CMD_RET_USAGE;
> > + }
> > +
> > + tmp = strdup(str);
> > + if (!tmp)
> > + return CMD_RET_FAILURE;
> > +
> > + token = strchr(tmp, ' ');
> > + while (!token) {
> > + wl_count++;
> > + token = strchr(token + 1, ' ');
> > + }
> > +
> > + strcpy(tmp, str);
> >
> This is redundant. strchr() doesn't modify the array.
>
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
-------------- 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/20180521/e3325d53/attachment.sig>