boot: bootutil: fix out of bounds sector reads in swap using scratch

boot/bootutil/src/swap_scratch.c

@@ -289,6 +289,17 @@ boot_slots_compatible(struct boot_loader_state *state)
         return 0;
     }
 
+#ifndef MCUBOOT_DECOMPRESS_IMAGES
+    /* The slots have to be the same size, unless a compressed image is
+     * stored in a smaller secondary slot.
+     */
+    if (flash_area_get_size(BOOT_IMG_AREA(state, BOOT_SLOT_PRIMARY)) !=
+        flash_area_get_size(BOOT_IMG_AREA(state, BOOT_SLOT_SECONDARY))) {
+        BOOT_LOG_WRN("Cannot upgrade: slots differ in size");
+        return 0;
+    }
+#endif
+
 #ifndef MCUBOOT_OVERWRITE_ONLY
     scratch_sz = boot_scratch_area_size(state);
 #endif
@@ -304,6 +315,22 @@ boot_slots_compatible(struct boot_loader_state *state)
     j = sz1 = secondary_slot_sz = 0;
     smaller = 0;
     while (i < num_sectors_primary || j < num_sectors_secondary) {
+        /* The next branch reads a primary sector when sz0 <= sz1 and a
+         * secondary sector when sz0 >= sz1; stop if that slot has run out.
+         */
+        if ((sz0 <= sz1 && i >= num_sectors_primary) ||
+            (sz0 >= sz1 && j >= num_sectors_secondary)) {
+#ifdef MCUBOOT_DECOMPRESS_IMAGES
+            /* Decompressed images are installed in overwrite mode, so
+             * slot1 may be smaller than slot0.
+             */
+            break;
+#else
+            BOOT_LOG_WRN("Cannot upgrade: slots are not compatible");
+            return 0;
+#endif
+        }
+
         if (sz0 == sz1) {
             sz0 += boot_img_sector_size(state, BOOT_SLOT_PRIMARY, i);
             sz1 += boot_img_sector_size(state, BOOT_SLOT_SECONDARY, j);
@@ -321,17 +348,7 @@ boot_slots_compatible(struct boot_loader_state *state)
             smaller = 1;
             i++;
         } else {
-            size_t sector_size = boot_img_sector_size(state, BOOT_SLOT_SECONDARY, j);
-
-#ifdef MCUBOOT_DECOMPRESS_IMAGES
-            if (sector_size == 0) {
-                /* Since this supports decompressed images, we can safely exit if slot1 is
-                 * smaller than slot0.
-                 */
-                break;
-            }
-#endif
-            sz1 += sector_size;
+            sz1 += boot_img_sector_size(state, BOOT_SLOT_SECONDARY, j);
             /* Guarantee that multiple sectors of the primary slot
              * fit into the secondary slot.
              */
@@ -975,6 +992,12 @@ int app_max_size(struct boot_loader_state *state)
     j = sz1 = 0;
     smaller = 0;
     while (i < num_sectors_primary || j < num_sectors_secondary) {
+        /* Stop at the end of the shorter slot, only the matched part is usable. */
+        if ((sz0 <= sz1 && i >= num_sectors_primary) ||
+            (sz0 >= sz1 && j >= num_sectors_secondary)) {
+            break;
+        }
+
         if (sz0 == sz1) {
             sz0 += boot_img_sector_size(state, BOOT_SLOT_PRIMARY, i);
             sz1 += boot_img_sector_size(state, BOOT_SLOT_SECONDARY, j);

docs/release-notes.d/swap-scratch-slots-compatible-bounds.md

@@ -0,0 +1,3 @@
+- Fixed ``boot_slots_compatible()`` and ``app_max_size()`` in swap using scratch
+  reading past the end of the sector table when the primary and secondary slots
+  have a different number of sectors. Such slots are now reported as incompatible.

sim/src/image.rs

@@ -138,6 +138,17 @@ impl ImagesBuilder {
     /// Some(builder) if is possible to test this configuration, or None if
     /// not possible (for example, if there aren't enough image slots).
     pub fn new(device: DeviceName, align: usize, erased_val: u8) -> Result<Self, String> {
+        Self::build(device, align, erased_val, true)
+    }
+
+    /// Like `new`, but ignores the capabilities the device lists as
+    /// unsupported.  For negative tests that boot a device the current
+    /// configuration is expected to reject.
+    pub fn new_unchecked(device: DeviceName, align: usize, erased_val: u8) -> Result<Self, String> {
+        Self::build(device, align, erased_val, false)
+    }
+
+    fn build(device: DeviceName, align: usize, erased_val: u8, check_caps: bool) -> Result<Self, String> {
         let (flash, areadesc, unsupported_caps) = Self::make_device(device, align, erased_val);
 
         // Swap-move and swap-offset require uniformly sized erase units, which
@@ -154,7 +165,7 @@ impl ImagesBuilder {
             && !areadesc.uses_native_sector_size(logical, scratch);
 
         for cap in unsupported_caps {
-            if !cap.present() {
+            if !check_caps || !cap.present() {
                 continue;
             }
             let relaxed = logical_makes_uniform
@@ -306,6 +317,34 @@ impl ImagesBuilder {
         }
     }
 
+    /// Iterate the devices whose primary and secondary slots differ in
+    /// size.  Swap using scratch cannot handle them, so `each_device`
+    /// skips them; the negative tests boot them anyway and expect the
+    /// upgrade to be refused.
+    pub fn each_unequal_slot_device<F>(f: F)
+        where F: Fn(Self)
+    {
+        for &dev in ALL_DEVICES {
+            for &align in test_alignments() {
+                for &erased_val in &[0, 0xff] {
+                    if Self::device_usable(dev, align, erased_val).is_err() {
+                        continue;
+                    }
+                    let (_, areadesc, _) = Self::make_device(dev, align, erased_val);
+                    let primary = areadesc.find(FlashId::Image0).map(|(_, len, _)| len);
+                    let secondary = areadesc.find(FlashId::Image1).map(|(_, len, _)| len);
+                    if primary.is_none() || primary == secondary {
+                        continue;
+                    }
+                    match Self::new_unchecked(dev, align, erased_val) {
+                        Ok(run) => f(run),
+                        Err(msg) => warn!("Skipping {}: {}", dev, msg),
+                    }
+                }
+            }
+        }
+    }
+
     /// Construct an `Images` that doesn't expect an upgrade to happen.
     pub fn make_no_upgrade_image(self, deps: &DepTest, img_manipulation: ImageManipulation) -> Images {
         self.make_no_upgrade_image_with_key(deps, img_manipulation, SigningKey::Primary)
@@ -488,6 +527,32 @@ impl ImagesBuilder {
         }
     }
 
+    /// Construct an `Images` with fixed-size images.  The maximal image
+    /// size cannot be estimated for slots that differ in size, so the
+    /// negative tests for such slots use this instead of `make_image`.
+    pub fn make_fixed_size_images(self) -> Images {
+        let mut flash = self.flash;
+        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,
+                ImageSize::Given(42784), &ram, &dep, ImageManipulation::None, Some(0));
+            let upgrades = install_image(&mut flash, &self.areadesc, &slots, 1,
+                ImageSize::Given(46928), &ram, &dep, ImageManipulation::None, Some(1));
+            OneImage {
+                slots,
+                primaries,
+                upgrades,
+            }}).collect();
+        Images {
+            flash,
+            areadesc: self.areadesc,
+            images,
+            total_count: None,
+            ram: self.ram,
+        }
+    }
+
     pub fn make_erased_secondary_image(self) -> Images {
         let mut flash = self.flash;
         let ram = self.ram.clone(); // TODO: Avoid this clone.
@@ -710,6 +775,10 @@ impl ImagesBuilder {
                 areadesc.add_flash_sectors(dev_id, &dev);
                 areadesc.add_image(0x008000, 0x03c000, FlashId::Image0, dev_id);
                 areadesc.add_image(0x044000, 0x03b000, FlashId::Image1, dev_id);
+                // Scratch is only here so a swap-using-scratch build can
+                // boot the device; the unequal slots must make it refuse
+                // the upgrade.
+                areadesc.add_image(0x07f000, 0x001000, FlashId::ImageScratch, dev_id);
 
                 let mut flash = SimMultiFlash::new();
                 flash.insert(dev_id, dev);
@@ -723,6 +792,8 @@ impl ImagesBuilder {
                 areadesc.add_flash_sectors(dev_id, &dev);
                 areadesc.add_image(0x008000, 0x03b000, FlashId::Image0, dev_id);
                 areadesc.add_image(0x043000, 0x03c000, FlashId::Image1, dev_id);
+                // See Nrf52840UnequalSlots.
+                areadesc.add_image(0x07f000, 0x001000, FlashId::ImageScratch, dev_id);
 
                 let mut flash = SimMultiFlash::new();
                 flash.insert(dev_id, dev);
@@ -1204,6 +1275,50 @@ impl Images {
         fails > 0
     }
 
+    /// Boot with an upgrade staged on a device whose primary and secondary
+    /// slots differ in size.  Swap using scratch cannot swap such slots, so
+    /// the bootloader must refuse the upgrade: the boot still succeeds into
+    /// the primary slot, and no flash is touched.
+    pub fn run_unequal_slots_rejected(&self) -> bool {
+        if !Caps::SwapUsingScratch.present() || !Caps::modifies_flash() {
+            return false;
+        }
+
+        let mut flash = self.flash.clone();
+        let mut fails = 0;
+
+        info!("Try upgrade with slots of different sizes");
+
+        self.mark_upgrades(&mut flash, 1);
+
+        let snapshot: Vec<(u8, Vec<u8>)> = flash.iter().map(|(&dev_id, dev)| {
+            let mut data = vec![0u8; dev.device_size()];
+            dev.read(0, &mut data).unwrap();
+            (dev_id, data)
+        }).collect();
+
+        if !c::boot_go(&mut flash, &self.areadesc, None, None, false).success() {
+            warn!("Boot failed with slots of different sizes");
+            fails += 1;
+        }
+
+        for (dev_id, before) in &snapshot {
+            let dev = flash.get(dev_id).unwrap();
+            let mut after = vec![0u8; dev.device_size()];
+            dev.read(0, &mut after).unwrap();
+            if before != &after {
+                warn!("Flash device {} was modified by the refused upgrade", dev_id);
+                fails += 1;
+            }
+        }
+
+        if fails > 0 {
+            error!("Expected a refused upgrade with the flash untouched");
+        }
+
+        fails > 0
+    }
+
     // Test that an upgrade is rejected.  Assumes that the image was build
     // such that the upgrade is instead a downgrade.
     pub fn run_nodowngrade(&self) -> bool {

sim/tests/core.rs

@@ -103,6 +103,20 @@ fn logical_sectors_reject_incompatible_devices() {
     });
 }
 
+// Swap using scratch needs the primary and secondary slots to be the same
+// size, so `each_device` skips the devices where they differ.  Boot them
+// anyway: the bootloader must refuse the upgrade and leave the flash alone,
+// rather than read past the end of the shorter slot's sector table.
+#[test]
+fn unequal_slots_rejected() {
+    testlog::setup();
+    ImagesBuilder::each_unequal_slot_device(|r| {
+        let image = r.make_fixed_size_images();
+        dump_image(&image, "unequal_slots_rejected");
+        assert!(!image.run_unequal_slots_rejected());
+    });
+}
+
 #[cfg(feature = "sig-ed25519")]
 mod multi_key {
     //! Multi-signing-key matrix.