calculate image hash including padding between header and code #4521
Conversation
…code [no changelog]
|
|
why doesn't the same change apply to bootloader image? |
|
it does apply to bootloader, in trezorlib bootloader uses FirmwareImage struct too, and on embedded side bootloader linker script is modified, along with image content check |
| const uint32_t headers_end_offset = hdr->hdrlen; | ||
| const uint32_t code_start_offset = IMAGE_CODE_ALIGN(headers_end_offset); | ||
| const uint32_t padding_end = IMAGE_CODE_ALIGN(headers_end_offset); | ||
|
|
||
| for (uint32_t i = headers_end_offset; i < code_start_offset; i++) { | ||
| for (uint32_t i = headers_end_offset; i < padding_end; i++) { | ||
| if (((uint8_t *)sdcard_buf)[i] != 0) { | ||
| return 0; | ||
| } | ||
| } | ||
|
|
||
| const uint32_t code_start_offset = IMAGE_CODE_ALIGN(headers_end_offset); | ||
|
|
There was a problem hiding this comment.
if that's the case, i don't understand this change.
it seems to me that you're (a) still checking explicitly for zero value padding (that's fine i guess) but also check_single_hash(..., code_start_offset) which is IMAGE_CODE_ALIGN(headers_end_offset)
There was a problem hiding this comment.
yeah that is a bug 40bcbd8
There was a problem hiding this comment.
looked briefly at the C code and it seems ok but I'll leave the main review to @cepetr
note: any reason to keep the padding-is-zero check? it seems that we don't have to now (contents are signed) and it may bite us later (e.g., if we want to fill empty spaces with random data, we may not be able to)
| @@ -264,7 +264,6 @@ secbool check_image_contents(const image_header *const hdr, uint32_t firstskip, | |||
|
|
|||
| // Check the firmware integrity, calculate and compare hashes | |||
| size_t offset = IMAGE_CODE_ALIGN(firstskip); | |||
There was a problem hiding this comment.
this should probably be const size_t something_else = ... ...
| @@ -276,6 +275,11 @@ secbool check_image_contents(const image_header *const hdr, uint32_t firstskip, | |||
| } | |||
| } | |||
|
|
|||
| // check hashes of image chunks | |||
| // we hash the image including the padding to the end of the area | |||
| offset = firstskip; | |||
There was a problem hiding this comment.
...and this should be size_t offset, otherwise it's rather confusing why the same variable is redefined this way
This PR alters the way image hashes are calculated.
Padding between headers and code is now included into hashed area.
In practice, this affects only U5G models, as image and vendor headers are aligned to 512B and such is also is the code alignment on F4 and U585.
T3W1, even though running on U5G, both its image headers and vendor headers are aligned to 1024B, by coincidence, so existing boardloaders and bootloaders should continue to work.
Therefore, only D002 will break and needs reflashing of board+bootloader to work.
Thanks to this change, code alignment can be omitted from trezorctl.