From d9c547c59012c435ce05be52aa3e2f92cfe9919f Mon Sep 17 00:00:00 2001 From: tychovrahe Date: Mon, 20 Jan 2025 22:26:56 +0100 Subject: [PATCH] fix(core): calculate image hash including padding between header and code [no changelog] --- core/embed/projects/boardloader/main.c | 9 +-------- core/embed/projects/bootloader/messages.c | 17 +---------------- core/embed/projects/bootloader_ci/messages.c | 2 +- core/embed/sys/linker/stm32u5g/bootloader.ld | 7 ++++++- core/embed/sys/linker/stm32u5g/firmware.ld | 7 ++++++- core/embed/sys/linker/stm32u5g/prodtest.ld | 6 +++++- core/embed/util/image/image.c | 15 ++++----------- python/src/trezorlib/firmware/core.py | 11 ----------- python/src/trezorlib/firmware/models.py | 15 --------------- 9 files changed, 24 insertions(+), 65 deletions(-) diff --git a/core/embed/projects/boardloader/main.c b/core/embed/projects/boardloader/main.c index 92b47ec374..2eab5d052f 100644 --- a/core/embed/projects/boardloader/main.c +++ b/core/embed/projects/boardloader/main.c @@ -181,14 +181,7 @@ static uint32_t check_sdcard(void) { _Static_assert(IMAGE_CHUNK_SIZE >= BOOTLOADER_MAXSIZE, "BOOTLOADER IMAGE MAXSIZE too large for IMAGE_CHUNK_SIZE"); - const uint32_t headers_end_offset = hdr->hdrlen; - const uint32_t code_start_offset = IMAGE_CODE_ALIGN(headers_end_offset); - - for (uint32_t i = headers_end_offset; i < code_start_offset; i++) { - if (((uint8_t *)sdcard_buf)[i] != 0) { - return 0; - } - } + const uint32_t code_start_offset = hdr->hdrlen; if (sectrue != (check_single_hash(hdr->hashes, diff --git a/core/embed/projects/bootloader/messages.c b/core/embed/projects/bootloader/messages.c index 78cad1d48a..f1371721d8 100644 --- a/core/embed/projects/bootloader/messages.c +++ b/core/embed/projects/bootloader/messages.c @@ -595,21 +595,6 @@ int process_msg_FirmwareUpload(uint8_t iface_num, uint32_t msg_size, memcpy(&hdr, received_hdr, sizeof(hdr)); - size_t headers_end = IMAGE_HEADER_SIZE + vhdr.hdrlen; - size_t tmp_headers_offset = - IMAGE_CODE_ALIGN(IMAGE_HEADER_SIZE + vhdr.hdrlen); - - // check padding between headers and the code - for (size_t i = headers_end; i < tmp_headers_offset; i++) { - if (CHUNK_BUFFER_PTR[i] != 0) { - MSG_SEND_INIT(Failure); - MSG_SEND_ASSIGN_VALUE(code, FailureType_Failure_ProcessError); - MSG_SEND_ASSIGN_STRING(message, "Invalid chunk padding"); - MSG_SEND(Failure); - return UPLOAD_ERR_INVALID_CHUNK_PADDING; - } - } - vendor_header current_vhdr; secbool is_new = secfalse; @@ -727,7 +712,7 @@ int process_msg_FirmwareUpload(uint8_t iface_num, uint32_t msg_size, ensure(erase_storage(NULL), NULL); } - headers_offset = IMAGE_CODE_ALIGN(IMAGE_HEADER_SIZE + vhdr.hdrlen); + headers_offset = IMAGE_HEADER_SIZE + vhdr.hdrlen; read_offset = IMAGE_INIT_CHUNK_SIZE; // request the rest of the first chunk diff --git a/core/embed/projects/bootloader_ci/messages.c b/core/embed/projects/bootloader_ci/messages.c index 29ed6e9c72..9e3251f7a4 100644 --- a/core/embed/projects/bootloader_ci/messages.c +++ b/core/embed/projects/bootloader_ci/messages.c @@ -556,7 +556,7 @@ int process_msg_FirmwareUpload(uint8_t iface_num, uint32_t msg_size, // no user confirmations, go directly to upload - headers_offset = IMAGE_CODE_ALIGN(IMAGE_HEADER_SIZE + vhdr.hdrlen); + headers_offset = IMAGE_HEADER_SIZE + vhdr.hdrlen; read_offset = IMAGE_INIT_CHUNK_SIZE; // request the rest of the first chunk diff --git a/core/embed/sys/linker/stm32u5g/bootloader.ld b/core/embed/sys/linker/stm32u5g/bootloader.ld index 1a4a5f05a6..75bfeb458e 100644 --- a/core/embed/sys/linker/stm32u5g/bootloader.ld +++ b/core/embed/sys/linker/stm32u5g/bootloader.ld @@ -55,13 +55,18 @@ _shutdown_clear_ram_2_end = MCU_SRAM6 + MCU_SRAM6_SIZE; _shutdown_clear_ram_3_start = MCU_SRAM4; _shutdown_clear_ram_3_end = MCU_SRAM4 + MCU_SRAM4_SIZE; -_codelen = SIZEOF(.flash) + SIZEOF(.data) + SIZEOF(.confidential); +_codelen = SIZEOF(.padding) + SIZEOF(.flash) + SIZEOF(.data) + SIZEOF(.confidential); SECTIONS { .header : ALIGN(4) { KEEP(*(.header)); } >FLASH AT>FLASH + .padding : ALIGN(4) { + . = ALIGN(4); + . = ALIGN(CODE_ALIGNMENT); + } >FLASH AT>FLASH + .flash : ALIGN(CODE_ALIGNMENT) { KEEP(*(.vector_table)); . = ALIGN(4); diff --git a/core/embed/sys/linker/stm32u5g/firmware.ld b/core/embed/sys/linker/stm32u5g/firmware.ld index 3bef222030..499be37ac9 100644 --- a/core/embed/sys/linker/stm32u5g/firmware.ld +++ b/core/embed/sys/linker/stm32u5g/firmware.ld @@ -24,7 +24,7 @@ confidential_lma = LOADADDR(.confidential); confidential_vma = ADDR(.confidential); confidential_size = SIZEOF(.confidential); -_codelen = SIZEOF(.flash) + SIZEOF(.data) + SIZEOF(.confidential); +_codelen = SIZEOF(.padding) + SIZEOF(.flash) + SIZEOF(.data) + SIZEOF(.confidential); _flash_start = ORIGIN(FLASH); _flash_end = ORIGIN(FLASH) + LENGTH(FLASH); _heap_start = ADDR(.heap); @@ -39,6 +39,11 @@ SECTIONS { KEEP(*(.header)); } >FLASH AT>FLASH + .padding : ALIGN(4) { + . = ALIGN(4); + . = ALIGN(CODE_ALIGNMENT); + } >FLASH AT>FLASH + .flash : ALIGN(CODE_ALIGNMENT) { KEEP(*(.kernel)); . = ALIGN(COREAPP_ALIGNMENT); diff --git a/core/embed/sys/linker/stm32u5g/prodtest.ld b/core/embed/sys/linker/stm32u5g/prodtest.ld index 288899125d..eb721427ed 100644 --- a/core/embed/sys/linker/stm32u5g/prodtest.ld +++ b/core/embed/sys/linker/stm32u5g/prodtest.ld @@ -56,7 +56,7 @@ _shutdown_clear_ram_3_start = MCU_SRAM4; _shutdown_clear_ram_3_end = MCU_SRAM4 + MCU_SRAM4_SIZE; -_codelen = SIZEOF(.flash) + SIZEOF(.data) + SIZEOF(.confidential); +_codelen = SIZEOF(.padding) + SIZEOF(.flash) + SIZEOF(.data) + SIZEOF(.confidential); _flash_start = ORIGIN(FLASH); _flash_end = ORIGIN(FLASH) + LENGTH(FLASH); @@ -67,6 +67,10 @@ SECTIONS { .header : ALIGN(4) { KEEP(*(.header)); + } >FLASH AT>FLASH + + .padding : ALIGN(4) { + . = ALIGN(4); . = ALIGN(CODE_ALIGNMENT); } >FLASH AT>FLASH diff --git a/core/embed/util/image/image.c b/core/embed/util/image/image.c index 553d8ae256..8ab6817bd9 100644 --- a/core/embed/util/image/image.c +++ b/core/embed/util/image/image.c @@ -263,18 +263,11 @@ 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); - size_t end_offset = offset + hdr->codelen; - // Check area between headers and code - uint32_t padding_size = offset - firstskip; - const uint8_t *addr = - (uint8_t *)flash_area_get_address(area, firstskip, padding_size); - for (size_t i = 0; i < padding_size; i++) { - if (*addr++ != 0) { - return secfalse; - } - } + // check hashes of image chunks + // we hash the image including the padding to the end of the area + size_t offset = firstskip; + size_t end_offset = offset + hdr->codelen; while (offset < end_offset) { size_t bytes_to_check = MIN(IMAGE_CHUNK_SIZE - (offset % IMAGE_CHUNK_SIZE), diff --git a/python/src/trezorlib/firmware/core.py b/python/src/trezorlib/firmware/core.py index eda8d0f7f9..ecae74bc6c 100644 --- a/python/src/trezorlib/firmware/core.py +++ b/python/src/trezorlib/firmware/core.py @@ -113,22 +113,11 @@ class FirmwareImage(Struct): SUBCON = c.Struct( "header" / FirmwareHeader.SUBCON, "_header_end" / c.Tell, - "padding" - / c.Padding( - lambda this: FirmwareImage.calc_padding( - this.header.hw_model, this._header_end - ) - ), "_code_offset" / c.Tell, "code" / c.Bytes(c.this.header.code_length), c.Terminated, ) - @staticmethod - def calc_padding(hw_model: bytes, len: int) -> int: - alignment = Model.from_hw_model(hw_model).code_alignment() - return ((len + alignment - 1) & ~(alignment - 1)) - len - def get_hash_params(self) -> "util.FirmwareHashParameters": return Model.from_hw_model(self.header.hw_model).hash_params() diff --git a/python/src/trezorlib/firmware/models.py b/python/src/trezorlib/firmware/models.py index 1c6d7a9007..f2d4a3a5ce 100644 --- a/python/src/trezorlib/firmware/models.py +++ b/python/src/trezorlib/firmware/models.py @@ -66,9 +66,6 @@ class Model(Enum): def hash_params(self) -> "FirmwareHashParameters": return MODEL_HASH_PARAMS_MAP[self] - def code_alignment(self) -> int: - return MODEL_CODE_ALIGNMENT_MAP[self] - @dataclass class ModelKeys: @@ -368,18 +365,6 @@ MODEL_HASH_PARAMS_MAP = { Model.D002: D002_HASH_PARAMS, } - -MODEL_CODE_ALIGNMENT_MAP = { - Model.T1B1: 0x200, - Model.T2T1: 0x200, - Model.T2B1: 0x200, - Model.T3T1: 0x200, - Model.T3B1: 0x200, - Model.T3W1: 0x200, - Model.D001: 0x200, - Model.D002: 0x400, -} - # deprecated aliases -- don't add more TREZOR_ONE_V1V2 = LEGACY_V1V2