Hi Simon,
On Sat, Apr 29, 2023 at 3:27 AM Simon Glass <s...@chromium.org> wrote:
>
> The Linux command line consists of a number of words with optional values.
> At present U-Boot allows this to be changed using the bootargs environment
> variable.
>
> But this is quite painful, since the command line can be very long.
>
> Add a function which can adjust a single field in the command line, so
> that it is easier to make changes before booting.
>
> Signed-off-by: Simon Glass <s...@chromium.org>
> ---
>
> boot/bootflow.c | 178 +++++++++++++++++++++++++++++++++++++++++++
> include/bootflow.h | 40 ++++++++++
> test/boot/bootflow.c | 154 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 372 insertions(+)
>
> diff --git a/boot/bootflow.c b/boot/bootflow.c
> index 62b7f45ab27..7e2442a9599 100644
> --- a/boot/bootflow.c
> +++ b/boot/bootflow.c
> @@ -611,3 +611,181 @@ static int on_bootargs(const char *name, const char
> *value, enum env_op op,
> U_BOOT_ENV_CALLBACK(bootargs, on_bootargs);
> #endif
>
> +static int copy_in(char *buf, char *end, const char *arg, int len,
> + const char *new_val)
> +{
> + char *to = buf;
> +
> + /* copy the arg name */
> + if (to + len >= end)
len seems to be a useless parameter? Isn't len=strlen(arg)?
> + return -E2BIG;
> + memcpy(to, arg, len);
> + to += len;
> +
> + if (new_val == BOOTFLOWCL_EMPTY) {
> + /* no value */
> + } else {
> + bool need_quote = strchr(new_val, ' ');
> + len = strlen(new_val);
> +
> + /* need space for value, equals sign and maybe two quotes */
> + if (to + 1 + 2 * need_quote + len >= end)
It's weird to do a multiply operation on a bool variable 'need_quote'
> + return -E2BIG;
> + *to++ = '=';
> + if (need_quote)
> + *to++ = '"';
> + memcpy(to, new_val, len);
> + to += len;
> + if (need_quote)
> + *to++ = '"';
> + }
> +
> + return to - buf;
> +}
> +
> +int cmdline_set_arg(char *buf, int maxlen, const char *cmdline,
> + const char *set_arg, const char *new_val, int *posp)
> +{
> + bool found_arg = false;
> + const char *from;
> + char *to, *end;
> + int set_arg_len;
> + char empty = '\0';
> + int ret;
> +
> + from = cmdline ?: ∅
> +
> + /* check if the value has quotes inside */
> + if (new_val && new_val != BOOTFLOWCL_EMPTY && strchr(new_val, '"'))
> + return -EBADF;
> +
> + set_arg_len = strlen(set_arg);
> + for (to = buf, end = buf + maxlen; *from;) {
> + const char *val, *arg_end, *val_end, *p;
> + bool in_quote;
> +
> + if (to >= end)
> + return -E2BIG;
I suggest we add more debug messages when such error code is returned,
with details of what condition causes the error, so user knows what
happened.
> + while (*from == ' ')
> + from++;
> + if (!*from)
> + break;
> +
> + /* find the end of this arg */
> + val = NULL;
> + arg_end = NULL;
> + val_end = NULL;
> + in_quote = false;
> + for (p = from;; p++) {
> + if (in_quote) {
> + if (!*p)
> + return -EINVAL;
> + if (*p == '"')
> + in_quote = false;
> + continue;
> + }
> + if (*p == '=') {
> + arg_end = p;
> + val = p + 1;
> + } else if (*p == '"') {
> + in_quote = true;
> + } else if (!*p || *p == ' ') {
> + val_end = p;
> + if (!arg_end)
> + arg_end = p;
> + break;
> + }
> + }
> + /*
> + * At this point val_end points to the end of the value, or
> the
> + * last char after the arg name, if there is no label.
> + * arg_end is the char after the arg name
> + * val points to the value, or NULL if there is none
> + * char after the value.
> + *
> + * fred=1234
> + * ^ ^^ ^
> + * from || |
> + * / \ \
> + * arg_end val val_end
> + */
> + log_debug("from %s arg_end %ld val %ld val_end %ld\n", from,
> + (long)(arg_end - from), (long)(val - from),
> + (long)(val_end - from));
> +
> + if (to != buf) {
> + if (to >= end)
> + return -E2BIG;
> + *to++ = ' ';
> + }
> +
> + /* if this is the target arg, update it */
> + if (!strncmp(from, set_arg, arg_end - from)) {
> + if (!buf) {
> + bool has_quote = val_end[-1] == '"';
> +
> + /*
> + * exclude any start/end quotes from
> + * calculations
> + */
> + if (!val)
> + val = val_end;
> + *posp = val - cmdline + has_quote;
> + return val_end - val - 2 * has_quote;
> + }
> + found_arg = true;
> + if (!new_val) {
> + /* delete this arg */
> + from = val_end + (*val_end == ' ');
> + log_debug("delete from: %s\n", from);
> + if (to != buf)
> + to--; /* drop the space we added */
> + continue;
> + }
> +
> + ret = copy_in(to, end, from, arg_end - from, new_val);
> + if (ret < 0)
> + return ret;
> + to += ret;
> +
> + /* if not the target arg, copy it unchanged */
> + } else if (to) {
> + int len;
> +
> + len = val_end - from;
> + if (to + len >= end)
> + return -E2BIG;
> + memcpy(to, from, len);
> + to += len;
> + }
> + from = val_end;
> + }
> +
> + /* If we didn't find the arg, add it */
> + if (!found_arg) {
> + /* trying to delete something that is not there */
> + if (!new_val || !buf)
> + return -ENOENT;
> + if (to >= end)
> + return -E2BIG;
> +
> + /* add a space to separate it from the previous arg */
> + if (to != buf && to[-1] != ' ')
> + *to++ = ' ';
> + ret = copy_in(to, end, set_arg, set_arg_len, new_val);
> + log_debug("ret=%d, to: %s buf: %s\n", ret, to, buf);
> + if (ret < 0)
> + return ret;
> + to += ret;
> + }
> +
> + /* delete any trailing space */
> + if (to > buf && to[-1] == ' ')
> + to--;
> +
> + if (to >= end)
> + return -E2BIG;
> + *to++ = '\0';
> +
> + return to - buf;
> +}
> diff --git a/include/bootflow.h b/include/bootflow.h
> index 9baa8672083..1ec77f3bf8a 100644
> --- a/include/bootflow.h
> +++ b/include/bootflow.h
> @@ -444,4 +444,44 @@ int bootflow_menu_apply_theme(struct expo *exp, ofnode
> node);
> int bootflow_menu_run(struct bootstd_priv *std, bool text_mode,
> struct bootflow **bflowp);
>
> +#define BOOTFLOWCL_EMPTY ((void *)1)
> +
> +/**
> + * cmdline_set_arg() - Update or read an argument in a cmdline string
> + *
> + * Handles updating a single arg in a cmdline string, returning it in a
> supplied
> + * buffer; also reading an arg from a cmdline string
> + *
> + * When updating, consecutive spaces are squashed as are spaces at the start
> and
> + * end.
> + *
> + * @buf: Working buffer to use (initial contents are ignored). Use NULL when
> + * reading
> + * @maxlen: Length of working buffer. Use 0 when reading
> + * @cmdline: Command line to update, in the form:
> + *
> + * fred mary= jane=123 john="has spaces"
> + *
> + * @set_arg: Argument to set or read (may or may not exist)
> + * @new_val: Value for the new argument. May not include quotes (") but may
> + * include embedded spaces, in which case it will be quoted when added to the
> + * command line. Use NULL to delete the argument from @cmdline,
> BOOTFLOWCL_EMPTY
> + * to set it to an empty value (no '=' sign after arg), "" to add an '=' sign
> + * but with an empty value. Use NULL when reading.
> + * @posp: Ignored when setting an argument; when getting an argument, returns
> + * the start position of its value in @cmdline, after the first quote, if any
> + *
> + * Return:
> + * For updating:
> + * length of new buffer (including \0 terminator) on success, -ENOENT if
> + * @new_val is NULL and @set_arg does not exist in @from, -EINVAL if a
> + * quoted arg-value in @from is not terminated with a quote, -EBADF if
> + * @new_val has spaces but does not start and end with quotes (or it has
> + * quotes in the middle of the string), -E2BIG if @maxlen is too small
> + * For reading:
> + * length of arg value (excluding quotes), -ENOENT if not found
> + */
> +int cmdline_set_arg(char *buf, int maxlen, const char *cmdline,
> + const char *set_arg, const char *new_val, int *posp);
> +
> #endif
> diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c
> index b9ac539107b..c42d86d916c 100644
> --- a/test/boot/bootflow.c
> +++ b/test/boot/bootflow.c
> @@ -683,3 +683,157 @@ static int bootflow_menu_theme(struct unit_test_state
> *uts)
> return 0;
> }
> BOOTSTD_TEST(bootflow_menu_theme, UT_TESTF_DM | UT_TESTF_SCAN_FDT);
> +
> +/**
> + * check_arg() - Check both the normal case and the buffer-overflow case
> + *
> + * @uts: Unit-test state
> + * @expect_ret: Expected return value (i.e. buffer length)
> + * @expect_str: String expected to be returned
> + * @buf: Buffer to use
> + * @from: Original cmdline to update
> + * @arg: Argument to update (e.g. "console")
> + * @val: Value to set (e.g. "ttyS2") or NULL to delete the argument if
> present,
> + * "" to set it to an empty value (e.g. "console=") and BOOTFLOWCL_EMPTY to
> add
> + * it without any value ("initrd")
> + */
> +static int check_arg(struct unit_test_state *uts, int expect_ret,
> + const char *expect_str, char *buf, const char *from,
> + const char *arg, const char *val)
> +{
> + /* check for writing outside the reported bounds */
> + buf[expect_ret] = '[';
> + ut_asserteq(expect_ret,
> + cmdline_set_arg(buf, expect_ret, from, arg, val, NULL));
> + ut_asserteq_str(expect_str, buf);
> + ut_asserteq('[', buf[expect_ret]);
> +
> + /* do the test again but with one less byte in the buffer */
> + ut_asserteq(-E2BIG, cmdline_set_arg(buf, expect_ret - 1, from, arg,
> + val, NULL));
> +
> + return 0;
> +}
> +
> +/* Test of bootflow_cmdline_set_arg() */
> +static int test_bootflow_cmdline_set(struct unit_test_state *uts)
> +{
> + char buf[50];
> + const int size = sizeof(buf);
> +
> + /*
> + * note that buffer-overflow tests are immediately each test case,
> just
> + * top keep the code together
> + */
> +
> + /* add an arg that doesn't already exist, starting from empty */
> + ut_asserteq(-ENOENT, cmdline_set_arg(buf, size, NULL, "me", NULL,
> + NULL));
> +
> + ut_assertok(check_arg(uts, 3, "me", buf, NULL, "me",
> BOOTFLOWCL_EMPTY));
> + ut_assertok(check_arg(uts, 4, "me=", buf, NULL, "me", ""));
> + ut_assertok(check_arg(uts, 8, "me=fred", buf, NULL, "me", "fred"));
> +
> + /* add an arg that doesn't already exist, starting from non-empty */
> + ut_assertok(check_arg(uts, 11, "arg=123 me", buf, "arg=123", "me",
> + BOOTFLOWCL_EMPTY));
> + ut_assertok(check_arg(uts, 12, "arg=123 me=", buf, "arg=123", "me",
> + ""));
> + ut_assertok(check_arg(uts, 16, "arg=123 me=fred", buf, "arg=123",
> "me",
> + "fred"));
> +
> + /* update an arg at the start */
> + ut_assertok(check_arg(uts, 1, "", buf, "arg=123", "arg", NULL));
> + ut_assertok(check_arg(uts, 4, "arg", buf, "arg=123", "arg",
> + BOOTFLOWCL_EMPTY));
> + ut_assertok(check_arg(uts, 5, "arg=", buf, "arg=123", "arg", ""));
> + ut_assertok(check_arg(uts, 6, "arg=1", buf, "arg=123", "arg", "1"));
> + ut_assertok(check_arg(uts, 9, "arg=1234", buf, "arg=123", "arg",
> + "1234"));
> +
> + /* update an arg at the end */
> + ut_assertok(check_arg(uts, 5, "mary", buf, "mary arg=123", "arg",
> + NULL));
> + ut_assertok(check_arg(uts, 9, "mary arg", buf, "mary arg=123", "arg",
> + BOOTFLOWCL_EMPTY));
> + ut_assertok(check_arg(uts, 10, "mary arg=", buf, "mary arg=123",
> "arg",
> + ""));
> + ut_assertok(check_arg(uts, 11, "mary arg=1", buf, "mary arg=123",
> "arg",
> + "1"));
> + ut_assertok(check_arg(uts, 14, "mary arg=1234", buf, "mary arg=123",
> + "arg", "1234"));
> +
> + /* update an arg in the middle */
> + ut_assertok(check_arg(uts, 16, "mary=abc john=2", buf,
> + "mary=abc arg=123 john=2", "arg", NULL));
> + ut_assertok(check_arg(uts, 20, "mary=abc arg john=2", buf,
> + "mary=abc arg=123 john=2", "arg",
> + BOOTFLOWCL_EMPTY));
> + ut_assertok(check_arg(uts, 21, "mary=abc arg= john=2", buf,
> + "mary=abc arg=123 john=2", "arg", ""));
> + ut_assertok(check_arg(uts, 22, "mary=abc arg=1 john=2", buf,
> + "mary=abc arg=123 john=2", "arg", "1"));
> + ut_assertok(check_arg(uts, 25, "mary=abc arg=1234 john=2", buf,
> + "mary=abc arg=123 john=2", "arg", "1234"));
> +
> + /* handle existing args with quotes */
> + ut_assertok(check_arg(uts, 16, "mary=\"abc\" john", buf,
> + "mary=\"abc\" arg=123 john", "arg", NULL));
> +
> + /* handle existing args with quoted spaces */
> + ut_assertok(check_arg(uts, 20, "mary=\"abc def\" john", buf,
> + "mary=\"abc def\" arg=123 john", "arg", NULL));
> +
> + ut_assertok(check_arg(uts, 34, "mary=\"abc def\" arg=123 john def=4",
> + buf, "mary=\"abc def\" arg=123 john", "def",
> + "4"));
> +
> + /* quote at the start */
> + ut_asserteq(-EBADF, cmdline_set_arg(buf, size,
> + "mary=\"abc def\" arg=\"123
> 456\"",
> + "arg", "\"4 5 6", NULL));
> +
> + /* quote at the end */
> + ut_asserteq(-EBADF, cmdline_set_arg(buf, size,
> + "mary=\"abc def\" arg=\"123
> 456\"",
> + "arg", "4 5 6\"", NULL));
> +
> + /* quote in the middle */
> + ut_asserteq(-EBADF, cmdline_set_arg(buf, size,
> + "mary=\"abc def\" arg=\"123
> 456\"",
> + "arg", "\"4 \"5 6\"", NULL));
> +
> + /* handle updating a quoted arg */
> + ut_assertok(check_arg(uts, 27, "mary=\"abc def\" arg=\"4 5 6\"", buf,
> + "mary=\"abc def\" arg=\"123 456\"", "arg",
> + "4 5 6"));
> +
> + /* changing a quoted arg to a non-quoted arg */
> + ut_assertok(check_arg(uts, 23, "mary=\"abc def\" arg=789", buf,
> + "mary=\"abc def\" arg=\"123 456\"", "arg",
> + "789"));
> +
> + /* changing a non-quoted arg to a quoted arg */
> + ut_assertok(check_arg(uts, 29, "mary=\"abc def\" arg=\"456 789\"",
> buf,
> + "mary=\"abc def\" arg=123", "arg", "456 789"));
> +
> + /* handling of spaces */
> + ut_assertok(check_arg(uts, 8, "arg=123", buf, " ", "arg", "123"));
> + ut_assertok(check_arg(uts, 8, "arg=123", buf, " ", "arg", "123"));
> + ut_assertok(check_arg(uts, 13, "john arg=123", buf, " john ", "arg",
> + "123"));
> + ut_assertok(check_arg(uts, 13, "john arg=123", buf, " john arg=123
> ",
> + "arg", "123"));
> + ut_assertok(check_arg(uts, 18, "john arg=123 mary", buf,
> + " john arg=123 mary ", "arg", "123"));
> +
> + /* unchanged arg */
> + ut_assertok(check_arg(uts, 3, "me", buf, "me", "me",
> BOOTFLOWCL_EMPTY));
> +
> + /* arg which starts with the same name */
> + ut_assertok(check_arg(uts, 28, "mary=abc johnathon=2 john=3", buf,
> + "mary=abc johnathon=2 john=1", "john", "3"));
> +
> + return 0;
> +}
> +BOOTSTD_TEST(test_bootflow_cmdline_set, 0);
> --
Regards,
Bin
