Re: [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 


Applied to u-boot-marvell/master.

Thanks,
Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

[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 
---
 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 mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot