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

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

[Stefano Babic]

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

-- 
2.7.4

[U-Boot] [PATCH v1 1/4] Rename aes.h to uboot_aes.h

[Stefano Babic]

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 */
-- 
2.7.4

[U-Boot] [PATCH v1 2/4] env: split fw_env.h in public and private parts

[Stefano Babic]

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 at 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
-- 
2.7.4

[U-Boot] [PATCH v1 3/4] env: add a version number to check API

[Stefano Babic]

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);
-- 
2.7.4

[U-Boot] [PATCH v1 4/4] env: fix memory leak in fw_env routines

[Stefano Babic]

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
  *
-- 
2.7.4

[U-Boot] [U-Boot,v1,1/4] Rename aes.h to uboot_aes.h

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170414/54ae2d8a/attachment.sig>

[U-Boot] [U-Boot, v1, 2/4] env: split fw_env.h in public and private parts

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170414/5c7f73a1/attachment.sig>

[U-Boot] [U-Boot, v1, 3/4] env: add a version number to check API

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170414/417c369d/attachment.sig>

[U-Boot] [U-Boot, v1, 4/4] env: fix memory leak in fw_env routines

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170414/8969b731/attachment.sig>