[U-Boot] [RFC PATCH 0/7] "Transient" (= export-restricted) environment vars
This series evolved around the idea of introducing a new env_op type to control (and possibly restrict) the export of variables. This is especially useful if one wants to prevent dynamic configuration information from ending up in a saved environment - the 'classic' example being network setup with "dhcp" followed by the "saveenv" command.
(The networking case is even further complicated by the fact that users may actually wish to setup and save a static IP configuration manually, which means that "locking up" the corresponding variables right away isn't a viable solution.)
See also: http://lists.denx.de/pipermail/u-boot/2015-September/227611.html http://lists.denx.de/pipermail/u-boot/2016-April/250237.html
Regards, B. Nortmann
BTW: What the correct 'subsystem'/prefix for the "core" changes related to env vars? patman isn't happy with my "env:" choice...
Bernhard Nortmann (7): env: Allow unconditional access if H_PROGRAMMATIC is set net: dm: Ignore unknown env_op_* constants env: Introduce "export" operation and (access flag) restriction env: Introduce "transient" and "system" access flags sunxi: env: flag fel_* environment vars as "system" env: Introduce setenv_transient() helper function env: Automatically mark dynamic configuration info as "do not export"
cmd/net.c | 34 +++++++++++++++++----------------- cmd/nvedit.c | 24 +++++++++++++++++++++++- common/env_flags.c | 21 ++++++++++++++++++--- include/common.h | 1 + include/configs/sunxi-common.h | 8 ++++++++ include/env_flags.h | 5 ++++- include/search.h | 1 + lib/hashtable.c | 4 ++++ net/eth-uclass.c | 2 ++ 9 files changed, 78 insertions(+), 22 deletions(-)
This patch modifies env_flags_validate() in such a way that any operation will *always* be allowed if H_PROGRAMMATIC is present.
Without this change, programmatically changing environment vars may fail depending on their flags, e.g. when trying to setenv*() a variable that is marked "read-only". http://lists.denx.de/pipermail/u-boot/2016-April/250237.html
Notes: H_FORCE may be insufficient for this purpose, as it can be disabled by CONFIG_ENV_ACCESS_IGNORE_FORCE. H_PROGRAMMATIC indicates "U-Boot internal" use. By contrast, any user interaction (from U-Boot prompt or scripts) is expected to be marked H_INTERACTIVE.
Signed-off-by: Bernhard Nortmann bernhard.nortmann@web.de
---
common/env_flags.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/common/env_flags.c b/common/env_flags.c index 921d377..1087f4e 100644 --- a/common/env_flags.c +++ b/common/env_flags.c @@ -523,6 +523,8 @@ int env_flags_validate(const ENTRY *item, const char *newval, enum env_op op, }
/* check for access permission */ + if (flag & H_PROGRAMMATIC) + return 0; #ifndef CONFIG_ENV_ACCESS_IGNORE_FORCE if (flag & H_FORCE) return 0;
This prevents a possible compiler warning similar to "net/eth-uclass.c:<line>:<pos>: warning: enumeration value 'env_op_*' not handled in switch [-Wswitch]".
Signed-off-by: Bernhard Nortmann bernhard.nortmann@web.de ---
net/eth-uclass.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/net/eth-uclass.c b/net/eth-uclass.c index c15cc4d..e38edd7 100644 --- a/net/eth-uclass.c +++ b/net/eth-uclass.c @@ -230,6 +230,8 @@ static int on_ethaddr(const char *name, const char *value, enum env_op op, break; case env_op_delete: memset(pdata->enetaddr, 0, 6); + default: + break; /* ignore */ } }
Hi Bernhard,
On 11 July 2016 at 12:14, Bernhard Nortmann bernhard.nortmann@web.de wrote:
This prevents a possible compiler warning similar to "net/eth-uclass.c:<line>:<pos>: warning: enumeration value 'env_op_*' not handled in switch [-Wswitch]".
Signed-off-by: Bernhard Nortmann bernhard.nortmann@web.de
net/eth-uclass.c | 2 ++ 1 file changed, 2 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
diff --git a/net/eth-uclass.c b/net/eth-uclass.c index c15cc4d..e38edd7 100644 --- a/net/eth-uclass.c +++ b/net/eth-uclass.c @@ -230,6 +230,8 @@ static int on_ethaddr(const char *name, const char *value, enum env_op op, break; case env_op_delete: memset(pdata->enetaddr, 0, 6);
default:break; /* ignore */
Presumably hitting the default would be an internal error. So in that case there is no point in returning an error, I think, and what you have is best.
} }-- 2.7.3
Regards, Simon
Hi Simon!
Am 15.07.2016 um 05:19 schrieb Simon Glass:
[...] Presumably hitting the default would be an internal error. So in that case there is no point in returning an error, I think, and what you have is best.
Regards, Simon
It's not pretty, but a "catch all" solution. The alternative is to handle all possible env_op_* constants explicitly, even if they don't affect (or relate to) eth*_addr at all. But that would mean this code has to be touched and adjusted every time someone comes up with a new env_op_*...
Regards, B. Nortmann
This patch introduces a new "export" environment operation (env_op_export) and the corresponding access flag ENV_FLAGS_VARACCESS_PREVENT_EXPORT; so that env_flags_validate() may now check requests to export specific variables.
In turn, hexport_r() makes uses of this ability to suppress the export of variables that are flagged accordingly.
Note that env_flags_validate() and hexport_r() will respect H_FORCE and H_PROGRAMMATIC flags, allowing to bypass the export filtering. H_PROGRAMMATIC gets used within env_print() to make sure all variables are listed. This is necessary because env_print() is essentially an "export to text" operation.
Signed-off-by: Bernhard Nortmann bernhard.nortmann@web.de ---
cmd/nvedit.c | 3 ++- common/env_flags.c | 8 +++++++- include/env_flags.h | 3 ++- include/search.h | 1 + lib/hashtable.c | 4 ++++ 5 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c index b67563b..88dbcb9 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -101,7 +101,8 @@ static int env_print(char *name, int flag) }
/* print whole list */ - len = hexport_r(&env_htab, '\n', flag, &res, 0, 0, NULL); + len = hexport_r(&env_htab, '\n', + flag | H_PROGRAMMATIC, &res, 0, 0, NULL);
if (len > 0) { puts(res); diff --git a/common/env_flags.c b/common/env_flags.c index 1087f4e..f39d952 100644 --- a/common/env_flags.c +++ b/common/env_flags.c @@ -510,7 +510,7 @@ int env_flags_validate(const ENTRY *item, const char *newval, enum env_op op, newval = newval ? : "";
/* validate the value to match the variable type */ - if (op != env_op_delete) { + if (op != env_op_delete && op != env_op_export) { enum env_flags_vartype type = (enum env_flags_vartype) (ENV_FLAGS_VARTYPE_BIN_MASK & item->flags);
@@ -560,6 +560,12 @@ int env_flags_validate(const ENTRY *item, const char *newval, enum env_op op, return 1; } break; + case env_op_export: + if (item->flags & ENV_FLAGS_VARACCESS_PREVENT_EXPORT) { + printf("## Don't export "%s"\n", name); + return 1; + } + break; }
return 0; diff --git a/include/env_flags.h b/include/env_flags.h index 0dcec06..7e2362a 100644 --- a/include/env_flags.h +++ b/include/env_flags.h @@ -173,6 +173,7 @@ int env_flags_validate(const ENTRY *item, const char *newval, enum env_op op, #define ENV_FLAGS_VARACCESS_PREVENT_CREATE 0x00000010 #define ENV_FLAGS_VARACCESS_PREVENT_OVERWR 0x00000020 #define ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR 0x00000040 -#define ENV_FLAGS_VARACCESS_BIN_MASK 0x00000078 +#define ENV_FLAGS_VARACCESS_PREVENT_EXPORT 0x00000080 +#define ENV_FLAGS_VARACCESS_BIN_MASK 0x000000F8
#endif /* __ENV_FLAGS_H__ */ diff --git a/include/search.h b/include/search.h index 343dbc3..bb95265 100644 --- a/include/search.h +++ b/include/search.h @@ -23,6 +23,7 @@ enum env_op { env_op_create, env_op_delete, env_op_overwrite, + env_op_export, };
/* Action which shall be performed in the call the hsearch. */ diff --git a/lib/hashtable.c b/lib/hashtable.c index 02b4105..708319d 100644 --- a/lib/hashtable.c +++ b/lib/hashtable.c @@ -621,6 +621,10 @@ ssize_t hexport_r(struct hsearch_data *htab, const char sep, int flag, if ((flag & H_HIDE_DOT) && ep->key[0] == '.') continue;
+ if (env_flags_validate(ep, NULL, env_op_export, + flag & H_FORCE) != 0) + continue; + list[n++] = ep;
totlen += strlen(ep->key) + 2;
Am 11.07.2016 um 20:14 schrieb Bernhard Nortmann:
This patch introduces a new "export" environment operation (env_op_export) and the corresponding access flag ENV_FLAGS_VARACCESS_PREVENT_EXPORT; so that env_flags_validate() may now check requests to export specific variables.
In turn, hexport_r() makes uses of this ability to suppress the export of variables that are flagged accordingly.
Note that env_flags_validate() and hexport_r() will respect H_FORCE and H_PROGRAMMATIC flags, allowing to bypass the export filtering. H_PROGRAMMATIC gets used within env_print() to make sure all variables are listed. This is necessary because env_print() is essentially an "export to text" operation.
Signed-off-by: Bernhard Nortmann bernhard.nortmann@web.de
[...]
diff --git a/lib/hashtable.c b/lib/hashtable.c index 02b4105..708319d 100644 --- a/lib/hashtable.c +++ b/lib/hashtable.c @@ -621,6 +621,10 @@ ssize_t hexport_r(struct hsearch_data *htab, const char sep, int flag, if ((flag & H_HIDE_DOT) && ep->key[0] == '.') continue;
if (env_flags_validate(ep, NULL, env_op_export,flag & H_FORCE) != 0)continue;list[n++] = ep; totlen += strlen(ep->key) + 2;
This is also a remnant of previous experiments, and a too narrow condition that doesn't match the commit message's description. If "flag" is to be fully respected (H_FORCE | H_PROGRAMMATIC bits), then the H_FORCE mask (bitwise and operation) needs to be removed. Fixed in my local branch.
Regards, B. Nortmann
"transient" (='t') is like "any", but requests that a variable should not be exported (ENV_FLAGS_VARACCESS_PREVENT_EXPORT).
"system" (='S') is meant for 'internal' variables that aren't supposed to be changed by the user. It corresponds to "transient" plus "read-only".
Signed-off-by: Bernhard Nortmann bernhard.nortmann@web.de ---
common/env_flags.c | 11 +++++++++-- include/env_flags.h | 2 ++ 2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/common/env_flags.c b/common/env_flags.c index f39d952..2c30c7f 100644 --- a/common/env_flags.c +++ b/common/env_flags.c @@ -28,7 +28,7 @@ #endif
static const char env_flags_vartype_rep[] = "sdxb" ENV_FLAGS_NET_VARTYPE_REPS; -static const char env_flags_varaccess_rep[] = "aroc"; +static const char env_flags_varaccess_rep[] = "aroctS"; static const int env_flags_varaccess_mask[] = { 0, ENV_FLAGS_VARACCESS_PREVENT_DELETE | @@ -37,7 +37,12 @@ static const int env_flags_varaccess_mask[] = { ENV_FLAGS_VARACCESS_PREVENT_DELETE | ENV_FLAGS_VARACCESS_PREVENT_OVERWR, ENV_FLAGS_VARACCESS_PREVENT_DELETE | - ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR}; + ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR, + ENV_FLAGS_VARACCESS_PREVENT_EXPORT, + ENV_FLAGS_VARACCESS_PREVENT_DELETE | + ENV_FLAGS_VARACCESS_PREVENT_CREATE | + ENV_FLAGS_VARACCESS_PREVENT_OVERWR | + ENV_FLAGS_VARACCESS_PREVENT_EXPORT};
#ifdef CONFIG_CMD_ENV_FLAGS static const char * const env_flags_vartype_names[] = { @@ -55,6 +60,8 @@ static const char * const env_flags_varaccess_names[] = { "read-only", "write-once", "change-default", + "transient", /* do not export/save */ + "system", /* = "transient" plus "read-only" */ };
/* diff --git a/include/env_flags.h b/include/env_flags.h index 7e2362a..9997a25 100644 --- a/include/env_flags.h +++ b/include/env_flags.h @@ -25,6 +25,8 @@ enum env_flags_varaccess { env_flags_varaccess_readonly, env_flags_varaccess_writeonce, env_flags_varaccess_changedefault, + env_flags_varaccess_transient, + env_flags_varaccess_locked, env_flags_varaccess_end };
Am 11.07.2016 um 20:14 schrieb Bernhard Nortmann:
"transient" (='t') is like "any", but requests that a variable should not be exported (ENV_FLAGS_VARACCESS_PREVENT_EXPORT).
"system" (='S') is meant for 'internal' variables that aren't supposed to be changed by the user. It corresponds to "transient" plus "read-only".
Signed-off-by: Bernhard Nortmann bernhard.nortmann@web.de
[...]
diff --git a/include/env_flags.h b/include/env_flags.h index 7e2362a..9997a25 100644 --- a/include/env_flags.h +++ b/include/env_flags.h @@ -25,6 +25,8 @@ enum env_flags_varaccess { env_flags_varaccess_readonly, env_flags_varaccess_writeonce, env_flags_varaccess_changedefault,
- env_flags_varaccess_transient,
- env_flags_varaccess_locked, env_flags_varaccess_end };
The "env_flags_varaccess_locked" is a remnant of a previous iteration and should of course read "env_flags_varaccess_system" instead. I've fixed this in my local branch, and the correction will be part of a future v2.
Regards, B. Nortmann
"fel_booted" and "fel_scriptaddr" have a special meaning and get used by U-Boot internally.
This patch aims at both preventing user modification of these values, and at excluding them from being stored on "saveenv".
As this is achieved by setting specialized access flags, also enable the "env flags" command.
Signed-off-by: Bernhard Nortmann bernhard.nortmann@web.de ---
include/configs/sunxi-common.h | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h index 94275a7..1b48cf2 100644 --- a/include/configs/sunxi-common.h +++ b/include/configs/sunxi-common.h @@ -433,6 +433,14 @@ extern int soft_i2c_gpio_scl; "fdt ram " FDT_ADDR_R " 0x100000;" \ "ramdisk ram " RAMDISK_ADDR_R " 0x4000000\0"
+/* + * flag environment vars for FEL mode ("usb boot") as special. "system" access + * means they're marked read-only, and shouldn't be written on "saveenv". + */ +#define CONFIG_ENV_FLAGS_LIST_STATIC "fel_booted:bS,fel_scriptaddr:xS" +/* support "env flags" command */ +#define CONFIG_CMD_ENV_FLAGS + #ifdef CONFIG_MMC #define BOOT_TARGET_DEVICES_MMC(func) func(MMC, mmc, 0) #if CONFIG_MMC_SUNXI_SLOT_EXTRA != -1
Like setenv(), but automatically marks the entry as "don't export".
Signed-off-by: Bernhard Nortmann bernhard.nortmann@web.de ---
cmd/nvedit.c | 21 +++++++++++++++++++++ include/common.h | 1 + 2 files changed, 22 insertions(+)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c index 88dbcb9..3c408f6 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -300,6 +300,27 @@ int setenv(const char *varname, const char *varvalue) }
/** + * Set a "transient" environment variable + * + * Like setenv(), but this automatically marks the + * resulting entry as transient (= "do not export"). + */ +int setenv_transient(const char *varname, const char *varvalue) +{ + int rc = setenv(varname, varvalue); + if (rc == 0) { + ENTRY e, *ep; + + e.key = varname; + e.data = NULL; + hsearch_r(e, FIND, &ep, &env_htab, 0); + if (ep) + ep->flags |= ENV_FLAGS_VARACCESS_PREVENT_EXPORT; + } + return rc; +} + +/** * Set an environment variable to an integer value * * @param varname Environment variable to set diff --git a/include/common.h b/include/common.h index 3feaae6..a8e019a 100644 --- a/include/common.h +++ b/include/common.h @@ -380,6 +380,7 @@ ulong getenv_hex(const char *varname, ulong default_val); int getenv_yesno(const char *var); int saveenv (void); int setenv (const char *, const char *); +int setenv_transient(const char *, const char *); int setenv_ulong(const char *varname, ulong value); int setenv_hex(const char *varname, ulong value); /**
This is an attempt to prevent such information from ending up in exported environment data, especially when doing "saveenv". (http://lists.denx.de/pipermail/u-boot/2015-September/227611.html)
The patch makes use of the new setenv_transient() helper for environment variables that get updated via network configuration: BOOTP/DHCP (netboot_update_env), CDP (cdp_update_env) and link-local protocol (do_link_local).
Signed-off-by: Bernhard Nortmann bernhard.nortmann@web.de ---
cmd/net.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/cmd/net.c b/cmd/net.c index b2f3c7b..84b34ce 100644 --- a/cmd/net.c +++ b/cmd/net.c @@ -116,23 +116,23 @@ static void netboot_update_env(void)
if (net_gateway.s_addr) { ip_to_string(net_gateway, tmp); - setenv("gatewayip", tmp); + setenv_transient("gatewayip", tmp); }
if (net_netmask.s_addr) { ip_to_string(net_netmask, tmp); - setenv("netmask", tmp); + setenv_transient("netmask", tmp); }
if (net_hostname[0]) - setenv("hostname", net_hostname); + setenv_transient("hostname", net_hostname);
if (net_root_path[0]) - setenv("rootpath", net_root_path); + setenv_transient("rootpath", net_root_path);
if (net_ip.s_addr) { ip_to_string(net_ip, tmp); - setenv("ipaddr", tmp); + setenv_transient("ipaddr", tmp); } #if !defined(CONFIG_BOOTP_SERVERIP) /* @@ -141,32 +141,32 @@ static void netboot_update_env(void) */ if (net_server_ip.s_addr) { ip_to_string(net_server_ip, tmp); - setenv("serverip", tmp); + setenv_transient("serverip", tmp); } #endif if (net_dns_server.s_addr) { ip_to_string(net_dns_server, tmp); - setenv("dnsip", tmp); + setenv_transient("dnsip", tmp); } #if defined(CONFIG_BOOTP_DNS2) if (net_dns_server2.s_addr) { ip_to_string(net_dns_server2, tmp); - setenv("dnsip2", tmp); + setenv_transient("dnsip2", tmp); } #endif if (net_nis_domain[0]) - setenv("domain", net_nis_domain); + setenv_transient("domain", net_nis_domain);
#if defined(CONFIG_CMD_SNTP) && defined(CONFIG_BOOTP_TIMEOFFSET) if (net_ntp_time_offset) { sprintf(tmp, "%d", net_ntp_time_offset); - setenv("timeoffset", tmp); + setenv_transient("timeoffset", tmp); } #endif #if defined(CONFIG_CMD_SNTP) && defined(CONFIG_BOOTP_NTPSERVER) if (net_ntp_server.s_addr) { ip_to_string(net_ntp_server, tmp); - setenv("ntpserverip", tmp); + setenv_transient("ntpserverip", tmp); } #endif } @@ -294,14 +294,14 @@ static void cdp_update_env(void) printf("CDP offered appliance VLAN %d\n", ntohs(cdp_appliance_vlan)); vlan_to_string(cdp_appliance_vlan, tmp); - setenv("vlan", tmp); + setenv_transient("vlan", tmp); net_our_vlan = cdp_appliance_vlan; }
if (cdp_native_vlan != htons(-1)) { printf("CDP offered native VLAN %d\n", ntohs(cdp_native_vlan)); vlan_to_string(cdp_native_vlan, tmp); - setenv("nvlan", tmp); + setenv_transient("nvlan", tmp); net_native_vlan = cdp_native_vlan; } } @@ -426,14 +426,14 @@ static int do_link_local(cmd_tbl_t *cmdtp, int flag, int argc,
net_gateway.s_addr = 0; ip_to_string(net_gateway, tmp); - setenv("gatewayip", tmp); + setenv_transient("gatewayip", tmp);
ip_to_string(net_netmask, tmp); - setenv("netmask", tmp); + setenv_transient("netmask", tmp);
ip_to_string(net_ip, tmp); - setenv("ipaddr", tmp); - setenv("llipaddr", tmp); /* store this for next time */ + setenv_transient("ipaddr", tmp); + setenv_transient("llipaddr", tmp); /* store this for next time */
return CMD_RET_SUCCESS; }
Hi Bernhard,
On Mon, Jul 11, 2016 at 1:14 PM, Bernhard Nortmann bernhard.nortmann@web.de wrote:
This series evolved around the idea of introducing a new env_op type to control (and possibly restrict) the export of variables. This is especially useful if one wants to prevent dynamic configuration information from ending up in a saved environment - the 'classic' example being network setup with "dhcp" followed by the "saveenv" command.
(The networking case is even further complicated by the fact that users may actually wish to setup and save a static IP configuration manually, which means that "locking up" the corresponding variables right away isn't a viable solution.)
See also: http://lists.denx.de/pipermail/u-boot/2015-September/227611.html http://lists.denx.de/pipermail/u-boot/2016-April/250237.html
Also see:
http://lists.denx.de/pipermail/u-boot/2010-May/071315.html http://lists.denx.de/pipermail/u-boot/2010-June/073031.html
Regards, B. Nortmann
BTW: What the correct 'subsystem'/prefix for the "core" changes related to env vars? patman isn't happy with my "env:" choice...
I still use "env:" and just silence patman.
Bernhard Nortmann (7): env: Allow unconditional access if H_PROGRAMMATIC is set net: dm: Ignore unknown env_op_* constants env: Introduce "export" operation and (access flag) restriction env: Introduce "transient" and "system" access flags sunxi: env: flag fel_* environment vars as "system" env: Introduce setenv_transient() helper function env: Automatically mark dynamic configuration info as "do not export"
cmd/net.c | 34 +++++++++++++++++----------------- cmd/nvedit.c | 24 +++++++++++++++++++++++- common/env_flags.c | 21 ++++++++++++++++++--- include/common.h | 1 + include/configs/sunxi-common.h | 8 ++++++++ include/env_flags.h | 5 ++++- include/search.h | 1 + lib/hashtable.c | 4 ++++ net/eth-uclass.c | 2 ++ 9 files changed, 78 insertions(+), 22 deletions(-)
-- 2.7.3
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
participants (3)
-
Bernhard Nortmann -
Joe Hershberger -
Simon Glass