[U-Boot] [PATCH v1 0/4] env tools fixes

This series addresses some issues when the environment tools are used as library and linked to an external program.
Stefano Babic (4): Rename aes.h to uboot_aes.h env: split fw_env.h in public and private parts env: add a version number to check API env: fix memory leak in fw_env routines
arch/arm/mach-tegra/tegra20/crypto.c | 2 +- cmd/aes.c | 2 +- common/env_common.c | 2 +- common/env_flags.c | 1 + include/{aes.h => uboot_aes.h} | 0 lib/aes.c | 2 +- tools/env/fw_env.c | 73 ++++++++++++++++++++++++++-------- tools/env/fw_env.h | 77 +++++++++++++----------------------- tools/env/fw_env_main.c | 1 + tools/env/fw_env_private.h | 55 ++++++++++++++++++++++++++ 10 files changed, 145 insertions(+), 70 deletions(-) rename include/{aes.h => uboot_aes.h} (100%) create mode 100644 tools/env/fw_env_private.h

aes.h is a too generic name if this file can be exported and used by a program. Rename it to avoid any conflicts with other files (for example, from openSSL).
Signed-off-by: Stefano Babic sbabic@denx.de
---
arch/arm/mach-tegra/tegra20/crypto.c | 2 +- cmd/aes.c | 2 +- common/env_common.c | 2 +- include/{aes.h => uboot_aes.h} | 0 lib/aes.c | 2 +- tools/env/fw_env.h | 2 +- 6 files changed, 5 insertions(+), 5 deletions(-) rename include/{aes.h => uboot_aes.h} (100%)
diff --git a/arch/arm/mach-tegra/tegra20/crypto.c b/arch/arm/mach-tegra/tegra20/crypto.c index 1b82fbb..eae7921 100644 --- a/arch/arm/mach-tegra/tegra20/crypto.c +++ b/arch/arm/mach-tegra/tegra20/crypto.c @@ -8,7 +8,7 @@ #include <common.h> #include <linux/errno.h> #include "crypto.h" -#include "aes.h" +#include "uboot_aes.h"
static u8 zero_key[16];
diff --git a/cmd/aes.c b/cmd/aes.c index 76da3ef..ee1ae13 100644 --- a/cmd/aes.c +++ b/cmd/aes.c @@ -9,7 +9,7 @@ #include <common.h> #include <command.h> #include <environment.h> -#include <aes.h> +#include <uboot_aes.h> #include <malloc.h> #include <asm/byteorder.h> #include <linux/compiler.h> diff --git a/common/env_common.c b/common/env_common.c index 7fb62e8..6845f8d 100644 --- a/common/env_common.c +++ b/common/env_common.c @@ -140,7 +140,7 @@ int set_default_vars(int nvars, char * const vars[]) }
#ifdef CONFIG_ENV_AES -#include <aes.h> +#include <uboot_aes.h> /** * env_aes_cbc_get_key() - Get AES-128-CBC key for the environment * diff --git a/include/aes.h b/include/uboot_aes.h similarity index 100% rename from include/aes.h rename to include/uboot_aes.h diff --git a/lib/aes.c b/lib/aes.c index 9d7a0a1..d6144e6 100644 --- a/lib/aes.c +++ b/lib/aes.c @@ -27,7 +27,7 @@ #else #include <string.h> #endif -#include "aes.h" +#include "uboot_aes.h"
/* forward s-box */ static const u8 sbox[256] = { diff --git a/tools/env/fw_env.h b/tools/env/fw_env.h index 05588ab..0d7130a 100644 --- a/tools/env/fw_env.h +++ b/tools/env/fw_env.h @@ -5,7 +5,7 @@ * SPDX-License-Identifier: GPL-2.0+ */
-#include <aes.h> +#include <uboot_aes.h> #include <stdint.h>
/* Pull in the current config to define the default environment */

On Wed, Apr 05, 2017 at 06:08:00PM +0200, Stefano Babic wrote:
aes.h is a too generic name if this file can be exported and used by a program. Rename it to avoid any conflicts with other files (for example, from openSSL).
Signed-off-by: Stefano Babic sbabic@denx.de
Applied to u-boot/master, thanks!

Move U-Boot private data into a separate file. This lets export fw_env.h to be used by external programs that want to change the environment using the library built in tools/env.
Signed-off-by: Stefano Babic sbabic@denx.de ---
common/env_flags.c | 1 + tools/env/fw_env.c | 1 + tools/env/fw_env.h | 51 +----------------------------------------- tools/env/fw_env_main.c | 1 + tools/env/fw_env_private.h | 55 ++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 59 insertions(+), 50 deletions(-) create mode 100644 tools/env/fw_env_private.h
diff --git a/common/env_flags.c b/common/env_flags.c index 921d377..3c50620 100644 --- a/common/env_flags.c +++ b/common/env_flags.c @@ -11,6 +11,7 @@ #ifdef USE_HOSTCC /* Eliminate "ANSI does not permit..." warnings */ #include <stdint.h> #include <stdio.h> +#include "fw_env_private.h" #include "fw_env.h" #include <env_attr.h> #include <env_flags.h> diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index 862a0b1..fc3f4ce 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -34,6 +34,7 @@ # include <mtd/mtd-user.h> #endif
+#include "fw_env_private.h" #include "fw_env.h"
struct env_opts default_opts = { diff --git a/tools/env/fw_env.h b/tools/env/fw_env.h index 0d7130a..3e5539d 100644 --- a/tools/env/fw_env.h +++ b/tools/env/fw_env.h @@ -5,57 +5,8 @@ * SPDX-License-Identifier: GPL-2.0+ */
-#include <uboot_aes.h> #include <stdint.h> - -/* Pull in the current config to define the default environment */ -#include <linux/kconfig.h> - -#ifndef __ASSEMBLY__ -#define __ASSEMBLY__ /* get only #defines from config.h */ -#include <config.h> -#undef __ASSEMBLY__ -#else -#include <config.h> -#endif - -/* - * To build the utility with the static configuration - * comment out the next line. - * See included "fw_env.config" sample file - * for notes on configuration. - */ -#define CONFIG_FILE "/etc/fw_env.config" - -#ifndef CONFIG_FILE -#define HAVE_REDUND /* For systems with 2 env sectors */ -#define DEVICE1_NAME "/dev/mtd1" -#define DEVICE2_NAME "/dev/mtd2" -#define DEVICE1_OFFSET 0x0000 -#define ENV1_SIZE 0x4000 -#define DEVICE1_ESIZE 0x4000 -#define DEVICE1_ENVSECTORS 2 -#define DEVICE2_OFFSET 0x0000 -#define ENV2_SIZE 0x4000 -#define DEVICE2_ESIZE 0x4000 -#define DEVICE2_ENVSECTORS 2 -#endif - -#ifndef CONFIG_BAUDRATE -#define CONFIG_BAUDRATE 115200 -#endif - -#ifndef CONFIG_BOOTDELAY -#define CONFIG_BOOTDELAY 5 /* autoboot after 5 seconds */ -#endif - -#ifndef CONFIG_BOOTCOMMAND -#define CONFIG_BOOTCOMMAND \ - "bootp; " \ - "setenv bootargs root=/dev/nfs nfsroot=${serverip}:${rootpath} " \ - "ip=${ipaddr}:${serverip}:${gatewayip}:${netmask}:${hostname}::off; " \ - "bootm" -#endif +#include <uboot_aes.h>
struct env_opts { #ifdef CONFIG_FILE diff --git a/tools/env/fw_env_main.c b/tools/env/fw_env_main.c index 443de36..b8bff26 100644 --- a/tools/env/fw_env_main.c +++ b/tools/env/fw_env_main.c @@ -34,6 +34,7 @@ #include <stdlib.h> #include <sys/file.h> #include <unistd.h> +#include "fw_env_private.h" #include "fw_env.h"
#define CMD_PRINTENV "fw_printenv" diff --git a/tools/env/fw_env_private.h b/tools/env/fw_env_private.h new file mode 100644 index 0000000..0c27da0 --- /dev/null +++ b/tools/env/fw_env_private.h @@ -0,0 +1,55 @@ +/* + * (C) Copyright 2002-2008 + * Wolfgang Denk, DENX Software Engineering, wd@denx.de. + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +/* Pull in the current config to define the default environment */ +#include <linux/kconfig.h> + +#ifndef __ASSEMBLY__ +#define __ASSEMBLY__ /* get only #defines from config.h */ +#include <config.h> +#undef __ASSEMBLY__ +#else +#include <config.h> +#endif + +/* + * To build the utility with the static configuration + * comment out the next line. + * See included "fw_env.config" sample file + * for notes on configuration. + */ +#define CONFIG_FILE "/etc/fw_env.config" + +#ifndef CONFIG_FILE +#define HAVE_REDUND /* For systems with 2 env sectors */ +#define DEVICE1_NAME "/dev/mtd1" +#define DEVICE2_NAME "/dev/mtd2" +#define DEVICE1_OFFSET 0x0000 +#define ENV1_SIZE 0x4000 +#define DEVICE1_ESIZE 0x4000 +#define DEVICE1_ENVSECTORS 2 +#define DEVICE2_OFFSET 0x0000 +#define ENV2_SIZE 0x4000 +#define DEVICE2_ESIZE 0x4000 +#define DEVICE2_ENVSECTORS 2 +#endif + +#ifndef CONFIG_BAUDRATE +#define CONFIG_BAUDRATE 115200 +#endif + +#ifndef CONFIG_BOOTDELAY +#define CONFIG_BOOTDELAY 5 /* autoboot after 5 seconds */ +#endif + +#ifndef CONFIG_BOOTCOMMAND +#define CONFIG_BOOTCOMMAND \ + "bootp; " \ + "setenv bootargs root=/dev/nfs nfsroot=${serverip}:${rootpath} "\ + "ip=${ipaddr}:${serverip}:${gatewayip}:${netmask}:${hostname}::off; "\ + "bootm" +#endif

On Wed, Apr 05, 2017 at 06:08:01PM +0200, Stefano Babic wrote:
Move U-Boot private data into a separate file. This lets export fw_env.h to be used by external programs that want to change the environment using the library built in tools/env.
Signed-off-by: Stefano Babic sbabic@denx.de
Applied to u-boot/master, thanks!

Changes in the environment library are difficult to tracked by programs using the library. Add simply an API version number that must be increased each time when the API is changed.
This can be detected and a program can work with different versions of the library.
Signed-off-by: Stefano Babic sbabic@denx.de ---
tools/env/fw_env.h | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/tools/env/fw_env.h b/tools/env/fw_env.h index 3e5539d..cf346b3 100644 --- a/tools/env/fw_env.h +++ b/tools/env/fw_env.h @@ -8,6 +8,13 @@ #include <stdint.h> #include <uboot_aes.h>
+/* + * Programs using the library must check which API is available, + * that varies depending on the U-Boot version. + * This can be changed in future + */ +#define FW_ENV_API_VERSION 1 + struct env_opts { #ifdef CONFIG_FILE char *config_file; @@ -140,4 +147,12 @@ int fw_env_write(char *name, char *value); */ int fw_env_close(struct env_opts *opts);
+/** + * fw_env_version - return the current version of the library + * + * Return: + * version string of the library + */ +char *fw_env_version(void); + unsigned long crc32(unsigned long, const unsigned char *, unsigned);

On Wed, Apr 05, 2017 at 06:08:02PM +0200, Stefano Babic wrote:
Changes in the environment library are difficult to tracked by programs using the library. Add simply an API version number that must be increased each time when the API is changed.
This can be detected and a program can work with different versions of the library.
Signed-off-by: Stefano Babic sbabic@denx.de
Applied to u-boot/master, thanks!

fw_env_open allocates buffers to store the environment, but these buffers are never freed. This becomes quite nasty using the fw_ tools as library, because each access to the environment (even just reading a variable) generates a memory leak equal to the size of the environment.
Fix this renaming fw_env_close() as fw_env_flush(), because the function really flushes the environment from RAM to storage, and add a fw_env_close function to free the allocated resources.
Signed-off-by: Stefano Babic sbabic@denx.de ---
tools/env/fw_env.c | 72 ++++++++++++++++++++++++++++++++++++++++++------------ tools/env/fw_env.h | 17 ++++++++++--- 2 files changed, 70 insertions(+), 19 deletions(-)
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index fc3f4ce..299e0c9 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -278,6 +278,7 @@ int fw_printenv(int argc, char *argv[], int value_only, struct env_opts *opts)
printf ("%s\n", env); } + fw_env_close(opts); return 0; }
@@ -300,10 +301,12 @@ int fw_printenv(int argc, char *argv[], int value_only, struct env_opts *opts) printf("%s=%s\n", name, val); }
+ fw_env_close(opts); + return rc; }
-int fw_env_close(struct env_opts *opts) +int fw_env_flush(struct env_opts *opts) { int ret;
@@ -472,6 +475,7 @@ int fw_setenv(int argc, char *argv[], struct env_opts *opts) char *name, **valv; char *value = NULL; int valc; + int ret;
if (!opts) opts = &default_opts; @@ -491,8 +495,10 @@ int fw_setenv(int argc, char *argv[], struct env_opts *opts) valv = argv + 1; valc = argc - 1;
- if (env_flags_validate_env_set_params(name, valv, valc) < 0) + if (env_flags_validate_env_set_params(name, valv, valc) < 0) { + fw_env_close(opts); return -1; + }
len = 0; for (i = 0; i < valc; ++i) { @@ -518,7 +524,10 @@ int fw_setenv(int argc, char *argv[], struct env_opts *opts)
free(value);
- return fw_env_close(opts); + ret = fw_env_flush(opts); + fw_env_close(opts); + + return ret; }
/* @@ -639,7 +648,9 @@ int fw_parse_script(char *fname, struct env_opts *opts) if (strcmp(fname, "-") != 0) fclose(fp);
- ret |= fw_env_close(opts); + ret |= fw_env_flush(opts); + + fw_env_close(opts);
return ret; } @@ -1105,11 +1116,11 @@ int fw_env_open(struct env_opts *opts) { int crc0, crc0_ok; unsigned char flag0; - void *addr0; + void *addr0 = NULL;
int crc1, crc1_ok; unsigned char flag1; - void *addr1; + void *addr1 = NULL;
int ret;
@@ -1120,14 +1131,15 @@ int fw_env_open(struct env_opts *opts) opts = &default_opts;
if (parse_config(opts)) /* should fill envdevices */ - return -1; + return -EINVAL;
addr0 = calloc(1, CUR_ENVSIZE); if (addr0 == NULL) { fprintf(stderr, "Not enough memory for environment (%ld bytes)\n", CUR_ENVSIZE); - return -1; + ret = -ENOMEM; + goto open_cleanup; }
/* read environment from FLASH to local buffer */ @@ -1146,8 +1158,10 @@ int fw_env_open(struct env_opts *opts) }
dev_current = 0; - if (flash_io (O_RDONLY)) - return -1; + if (flash_io(O_RDONLY)) { + ret = -EIO; + goto open_cleanup; + }
crc0 = crc32 (0, (uint8_t *) environment.data, ENV_SIZE);
@@ -1155,7 +1169,7 @@ int fw_env_open(struct env_opts *opts) ret = env_aes_cbc_crypt(environment.data, 0, opts->aes_key); if (ret) - return ret; + goto open_cleanup; }
crc0_ok = (crc0 == *environment.crc); @@ -1174,7 +1188,8 @@ int fw_env_open(struct env_opts *opts) fprintf(stderr, "Not enough memory for environment (%ld bytes)\n", CUR_ENVSIZE); - return -1; + ret = -ENOMEM; + goto open_cleanup; } redundant = addr1;
@@ -1183,8 +1198,10 @@ int fw_env_open(struct env_opts *opts) * other pointers in environment still point inside addr0 */ environment.image = addr1; - if (flash_io (O_RDONLY)) - return -1; + if (flash_io(O_RDONLY)) { + ret = -EIO; + goto open_cleanup; + }
/* Check flag scheme compatibility */ if (DEVTYPE(dev_current) == MTD_NORFLASH && @@ -1204,7 +1221,8 @@ int fw_env_open(struct env_opts *opts) environment.flag_scheme = FLAG_INCREMENTAL; } else { fprintf (stderr, "Incompatible flash types!\n"); - return -1; + ret = -EINVAL; + goto open_cleanup; }
crc1 = crc32 (0, (uint8_t *) redundant->data, ENV_SIZE); @@ -1213,7 +1231,7 @@ int fw_env_open(struct env_opts *opts) ret = env_aes_cbc_crypt(redundant->data, 0, opts->aes_key); if (ret) - return ret; + goto open_cleanup; }
crc1_ok = (crc1 == redundant->crc); @@ -1285,6 +1303,28 @@ int fw_env_open(struct env_opts *opts) #endif } return 0; + +open_cleanup: + if (addr0) + free(addr0); + + if (addr1) + free(addr0); + + return ret; +} + +/* + * Simply free allocated buffer with environment + */ +int fw_env_close(struct env_opts *opts) +{ + if (environment.image) + free(environment.image); + + environment.image = NULL; + + return 0; }
static int check_device_config(int dev) diff --git a/tools/env/fw_env.h b/tools/env/fw_env.h index cf346b3..04bb646 100644 --- a/tools/env/fw_env.h +++ b/tools/env/fw_env.h @@ -53,7 +53,7 @@ int fw_printenv(int argc, char *argv[], int value_only, struct env_opts *opts); * @opts: how to retrieve environment from flash, defaults are used if NULL * * Description: - * Uses fw_env_open, fw_env_write, fw_env_close + * Uses fw_env_open, fw_env_write, fw_env_flush * * Return: * 0 on success, -1 on failure (modifies errno) @@ -70,7 +70,7 @@ int fw_setenv(int argc, char *argv[], struct env_opts *opts); * @opts: encryption key, configuration file, defaults are used if NULL * * Description: - * Uses fw_env_open, fw_env_write, fw_env_close + * Uses fw_env_open, fw_env_write, fw_env_flush * * Return: * 0 success, -1 on failure (modifies errno) @@ -138,7 +138,17 @@ char *fw_getenv(char *name); int fw_env_write(char *name, char *value);
/** - * fw_env_close - write the environment from RAM cache back to flash + * fw_env_flush - write the environment from RAM cache back to flash + * + * @opts: encryption key, configuration file, defaults are used if NULL + * + * Return: + * 0 on success, -1 on failure (modifies errno) + */ +int fw_env_flush(struct env_opts *opts); + +/** + * fw_env_close - free allocated structure and close env * * @opts: encryption key, configuration file, defaults are used if NULL * @@ -147,6 +157,7 @@ int fw_env_write(char *name, char *value); */ int fw_env_close(struct env_opts *opts);
+ /** * fw_env_version - return the current version of the library *

On Wed, Apr 05, 2017 at 06:08:03PM +0200, Stefano Babic wrote:
fw_env_open allocates buffers to store the environment, but these buffers are never freed. This becomes quite nasty using the fw_ tools as library, because each access to the environment (even just reading a variable) generates a memory leak equal to the size of the environment.
Fix this renaming fw_env_close() as fw_env_flush(), because the function really flushes the environment from RAM to storage, and add a fw_env_close function to free the allocated resources.
Signed-off-by: Stefano Babic sbabic@denx.de
Applied to u-boot/master, thanks!
participants (2)
-
Stefano Babic
-
Tom Rini