[U-Boot] [PATCH] mkimage: SEGFAULT with imximage on 64 bit systems

[U-Boot] [PATCH] mkimage: SEGFAULT with imximage on 64 bit systems

[Stefano Babic]

Running mkimage to generate an imximage produces a SEGFAULT
on 64 bit machines due to pointer arithmetic limited to 32 bit.

Signed-off-by: Stefano Babic <sbabic@denx.de>
---
 tools/imximage.c |   10 +++++-----
 tools/imximage.h |    2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/imximage.c b/tools/imximage.c
index 59923ff..76cd903 100644
--- a/tools/imximage.c
+++ b/tools/imximage.c
@@ -111,7 +111,7 @@ static void imximage_print_header(const void *ptr)
 		exit(EXIT_FAILURE);
 	}
 
-	ext_header = (flash_cfg_parms_t *) ((uint32_t)&imx_hdr->dcd_table +
+	ext_header = (flash_cfg_parms_t *) ((uint64_t)&imx_hdr->dcd_table +
 			sizeof(dcd_preamble_t) + size);
 
 	printf("Image Type:   Freescale IMX Boot Image\n");
@@ -264,15 +264,15 @@ static void imximage_set_header(void *ptr, struct stat *sbuf, int ifd,
 	fhdr->app_code_jump_vector = params->ep;
 
 	base_offset = fhdr->app_dest_ptr + hdr->flash_offset ;
-	fhdr->dcd_ptr_ptr = (uint32_t) ((uint32_t)&fhdr->dcd_ptr -
-		(uint32_t)&fhdr->app_code_jump_vector) + base_offset ;
+	fhdr->dcd_ptr_ptr = (uint32_t) ((uint64_t)&fhdr->dcd_ptr -
+		(uint64_t)&fhdr->app_code_jump_vector) + base_offset ;
 
 	fhdr->dcd_ptr = base_offset +
 			((uint32_t)&hdr->dcd_table -
 			(uint32_t)&hdr->fhdr);
 
 	/* The external flash header must be at the end of the DCD table */
-	ext_header = (flash_cfg_parms_t *) ((uint32_t)&hdr->dcd_table +
+	ext_header = (flash_cfg_parms_t *) ((uint64_t)&hdr->dcd_table +
 			dcd_len +
 			sizeof(dcd_preamble_t));
 	ext_header->length = sbuf->st_size +
@@ -281,7 +281,7 @@ static void imximage_set_header(void *ptr, struct stat *sbuf, int ifd,
 
 	/* Security feature are not supported */
 	fhdr->app_code_csf = 0;
-	fhdr->super_root_key = NULL;
+	fhdr->super_root_key = 0;
 
 }
 
diff --git a/tools/imximage.h b/tools/imximage.h
index c579f51..b4d926d 100644
--- a/tools/imximage.h
+++ b/tools/imximage.h
@@ -81,7 +81,7 @@ typedef struct {
 	uint32_t app_code_barker;
 	uint32_t app_code_csf;
 	uint32_t dcd_ptr_ptr;
-	hab_rsa_public_key *super_root_key;
+	uint32_t super_root_key;
 	uint32_t dcd_ptr;
 	uint32_t app_dest_ptr;
 } flash_header_t;
-- 
1.6.3.3

[U-Boot] [PATCH] mkimage: SEGFAULT with imximage on 64 bit systems

[Mike Frysinger]

On Friday 29 January 2010 05:22:13 Stefano Babic wrote:
> Running mkimage to generate an imximage produces a SEGFAULT
> on 64 bit machines due to pointer arithmetic limited to 32 bit.

man this is terrible terrible code.  using 64bit casts may fix 64bit systems, 
but doesnt seem right on 32bit systems as you'd introduce more pointer/size 
mismatches.

just glancing through this file shows other random issues:
	- the structs seem to overlay the file on disk ?  but there's no code to 
do target endianness to host endianness translation ?
	- the structs are missing ((packed)) attributes

> -	ext_header = (flash_cfg_parms_t *) ((uint32_t)&imx_hdr->dcd_table +
> +	ext_header = (flash_cfg_parms_t *) ((uint64_t)&imx_hdr->dcd_table +
>  			sizeof(dcd_preamble_t) + size);

if the only thing you want is imx_hdr->ext_header, why not do it that way:
	ext_header = &imx_hdr->ext_header

hardcoding the layout of a struct into random places in the source is asking 
for unnecessary trouble.

>  	base_offset = fhdr->app_dest_ptr + hdr->flash_offset ;
> -	fhdr->dcd_ptr_ptr = (uint32_t) ((uint32_t)&fhdr->dcd_ptr -
> -		(uint32_t)&fhdr->app_code_jump_vector) + base_offset ;
> +	fhdr->dcd_ptr_ptr = (uint32_t) ((uint64_t)&fhdr->dcd_ptr -
> +		(uint64_t)&fhdr->app_code_jump_vector) + base_offset ;

you dont need casts to do simple pointer arithmetic:
fhdr->dcd_ptr_ptr = (uint32_t) (&fhdr->dcd_ptr - &fhdr->app_code_jump_vector)
	+ base_offset;
(there also shouldnt be a space before that semicolon)

then again, this looks like you're doing constant subtraction.  the distance 
between two struct members is always going to be the same, so why dont you use 
offsetof() to avoid the random pointer ugliness.

>  	/* The external flash header must be at the end of the DCD table */
> -	ext_header = (flash_cfg_parms_t *) ((uint32_t)&hdr->dcd_table +
> +	ext_header = (flash_cfg_parms_t *) ((uint64_t)&hdr->dcd_table +
>  			dcd_len +
>  			sizeof(dcd_preamble_t));

same issue as the first hunk
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100129/83bc403f/attachment.pgp 

[U-Boot] [PATCH] mkimage: SEGFAULT with imximage on 64 bit systems

[Stefano Babic]

Mike Frysinger wrote:
> man this is terrible terrible code.  using 64bit casts may fix 64bit systems, 
> but doesnt seem right on 32bit systems as you'd introduce more pointer/size 
> mismatches.

You are right - it works on 32 bit systems for the only reason that I
compute a fix offset inside a structure, as you pointed out later.

> if the only thing you want is imx_hdr->ext_header, why not do it that way:
> 	ext_header = &imx_hdr->ext_header

I can't, this is wrong. The ext_header is not fixed, but depends on the
size of the DCD table, that is variable. This table is set via a
configuration file and is specific for each board.
The ext_header pointer must be set at the next 32bit location after this
table.

> then again, this looks like you're doing constant subtraction.  the distance 
> between two struct members is always going to be the same, so why dont you use 
> offsetof() to avoid the random pointer ugliness.

Sometimes I miss the easiest solution. Thanks to point this out !

Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH V2] mkimage: SEGFAULT with imximage on 64 bit systems

[Stefano Babic]

Running mkimage to generate an imximage produces a SEGFAULT
on 64 bit machines due to pointer arithmetic limited to 32 bit.

Signed-off-by: Stefano Babic <sbabic@denx.de>
---
 tools/imximage.c |   30 ++++++++++++++----------------
 tools/imximage.h |    2 +-
 2 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/tools/imximage.c b/tools/imximage.c
index 59923ff..43da678 100644
--- a/tools/imximage.c
+++ b/tools/imximage.c
@@ -101,22 +101,23 @@ static void imximage_print_header(const void *ptr)
 	struct imx_header *imx_hdr = (struct imx_header *) ptr;
 	flash_header_t *hdr = &imx_hdr->fhdr;
 	uint32_t size;
-	flash_cfg_parms_t *ext_header;
+	uint32_t length;
+	dcd_t *dcd = &imx_hdr->dcd_table;
 
 	size = imx_hdr->dcd_table.preamble.length;
 	if (size > (MAX_HW_CFG_SIZE * sizeof(dcd_type_addr_data_t))) {
 		fprintf(stderr,
 			"Error: Image corrupt DCD size %d exceed maximum %d\n",
-			size / sizeof(dcd_type_addr_data_t), MAX_HW_CFG_SIZE);
+			(uint32_t)(size / sizeof(dcd_type_addr_data_t)),
+			MAX_HW_CFG_SIZE);
 		exit(EXIT_FAILURE);
 	}
 
-	ext_header = (flash_cfg_parms_t *) ((uint32_t)&imx_hdr->dcd_table +
-			sizeof(dcd_preamble_t) + size);
+	length =  dcd->preamble.length / sizeof(dcd_type_addr_data_t);
 
 	printf("Image Type:   Freescale IMX Boot Image\n");
 	printf("Data Size:    ");
-	genimg_print_size(ext_header->length);
+	genimg_print_size(dcd->addr_data[length].type);
 	printf("Load Address: %08x\n", (unsigned int)hdr->app_dest_ptr);
 	printf("Entry Point:  %08x\n", (unsigned int)hdr->app_code_jump_vector);
 }
@@ -237,7 +238,7 @@ static uint32_t imximage_parse_cfg_file(struct imx_header *imxhdr, char *name)
 	dcd->preamble.length = dcd_len * sizeof(dcd_type_addr_data_t);
 	fclose(fd);
 
-	return dcd->preamble.length;
+	return dcd_len;
 }
 
 static void imximage_set_header(void *ptr, struct stat *sbuf, int ifd,
@@ -246,7 +247,7 @@ static void imximage_set_header(void *ptr, struct stat *sbuf, int ifd,
 	struct imx_header *hdr = (struct imx_header *)ptr;
 	flash_header_t *fhdr = &hdr->fhdr;
 	int dcd_len;
-	flash_cfg_parms_t *ext_header;
+	dcd_t *dcd = &hdr->dcd_table;
 	uint32_t base_offset;
 
 	/* Set default offset */
@@ -264,24 +265,21 @@ static void imximage_set_header(void *ptr, struct stat *sbuf, int ifd,
 	fhdr->app_code_jump_vector = params->ep;
 
 	base_offset = fhdr->app_dest_ptr + hdr->flash_offset ;
-	fhdr->dcd_ptr_ptr = (uint32_t) ((uint32_t)&fhdr->dcd_ptr -
-		(uint32_t)&fhdr->app_code_jump_vector) + base_offset ;
+	fhdr->dcd_ptr_ptr = (uint32_t) (offsetof(flash_header_t, dcd_ptr) -
+		offsetof(flash_header_t, app_code_jump_vector) +
+		base_offset);
 
 	fhdr->dcd_ptr = base_offset +
-			((uint32_t)&hdr->dcd_table -
-			(uint32_t)&hdr->fhdr);
+			offsetof(struct imx_header, dcd_table);
 
 	/* The external flash header must be at the end of the DCD table */
-	ext_header = (flash_cfg_parms_t *) ((uint32_t)&hdr->dcd_table +
-			dcd_len +
-			sizeof(dcd_preamble_t));
-	ext_header->length = sbuf->st_size +
+	dcd->addr_data[dcd_len].type = sbuf->st_size +
 				hdr->flash_offset +
 				sizeof(struct imx_header);
 
 	/* Security feature are not supported */
 	fhdr->app_code_csf = 0;
-	fhdr->super_root_key = NULL;
+	fhdr->super_root_key = 0;
 
 }
 
diff --git a/tools/imximage.h b/tools/imximage.h
index c579f51..b4d926d 100644
--- a/tools/imximage.h
+++ b/tools/imximage.h
@@ -81,7 +81,7 @@ typedef struct {
 	uint32_t app_code_barker;
 	uint32_t app_code_csf;
 	uint32_t dcd_ptr_ptr;
-	hab_rsa_public_key *super_root_key;
+	uint32_t super_root_key;
 	uint32_t dcd_ptr;
 	uint32_t app_dest_ptr;
 } flash_header_t;
-- 
1.6.3.3

[U-Boot] [PATCH V2] mkimage: SEGFAULT with imximage on 64 bit systems

[Kim Phillips]

On Fri, 5 Feb 2010 15:16:02 +0100
Stefano Babic <sbabic@denx.de> wrote:

> Running mkimage to generate an imximage produces a SEGFAULT
> on 64 bit machines due to pointer arithmetic limited to 32 bit.
>
> Signed-off-by: Stefano Babic <sbabic@denx.de>
> ---

Acked-by: Kim Phillips <kim.phillips@freescale.com>

..in that it gets rid of most warnings in a typical 83xx build:

... PCI HOST Configuring for MPC837XEMDS board...
imximage.c: In function ?imximage_print_header?:
imximage.c:110: warning: format ?%d? expects type ?int?, but argument 3 has type ?long unsigned int?
imximage.c:114: warning: cast from pointer to integer of different size
imximage.c: In function ?imximage_parse_cfg_file?:
imximage.c:145: warning: passing argument 2 of ?getline? from incompatible pointer type
/usr/include/bits/stdio.h:116: note: expected ?size_t *? but argument is of type ?uint32_t *?
imximage.c: In function ?imximage_set_header?:
imximage.c:267: warning: cast from pointer to integer of different size
imximage.c:268: warning: cast from pointer to integer of different size
imximage.c:271: warning: cast from pointer to integer of different size
imximage.c:272: warning: cast from pointer to integer of different size
imximage.c:275: warning: cast from pointer to integer of different size

I'm about to send another patch fix a fix for the last warning.

Kim

[U-Boot] [PATCH V2] mkimage: SEGFAULT with imximage on 64 bit systems

[Wolfgang Denk]

Dear Stefano Babic,

In message <1265379362-8794-1-git-send-email-sbabic@denx.de> you wrote:
> Running mkimage to generate an imximage produces a SEGFAULT
> on 64 bit machines due to pointer arithmetic limited to 32 bit.
> 
> Signed-off-by: Stefano Babic <sbabic@denx.de>
> ---
>  tools/imximage.c |   30 ++++++++++++++----------------
>  tools/imximage.h |    2 +-
>  2 files changed, 15 insertions(+), 17 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Nothing ever becomes real till it is experienced -- even a proverb is
no proverb to you till your life has illustrated it.     - John Keats