From bd9e33cebf3b749ec193ffbfe2f55c694aca54f7 Mon Sep 17 00:00:00 2001 From: Thomas Altenbach Date: Sun, 27 Apr 2025 19:30:53 +0200 Subject: [PATCH 1/4] boot: bootutil: Fix max image size computation for swap-move/swap-offset When computing the maximum image size in bootutil_max_image_size for swap-move or swap-offset strategy, the computation was using the size of the flash area provided as argument and was not taking into account the size of the padding sector. This was causing an incorrect size to be returned in some cases, for example when the two slots have the same size or when the slots haven't the same size but the routine is called for the slot containing the padding sector. For example, let's imagine swap-move is being used on a device having a sector size S and two slots of N bytes. This is valid configuration and the maximum image size is N - S - T, T being the size of the trailer rounded up to the next multiple of S. When calling bootutil_max_image_size with either the primary or secondary slot, the size N - T is returned, which is incorrect. This commit fixes the issue by computing always the maximum image using the size of the slot containing the padding and substracting the size of the padding and of the aligned trailer. Signed-off-by: Thomas Altenbach --- boot/bootutil/src/bootutil_misc.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/boot/bootutil/src/bootutil_misc.c b/boot/bootutil/src/bootutil_misc.c index 2043cf493..c3517ce1f 100644 --- a/boot/bootutil/src/bootutil_misc.c +++ b/boot/bootutil/src/bootutil_misc.c @@ -476,18 +476,27 @@ uint32_t bootutil_max_image_size(struct boot_loader_state *state, const struct f return slot_trailer_off - trailer_padding; #elif defined(MCUBOOT_SWAP_USING_MOVE) || defined(MCUBOOT_SWAP_USING_OFFSET) - (void) state; + (void) fap; - struct flash_sector sector; - /* get the last sector offset */ - int rc = flash_area_get_sector(fap, boot_status_off(fap), §or); - if (rc) { - BOOT_LOG_ERR("Unable to determine flash sector of the image trailer"); - return 0; /* Returning of zero here should cause any check which uses - * this value to fail. - */ - } - return flash_sector_get_off(§or); + /* The slot whose size is used to compute the maximum image size must be the one containing the + * padding required for the swap. */ +#ifdef MCUBOOT_SWAP_USING_MOVE + size_t slot = BOOT_PRIMARY_SLOT; +#else + size_t slot = BOOT_SECONDARY_SLOT; +#endif + + const struct flash_area *fap_padded_slot = BOOT_IMG_AREA(state, slot); + assert(fap_padded_slot != NULL); + + size_t trailer_sz = boot_trailer_sz(BOOT_WRITE_SZ(state)); + size_t sector_sz = boot_img_sector_size(state, slot, 0); + size_t padding_sz = sector_sz; + + /* The trailer size needs to be sector-aligned */ + trailer_sz = ALIGN_UP(trailer_sz, sector_sz); + + return flash_area_get_size(fap_padded_slot) - trailer_sz - padding_sz; #elif defined(MCUBOOT_OVERWRITE_ONLY) (void) state; return boot_swap_info_off(fap); From 3589fb976f3df487bbff5473671c18df8647191c Mon Sep 17 00:00:00 2001 From: Thomas Altenbach Date: Sun, 27 Apr 2025 19:51:57 +0200 Subject: [PATCH 2/4] boot: bootutil: swap-offset: Fix image size check during validation When checking the size of an image in bootutil_img_validate, the offset to the end of the TLV area was used as the image size in all cases. However, when using swap-offset, the upgrade image is written in the secondary with an offset. This offset is not part of the image and must therefore not be taken into account in the image size. Signed-off-by: Thomas Altenbach --- boot/bootutil/src/image_validate.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/boot/bootutil/src/image_validate.c b/boot/bootutil/src/image_validate.c index 61cbf4de0..123d8c46a 100644 --- a/boot/bootutil/src/image_validate.c +++ b/boot/bootutil/src/image_validate.c @@ -490,6 +490,7 @@ bootutil_img_validate(struct boot_loader_state *state, uint32_t off; uint16_t len; uint16_t type; + uint32_t img_sz; #ifdef EXPECTED_SIG_TLV FIH_DECLARE(valid_signature, FIH_FAILURE); #ifndef MCUBOOT_BUILTIN_KEY @@ -555,7 +556,13 @@ bootutil_img_validate(struct boot_loader_state *state, goto out; } - if (it.tlv_end > bootutil_max_image_size(state, fap)) { +#ifdef MCUBOOT_SWAP_USING_OFFSET + img_sz = it.tlv_end - it.start_off; +#else + img_sz = it.tlv_end; +#endif + + if (img_sz > bootutil_max_image_size(state, fap)) { rc = -1; goto out; } From cfa8e42209b0aca8d7ff7ec31bfca87ab454644e Mon Sep 17 00:00:00 2001 From: Thomas Altenbach Date: Sun, 30 Mar 2025 19:43:56 +0200 Subject: [PATCH 3/4] sim: Fix largest image computation for swap-move and swap-offset For the swap-move and swap-offset strategies, the computation of the largest image size was not taking taking into account the padding that is needed when using those strategies. Due to this limitation, the simulator is currently using hardcoded image sizes, smaller than the maximum possible size, when running tests for the swap-move or swap-offset strategies. This commit fixes the maximum image size computation for those strategies. Signed-off-by: Thomas Altenbach --- sim/src/image.rs | 126 ++++++++++++++++++++++++++--------------------- 1 file changed, 71 insertions(+), 55 deletions(-) diff --git a/sim/src/image.rs b/sim/src/image.rs index dcea8c577..02ff04449 100644 --- a/sim/src/image.rs +++ b/sim/src/image.rs @@ -234,21 +234,21 @@ impl ImagesBuilder { let (primaries,upgrades) = if img_manipulation == ImageManipulation::CorruptHigherVersionImage && !higher_version_corrupted { higher_version_corrupted = true; - let prim = install_image(&mut flash, &self.areadesc, &slots[0], - maximal(42784), &ram, &*dep, ImageManipulation::None, Some(0), false); + let prim = install_image(&mut flash, &self.areadesc, &slots, 0, + maximal(42784), &ram, &*dep, ImageManipulation::None, Some(0)); let upgr = match deps.depends[image_num] { DepType::NoUpgrade => install_no_image(), - _ => install_image(&mut flash, &self.areadesc, &slots[1], - maximal(46928), &ram, &*dep, ImageManipulation::BadSignature, Some(0), true) + _ => install_image(&mut flash, &self.areadesc, &slots, 1, + maximal(46928), &ram, &*dep, ImageManipulation::BadSignature, Some(0)) }; (prim, upgr) } else { - let prim = install_image(&mut flash, &self.areadesc, &slots[0], - maximal(42784), &ram, &*dep, img_manipulation, Some(0), false); + let prim = install_image(&mut flash, &self.areadesc, &slots, 0, + maximal(42784), &ram, &*dep, img_manipulation, Some(0)); let upgr = match deps.depends[image_num] { DepType::NoUpgrade => install_no_image(), - _ => install_image(&mut flash, &self.areadesc, &slots[1], - maximal(46928), &ram, &*dep, img_manipulation, Some(0), true) + _ => install_image(&mut flash, &self.areadesc, &slots, 1, + maximal(46928), &ram, &*dep, img_manipulation, Some(0)) }; (prim, upgr) }; @@ -298,10 +298,10 @@ impl ImagesBuilder { let ram = self.ram.clone(); // TODO: Avoid this clone. let images = self.slots.into_iter().enumerate().map(|(image_num, slots)| { let dep = BoringDep::new(image_num, &NO_DEPS); - let primaries = install_image(&mut bad_flash, &self.areadesc, &slots[0], - maximal(32784), &ram, &dep, ImageManipulation::None, Some(0), false); - let upgrades = install_image(&mut bad_flash, &self.areadesc, &slots[1], - maximal(41928), &ram, &dep, ImageManipulation::BadSignature, Some(0), true); + let primaries = install_image(&mut bad_flash, &self.areadesc, &slots, 0, + maximal(32784), &ram, &dep, ImageManipulation::None, Some(0)); + let upgrades = install_image(&mut bad_flash, &self.areadesc, &slots, 1, + maximal(41928), &ram, &dep, ImageManipulation::BadSignature, Some(0)); OneImage { slots, primaries, @@ -321,10 +321,10 @@ impl ImagesBuilder { let ram = self.ram.clone(); // TODO: Avoid this clone. let images = self.slots.into_iter().enumerate().map(|(image_num, slots)| { let dep = BoringDep::new(image_num, &NO_DEPS); - let primaries = install_image(&mut bad_flash, &self.areadesc, &slots[0], - maximal(32784), &ram, &dep, ImageManipulation::None, Some(0), false); - let upgrades = install_image(&mut bad_flash, &self.areadesc, &slots[1], - ImageSize::Oversized, &ram, &dep, ImageManipulation::None, Some(0), true); + let primaries = install_image(&mut bad_flash, &self.areadesc, &slots, 0, + maximal(32784), &ram, &dep, ImageManipulation::None, Some(0)); + let upgrades = install_image(&mut bad_flash, &self.areadesc, &slots, 1, + ImageSize::Oversized, &ram, &dep, ImageManipulation::None, Some(0)); OneImage { slots, primaries, @@ -344,8 +344,8 @@ impl ImagesBuilder { let ram = self.ram.clone(); // TODO: Avoid this clone. let images = self.slots.into_iter().enumerate().map(|(image_num, slots)| { let dep = BoringDep::new(image_num, &NO_DEPS); - let primaries = install_image(&mut flash, &self.areadesc, &slots[0], - maximal(32784), &ram, &dep,ImageManipulation::None, Some(0), false); + let primaries = install_image(&mut flash, &self.areadesc, &slots, 0, + maximal(32784), &ram, &dep,ImageManipulation::None, Some(0)); let upgrades = install_no_image(); OneImage { slots, @@ -367,8 +367,8 @@ impl ImagesBuilder { let images = self.slots.into_iter().enumerate().map(|(image_num, slots)| { let dep = BoringDep::new(image_num, &NO_DEPS); let primaries = install_no_image(); - let upgrades = install_image(&mut flash, &self.areadesc, &slots[1], - maximal(32784), &ram, &dep, ImageManipulation::None, Some(0), true); + let upgrades = install_image(&mut flash, &self.areadesc, &slots, 1, + maximal(32784), &ram, &dep, ImageManipulation::None, Some(0)); OneImage { slots, primaries, @@ -389,8 +389,8 @@ impl ImagesBuilder { let images = self.slots.into_iter().enumerate().map(|(image_num, slots)| { let dep = BoringDep::new(image_num, &NO_DEPS); let primaries = install_no_image(); - let upgrades = install_image(&mut flash, &self.areadesc, &slots[1], - ImageSize::Oversized, &ram, &dep, ImageManipulation::None, Some(0), true); + let upgrades = install_image(&mut flash, &self.areadesc, &slots, 1, + ImageSize::Oversized, &ram, &dep, ImageManipulation::None, Some(0)); OneImage { slots, primaries, @@ -411,10 +411,10 @@ impl ImagesBuilder { let ram = self.ram.clone(); // TODO: Avoid this clone. let images = self.slots.into_iter().enumerate().map(|(image_num, slots)| { let dep = BoringDep::new(image_num, &NO_DEPS); - let primaries = install_image(&mut flash, &self.areadesc, &slots[0], - maximal(32784), &ram, &dep, ImageManipulation::None, security_cnt, false); - let upgrades = install_image(&mut flash, &self.areadesc, &slots[1], - maximal(41928), &ram, &dep, ImageManipulation::None, security_cnt.map(|v| v + 1), true); + let primaries = install_image(&mut flash, &self.areadesc, &slots, 0, + maximal(32784), &ram, &dep, ImageManipulation::None, security_cnt); + let upgrades = install_image(&mut flash, &self.areadesc, &slots, 1, + maximal(41928), &ram, &dep, ImageManipulation::None, security_cnt.map(|v| v + 1)); OneImage { slots, primaries, @@ -1819,19 +1819,52 @@ fn image_largest_trailer(dev: &dyn Flash, areadesc: &AreaDesc, slot: &SlotInfo) trailer } +// Computes the padding required in the primary or secondary slot to be able to perform an upgrade. +// This is needed only for the swap-move and swap-offset upgrade strategies. +fn required_slot_padding(dev: &dyn Flash) -> usize { + let mut required_padding = 0; + + if Caps::SwapUsingMove.present() || Caps::SwapUsingOffset.present() { + // Assumes equally-sized sectors + let sector_size = dev.sector_iter().next().unwrap().size; + + required_padding = sector_size; + }; + + required_padding +} + +// Computes the largest possible firmware image size, not including the header and TLV area. +fn compute_largest_image_size(dev: &dyn Flash, areadesc: &AreaDesc, slots: &[SlotInfo], + slot_ind: usize, hdr_size: usize, tlv: &dyn ManifestGen) -> usize { + let slot_len = if Caps::SwapUsingOffset.present() { + slots[1].len + } else { + slots[0].len + }; + + let trailer = image_largest_trailer(dev, areadesc, &slots[slot_ind]); + let padding = required_slot_padding(dev); + let tlv_len = tlv.estimate_size(); + info!("slot: 0x{:x}, HDR: 0x{:x}, trailer: 0x{:x}, tlv_len: 0x{:x}, padding: 0x{:x}", + slot_len, hdr_size, trailer, tlv_len, padding); + + slot_len - hdr_size - trailer - tlv_len - padding +} + /// Install a "program" into the given image. This fakes the image header, or at least all of the /// fields used by the given code. Returns a copy of the image that was written. -fn install_image(flash: &mut SimMultiFlash, areadesc: &AreaDesc, slot: &SlotInfo, len: ImageSize, - ram: &RamData, - deps: &dyn Depender, img_manipulation: ImageManipulation, security_counter:Option, secondary_slot:bool) -> ImageData { +fn install_image(flash: &mut SimMultiFlash, areadesc: &AreaDesc, slots: &[SlotInfo], + slot_ind: usize, len: ImageSize, ram: &RamData, + deps: &dyn Depender, img_manipulation: ImageManipulation, security_counter:Option) -> ImageData { + let slot = &slots[slot_ind]; let mut offset = slot.base_off; - let slot_len = slot.len; let dev_id = slot.dev_id; let dev = flash.get_mut(&dev_id).unwrap(); let mut tlv: Box = Box::new(make_tlv()); - if Caps::SwapUsingOffset.present() && secondary_slot { + if Caps::SwapUsingOffset.present() && slot_ind == 1 { let sector_size = dev.sector_iter().next().unwrap().size as usize; offset += sector_size; } @@ -1863,30 +1896,13 @@ fn install_image(flash: &mut SimMultiFlash, areadesc: &AreaDesc, slot: &SlotInfo let len = match len { ImageSize::Given(size) => size, - ImageSize::Largest => { - let trailer = image_largest_trailer(dev, &areadesc, &slot); - let tlv_len = tlv.estimate_size(); - info!("slot: 0x{:x}, HDR: 0x{:x}, trailer: 0x{:x}", - slot_len, HDR_SIZE, trailer); - slot_len - HDR_SIZE - trailer - tlv_len - }, + ImageSize::Largest => compute_largest_image_size(dev, areadesc, slots, slot_ind, + HDR_SIZE, tlv.as_ref()), ImageSize::Oversized => { - let trailer = image_largest_trailer(dev, &areadesc, &slot); - let tlv_len = tlv.estimate_size(); - let mut sector_offset = 0; - - if Caps::SwapUsingOffset.present() && secondary_slot { - // This accounts for when both slots have the same size, it will not work where - // the second slot is one sector larger than the primary - sector_offset = dev.sector_iter().next().unwrap().size as usize; - } - - info!("slot: 0x{:x}, HDR: 0x{:x}, trailer: 0x{:x}", - slot_len, HDR_SIZE, trailer); - - slot_len - HDR_SIZE - trailer - tlv_len - sector_offset + dev.align() + let largest_img_sz = compute_largest_image_size(dev, areadesc, slots, slot_ind, + HDR_SIZE, tlv.as_ref()); + largest_img_sz + dev.align() } - }; // Generate a boot header. Note that the size doesn't include the header. @@ -1995,7 +2011,7 @@ fn install_image(flash: &mut SimMultiFlash, areadesc: &AreaDesc, slot: &SlotInfo enc_copy = Some(enc); - dev.erase(offset, slot_len).unwrap(); + dev.erase(offset, slot.len).unwrap(); } else { enc_copy = None; } @@ -2020,7 +2036,7 @@ fn install_image(flash: &mut SimMultiFlash, areadesc: &AreaDesc, slot: &SlotInfo let enc_copy: Option>; if is_encrypted { - dev.erase(offset, slot_len).unwrap(); + dev.erase(offset, slot.len).unwrap(); dev.write(offset, &encbuf).unwrap(); From 52ba928d2683e6c22604ebd376c426a8f0a20309 Mon Sep 17 00:00:00 2001 From: Thomas Altenbach Date: Sun, 27 Apr 2025 20:01:13 +0200 Subject: [PATCH 4/4] sim: Use largest image for all upgrade strategies The simulator was testing the upgrade with the largest image possible for all strategies, except for overwrite-only, swap-move and swap-offset because some tests were failing when the maximum image size was used. For overwrite-only, this was due to an incorrect trailer size computation. This has been fixed by 88294be188c25e4b7d02cc9c55fabfc9286c83b7. For swap-move and swap-offset, this was due to the simulator not taking into account the padding needed by those strategies in the primary or secondary slot, but also to incorrect computation of the maximum image size in some cases by the MCUboot library. Both issues have been fixed by the previous commits. Since all those issues have been fixed, the simulator can now be configured to test upgrade with the largest possible image for all strategies. Note that logic needed to generate image of a given image is kept even if not useful anymore at the moment, since that might be needed when test will be added to ensure proper behavior when images of different sizes are used. Signed-off-by: Thomas Altenbach --- sim/src/image.rs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/sim/src/image.rs b/sim/src/image.rs index 02ff04449..a8f360471 100644 --- a/sim/src/image.rs +++ b/sim/src/image.rs @@ -1747,6 +1747,7 @@ fn show_flash(flash: &dyn Flash) { #[derive(Debug)] enum ImageSize { /// Make the image the specified given size. + #[allow(dead_code)] Given(usize), /// Make the image as large as it can be for the partition/device. Largest, @@ -2410,15 +2411,8 @@ trait AsRaw : Sized { /// Determine whether it makes sense to test this configuration with a maximally-sized image. /// Returns an ImageSize representing the best size to test, possibly just with the given size. -fn maximal(size: usize) -> ImageSize { - if Caps::OverwriteUpgrade.present() || - Caps::SwapUsingOffset.present() || - Caps::SwapUsingMove.present() - { - ImageSize::Given(size) - } else { - ImageSize::Largest - } +fn maximal(_size: usize) -> ImageSize { + ImageSize::Largest } pub fn show_sizes() {