[U-Boot-Users] New Image format: headers hashes.

[U-Boot-Users] New Image format: headers hashes.

[Luigi 'Comio' Mantellini]

Hi Guys,

I ask you fro a comment regarding the new image format.

I'm studying the mkimage code (and image.c file) to understand how new
image works. I have the following observations/questions:

- The mkimage calculate hashes before that timestamp is added to image.
is it better exchange the order of these operations (before add
timestamp then calculate hashes)?
- Does the new format assure that the complete image (_all_ sections,
images, ramdisks, configurations, blobs) are covered by at least an hash
value? should mkimage add at least a hash value for the root node?


Thanks a lot,

luigi

-- 
     ______       Luigi Mantellini
   .'______'.     R&D - Software
  (.'      '.)    Industrie Dial Face S.p.A.
  ( :=----=: )    Via Canzo, 4
  ('.______.')    20068 Peschiera Borromeo (MI), Italy
   '.______.'     Tel.: +39 02 5167 2813
                  Fax:  +39 02 5167 2459
Ind.  Dial Face   Email: luigi.mantellini at idf-hit.com
www.idf-hit.com   GPG fingerprint: 3DD1 7B71 FBDF 6376 1B4A
                                   B003 175F E979 907E 1650

[U-Boot-Users] New Image format: headers hashes.

[Luigi 'Comio' Mantellini]

On gio, 2008-03-27 at 12:13 +0100, Luigi 'Comio' Mantellini wrote:

> 
> 
> - The mkimage calculate hashes before that timestamp is added to
> image.
> is it better exchange the order of these operations (before add
> timestamp then calculate hashes)?
> - Does the new format assure that the complete image (_all_ sections,
> images, ramdisks, configurations, blobs) are covered by at least an
> hash
> value? should mkimage add at least a hash value for the root node?
> 


In addition to the previous question:

- I'm interested to calculate and store from U-boot code but the
fit_image_set_hashes is compiled only when the USE_HOSTCC symbol is
defined. Why? Is there a particular reason? I think that a lot of useful
functions (ifdef-ed by USE_HOSTCC) should be available also in u-boot
code, anyway the linker will drop the unused functions.


Any comments?

thanks

luigi
Industrie Dial Face S.p.A.
Luigi Mantellini
R&D - Software
Industrie Dial Face S.p.A.
Via Canzo, 4 
20068 Peschiera Borromeo (MI), Italy
Tel.:   +39 02 5167 2813
Fax:    +39 02 5167 2459
E-mail: luigi.mantellini at idf-hit.com
GPG fingerprint: 3DD1 7B71 FBDF 6376
1B4A
                 B003 175F E979 907E
1650
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.denx.de/pipermail/u-boot/attachments/20080327/b799b42e/attachment.htm 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: idf_logo.gif
Type: image/gif
Size: 4122 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20080327/b799b42e/attachment.gif 

[U-Boot-Users] New Image format: headers hashes.

[Luigi 'Comio' Mantellini]

Continuing my monologue:

An observation about the fit_image_check_hashes and fit_image_set_hashes
functions:

- The hashes protect only the data sections of the image nodes...
ignoring the others fields (like "load", "entry", etc...). I don't like
this :S I think that the hash values must protect all fields of a node
(like the "old" image format).

to be continue...

ciao

luigi

On gio, 2008-03-27 at 17:06 +0100, Luigi 'Comio' Mantellini wrote:
> On gio, 2008-03-27 at 12:13 +0100, Luigi 'Comio' Mantellini wrote:
> > 
> > 
> > - The mkimage calculate hashes before that timestamp is added to
> > image.
> > is it better exchange the order of these operations (before add
> > timestamp then calculate hashes)?
> > - Does the new format assure that the complete image (_all_
> > sections,
> > images, ramdisks, configurations, blobs) are covered by at least an
> > hash
> > value? should mkimage add at least a hash value for the root node?
> > 
> 
> In addition to the previous question:
> 
> - I'm interested to calculate and store from U-boot code but the
> fit_image_set_hashes is compiled only when the USE_HOSTCC symbol is
> defined. Why? Is there a particular reason? I think that a lot of
> useful functions (ifdef-ed by USE_HOSTCC) should be available also in
> u-boot code, anyway the linker will drop the unused functions.
> 
> 
> Any comments?
> 
> thanks
> 
> luigi
-- 
     ______       Luigi Mantellini
   .'______'.     R&D - Software
  (.'      '.)    Industrie Dial Face S.p.A.
  ( :=----=: )    Via Canzo, 4
  ('.______.')    20068 Peschiera Borromeo (MI), Italy
   '.______.'     Tel.: +39 02 5167 2813
                  Fax:  +39 02 5167 2459
Ind.  Dial Face   Email: luigi.mantellini at idf-hit.com
www.idf-hit.com   GPG fingerprint: 3DD1 7B71 FBDF 6376 1B4A
                                   B003 175F E979 907E 1650

[U-Boot-Users] New Image format: headers hashes.

[Bartlomiej Sieka]

Hi Luigi,

You wrote:
> On gio, 2008-03-27 at 12:13 +0100, Luigi 'Comio' Mantellini wrote:
> 
>>
>> - The mkimage calculate hashes before that timestamp is added to
>> image.
>> is it better exchange the order of these operations (before add
>> timestamp then calculate hashes)?

Please see below.


>> - Does the new format assure that the complete image (_all_ sections,
>> images, ramdisks, configurations, blobs) are covered by at least an
>> hash
>> value?

Currently, the hash calculation is performed only over binary data of
component images (kernel, ramdisk, fdt, etc). In particular, timestamp
information is not protected by the hash, so the ordering of hash
calculation and timestamp addition doesn't matter.


 >> should mkimage add at least a hash value for the root node?

Adding a hash value for the root node only will not be easy. An easier
approach I think would be to add a hash value for the entire FIT image.


> In addition to the previous question:
> 
> - I'm interested to calculate and store from U-boot code but the 
> fit_image_set_hashes is compiled only when the USE_HOSTCC symbol is 
> defined. Why? Is there a particular reason?

There's no need for the function that you mention in current U-Boot
code, that's the reason why it's under USE_HOSTCC. If you're planning on
adding code to U-Boot that will use fit_image_set_hashes(), then just
remove the USE_HOSTCC #define's from around this function.


 > I think that a lot of
 > useful functions (ifdef-ed by USE_HOSTCC) should be available also in
 > u-boot code, anyway the linker will drop the unused functions.

I don't think the linker will do this in case of U-Boot:

With the following patch:

diff --git a/common/image.c b/common/image.c
index f04826a..850a9e4 100644
--- a/common/image.c
+++ b/common/image.c
@@ -1950,7 +1950,6 @@ static int calculate_hash (const void *data, int 
data_len,
  	return 0;
  }

-#ifdef USE_HOSTCC
  /**
   * fit_set_hashes - process FIT component image nodes and calculate hashes
   * @fit: pointer to the FIT format image header
@@ -2119,7 +2118,6 @@ int fit_image_hash_set_value (void *fit, int 
noffset, uint

  	return 0;
  }
-#endif /* USE_HOSTCC */

  /**
   * fit_image_check_hashes - verify data intergity


I see:

$ ${CROSS_COMPILE}nm -S common/image.o | grep fit_image_set_hashes
0000155c 000001f4 T fit_image_set_hashes
$ ${CROSS_COMPILE}nm -S common/libcommon.a | grep fit_image_set_hashes
0000155c 000001f4 T fit_image_set_hashes
$ ${CROSS_COMPILE}nm -S u-boot | grep fit_image_set_hashes
fffb1924 000001f4 T fit_image_set_hashes


There's also size increase with the patch applied:
    text    data     bss     dec     hex filename
  284615   15556  357120  657291   a078b ./u-boot

    text    data     bss     dec     hex filename
  285875   15572  357120  658567   a0c87 u-boot


HTH,
Bartlomiej

[U-Boot-Users] New Image format: headers hashes.

[Luigi 'Comio' Mantellini]

Hi Bartlomiej,

thank you for the answers. See my comments below.

On gio, 2008-04-03 at 11:31 +0200, Bartlomiej Sieka wrote:
> Hi Luigi,
> 
> You wrote:
> > On gio, 2008-03-27 at 12:13 +0100, Luigi 'Comio' Mantellini wrote:
> > 
> >>
> >> - The mkimage calculate hashes before that timestamp is added to
> >> image.
> >> is it better exchange the order of these operations (before add
> >> timestamp then calculate hashes)?
> 
> Please see below.
> 
> 
> >> - Does the new format assure that the complete image (_all_ sections,
> >> images, ramdisks, configurations, blobs) are covered by at least an
> >> hash
> >> value?
> 
> Currently, the hash calculation is performed only over binary data of
> component images (kernel, ramdisk, fdt, etc). In particular, timestamp
> information is not protected by the hash, so the ordering of hash
> calculation and timestamp addition doesn't matter.
> 

Ok. I agree

> 
>  >> should mkimage add at least a hash value for the root node?
> 
> Adding a hash value for the root node only will not be easy. An easier
> approach I think would be to add a hash value for the entire FIT image.
> 
Ok.

> 
> > In addition to the previous question:
> > 
> > - I'm interested to calculate and store from U-boot code but the 
> > fit_image_set_hashes is compiled only when the USE_HOSTCC symbol is 
> > defined. Why? Is there a particular reason?
> 
> There's no need for the function that you mention in current U-Boot
> code, that's the reason why it's under USE_HOSTCC. If you're planning on
> adding code to U-Boot that will use fit_image_set_hashes(), then just
> remove the USE_HOSTCC #define's from around this function.
> 
> 
>  > I think that a lot of
>  > useful functions (ifdef-ed by USE_HOSTCC) should be available also in
>  > u-boot code, anyway the linker will drop the unused functions.
> 
> I don't think the linker will do this in case of U-Boot:
> 
> With the following patch:
> 
> diff --git a/common/image.c b/common/image.c
> index f04826a..850a9e4 100644
> --- a/common/image.c
> +++ b/common/image.c
> @@ -1950,7 +1950,6 @@ static int calculate_hash (const void *data, int 
> data_len,
>   	return 0;
>   }
> 
> -#ifdef USE_HOSTCC
>   /**
>    * fit_set_hashes - process FIT component image nodes and calculate hashes
>    * @fit: pointer to the FIT format image header
> @@ -2119,7 +2118,6 @@ int fit_image_hash_set_value (void *fit, int 
> noffset, uint
> 
>   	return 0;
>   }
> -#endif /* USE_HOSTCC */
> 
>   /**
>    * fit_image_check_hashes - verify data intergity
> 
> 
> I see:
> 
> $ ${CROSS_COMPILE}nm -S common/image.o | grep fit_image_set_hashes
> 0000155c 000001f4 T fit_image_set_hashes
> $ ${CROSS_COMPILE}nm -S common/libcommon.a | grep fit_image_set_hashes
> 0000155c 000001f4 T fit_image_set_hashes
> $ ${CROSS_COMPILE}nm -S u-boot | grep fit_image_set_hashes
> fffb1924 000001f4 T fit_image_set_hashes
> 
> 
> There's also size increase with the patch applied:
>     text    data     bss     dec     hex filename
>   284615   15556  357120  657291   a078b ./u-boot
> 
>     text    data     bss     dec     hex filename
>   285875   15572  357120  658567   a0c87 u-boot
> 
> 

Before to make this test, you should strip the elf-file in order to omit
the non-used code. Alternatively you should check the size of the
u-boot.bin file (that is striped by definition).

Best regards.

luigi

> HTH,
> Bartlomiej
-- 
     ______       Luigi Mantellini
   .'______'.     R&D - Software
  (.'      '.)    Industrie Dial Face S.p.A.
  ( :=----=: )    Via Canzo, 4
  ('.______.')    20068 Peschiera Borromeo (MI), Italy
   '.______.'     Tel.: +39 02 5167 2813
                  Fax:  +39 02 5167 2459
Ind.  Dial Face   Email: luigi.mantellini at idf-hit.com
www.idf-hit.com   GPG fingerprint: 3DD1 7B71 FBDF 6376 1B4A
                                   B003 175F E979 907E 1650

[U-Boot-Users] New Image format: headers hashes.

[Bartlomiej Sieka]

Luigi 'Comio' Mantellini wrote:
> Hi Bartlomiej,
> 
> thank you for the answers. See my comments below.
> 
> On gio, 2008-04-03 at 11:31 +0200, Bartlomiej Sieka wrote:
>> Hi Luigi,
>>
>> You wrote:
>>> On gio, 2008-03-27 at 12:13 +0100, L
>>> - I'm interested to calculate and store from U-boot code but the 
>>> fit_image_set_hashes is compiled only when the USE_HOSTCC symbol is 
>>> defined. Why? Is there a particular reason?
>> There's no need for the function that you mention in current U-Boot
>> code, that's the reason why it's under USE_HOSTCC. If you're planning on
>> adding code to U-Boot that will use fit_image_set_hashes(), then just
>> remove the USE_HOSTCC #define's from around this function.
>>
>>
>>  > I think that a lot of
>>  > useful functions (ifdef-ed by USE_HOSTCC) should be available also in
>>  > u-boot code, anyway the linker will drop the unused functions.
>>
>> I don't think the linker will do this in case of U-Boot:
>>
>> With the following patch:
>>
>> diff --git a/common/image.c b/common/image.c
>> index f04826a..850a9e4 100644
>> --- a/common/image.c
>> +++ b/common/image.c
>> @@ -1950,7 +1950,6 @@ static int calculate_hash (const void *data, int 
>> data_len,
>>   	return 0;
>>   }
>>
>> -#ifdef USE_HOSTCC
>>   /**
>>    * fit_set_hashes - process FIT component image nodes and calculate hashes
>>    * @fit: pointer to the FIT format image header
>> @@ -2119,7 +2118,6 @@ int fit_image_hash_set_value (void *fit, int 
>> noffset, uint
>>
>>   	return 0;
>>   }
>> -#endif /* USE_HOSTCC */
>>
>>   /**
>>    * fit_image_check_hashes - verify data intergity
>>
>>
>> I see:
>>
>> $ ${CROSS_COMPILE}nm -S common/image.o | grep fit_image_set_hashes
>> 0000155c 000001f4 T fit_image_set_hashes
>> $ ${CROSS_COMPILE}nm -S common/libcommon.a | grep fit_image_set_hashes
>> 0000155c 000001f4 T fit_image_set_hashes
>> $ ${CROSS_COMPILE}nm -S u-boot | grep fit_image_set_hashes
>> fffb1924 000001f4 T fit_image_set_hashes
>>
>>
>> There's also size increase with the patch applied:
>>     text    data     bss     dec     hex filename
>>   284615   15556  357120  657291   a078b ./u-boot
>>
>>     text    data     bss     dec     hex filename
>>   285875   15572  357120  658567   a0c87 u-boot
>>
>>
> 
> Before to make this test, you should strip the elf-file in order to omit
> the non-used code. Alternatively you should check the size of the
> u-boot.bin file (that is striped by definition).

"objcopy -O binary" which is used to create u-boot.bin from u-boot
doesn't remove unused functions -- code for fit_image_set_hashes (and
other functions removed from under #ifdef USE_HOSTCC by the patch) is
present in u-boot.bin.

If you add enough of such dead code, you'll see the increase in size of 
u-boot.bin, or run into linking problems -- depending on the particular 
linker script used for the target.

Regards,
Bartlomiej

[U-Boot-Users] New Image format: headers hashes.

[Luigi 'Comio' Mantellini]

Hi Bartlomej,


On gio, 2008-04-03 at 13:48 +0200, Bartlomiej Sieka wrote:
...
> >>
> > 
> > Before to make this test, you should strip the elf-file in order to omit
> > the non-used code. Alternatively you should check the size of the
> > u-boot.bin file (that is striped by definition).
> 
> "objcopy -O binary" which is used to create u-boot.bin from u-boot
> doesn't remove unused functions -- code for fit_image_set_hashes (and
> other functions removed from under #ifdef USE_HOSTCC by the patch) is
> present in u-boot.bin.

I'm sorry, I believed (wrongly) that linker was sufficiently intelligent
to discard the unused symbols and reallocate the rest.
Do you know tools (or ld options) that allow to discard unused
symbols/functions?

Thanks a lot. today I learned something new.

luigi

> If you add enough of such dead code, you'll see the increase in size of 
> u-boot.bin, or run into linking problems -- depending on the particular 
> linker script used for the target.
> 
> Regards,
> Bartlomiej
-- 
     ______       Luigi Mantellini
   .'______'.     R&D - Software
  (.'      '.)    Industrie Dial Face S.p.A.
  ( :=----=: )    Via Canzo, 4
  ('.______.')    20068 Peschiera Borromeo (MI), Italy
   '.______.'     Tel.: +39 02 5167 2813
                  Fax:  +39 02 5167 2459
Ind.  Dial Face   Email: luigi.mantellini at idf-hit.com
www.idf-hit.com   GPG fingerprint: 3DD1 7B71 FBDF 6376 1B4A
                                   B003 175F E979 907E 1650