Fix double swap on interrupted revert

This fixes #480.

When mcuboot rewrites image trailers during a swap, some information is
lost.  If a reset occurs before the swap completes, mcuboot may not be
able to determine what which swap type to resume upon startup.
Specifically, if a "revert" swap gets interupted, mcuboot will perform
an extraneous swap on the subsequent boot.  See
mcu-tools/mcuboot#480 for details.

This commit adds an additional field to the image trailer: `swap-type`.
The new trailer structure is illustrated below:

```
     0                   1                   2                   3
     0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    ~                                                               ~
    ~    Swap status (BOOT_MAX_IMG_SECTORS * min-write-size * 3)    ~
    ~                                                               ~
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    ~                 Encryption key 0 (16 octets) [*]              ~
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    ~                 Encryption key 1 (16 octets) [*]              ~
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |           Swap size           |    0xff padding (4 octets)    |
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |   Swap type   |           0xff padding (7 octets)             ~
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |   Copy done   |           0xff padding (7 octets)             ~
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |   Image OK    |           0xff padding (7 octets)             ~
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    ~                       MAGIC (16 octets)                       ~
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
```

The `swap-type` field contains one of the `BOOT_SWAP_TYPE_[...]` constants.
Every time a trailer is written, this field is written along with it.
When resuming an interrupted swap, mcuboot uses this field alone to
determine the type of swap being resumed. For new swap operations
(non-resume case), this field is not read at all; instead, mcuboot
consults the `boot_swap_tables` array to determine the swap operation to
perform (as it did prior to this commit).

Some additional changes were necessary to make all the simulated unit
tests pass:

* Before initiating a new swap operation, always write the image trailer
to the scratch area.  This step allows mcuboot to persist the
`swap-type` field somewhere before erasing the trailer in the primary
slot.  If a reset occurs immediately after the erase, mcuboot recovers
by using the trailer in the scratch area.

* Related to the above: if the scratch area is being used to hold status
bytes (because there are no spare sectors in the primary slot), erase
the scratch area immediately after the trailer gets written to the
primary slot.  This eliminates ambiguity regarding the location of the
current trailer in case a reset occurs shortly afterwards.

Signed-off-by: Christopher Collins <[email protected]>

Author: ccollins476ad

boot/bootutil/include/bootutil/bootutil.h

@@ -79,10 +79,12 @@ struct boot_rsp {
  * when attempting to read/write a trailer.
  */
 struct image_trailer {
-    uint8_t copy_done;
+    uint8_t swap_type;
     uint8_t pad1[MAX_FLASH_ALIGN - 1];
-    uint8_t image_ok;
+    uint8_t copy_done;
     uint8_t pad2[MAX_FLASH_ALIGN - 1];
+    uint8_t image_ok;
+    uint8_t pad3[MAX_FLASH_ALIGN - 1];
     uint8_t magic[16];
 };
 

boot/bootutil/src/bootutil_misc.c

@@ -119,6 +119,32 @@ boot_flag_decode(uint8_t flag)
     return BOOT_FLAG_SET;
 }
 
+/**
+ * Determines if a status source table is satisfied by the specified magic
+ * code.
+ *
+ * @param tbl_val               A magic field from a status source table.
+ * @param val                   The magic value in a trailer, encoded as a
+ *                                  BOOT_MAGIC_[...].
+ *
+ * @return                      1 if the two values are compatible;
+ *                              0 otherwise.
+ */
+int
+boot_magic_compatible_check(uint8_t tbl_val, uint8_t val)
+{
+    switch (tbl_val) {
+    case BOOT_MAGIC_ANY:
+        return 1;
+
+    case BOOT_MAGIC_NOTGOOD:
+        return val != BOOT_MAGIC_GOOD;
+
+    default:
+        return tbl_val == val;
+    }
+}
+
 uint32_t
 boot_trailer_sz(uint8_t min_write_sz)
 {
@@ -136,7 +162,6 @@ boot_trailer_sz(uint8_t min_write_sz)
 static uint32_t
 boot_magic_off(const struct flash_area *fap)
 {
-    assert(offsetof(struct image_trailer, magic) == 16);
     return fap->fa_size - BOOT_MAGIC_SZ;
 }
 
@@ -168,17 +193,21 @@ boot_status_off(const struct flash_area *fap)
     return fap->fa_size - off_from_end;
 }
 
+uint32_t
+boot_swap_type_off(const struct flash_area *fap)
+{
+    return fap->fa_size - BOOT_MAGIC_SZ - BOOT_MAX_ALIGN * 3;
+}
+
 static uint32_t
 boot_copy_done_off(const struct flash_area *fap)
 {
-    assert(offsetof(struct image_trailer, copy_done) == 0);
     return fap->fa_size - BOOT_MAGIC_SZ - BOOT_MAX_ALIGN * 2;
 }
 
 static uint32_t
 boot_image_ok_off(const struct flash_area *fap)
 {
-    assert(offsetof(struct image_trailer, image_ok) == 8);
     return fap->fa_size - BOOT_MAGIC_SZ - BOOT_MAX_ALIGN;
 }
 
@@ -216,6 +245,16 @@ boot_read_swap_state(const struct flash_area *fap,
         state->magic = boot_magic_decode(magic);
     }
 
+    off = boot_swap_type_off(fap);
+    rc = flash_area_read_is_empty(fap, off, &state->swap_type,
+            sizeof state->swap_type);
+    if (rc < 0) {
+        return BOOT_EFLASH;
+    }
+    if (rc == 1 || state->swap_type > BOOT_SWAP_TYPE_REVERT) {
+        state->swap_type = BOOT_SWAP_TYPE_NONE;
+    }
+
     off = boot_copy_done_off(fap);
     rc = flash_area_read_is_empty(fap, off, &state->copy_done,
             sizeof state->copy_done);
@@ -405,34 +444,19 @@ boot_write_magic(const struct flash_area *fap)
 }
 
 static int
-boot_write_flag(int flag, const struct flash_area *fap)
+boot_write_trailer_byte(const struct flash_area *fap, uint32_t off,
+                        uint8_t val)
 {
-    uint32_t off;
-    int rc;
     uint8_t buf[BOOT_MAX_ALIGN];
     uint8_t align;
     uint8_t erased_val;
-
-    switch (flag) {
-    case BOOT_FLAG_COPY_DONE:
-        off = boot_copy_done_off(fap);
-        BOOT_LOG_DBG("writing copy_done; fa_id=%d off=0x%x (0x%x)",
-                     fap->fa_id, off, fap->fa_off + off);
-        break;
-    case BOOT_FLAG_IMAGE_OK:
-        off = boot_image_ok_off(fap);
-        BOOT_LOG_DBG("writing image_ok; fa_id=%d off=0x%x (0x%x)",
-                     fap->fa_id, off, fap->fa_off + off);
-        break;
-    default:
-        return BOOT_EBADARGS;
-    }
+    int rc;
 
     align = flash_area_align(fap);
     assert(align <= BOOT_MAX_ALIGN);
     erased_val = flash_area_erased_val(fap);
     memset(buf, erased_val, BOOT_MAX_ALIGN);
-    buf[0] = BOOT_FLAG_SET;
+    buf[0] = val;
 
     rc = flash_area_write(fap, off, buf, align);
     if (rc != 0) {
@@ -445,13 +469,39 @@ boot_write_flag(int flag, const struct flash_area *fap)
 int
 boot_write_copy_done(const struct flash_area *fap)
 {
-    return boot_write_flag(BOOT_FLAG_COPY_DONE, fap);
+    uint32_t off;
+
+    off = boot_copy_done_off(fap);
+    BOOT_LOG_DBG("writing copy_done; fa_id=%d off=0x%x (0x%x)",
+                 fap->fa_id, off, fap->fa_off + off);
+    return boot_write_trailer_byte(fap, off, BOOT_FLAG_SET);
 }
 
 int
 boot_write_image_ok(const struct flash_area *fap)
 {
-    return boot_write_flag(BOOT_FLAG_IMAGE_OK, fap);
+    uint32_t off;
+
+    off = boot_image_ok_off(fap);
+    BOOT_LOG_DBG("writing image_ok; fa_id=%d off=0x%x (0x%x)",
+                 fap->fa_id, off, fap->fa_off + off);
+    return boot_write_trailer_byte(fap, off, BOOT_FLAG_SET);
+}
+
+/**
+ * Writes the specified value to the `swap-type` field of an image trailer.
+ * This value is persisted so that the boot loader knows what swap operation to
+ * resume in case of an unexpected reset.
+ */
+int
+boot_write_swap_type(const struct flash_area *fap, uint8_t swap_type)
+{
+    uint32_t off;
+
+    off = boot_swap_type_off(fap);
+    BOOT_LOG_DBG("writing swap_type; fa_id=%d off=0x%x (0x%x), swap_type=0x%x",
+                 fap->fa_id, off, fap->fa_off + off, swap_type);
+    return boot_write_trailer_byte(fap, off, swap_type);
 }
 
 int
@@ -524,10 +574,10 @@ boot_swap_type(void)
     for (i = 0; i < BOOT_SWAP_TABLES_COUNT; i++) {
         table = boot_swap_tables + i;
 
-        if ((table->magic_primary_slot == BOOT_MAGIC_ANY     ||
-                table->magic_primary_slot == primary_slot.magic) &&
-            (table->magic_secondary_slot == BOOT_MAGIC_ANY   ||
-                table->magic_secondary_slot == secondary_slot.magic) &&
+        if (boot_magic_compatible_check(table->magic_primary_slot,
+                                        primary_slot.magic) &&
+            boot_magic_compatible_check(table->magic_secondary_slot,
+                                        secondary_slot.magic) &&
             (table->image_ok_primary_slot == BOOT_FLAG_ANY   ||
                 table->image_ok_primary_slot == primary_slot.image_ok) &&
             (table->image_ok_secondary_slot == BOOT_FLAG_ANY ||
@@ -566,6 +616,7 @@ boot_set_pending(int permanent)
 {
     const struct flash_area *fap;
     struct boot_swap_state state_secondary_slot;
+    uint8_t swap_type;
     int rc;
 
     rc = boot_read_swap_state_by_id(FLASH_AREA_IMAGE_SECONDARY,
@@ -591,6 +642,15 @@ boot_set_pending(int permanent)
             rc = boot_write_image_ok(fap);
         }
 
+        if (rc == 0) {
+            if (permanent) {
+                swap_type = BOOT_SWAP_TYPE_PERM;
+            } else {
+                swap_type = BOOT_SWAP_TYPE_TEST;
+            }
+            rc = boot_write_swap_type(fap, swap_type);
+        }
+
         flash_area_close(fap);
         return rc;
 

boot/bootutil/src/bootutil_priv.h

@@ -61,6 +61,7 @@ struct boot_status {
     uint32_t idx;         /* Which area we're operating on */
     uint8_t state;        /* Which part of the swapping process are we at */
     uint8_t use_scratch;  /* Are status bytes ever written to scratch? */
+    uint8_t swap_type;    /* The type of swap in effect */
     uint32_t swap_size;   /* Total size of swapped image */
 #ifdef MCUBOOT_ENC_IMAGES
     uint8_t enckey[2][BOOT_ENC_KEY_SIZE];
@@ -71,6 +72,7 @@ struct boot_status {
 #define BOOT_MAGIC_BAD      2
 #define BOOT_MAGIC_UNSET    3
 #define BOOT_MAGIC_ANY      4  /* NOTE: control only, not dependent on sector */
+#define BOOT_MAGIC_NOTGOOD  5  /* NOTE: control only, not dependent on sector */
 
 /*
  * NOTE: leave BOOT_FLAG_SET equal to one, this is written to flash!
@@ -89,31 +91,42 @@ struct boot_status {
 /**
  * End-of-image slot structure.
  *
- *  0                   1                   2                   3
- *  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
- * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
- * ~                                                               ~
- * ~                Swap status (variable, aligned)                ~
- * ~                                                               ~
- * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
- * |                          Swap size                            |
- * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
- * ~             padding with erased val (MAX ALIGN - 4)           ~
- * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
- * |   Copy done   |   padding with erased val (MAX ALIGN - 1)     ~
- * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
- * |   Image OK    |   padding with erased val (MAX ALIGN - 1)     ~
- * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
- * ~                        MAGIC (16 octets)                      ~
- * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *   0                   1                   2                   3
+ *   0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+ *  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *  ~                                                               ~
+ *  ~    Swap status (BOOT_MAX_IMG_SECTORS * min-write-size * 3)    ~
+ *  ~                                                               ~
+ *  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *  |                 Encryption key 0 (16 octets) [*]              |
+ *  |                                                               |
+ *  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *  |                 Encryption key 1 (16 octets) [*]              |
+ *  |                                                               |
+ *  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *  |                      Swap size (4 octets)                     |
+ *  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *  |   Swap type   |           0xff padding (7 octets)             |
+ *  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *  |   Copy done   |           0xff padding (7 octets)             |
+ *  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *  |   Image OK    |           0xff padding (7 octets)             |
+ *  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *  |                       MAGIC (16 octets)                       |
+ *  |                                                               |
+ *  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *
+ * [*]: Only present if the encryption option is enabled
+ *      (`MCUBOOT_ENC_IMAGES`).
  */
 
 extern const uint32_t boot_img_magic[4];
 
 struct boot_swap_state {
-    uint8_t magic;  /* One of the BOOT_MAGIC_[...] values. */
-    uint8_t copy_done;
-    uint8_t image_ok;
+    uint8_t magic;      /* One of the BOOT_MAGIC_[...] values. */
+    uint8_t swap_type;  /* One of the BOOT_SWAP_TYPE_[...] values. */
+    uint8_t copy_done;  /* One of the BOOT_FLAG_[...] values. */
+    uint8_t image_ok;   /* One of the BOOT_FLAG_[...] values. */
 };
 
 #define BOOT_MAX_IMG_SECTORS       MCUBOOT_MAX_IMG_SECTORS
@@ -143,9 +156,6 @@ struct boot_swap_state {
 #define BOOT_STATUS_SOURCE_SCRATCH      1
 #define BOOT_STATUS_SOURCE_PRIMARY_SLOT 2
 
-#define BOOT_FLAG_IMAGE_OK              0
-#define BOOT_FLAG_COPY_DONE             1
-
 extern const uint32_t BOOT_MAGIC_SZ;
 
 /**
@@ -180,9 +190,11 @@ struct boot_loader_state {
 int bootutil_verify_sig(uint8_t *hash, uint32_t hlen, uint8_t *sig,
                         size_t slen, uint8_t key_id);
 
+int boot_magic_compatible_check(uint8_t tbl_val, uint8_t val);
 uint32_t boot_trailer_sz(uint8_t min_write_sz);
 int boot_status_entries(const struct flash_area *fap);
 uint32_t boot_status_off(const struct flash_area *fap);
+uint32_t boot_swap_type_off(const struct flash_area *fap);
 int boot_read_swap_state(const struct flash_area *fap,
                          struct boot_swap_state *state);
 int boot_read_swap_state_by_id(int flash_area_id,
@@ -192,6 +204,7 @@ int boot_write_status(struct boot_status *bs);
 int boot_schedule_test_swap(void);
 int boot_write_copy_done(const struct flash_area *fap);
 int boot_write_image_ok(const struct flash_area *fap);
+int boot_write_swap_type(const struct flash_area *fap, uint8_t swap_type);
 int boot_write_swap_size(const struct flash_area *fap, uint32_t swap_size);
 int boot_read_swap_size(uint32_t *swap_size);
 #ifdef MCUBOOT_ENC_IMAGES