From 4cab08e32b7d0d9956edded2cd8d238690a6c435 Mon Sep 17 00:00:00 2001 From: Thomas Altenbach Date: Fri, 26 Apr 2024 18:31:57 +0200 Subject: [PATCH 1/5] bootutil: Properly retrieve image headers after interrupted swap-scratch For swap using scratch, the boot_read_image_header routine, responsible for reading the image headers, was always looking for the primary and secondary image's headers at the beginning of respectively the primary and secondary slots, regardless of the current boot status. This means if during a swap-scratch upgrade a reset happens after the sector containing the image header in the primary or secondary slot has been erased, invalid image headers were read since at that time the location of the headers has changed. Currently, this doesn't seem to cause any issue because the swap-scratch algorithm is implemented in such a way the content of the headers is no more necessary when the headers are erased. However, to be able to decrypt the secondary image when copied to the primary slot instead of when copied to the scratch area, properly reading the secondary image's header is required even after it has been erased from the secondary slot. To that end, the boot_read_image_header is modified to determine from the boot status the current location of the image headers and to always read the actual header, no matter the current state of the upgrade process. Signed-off-by: Thomas Altenbach --- boot/bootutil/src/loader.c | 2 +- boot/bootutil/src/swap_scratch.c | 233 ++++++++++++++++++++++--------- 2 files changed, 171 insertions(+), 64 deletions(-) diff --git a/boot/bootutil/src/loader.c b/boot/bootutil/src/loader.c index bd3a7f09c..54e4d9b75 100644 --- a/boot/bootutil/src/loader.c +++ b/boot/bootutil/src/loader.c @@ -1872,7 +1872,7 @@ boot_prepare_image_for_update(struct boot_loader_state *state, } #endif -#ifdef MCUBOOT_SWAP_USING_MOVE +#if defined(MCUBOOT_SWAP_USING_SCRATCH) || defined(MCUBOOT_SWAP_USING_MOVE) /* * Must re-read image headers because the boot status might * have been updated in the previous function call. diff --git a/boot/bootutil/src/swap_scratch.c b/boot/bootutil/src/swap_scratch.c index 66cbdce5f..d0f822e82 100644 --- a/boot/bootutil/src/swap_scratch.c +++ b/boot/bootutil/src/swap_scratch.c @@ -47,35 +47,6 @@ int boot_status_fails = 0; #define BOOT_STATUS_ASSERT(x) ASSERT(x) #endif -int -boot_read_image_header(struct boot_loader_state *state, int slot, - struct image_header *out_hdr, struct boot_status *bs) -{ - const struct flash_area *fap; - int area_id; - int rc = 0; - - (void)bs; - -#if (BOOT_IMAGE_NUMBER == 1) - (void)state; -#endif - - area_id = flash_area_id_from_multi_image_slot(BOOT_CURR_IMG(state), slot); - - rc = flash_area_open(area_id, &fap); - if (rc == 0) { - rc = flash_area_read(fap, 0, out_hdr, sizeof *out_hdr); - flash_area_close(fap); - } - - if (rc != 0) { - rc = BOOT_EFLASH; - } - - return rc; -} - #if !defined(MCUBOOT_DIRECT_XIP) && !defined(MCUBOOT_RAM_LOAD) /** * Reads the status of a partially-completed swap, if any. This is necessary @@ -470,6 +441,84 @@ boot_copy_sz(const struct boot_loader_state *state, int last_sector_idx, return sz; } +/** + * Finds the index of the last sector in the primary slot that needs swapping. + * + * @param state Current bootloader's state. + * @param copy_size Total number of bytes to swap. + * + * @return Index of the last sector in the primary slot that needs swapping. + */ +static int +find_last_sector_idx(const struct boot_loader_state *state, uint32_t copy_size) +{ + int last_sector_idx; + uint32_t primary_slot_size; + uint32_t secondary_slot_size; + + primary_slot_size = 0; + secondary_slot_size = 0; + last_sector_idx = 0; + + /* + * Knowing the size of the largest image between both slots, here we + * find what is the last sector in the primary slot that needs swapping. + * Since we already know that both slots are compatible, the secondary + * slot's last sector is not really required after this check is finished. + */ + while (1) { + if ((primary_slot_size < copy_size) || + (primary_slot_size < secondary_slot_size)) { + primary_slot_size += boot_img_sector_size(state, + BOOT_PRIMARY_SLOT, + last_sector_idx); + } + if ((secondary_slot_size < copy_size) || + (secondary_slot_size < primary_slot_size)) { + secondary_slot_size += boot_img_sector_size(state, + BOOT_SECONDARY_SLOT, + last_sector_idx); + } + if (primary_slot_size >= copy_size && + secondary_slot_size >= copy_size && + primary_slot_size == secondary_slot_size) { + break; + } + last_sector_idx++; + } + + return last_sector_idx; +} + +/** + * Finds the number of swap operations that have to be performed to swap the two images. + * + * @param state Current bootloader's state. + * @param copy_size Total number of bytes to swap. + * + * @return The number of swap operations that have to be performed. +*/ +static uint32_t +find_swap_count(const struct boot_loader_state *state, uint32_t copy_size) +{ + int first_sector_idx; + int last_sector_idx; + uint32_t swap_count; + + last_sector_idx = find_last_sector_idx(state, copy_size); + + swap_count = 0; + + while (last_sector_idx >= 0) { + boot_copy_sz(state, last_sector_idx, &first_sector_idx); + + last_sector_idx = first_sector_idx - 1; + swap_count++; + } + + return swap_count; +} + /** * Swaps the contents of two flash regions within the two image slots. * @@ -691,43 +740,10 @@ swap_run(struct boot_loader_state *state, struct boot_status *bs, int first_sector_idx; int last_sector_idx; uint32_t swap_idx; - int last_idx_secondary_slot; - uint32_t primary_slot_size; - uint32_t secondary_slot_size; - primary_slot_size = 0; - secondary_slot_size = 0; - last_sector_idx = 0; - last_idx_secondary_slot = 0; BOOT_LOG_INF("Starting swap using scratch algorithm."); - /* - * Knowing the size of the largest image between both slots, here we - * find what is the last sector in the primary slot that needs swapping. - * Since we already know that both slots are compatible, the secondary - * slot's last sector is not really required after this check is finished. - */ - while (1) { - if ((primary_slot_size < copy_size) || - (primary_slot_size < secondary_slot_size)) { - primary_slot_size += boot_img_sector_size(state, - BOOT_PRIMARY_SLOT, - last_sector_idx); - } - if ((secondary_slot_size < copy_size) || - (secondary_slot_size < primary_slot_size)) { - secondary_slot_size += boot_img_sector_size(state, - BOOT_SECONDARY_SLOT, - last_idx_secondary_slot); - } - if (primary_slot_size >= copy_size && - secondary_slot_size >= copy_size && - primary_slot_size == secondary_slot_size) { - break; - } - last_sector_idx++; - last_idx_secondary_slot++; - } + last_sector_idx = find_last_sector_idx(state, copy_size); swap_idx = 0; while (last_sector_idx >= 0) { @@ -857,4 +873,95 @@ int app_max_size(struct boot_loader_state *state) #endif /* !MCUBOOT_DIRECT_XIP && !MCUBOOT_RAM_LOAD */ +int +boot_read_image_header(struct boot_loader_state *state, int slot, + struct image_header *out_hdr, struct boot_status *bs) +{ + const struct flash_area *fap; +#ifdef MCUBOOT_SWAP_USING_SCRATCH + uint32_t swap_count; + uint32_t swap_size; +#endif + int area_id; + int hdr_slot; + int rc = 0; + +#ifndef MCUBOOT_SWAP_USING_SCRATCH + (void)bs; +#endif + +#if (BOOT_IMAGE_NUMBER == 1) + (void)state; +#endif + + hdr_slot = slot; + +#ifdef MCUBOOT_SWAP_USING_SCRATCH + /* If the slots are being swapped, the headers might have been moved to scratch area or to the + * other slot depending on the progress of the swap process. + */ + if (bs && !boot_status_is_reset(bs)) { + rc = boot_find_status(BOOT_CURR_IMG(state), &fap); + + if (rc != 0) { + rc = BOOT_EFLASH; + goto done; + } + + rc = boot_read_swap_size(fap, &swap_size); + flash_area_close(fap); + + if (rc != 0) { + rc = BOOT_EFLASH; + goto done; + } + + swap_count = find_swap_count(state, swap_size); + + if (bs->idx - BOOT_STATUS_IDX_0 >= swap_count) { + /* If all segments have been swapped, the header is located in the other slot */ + hdr_slot = (slot == BOOT_PRIMARY_SLOT) ? BOOT_SECONDARY_SLOT : BOOT_PRIMARY_SLOT; + } else if (bs->idx - BOOT_STATUS_IDX_0 == swap_count - 1) { + /* If the last swap operation is in progress, the headers are currently being swapped + * since the first segment of each slot is the last to be processed. + */ + + if (slot == BOOT_SECONDARY_SLOT && bs->state >= BOOT_STATUS_STATE_1) { + /* After BOOT_STATUS_STATE_1, the secondary image's header has been moved to the + * scratch area. + */ + hdr_slot = BOOT_NUM_SLOTS; + } else if (slot == BOOT_PRIMARY_SLOT && bs->state >= BOOT_STATUS_STATE_2) { + /* After BOOT_STATUS_STATE_2, the primary image's header has been moved to the + * secondary slot. + */ + hdr_slot = BOOT_SECONDARY_SLOT; + } + } + } + + if (hdr_slot == BOOT_NUM_SLOTS) { + area_id = FLASH_AREA_IMAGE_SCRATCH; + } else { + area_id = flash_area_id_from_multi_image_slot(BOOT_CURR_IMG(state), hdr_slot); + } +#else + area_id = flash_area_id_from_multi_image_slot(BOOT_CURR_IMG(state), hdr_slot); +#endif + + rc = flash_area_open(area_id, &fap); + if (rc == 0) { + rc = flash_area_read(fap, 0, out_hdr, sizeof *out_hdr); + flash_area_close(fap); + } + + if (rc != 0) { + rc = BOOT_EFLASH; + goto done; + } + +done: + return rc; +} + #endif /* !MCUBOOT_SWAP_USING_MOVE */ From 9809c9071f9e5b89d224f5dd244a0cd79b50e312 Mon Sep 17 00:00:00 2001 From: Thomas Altenbach Date: Thu, 25 Apr 2024 15:29:21 +0200 Subject: [PATCH 2/5] bootutil: Keep image encrypted in scratch area Currently, when swap using scratch is used with encrypted images, MCUboot is decrypting the images during the copy from the secondary slot to the scratch area. This means the scratch area contains plaintext image data and therefore that the scratch area must be placed in the MCU's internal flash memory. This commit makes the necessary changes to perform the decryption when copying from the scratch area to the primary slot instead, making possible to place the scratch area in an external flash memory since the scratch area is now encrypted. Note that BOOT_SWAP_SAVE_ENCTLV must be enabled if the scratch area is placed in external flash memory. Signed-off-by: Thomas Altenbach --- boot/boot_serial/src/boot_serial_encryption.c | 2 +- boot/bootutil/include/bootutil/enc_key.h | 2 +- boot/bootutil/src/encrypted.c | 4 +- boot/bootutil/src/image_validate.c | 2 +- boot/bootutil/src/loader.c | 38 +++++++++---------- 5 files changed, 24 insertions(+), 24 deletions(-) diff --git a/boot/boot_serial/src/boot_serial_encryption.c b/boot/boot_serial/src/boot_serial_encryption.c index 6201e6b69..51d25024e 100644 --- a/boot/boot_serial/src/boot_serial_encryption.c +++ b/boot/boot_serial/src/boot_serial_encryption.c @@ -175,7 +175,7 @@ decrypt_region_inplace(struct boot_loader_state *state, blk_sz = tlv_off - (off + bytes_copied); } } - boot_encrypt(BOOT_CURR_ENC(state), image_index, fap, + boot_encrypt(BOOT_CURR_ENC(state), image_index, flash_area_get_id(fap), (off + bytes_copied + idx) - hdr->ih_hdr_size, blk_sz, blk_off, &buf[idx]); } diff --git a/boot/bootutil/include/bootutil/enc_key.h b/boot/bootutil/include/bootutil/enc_key.h index 768dd8e7e..d8dab9013 100644 --- a/boot/bootutil/include/bootutil/enc_key.h +++ b/boot/bootutil/include/bootutil/enc_key.h @@ -59,7 +59,7 @@ int boot_enc_decrypt(const uint8_t *buf, uint8_t *enckey); bool boot_enc_valid(struct enc_key_data *enc_state, int image_index, const struct flash_area *fap); void boot_encrypt(struct enc_key_data *enc_state, int image_index, - const struct flash_area *fap, uint32_t off, uint32_t sz, + int fa_id, uint32_t off, uint32_t sz, uint32_t blk_off, uint8_t *buf); void boot_enc_zeroize(struct enc_key_data *enc_state); diff --git a/boot/bootutil/src/encrypted.c b/boot/bootutil/src/encrypted.c index 212774e3a..44975cc49 100644 --- a/boot/bootutil/src/encrypted.c +++ b/boot/bootutil/src/encrypted.c @@ -681,7 +681,7 @@ boot_enc_valid(struct enc_key_data *enc_state, int image_index, void boot_encrypt(struct enc_key_data *enc_state, int image_index, - const struct flash_area *fap, uint32_t off, uint32_t sz, + int fa_id, uint32_t off, uint32_t sz, uint32_t blk_off, uint8_t *buf) { struct enc_key_data *enc; @@ -701,7 +701,7 @@ boot_encrypt(struct enc_key_data *enc_state, int image_index, nonce[14] = (uint8_t)(off >> 8); nonce[15] = (uint8_t)off; - rc = flash_area_id_to_multi_image_slot(image_index, flash_area_get_id(fap)); + rc = flash_area_id_to_multi_image_slot(image_index, fa_id); if (rc < 0) { assert(0); return; diff --git a/boot/bootutil/src/image_validate.c b/boot/bootutil/src/image_validate.c index a697676b6..b4e8b7983 100644 --- a/boot/bootutil/src/image_validate.c +++ b/boot/bootutil/src/image_validate.c @@ -150,7 +150,7 @@ bootutil_img_hash(struct enc_key_data *enc_state, int image_index, /* Only payload is encrypted (area between header and TLVs) */ if (off >= hdr_size && off < tlv_off) { blk_off = (off - hdr_size) & 0xf; - boot_encrypt(enc_state, image_index, fap, off - hdr_size, + boot_encrypt(enc_state, image_index, flash_area_get_id(fap), off - hdr_size, blk_sz, blk_off, tmp_buf); } } diff --git a/boot/bootutil/src/loader.c b/boot/bootutil/src/loader.c index 54e4d9b75..ef1dfe897 100644 --- a/boot/bootutil/src/loader.c +++ b/boot/bootutil/src/loader.c @@ -1218,10 +1218,13 @@ boot_copy_region(struct boot_loader_state *state, uint32_t off; uint32_t tlv_off; size_t blk_off; + int enc_area_id; struct image_header *hdr; uint16_t idx; uint32_t blk_sz; uint8_t image_index; + bool encrypted_src; + bool encrypted_dst; #endif TARGET_STATIC uint8_t buf[BUF_SZ] __attribute__((aligned(4))); @@ -1245,25 +1248,22 @@ boot_copy_region(struct boot_loader_state *state, #ifdef MCUBOOT_ENC_IMAGES image_index = BOOT_CURR_IMG(state); - if ((flash_area_get_id(fap_src) == FLASH_AREA_IMAGE_SECONDARY(image_index) || - flash_area_get_id(fap_dst) == FLASH_AREA_IMAGE_SECONDARY(image_index)) && - !(flash_area_get_id(fap_src) == FLASH_AREA_IMAGE_SECONDARY(image_index) && - flash_area_get_id(fap_dst) == FLASH_AREA_IMAGE_SECONDARY(image_index))) { - /* assume the secondary slot as src, needs decryption */ - hdr = boot_img_hdr(state, BOOT_SECONDARY_SLOT); -#if !defined(MCUBOOT_SWAP_USING_MOVE) - off = off_src; - if (flash_area_get_id(fap_dst) == FLASH_AREA_IMAGE_SECONDARY(image_index)) { - /* might need encryption (metadata from the primary slot) */ - hdr = boot_img_hdr(state, BOOT_PRIMARY_SLOT); - off = off_dst; - } -#else + encrypted_src = (flash_area_get_id(fap_src) != FLASH_AREA_IMAGE_PRIMARY(image_index)); + encrypted_dst = (flash_area_get_id(fap_dst) != FLASH_AREA_IMAGE_PRIMARY(image_index)); + + if (encrypted_src != encrypted_dst) { off = off_dst; - if (flash_area_get_id(fap_dst) == FLASH_AREA_IMAGE_SECONDARY(image_index)) { + + if (encrypted_dst) { + /* Need encryption, metadata from the primary slot */ hdr = boot_img_hdr(state, BOOT_PRIMARY_SLOT); + enc_area_id = FLASH_AREA_IMAGE_PRIMARY(image_index); + } else { + /* Need decryption, metadata from the secondary slot */ + hdr = boot_img_hdr(state, BOOT_SECONDARY_SLOT); + enc_area_id = FLASH_AREA_IMAGE_SECONDARY(image_index); } -#endif + if (IS_ENCRYPTED(hdr)) { uint32_t abs_off = off + bytes_copied; if (abs_off < hdr->ih_hdr_size) { @@ -1294,7 +1294,7 @@ boot_copy_region(struct boot_loader_state *state, blk_sz = tlv_off - abs_off; } } - boot_encrypt(BOOT_CURR_ENC(state), image_index, fap_src, + boot_encrypt(BOOT_CURR_ENC(state), image_index, enc_area_id, (abs_off + idx) - hdr->ih_hdr_size, blk_sz, blk_off, &buf[idx]); } @@ -2774,13 +2774,13 @@ boot_decrypt_and_copy_image_to_sram(struct boot_loader_state *state, * Part of the chunk is encrypted payload */ blk_off = ((bytes_copied) - hdr->ih_hdr_size) & 0xf; blk_sz = tlv_off - (bytes_copied); - boot_encrypt(BOOT_CURR_ENC(state), image_index, fap_src, + boot_encrypt(BOOT_CURR_ENC(state), image_index, area_id, (bytes_copied + idx) - hdr->ih_hdr_size, blk_sz, blk_off, cur_dst); } else { /* Image encrypted payload section */ blk_off = ((bytes_copied) - hdr->ih_hdr_size) & 0xf; - boot_encrypt(BOOT_CURR_ENC(state), image_index, fap_src, + boot_encrypt(BOOT_CURR_ENC(state), image_index, area_id, (bytes_copied + idx) - hdr->ih_hdr_size, blk_sz, blk_off, cur_dst); } From fb187ec691d9250eb696977e067b4804af08c829 Mon Sep 17 00:00:00 2001 From: Thomas Altenbach Date: Tue, 30 Apr 2024 17:19:55 +0200 Subject: [PATCH 3/5] docs: Update documentation regarding encrypted scratch area When using swap using scratch, the decryption now happens when copying from the scratch area to the primary slot, which means the image is stored encrypted in the scratch area. This commit updates the documentation accordingly. Signed-off-by: Thomas Altenbach --- docs/encrypted_images.md | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/docs/encrypted_images.md b/docs/encrypted_images.md index 834428f3d..85d26d8e1 100644 --- a/docs/encrypted_images.md +++ b/docs/encrypted_images.md @@ -120,12 +120,14 @@ start the validation process, decrypting the blocks before check. A good image being determined, the upgrade consists in reading the blocks from the `secondary slot`, decrypting and writing to the `primary slot`. -If swap is used for the upgrade process, the encryption happens when -copying the sectors of the `secondary slot` to the scratch area. - -The `scratch` area is not encrypted, so it must reside in the internal -flash of the MCU to avoid attacks that could interrupt the upgrade and -dump the data. +If swap using scratch is used for the upgrade process, the decryption happens +when copying the content of the scratch area to the `primary slot`, which means +the scratch area does not contain the image unencrypted. However, unless +`MCUBOOT_SWAP_SAVE_ENCTLV` is enabled, the decryption keys are stored in +plaintext in the scratch area. Therefore, `MCUBOOT_SWAP_SAVE_ENCTLV` must be +enabled if the scratch area does not reside in the internal flash memory of the +MCU, to avoid attacks that could interrupt the upgrade and read the plaintext +decryption keys from external flash memory. Also when swap is used, the image in the `primary slot` is checked for presence of the `ENCRYPTED` flag and the key TLV. If those are present the From 3a7bfde26d87011862535708724d2570eaf16fb3 Mon Sep 17 00:00:00 2001 From: Thomas Altenbach Date: Thu, 2 May 2024 14:56:21 +0200 Subject: [PATCH 4/5] sim: Fix MCUBOOT_SWAP_USING_SCRATCH defined in direct-xip and ram-load When 'direct-xip' or 'ram-load' features were enabled, CONFIG_BOOT_SWAP_USING_SCRATCH and MCUBOOT_SWAP_USING_SCRATCH were defined even though swap using scratch wasn't used. This commit fixes the issue. Signed-off-by: Thomas Altenbach --- sim/mcuboot-sys/build.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sim/mcuboot-sys/build.rs b/sim/mcuboot-sys/build.rs index 4221292f5..ea17d8014 100644 --- a/sim/mcuboot-sys/build.rs +++ b/sim/mcuboot-sys/build.rs @@ -255,7 +255,7 @@ fn main() { if swap_move { conf.conf.define("MCUBOOT_SWAP_USING_MOVE", None); - } else if !overwrite_only { + } else if !overwrite_only && !direct_xip && !ram_load { conf.conf.define("CONFIG_BOOT_SWAP_USING_SCRATCH", None); conf.conf.define("MCUBOOT_SWAP_USING_SCRATCH", None); } From 493d90a9580b42b468e3f5aa77cdd5164a938866 Mon Sep 17 00:00:00 2001 From: Thomas Altenbach Date: Mon, 15 Jul 2024 14:15:03 +0200 Subject: [PATCH 5/5] docs: release-notes: Add snippet on encrypted scratch area Add release note snippet regarding the change made to the swap with scratch algorithm to avoid having plaintext firmware data stored in the scratch area. Signed-off-by: Thomas Altenbach --- docs/release-notes.d/encrypted-scratch-partition.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 docs/release-notes.d/encrypted-scratch-partition.md diff --git a/docs/release-notes.d/encrypted-scratch-partition.md b/docs/release-notes.d/encrypted-scratch-partition.md new file mode 100644 index 000000000..ea5160594 --- /dev/null +++ b/docs/release-notes.d/encrypted-scratch-partition.md @@ -0,0 +1,3 @@ +- When using swap with scratch, the image is now decrypted when copying from + the scratch partition to the primary slot. Therefore, the sratch partition + doesn't contain plaintext firmware data anymore.