Hi,
On Mon, 26 Aug 2024 at 06:51, <lukas.funke-...@weidmueller.com> wrote:
>
> From: Lukas Funke <lukas.fu...@weidmueller.com>
>
> This commit extends the flags-variable with ranges for environment
> variables. The range is appended to the variable flags using
> the '@'-character. A range can be decimal (min/max), bitmask or regular
> expression (64 byte).
>
> Value ranges for variables can be used to make the environment more
> robust against faults such as invalid user input or attacks.
>
> Decimal-range: foo:da@<min>-<max>
> Hexadecimal-range(bitmask): bar:xa@0xdeadbeed
> Regex-range: mystring:sa@r"klaus|haus|maus"
>
> Example:
>
> "decimalvalue:dw@100-200,"
> ...
>
> => env set decimalvalue 1
> 1 < 100 || 1 > 200
> => env set decimalvalue 150
> =>
>
> Signed-off-by: Lukas Funke <lukas.fu...@weidmueller.com>
> ---
>
> cmd/nvedit.c | 30 +++---
> env/flags.c | 217 +++++++++++++++++++++++++++++++++++++-
> include/configs/sandbox.h | 5 +
> include/env_flags.h | 27 ++++-
> include/search.h | 1 +
> test/env/Makefile | 1 +
> test/env/range.c | 113 ++++++++++++++++++++
> 7 files changed, 377 insertions(+), 17 deletions(-)
> create mode 100644 test/env/range.c
Can you put this behind a Kconfig? We should avoid the code-size
increase for existing boards.
Also please add doc/
Unfortunately I have a lot of minor comments about this patch, partly
due to the existing env code.
>
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index 98a687bcabb..f02545c7a55 100644
> --- a/cmd/nvedit.c
> +++ b/cmd/nvedit.c
> @@ -352,10 +352,13 @@ static int print_static_flags(const char *var_name,
> const char *flags,
> {
> enum env_flags_vartype type = env_flags_parse_vartype(flags);
> enum env_flags_varaccess access = env_flags_parse_varaccess(flags);
> + struct env_flags_range *range = env_flags_parse_varrange(flags);
>
> - printf("\t%-20s %-20s %-20s\n", var_name,
> + printf("\t%-20s %-20s %-20s %-20s\n", var_name,
> env_flags_get_vartype_name(type),
> - env_flags_get_varaccess_name(access));
> + env_flags_get_varaccess_name(access),
> + env_flags_get_varrange_name(range, type));
This is too long. Perhaps we need to rename the env_flags_get_...
functions to envf_get_...
> + free(range);
>
> return 0;
> }
> @@ -364,6 +367,7 @@ static int print_active_flags(struct env_entry *entry)
> {
> enum env_flags_vartype type;
> enum env_flags_varaccess access;
> + struct env_flags_range *range;
>
> if (entry->flags == 0)
> return 0;
> @@ -371,9 +375,11 @@ static int print_active_flags(struct env_entry *entry)
> type = (enum env_flags_vartype)
> (entry->flags & ENV_FLAGS_VARTYPE_BIN_MASK);
> access = env_flags_parse_varaccess_from_binflags(entry->flags);
> - printf("\t%-20s %-20s %-20s\n", entry->key,
> + range = (struct env_flags_range *)entry->range;
> + printf("\t%-20s %-20s %-20s %-20s\n", entry->key,
> env_flags_get_vartype_name(type),
> - env_flags_get_varaccess_name(access));
> + env_flags_get_varaccess_name(access),
> + env_flags_get_varrange_name(range, type));
>
> return 0;
> }
> @@ -401,19 +407,19 @@ int do_env_flags(struct cmd_tbl *cmdtp, int flag, int
> argc, char *const argv[])
>
> /* Print the static flags that may exist */
> puts("Static flags:\n");
> - printf("\t%-20s %-20s %-20s\n", "Variable Name", "Variable Type",
> - "Variable Access");
> - printf("\t%-20s %-20s %-20s\n", "-------------", "-------------",
> - "---------------");
> + printf("\t%-20s %-20s %-20s %-20s\n", "Variable Name", "Variable
> Type",
> + "Variable Access", "Variable Range");
> + printf("\t%-20s %-20s %-20s %-20s\n", "-------------",
> "-------------",
> + "---------------", "---------------");
> env_attr_walk(ENV_FLAGS_LIST_STATIC, print_static_flags, NULL);
> puts("\n");
>
> /* walk through each variable and print the flags if non-default */
> puts("Active flags:\n");
> - printf("\t%-20s %-20s %-20s\n", "Variable Name", "Variable Type",
> - "Variable Access");
> - printf("\t%-20s %-20s %-20s\n", "-------------", "-------------",
> - "---------------");
> + printf("\t%-20s %-20s %-20s %-20s\n", "Variable Name", "Variable
> Type",
> + "Variable Access", "Variable Range");
> + printf("\t%-20s %-20s %-20s %-20s\n", "-------------",
> "-------------",
> + "---------------", "---------------");
> hwalk_r(&env_htab, print_active_flags);
> return 0;
> }
> diff --git a/env/flags.c b/env/flags.c
> index 233fd460d84..dc0a9c5764f 100644
> --- a/env/flags.c
> +++ b/env/flags.c
> @@ -20,6 +20,9 @@
> #else
> #include <linux/kernel.h>
> #include <env_internal.h>
> +#include <vsprintf.h>
> +#include <malloc.h>
> +#include <slre.h>
Please check include ordering.
> #endif
>
> #ifdef CONFIG_NET
> @@ -34,6 +37,8 @@
> #define ENV_FLAGS_WRITEABLE_VARACCESS_REPS ""
> #endif
>
> +#define ENV_FLAGS_RANGE_SEPERATOR '@'
Too long... how about ENVF_RANGE_SEP ?
> +
> static const char env_flags_vartype_rep[] = "sdxb"
> ENV_FLAGS_NET_VARTYPE_REPS;
> static const char env_flags_varaccess_rep[] =
> "aroc" ENV_FLAGS_WRITEABLE_VARACCESS_REPS;
> @@ -115,6 +120,35 @@ const char *env_flags_get_varaccess_name(enum
> env_flags_varaccess access)
> {
> return env_flags_varaccess_names[access];
> }
> +
> +const char *env_flags_get_varrange_name(struct env_flags_range *range,
> + enum env_flags_vartype type)
> +{
> + static char range_str[64];
Eek, best to avoid statics. How about the caller passes in the buffer?
> +
> + if (!range)
> + return "";
> +
> + switch (type) {
> + case env_flags_vartype_decimal: {
> + snprintf(range_str, sizeof(range_str), "%lld-%lld",
> + range->u.int_range.min, range->u.int_range.max);
> + break;
> + }
Please drop {} around these
> + case env_flags_vartype_hex: {
> + snprintf(range_str, sizeof(range_str), "0x%llx",
> range->u.bitmask);
"%#llx"
Do we really want 64-bit values?
Also see simple_xtoa(), simple_itoa()
> + break;
> + }
> + case env_flags_vartype_string: {
> + strlcpy(range_str, range->u.re, sizeof(range_str));
> + break;
> + }
> + default:
> + return "";
> + }
> +
> + return range_str;
> +}
> #endif /* CONFIG_CMD_ENV_FLAGS */
>
> /*
> @@ -151,6 +185,9 @@ enum env_flags_varaccess env_flags_parse_varaccess(const
> char *flags)
> if (strlen(flags) <= ENV_FLAGS_VARACCESS_LOC)
> return va_default;
>
> + if (flags[ENV_FLAGS_VARACCESS_LOC] == ENV_FLAGS_RANGE_SEPERATOR)
> + return va_default;
> +
> access = strchr(env_flags_varaccess_rep,
> flags[ENV_FLAGS_VARACCESS_LOC]);
>
> @@ -211,6 +248,138 @@ static void skip_num(int hex, const char *value, const
> char **end,
> *end = value;
> }
>
> +struct env_flags_range *env_flags_parse_varrange(const char *flags)
> +{
> + char *range;
> + struct env_flags_range *r;
> + enum env_flags_vartype type;
> +
> + if (!flags)
> + return NULL;
> +
> + range = strchr(flags, ENV_FLAGS_RANGE_SEPERATOR);
> + if (!range)
> + return NULL;
> +
> + range++;
> +
> + r = (struct env_flags_range *)malloc(sizeof(struct env_flags_range));
> + if (!r)
> + return NULL;
Isn't that an error?
> +
> + type = env_flags_parse_vartype(flags);
> +
> + /* parse regex range r"someregex" */
> + if (!strncmp(range, "r\"", 2)) {
> + if (IS_ENABLED(CONFIG_REGEX)) {
> + if (type != env_flags_vartype_string) {
> + free(r);
> + return NULL;
> + }
> +
> + char *rstart = strchr(range, '"');
> + char *rend = strrchr(range, '"');
> +
> + memset(r->u.re, 0, sizeof(r->u.re));
> +
> + if (rstart == rend) /* invalid format r" */
> + return NULL;
> +
> + rstart++;
> + if (rstart == rend) /* empty regex is okay */
> + return r;
> +
> + if ((rend - rstart) < sizeof(r->u.re))
> + strlcpy(r->u.re, rstart, rend - rstart);
> + else
> + return NULL; /* too big */
> + }
> + } else if (is_hex_prefix(range)) {
> + /* parse bitmask range 0x[a-fA-F0-9]+ */
> + if (type != env_flags_vartype_hex) {
> + free(r);
> + return NULL;
> + }
> + r->u.bitmask = (u64)simple_strtol(range, NULL, 16);
> + } else {
> + /* parse integer range [0-9]+-[0-9]+ */
> + s64 min, max;
This supports a signed range, but might someone want unsigned?
> +
> + if (type != env_flags_vartype_decimal) {
> + free(r);
> + return NULL;
> + }
> +
> + char *lhs = strcpy(r->u.re, range); /* exploit regex buffer */
> + char *rhs = strchr(lhs[0] == '-' ? lhs + 1 : lhs, '-');
> +
> + if (!rhs) {
> + free(r);
> + return NULL;
> + }
> +
> + *rhs++ = '\0';
> + min = simple_strtol(lhs, NULL, 10);
> + max = simple_strtol(rhs, NULL, 10);
> + r->u.int_range.min = min;
> + r->u.int_range.max = max;
> + }
> +
> + return r;
> +}
In general, this function should likely return an error code, with r
returned as a pointer argument.
> +
> +static int _env_flags_validate_range(const struct env_flags_range *range,
> + enum env_flags_vartype type,
> + const char *newval)
> +{
> + if (!range)
> + return -1;
> +
> + switch (type) {
> + case env_flags_vartype_decimal: {
> + s64 value = simple_strtol(newval, NULL, 10);
> + s64 min = range->u.int_range.min;
> + s64 max = range->u.int_range.max;
> +
> + if (value < min || value > max) {
> + printf("## Error: value out of bounds\n"
> + "%lld < %lld || %lld > %lld\n", value, min,
> value, max);
> + return -1;
> + }
> + break;
> + }
> + case env_flags_vartype_hex: {
> + u64 value = (u64)simple_strtoll(newval, NULL, 16);
> + u64 mask = range->u.bitmask;
> +
> + if ((value | mask) != mask) {
> + printf("## Error: value out of bounds\n"
> + "value = 0x%llx, mask 0x%llx\n", value, mask);
> + return -1;
> + }
> + break;
> + }
> + case env_flags_vartype_string: {
> +#if defined(CONFIG_REGEX)
Can you use: if IS_ENABLED() ?
> + struct slre slre;
> +
> + if (slre_compile(&slre, range->u.re)) {
> + if (slre_match(&slre, newval, strlen(newval), NULL)
> == 0) {
> + printf("## Error: value does not match
> regex\n"
> + "value = %s, re = %s\n", newval,
> range->u.re);
> + return -1;
> + }
> + }
> +#endif
> + break;
> + }
> + default:
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> #ifdef CONFIG_NET
> int eth_validate_ethaddr_str(const char *addr)
> {
> @@ -241,7 +410,7 @@ int eth_validate_ethaddr_str(const char *addr)
> * with that format
> */
> static int _env_flags_validate_type(const char *value,
> - enum env_flags_vartype type)
> + enum env_flags_vartype type)
> {
> const char *end;
> #ifdef CONFIG_NET
> @@ -360,6 +529,20 @@ enum env_flags_varaccess env_flags_get_varaccess(const
> char *name)
> return env_flags_parse_varaccess(flags);
> }
>
> +/*
> + * Look up the range of a variable directly from the .flags var.
Needs a proper comment...what does it return?
> + */
> +struct env_flags_range *env_flags_get_range(const char *name)
> +{
> + const char *flags_list = env_get(ENV_FLAGS_VAR);
> + char flags[ENV_FLAGS_ATTR_MAX_LEN + 1];
> +
> + if (env_flags_lookup(flags_list, name, flags))
> + return NULL;
> +
> + return env_flags_parse_varrange(flags);
> +}
> +
> /*
> * Validate that the proposed new value for "name" is valid according to the
> * defined flags for that variable, if any.
> @@ -395,6 +578,25 @@ int env_flags_validate_varaccess(const char *name, int
> check_mask)
> return (check_mask & access_mask) != 0;
> }
>
> +/*
> + * Validate that the variable "name" is valid according to
> + * the defined range for that variable, if any.
same
> + */
> +int env_flags_validate_range(const char *name, const char *newval)
> +{
> + int r;
> + enum env_flags_vartype type;
> + struct env_flags_range *range;
> +
> + type = env_flags_get_type(name);
> + range = env_flags_get_range(name);
> +
> + r = _env_flags_validate_range(range, type, newval);
> +
> + free(range);
> + return r;
> +}
> +
> /*
> * Validate the parameters to "env set" directly
> */
> @@ -460,8 +662,10 @@ void env_flags_init(struct env_entry *var_entry)
> ret = env_flags_lookup(flags_list, var_name, flags);
>
> /* if any flags were found, set the binary form to the entry */
> - if (!ret && strlen(flags))
> + if (!ret && strlen(flags)) {
> var_entry->flags = env_parse_flags_to_bin(flags);
> + var_entry->range = env_flags_parse_varrange(flags);
error handling?
> + }
> }
>
> /*
> @@ -489,11 +693,13 @@ static int set_flags(const char *name, const char
> *value, void *priv)
> /* does the env variable actually exist? */
> if (ep != NULL) {
> /* the flag list is empty, so clear the flags */
> - if (value == NULL || strlen(value) == 0)
> + if (value == NULL || strlen(value) == 0) {
Should be: if !value || !strlen(value)
or even !value || !*value to maybe save code space
> ep->flags = 0;
> - else
> + } else {
> /* assign the requested flags */
> ep->flags = env_parse_flags_to_bin(value);
> + ep->range = env_flags_parse_varrange(value);
> + }
> }
>
> return 0;
> @@ -547,6 +753,9 @@ int env_flags_validate(const struct env_entry *item,
> const char *newval,
> name, newval, env_flags_vartype_rep[type]);
> return -1;
> }
> + if (item->range &&
> + _env_flags_validate_range(item->range, type, newval) < 0)
> + return -1;
> }
>
> /* check for access permission */
> diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
> index 2372485c84e..8dec2b2cc89 100644
> --- a/include/configs/sandbox.h
> +++ b/include/configs/sandbox.h
> @@ -21,4 +21,9 @@
> /* Unused but necessary to build */
> #define CFG_SYS_UBOOT_BASE CONFIG_TEXT_BASE
>
> +#define CFG_ENV_FLAGS_LIST_STATIC \
> + "decimalvalue:da@100-200," \
> + "regexvalue:sa@r\"foo|bar\"," \
> + "bitmaskvalue:xa@0xf00f"
> +
> #endif
> diff --git a/include/env_flags.h b/include/env_flags.h
> index 2476043b0e3..dfe45ce04d5 100644
> --- a/include/env_flags.h
> +++ b/include/env_flags.h
> @@ -32,8 +32,19 @@ enum env_flags_varaccess {
> env_flags_varaccess_end
> };
>
> +struct env_flags_range {
Please comment the struct
> + union {
> + char re[64];
> + u64 bitmask;
> + struct {
> + s64 min;
> + s64 max;
> + } int_range;
> + } u;
> +};
> +
> #define ENV_FLAGS_VAR ".flags"
> -#define ENV_FLAGS_ATTR_MAX_LEN 2
> +#define ENV_FLAGS_ATTR_MAX_LEN 64
Please comment this item. I don't know what it means.
> #define ENV_FLAGS_VARTYPE_LOC 0
> #define ENV_FLAGS_VARACCESS_LOC 1
>
> @@ -108,6 +119,11 @@ const char *env_flags_get_vartype_name(enum
> env_flags_vartype type);
> * Return the name of the access.
> */
> const char *env_flags_get_varaccess_name(enum env_flags_varaccess access);
> +/*
> + * Return the range of the access.
That is too vague, please add a proper function comment in the normal
U-Boot style.
> + */
> +const char *env_flags_get_varrange_name(struct env_flags_range *range,
> +
> enum env_flags_vartype type);
> #endif
>
> /*
> @@ -118,6 +134,10 @@ enum env_flags_vartype env_flags_parse_vartype(const
> char *flags);
> * Parse the flags string from a .flags attribute list into the varaccess
> enum.
> */
> enum env_flags_varaccess env_flags_parse_varaccess(const char *flags);
> +/*
> + * Parse the flags string from a .flags attribute list into the varaccess
> enum.
same here
I know you are just following the existing code, but please try to
improve it :-)
> + */
> +struct env_flags_range *env_flags_parse_varrange(const char *flags);
> /*
> * Parse the binary flags from a hash table entry into the varaccess enum.
> */
> @@ -154,6 +174,11 @@ int env_flags_validate_access(const char *name, int
> check_mask);
> * the defined flags for that variable, if any.
> */
> int env_flags_validate_varaccess(const char *name, int check_mask);
> +/*
> + * Validate that the variable "name" is valid according to
> + * the defined range for that variable, if any.
> + */
> +int env_flags_validate_range(const char *name, const char *newval)
> /*
> * Validate the parameters passed to "env set" for type compliance
> */
> diff --git a/include/search.h b/include/search.h
> index 7faf23f4aca..c3de3109a4d 100644
> --- a/include/search.h
> +++ b/include/search.h
> @@ -34,6 +34,7 @@ struct env_entry {
> int flags);
> #endif
> int flags;
> + void *range;
Please add comment...also why is this void * ?
> };
>
> /*
> diff --git a/test/env/Makefile b/test/env/Makefile
> index 9a98fd47966..717cbc5555e 100644
> --- a/test/env/Makefile
> +++ b/test/env/Makefile
> @@ -6,3 +6,4 @@ obj-y += cmd_ut_env.o
> obj-y += attr.o
> obj-y += hashtable.o
> obj-$(CONFIG_ENV_IMPORT_FDT) += fdt.o
> +obj-y += range.o
> diff --git a/test/env/range.c b/test/env/range.c
> new file mode 100644
> index 00000000000..ed2cf27ef91
> --- /dev/null
> +++ b/test/env/range.c
> @@ -0,0 +1,113 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * (C) Copyright 2024
> + * Lukas Funke, Weidmueller GmbH, lukas.fu...@weidmueller.com
> + */
> +
> +#include <vsprintf.h>
This should go below env_flags
> +#include <command.h>
> +#include <env_attr.h>
> +#include <env_flags.h>
> +#include <test/env.h>
> +#include <test/ut.h>
> +
> +/*
> + * Set values according to their range
> + */
> +static int env_test_env_set_range(struct unit_test_state *uts)
> +{
> + int r;
> + char *value;
> +
> + r = env_set("decimalvalue", "100");
> + ut_asserteq(0, r);
ut_assertok(env_set("decimalvalue", "100"));
> + value = env_get("decimalvalue");
> + ut_asserteq_str("100", value);
> + r = env_set("decimalvalue", "500");
> + ut_asserteq(1, r);
please combine the line pairs into a single line
> +
> + r = env_set("regexvalue", "foo");
> + ut_asserteq(0, r);
> + value = env_get("regexvalue");
> + ut_asserteq_str("foo", value);
> + r = env_set("regexvalue", "klaus");
> + ut_asserteq(1, r);
> +
> + r = env_set("bitmaskvalue", "0xf00f");
> + ut_asserteq(0, r);
> + value = env_get("bitmaskvalue");
> + ut_asserteq_str("0xf00f", value);
> + r = env_set("bitmaskvalue", "0x0110");
> + ut_asserteq(1, r);
> +
> + return 0;
> +}
> +
Drop blank line (convention we use in tests)
> +ENV_TEST(env_test_env_set_range, 0);
Please add UTF_CONSOLE and assert_nextline() calls above, so you can
check the error output.
> +
> +static int env_test_range_to_str(struct unit_test_state *uts)
> +{
> + struct env_flags_range range;
> +
> + range.u.int_range.min = 123;
> + range.u.int_range.max = 456;
> + ut_asserteq_str("123-456",
> + env_flags_get_varrange_name(&range,
> +
> env_flags_vartype_decimal));
> +
> + strcpy(&range.u.re[0], "^(some|value)_[0-9a-zA-Z]");
> + ut_asserteq_str("^(some|value)_[0-9a-zA-Z]",
> + env_flags_get_varrange_name(&range,
> +
> env_flags_vartype_string));
> +
> + range.u.bitmask = 0x12300000;
> + ut_asserteq_str("0x12300000",
> + env_flags_get_varrange_name(&range,
> + env_flags_vartype_hex));
blank line before final return
> + return 0;
> +}
> +
> +ENV_TEST(env_test_range_to_str, 0);
> +
> +static int env_test_range_parse(struct unit_test_state *uts)
> +{
> + char str[64];
> + struct env_flags_range *range;
> +
> + range = env_flags_parse_varrange("s@r\"somevalue\"");
> + ut_assertnonnull(range);
> + free(range);
> +
> + range = env_flags_parse_varrange("sw");
> + ut_assertnull(range);
> +
> + snprintf(str, sizeof(str), "d@%ld-%ld", LONG_MIN, LONG_MAX);
> + range = env_flags_parse_varrange(str);
> + ut_assertnonnull(range);
> + ut_assert(range->u.int_range.min == LONG_MIN);
ut_asserteq(LONG_MIN, range->u.int_range.min)
> + ut_assert(range->u.int_range.max == LONG_MAX);
> + free(range);
> +
> + snprintf(str, sizeof(str), "d@0-%ld", LONG_MAX);
> + range = env_flags_parse_varrange(str);
> + ut_assertnonnull(range);
> + ut_assert(range->u.int_range.min == 0);
> + ut_assert(range->u.int_range.max == LONG_MAX);
> + free(range);
> +
> + snprintf(str, sizeof(str), "d@%ld-0", LONG_MIN);
> + range = env_flags_parse_varrange(str);
> + ut_assertnonnull(range);
> + ut_assert(range->u.int_range.min == LONG_MIN);
> + ut_assert(range->u.int_range.max == 0);
> + free(range);
> +
> + range = env_flags_parse_varrange("x@0xff0000ff");
> + ut_assertnonnull(range);
> + ut_assert(range->u.bitmask == 0xff0000ff);
> + free(range);
You could check there are no memory leaks here. See for example
lib_test_abuf_set().
> +
> + return 0;
> +}
> +
> +ENV_TEST(env_test_range_parse, 0);
> --
> 2.30.2
>
Regards,
Simon
