boot: bootutil: Fix swap-scratch when the trailer is larger than the last sector

boot/bootutil/src/bootutil_misc.c

@@ -331,12 +331,87 @@ boot_write_enc_key(const struct flash_area *fap, uint8_t slot,
 }
 #endif
 
-uint32_t bootutil_max_image_size(const struct flash_area *fap)
+#ifdef MCUBOOT_SWAP_USING_SCRATCH
+size_t
+boot_get_first_trailer_sector(struct boot_loader_state *state, size_t slot, size_t trailer_sz)
 {
-#if defined(MCUBOOT_SWAP_USING_SCRATCH) || defined(MCUBOOT_SINGLE_APPLICATION_SLOT) || \
-    defined(MCUBOOT_FIRMWARE_LOADER) || defined(MCUBOOT_SINGLE_APPLICATION_SLOT_RAM_LOAD)
+    size_t first_trailer_sector = boot_img_num_sectors(state, slot) - 1;
+    size_t sector_sz = boot_img_sector_size(state, slot, first_trailer_sector);
+    size_t trailer_sector_sz = sector_sz;
+
+    while (trailer_sector_sz < trailer_sz) {
+        /* Consider that the image trailer may span across sectors of different sizes */
+        --first_trailer_sector;
+        sector_sz = boot_img_sector_size(state, slot, first_trailer_sector);
+
+        trailer_sector_sz += sector_sz;
+    }
+
+    return first_trailer_sector;
+}
+
+/**
+ * Returns the offset to the end of the first sector of a given slot that holds image trailer data.
+ *
+ * @param state      Current bootloader's state.
+ * @param slot       The index of the slot to consider.
+ * @param trailer_sz The size of the trailer, in bytes.
+ *
+ * @return The offset to the end of the first sector of the slot that holds image trailer data.
+ */
+static uint32_t
+get_first_trailer_sector_end_off(struct boot_loader_state *state, size_t slot, size_t trailer_sz)
+{
+    size_t first_trailer_sector = boot_get_first_trailer_sector(state, slot, trailer_sz);
+
+    return boot_img_sector_off(state, slot, first_trailer_sector) +
+           boot_img_sector_size(state, slot, first_trailer_sector);
+}
+#endif /* MCUBOOT_SWAP_USING_SCRATCH */
+
+uint32_t bootutil_max_image_size(struct boot_loader_state *state, const struct flash_area *fap)
+{
+#if defined(MCUBOOT_SINGLE_APPLICATION_SLOT) ||      \
+    defined(MCUBOOT_FIRMWARE_LOADER) ||              \
+    defined(MCUBOOT_SINGLE_APPLICATION_SLOT_RAM_LOAD)
+    (void) state;
     return boot_status_off(fap);
+#elif defined(MCUBOOT_SWAP_USING_SCRATCH)
+    size_t slot_trailer_sz = boot_trailer_sz(BOOT_WRITE_SZ(state));
+    size_t slot_trailer_off = flash_area_get_size(fap) - slot_trailer_sz;
+
+    /* If the trailer doesn't fit in the last sector of the primary or secondary slot, some padding
+     * might have to be inserted between the end of the firmware image and the beginning of the
+     * trailer to ensure there is enough space for the trailer in the scratch area when the last
+     * sector of the secondary will be copied to the scratch area.
+     *
+     * The value of the padding depends on the amount of trailer data that is contained in the first
+     * trailer containing part of the trailer in the primary and secondary slot.
+     */
+    size_t trailer_sector_primary_end_off =
+        get_first_trailer_sector_end_off(state, BOOT_PRIMARY_SLOT, slot_trailer_sz);
+    size_t trailer_sector_secondary_end_off =
+        get_first_trailer_sector_end_off(state, BOOT_SECONDARY_SLOT, slot_trailer_sz);
+
+    size_t trailer_sz_in_first_sector;
+
+    if (trailer_sector_primary_end_off > trailer_sector_secondary_end_off) {
+        trailer_sz_in_first_sector = trailer_sector_primary_end_off - slot_trailer_off;
+    } else {
+        trailer_sz_in_first_sector = trailer_sector_secondary_end_off - slot_trailer_off;
+    }
+
+    size_t trailer_padding = 0;
+    size_t scratch_trailer_sz = boot_scratch_trailer_sz(BOOT_WRITE_SZ(state));
+
+    if (scratch_trailer_sz > trailer_sz_in_first_sector) {
+        trailer_padding = scratch_trailer_sz - trailer_sz_in_first_sector;
+    }
+
+    return slot_trailer_off - trailer_padding;
 #elif defined(MCUBOOT_SWAP_USING_MOVE) || defined(MCUBOOT_SWAP_USING_OFFSET)
+    (void) state;
+
     struct flash_sector sector;
     /* get the last sector offset */
     int rc = flash_area_get_sector(fap, boot_status_off(fap), &sector);
@@ -348,10 +423,13 @@ uint32_t bootutil_max_image_size(const struct flash_area *fap)
     }
     return flash_sector_get_off(&sector);
 #elif defined(MCUBOOT_OVERWRITE_ONLY)
+    (void) state;
     return boot_swap_info_off(fap);
 #elif defined(MCUBOOT_DIRECT_XIP)
+    (void) state;
     return boot_swap_info_off(fap);
 #elif defined(MCUBOOT_RAM_LOAD)
+    (void) state;
     return boot_swap_info_off(fap);
 #endif
 }

boot/bootutil/src/bootutil_priv.h

@@ -344,6 +344,20 @@ int boot_read_enc_key(const struct flash_area *fap, uint8_t slot,
                       struct boot_status *bs);
 #endif
 
+#ifdef MCUBOOT_SWAP_USING_SCRATCH
+/**
+ * Finds the first sector of a given slot that holds image trailer data.
+ *
+ * @param state      Current bootloader's state.
+ * @param slot       The index of the slot to consider.
+ * @param trailer_sz The size of the trailer, in bytes.
+ *
+ * @return The index of the first sector of the slot that holds image trailer data.
+ */
+size_t
+boot_get_first_trailer_sector(struct boot_loader_state *state, size_t slot, size_t trailer_sz);
+#endif
+
 /**
  * Checks that a buffer is erased according to what the erase value for the
  * flash device provided in `flash_area` is.
@@ -511,7 +525,7 @@ int boot_load_image_to_sram(struct boot_loader_state *state);
 
 #endif /* MCUBOOT_RAM_LOAD */
 
-uint32_t bootutil_max_image_size(const struct flash_area *fap);
+uint32_t bootutil_max_image_size(struct boot_loader_state *state, const struct flash_area *fap);
 
 int boot_read_image_size(struct boot_loader_state *state, int slot,
                          uint32_t *size);

boot/bootutil/src/image_validate.c

@@ -555,7 +555,7 @@ bootutil_img_validate(struct boot_loader_state *state,
         goto out;
     }
 
-    if (it.tlv_end > bootutil_max_image_size(fap)) {
+    if (it.tlv_end > bootutil_max_image_size(state, fap)) {
         rc = -1;
         goto out;
     }

boot/bootutil/src/swap_scratch.c

@@ -551,66 +551,67 @@ boot_swap_sectors(int idx, uint32_t sz, struct boot_loader_state *state,
     const struct flash_area *fap_scratch;
     uint32_t copy_sz;
     uint32_t trailer_sz;
-    uint32_t sector_sz;
     uint32_t img_off;
     uint32_t scratch_trailer_off;
     struct boot_swap_state swap_state;
-    size_t last_sector;
+    size_t first_trailer_sector_primary;
     bool erase_scratch;
     uint8_t image_index;
     int rc;
 
+    image_index = BOOT_CURR_IMG(state);
+
+    rc = flash_area_open(FLASH_AREA_IMAGE_PRIMARY(image_index), &fap_primary_slot);
+    assert (rc == 0);
+
+    rc = flash_area_open(FLASH_AREA_IMAGE_SECONDARY(image_index), &fap_secondary_slot);
+    assert (rc == 0);
+
+    rc = flash_area_open(FLASH_AREA_IMAGE_SCRATCH, &fap_scratch);
+    assert (rc == 0);
+
     /* Calculate offset from start of image area. */
     img_off = boot_img_sector_off(state, BOOT_PRIMARY_SLOT, idx);
 
     copy_sz = sz;
     trailer_sz = boot_trailer_sz(BOOT_WRITE_SZ(state));
 
     /* sz in this function is always sized on a multiple of the sector size.
-     * The check against the start offset of the last sector
-     * is to determine if we're swapping the last sector. The last sector
-     * needs special handling because it's where the trailer lives. If we're
-     * copying it, we need to use scratch to write the trailer temporarily.
+     * The check against the start offset of the first trailer sector is to determine if we're
+     * swapping that sector, which might contains both part of the firmware image and part of the
+     * trailer (or the whole trailer if the latter is small enough). Therefore, that sector needs
+     * special handling: if we're copying it, we need to use scratch to write the trailer
+     * temporarily.
+     *
+     * Since the primary and secondary slots don't necessarily have the same layout, the index of
+     * the first trailer sector may be different for each slot.
      *
      * NOTE: `use_scratch` is a temporary flag (never written to flash) which
-     * controls if special handling is needed (swapping last sector).
+     * controls if special handling is needed (swapping the first trailer sector).
      */
-    last_sector = boot_img_num_sectors(state, BOOT_PRIMARY_SLOT) - 1;
-    sector_sz = boot_img_sector_size(state, BOOT_PRIMARY_SLOT, last_sector);
-
-    if (sector_sz < trailer_sz) {
-        uint32_t trailer_sector_sz = sector_sz;
+    first_trailer_sector_primary =
+        boot_get_first_trailer_sector(state, BOOT_PRIMARY_SLOT, trailer_sz);
 
-        while (trailer_sector_sz < trailer_sz) {
-            /* Consider that the image trailer may span across sectors of
-             * different sizes.
-             */
-            sector_sz = boot_img_sector_size(state, BOOT_PRIMARY_SLOT, --last_sector);
+    /* Check if the currently swapped sector(s) contain the trailer or part of it */
+    if ((img_off + sz) >
+        boot_img_sector_off(state, BOOT_PRIMARY_SLOT, first_trailer_sector_primary)) {
+        copy_sz = flash_area_get_size(fap_primary_slot) - img_off - trailer_sz;
+
+        /* Check if the computed copy size would cause the beginning of the trailer in the scratch
+         * area to be overwritten. If so, adjust the copy size to avoid this.
+         *
+         * This could happen if the trailer is larger than a single sector since in that case the
+         * first part of the trailer may be smaller than the trailer in the scratch area.
+         */
+        scratch_trailer_off = boot_status_off(fap_scratch);
 
-            trailer_sector_sz += sector_sz;
+        if (copy_sz > scratch_trailer_off) {
+            copy_sz = scratch_trailer_off;
         }
     }
 
-    if ((img_off + sz) >
-        boot_img_sector_off(state, BOOT_PRIMARY_SLOT, last_sector)) {
-        copy_sz -= trailer_sz;
-    }
-
     bs->use_scratch = (bs->idx == BOOT_STATUS_IDX_0 && copy_sz != sz);
 
-    image_index = BOOT_CURR_IMG(state);
-
-    rc = flash_area_open(FLASH_AREA_IMAGE_PRIMARY(image_index),
-            &fap_primary_slot);
-    assert (rc == 0);
-
-    rc = flash_area_open(FLASH_AREA_IMAGE_SECONDARY(image_index),
-            &fap_secondary_slot);
-    assert (rc == 0);
-
-    rc = flash_area_open(FLASH_AREA_IMAGE_SCRATCH, &fap_scratch);
-    assert (rc == 0);
-
     if (bs->state == BOOT_STATUS_STATE_0) {
         BOOT_LOG_DBG("erasing scratch area");
         rc = boot_erase_region(fap_scratch, 0, flash_area_get_size(fap_scratch));
@@ -652,29 +653,69 @@ boot_swap_sectors(int idx, uint32_t sz, struct boot_loader_state *state,
     }
 
     if (bs->state == BOOT_STATUS_STATE_1) {
-        rc = boot_erase_region(fap_secondary_slot, img_off, sz);
-        assert(rc == 0);
-
-        rc = boot_copy_region(state, fap_primary_slot, fap_secondary_slot,
-                              img_off, img_off, copy_sz);
-        assert(rc == 0);
+        uint32_t erase_sz = sz;
 
-        if (bs->idx == BOOT_STATUS_IDX_0 && !bs->use_scratch) {
-            /* If not all sectors of the slot are being swapped,
-             * guarantee here that only the primary slot will have the state.
-             */
+        if (bs->idx == BOOT_STATUS_IDX_0) {
+            /* Guarantee here that only the primary slot will have the state.
+             *
+             * This is necessary even though the current area being swapped contains part of the
+             * trailer since in case the trailer spreads over multiple sector erasing the [img_off,
+             * img_off + sz) might not erase the entire trailer.
+              */
             rc = swap_erase_trailer_sectors(state, fap_secondary_slot);
             assert(rc == 0);
+
+            if (bs->use_scratch) {
+                /* If the area being swapped contains the trailer or part of it, ensure the
+                 * sector(s) containing the beginning of the trailer won't be erased again.
+                 */
+                size_t trailer_sector_secondary =
+                    boot_get_first_trailer_sector(state, BOOT_SECONDARY_SLOT, trailer_sz);
+
+                uint32_t trailer_sector_offset =
+                    boot_img_sector_off(state, BOOT_SECONDARY_SLOT, trailer_sector_secondary);
+
+                erase_sz = trailer_sector_offset - img_off;
+            }
+        }
+
+        if (erase_sz > 0) {
+            rc = boot_erase_region(fap_secondary_slot, img_off, erase_sz);
+            assert(rc == 0);
         }
 
+        rc = boot_copy_region(state, fap_primary_slot, fap_secondary_slot,
+                              img_off, img_off, copy_sz);
+        assert(rc == 0);
+
         rc = boot_write_status(state, bs);
         bs->state = BOOT_STATUS_STATE_2;
         BOOT_STATUS_ASSERT(rc == 0);
     }
 
     if (bs->state == BOOT_STATUS_STATE_2) {
-        rc = boot_erase_region(fap_primary_slot, img_off, sz);
-        assert(rc == 0);
+        uint32_t erase_sz = sz;
+
+        if (bs->use_scratch) {
+            /* The current area that is being swapped contains the trailer or part of it. In that
+             * case, make sure to erase all sectors containing the trailer in the primary slot to be
+             * able to write the new trailer. This is not always equivalent to erasing the [img_off,
+             * img_off + sz) range when the trailer spreads across multiple sectors.
+             */
+            rc = swap_erase_trailer_sectors(state, fap_primary_slot);
+            assert(rc == 0);
+
+            /* Ensure the sector(s) containing the beginning of the trailer won't be erased twice */
+            uint32_t trailer_sector_off =
+                boot_img_sector_off(state, BOOT_PRIMARY_SLOT, first_trailer_sector_primary);
+
+            erase_sz = trailer_sector_off - img_off;
+        }
+
+        if (erase_sz > 0) {
+            rc = boot_erase_region(fap_primary_slot, img_off, erase_sz);
+            assert(rc == 0);
+        }
 
         /* NOTE: If this is the final sector, we exclude the image trailer from
          * this copy (copy_sz was truncated earlier).

sim/mcuboot-sys/src/area.rs

@@ -149,6 +149,13 @@ impl AreaDesc {
     pub fn iter_areas(&self) -> impl Iterator<Item = &FlashArea> {
         self.whole.iter()
     }
+
+    /// Return the list of sectors of a given flash area.
+    pub fn get_area_sectors(&self, flash_id: FlashId) -> Option<&Vec<FlashArea>> {
+        self.areas.iter()
+            .filter(|area| !area.is_empty())
+            .find(|area| area[0].flash_id == flash_id)
+    }
 }
 
 /// The area descriptor, C format.