[U-Boot] [PATCH] arm: mvebu: kwbimage: inline function to fix use-after-free

[U-Boot] [PATCH] arm: mvebu: kwbimage: inline function to fix use-after-free

[Patrick Wildt]

image_version_file()'s only use is to return the version number of the
specified image, and it's only called by kwbimage_generate().  This
version function mallocs "image_cfg" and reads the contents of the image
into that buffer.  Before return to its caller it frees the buffer.

After extracting the version, kwb_image_generate() tries to calculate
the header size by calling image_headersz_v1().  This function now
accesses "image_cfg", which has already been freed.

Since image_version_file() is only used by a single function, inline it
into kwbimage_generate() and only free the buffer after it is no longer
needed.  This also improves code readability since the code is mostly
equal to kwbimage_set_header().

Signed-off-by: Patrick Wildt <patrick@blueri.se>
---
 tools/kwbimage.c | 93 +++++++++++++++++++++++++++++---------------------------
 1 file changed, 48 insertions(+), 45 deletions(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index 2c637c7446..69ab181db9 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -1452,47 +1452,6 @@ static int image_get_version(void)
 	return e->version;
 }
 
-static int image_version_file(const char *input)
-{
-	FILE *fcfg;
-	int version;
-	int ret;
-
-	fcfg = fopen(input, "r");
-	if (!fcfg) {
-		fprintf(stderr, "Could not open input file %s\n", input);
-		return -1;
-	}
-
-	image_cfg = malloc(IMAGE_CFG_ELEMENT_MAX *
-			   sizeof(struct image_cfg_element));
-	if (!image_cfg) {
-		fprintf(stderr, "Cannot allocate memory\n");
-		fclose(fcfg);
-		return -1;
-	}
-
-	memset(image_cfg, 0,
-	       IMAGE_CFG_ELEMENT_MAX * sizeof(struct image_cfg_element));
-	rewind(fcfg);
-
-	ret = image_create_config_parse(fcfg);
-	fclose(fcfg);
-	if (ret) {
-		free(image_cfg);
-		return -1;
-	}
-
-	version = image_get_version();
-	/* Fallback to version 0 is no version is provided in the cfg file */
-	if (version == -1)
-		version = 0;
-
-	free(image_cfg);
-
-	return version;
-}
-
 static void kwbimage_set_header(void *ptr, struct stat *sbuf, int ifd,
 				struct image_tool_params *params)
 {
@@ -1633,18 +1592,62 @@ static int kwbimage_verify_header(unsigned char *ptr, int image_size,
 static int kwbimage_generate(struct image_tool_params *params,
 			     struct image_type_params *tparams)
 {
+	FILE *fcfg;
 	int alloc_len;
+	int version;
 	void *hdr;
-	int version = 0;
+	int ret;
 
-	version = image_version_file(params->imagename);
-	if (version == 0) {
+	fcfg = fopen(params->imagename, "r");
+	if (!fcfg) {
+		fprintf(stderr, "Could not open input file %s\n",
+			params->imagename);
+		exit(EXIT_FAILURE);
+	}
+
+	image_cfg = malloc(IMAGE_CFG_ELEMENT_MAX *
+			   sizeof(struct image_cfg_element));
+	if (!image_cfg) {
+		fprintf(stderr, "Cannot allocate memory\n");
+		fclose(fcfg);
+		exit(EXIT_FAILURE);
+	}
+
+	memset(image_cfg, 0,
+	       IMAGE_CFG_ELEMENT_MAX * sizeof(struct image_cfg_element));
+	rewind(fcfg);
+
+	ret = image_create_config_parse(fcfg);
+	fclose(fcfg);
+	if (ret) {
+		free(image_cfg);
+		exit(EXIT_FAILURE);
+	}
+
+	version = image_get_version();
+	switch (version) {
+		/*
+		 * Fallback to version 0 if no version is provided in the
+		 * cfg file
+		 */
+	case -1:
+	case 0:
 		alloc_len = sizeof(struct main_hdr_v0) +
 			sizeof(struct ext_hdr_v0);
-	} else {
+		break;
+
+	case 1:
 		alloc_len = image_headersz_v1(NULL);
+		break;
+
+	default:
+		fprintf(stderr, "Unsupported version %d\n", version);
+		free(image_cfg);
+		exit(EXIT_FAILURE);
 	}
 
+	free(image_cfg);
+
 	hdr = malloc(alloc_len);
 	if (!hdr) {
 		fprintf(stderr, "%s: malloc return failure: %s\n",
-- 
2.12.2

[U-Boot] [PATCH] arm: mvebu: kwbimage: inline function to fix use-after-free

[Stefan Roese]

On 10.05.2017 22:18, Patrick Wildt wrote:
> image_version_file()'s only use is to return the version number of the
> specified image, and it's only called by kwbimage_generate().  This
> version function mallocs "image_cfg" and reads the contents of the image
> into that buffer.  Before return to its caller it frees the buffer.
> 
> After extracting the version, kwb_image_generate() tries to calculate
> the header size by calling image_headersz_v1().  This function now
> accesses "image_cfg", which has already been freed.
> 
> Since image_version_file() is only used by a single function, inline it
> into kwbimage_generate() and only free the buffer after it is no longer
> needed.  This also improves code readability since the code is mostly
> equal to kwbimage_set_header().
> 
> Signed-off-by: Patrick Wildt <patrick@blueri.se>

Applied to u-boot-marvell/master.

Thanks,
Stefan