espressif: flash: fix image wrong state after swap-scratch when flash encryption is enabled

When hardware flash encryption is enabled, force expected erased
value (0xFF) into flash when erasing a region, and also always
do a real erase before writing data into flash.

This is handled on this implementation because MCUboot's state
machine relies on erased valued data (0xFF) readed from a
previously erased region that was not written yet, however when
hardware flash encryption is enabled, the flash read always
decrypts whats being read from flash, thus a region that was
erased would not be read as what MCUboot expected (0xFF).

Signed-off-by: Almir Okato <almir.okato@espressif.com>

Author: almir-okato

boot/espressif/hal/src/flash_encrypt.c

@@ -423,7 +423,7 @@ static esp_err_t encrypt_primary_slot(void)
      * MCUboot header
      */
     err = bootloader_flash_read(CONFIG_ESP_IMAGE0_PRIMARY_START_ADDRESS + 0x20,
-                                &img_header, sizeof(esp_image_load_header_t), true);
+                                &img_header, sizeof(esp_image_load_header_t), false);
     if (err != ESP_OK) {
         ESP_LOGE(TAG, "Failed to read slot img header");
         return err;
@@ -464,7 +464,7 @@ esp_err_t esp_flash_encrypt_region(uint32_t src_addr, size_t data_length)
         wdt_hal_feed(&rtc_wdt_ctx);
         wdt_hal_write_protect_enable(&rtc_wdt_ctx);
         uint32_t sec_start = i + src_addr;
-        err = bootloader_flash_read(sec_start, buf, FLASH_SECTOR_SIZE, true);
+        err = bootloader_flash_read(sec_start, buf, FLASH_SECTOR_SIZE, false);
         if (err != ESP_OK) {
             goto flash_failed;
         }

boot/espressif/port/esp_mcuboot.c

@@ -237,26 +237,38 @@ int flash_area_read(const struct flash_area *fa, uint32_t off, void *dst,
     return 0;
 }
 
-static bool aligned_flash_write(size_t dest_addr, const void *src, size_t size)
+static bool aligned_flash_write(size_t dest_addr, const void *src, size_t size, bool erase)
 {
 #ifdef CONFIG_SECURE_FLASH_ENC_ENABLED
         bool flash_encryption_enabled = esp_flash_encryption_enabled();
 #else
         bool flash_encryption_enabled = false;
 #endif
-    size_t alignment = flash_encryption_enabled ? 32 : 4;
+
+    /* When flash encryption is enabled, write alignment is 32 bytes, however to avoid
+     * inconsistences the region may be erased right before writting, thus the alignment
+     * is set to the erase required alignment (FLASH_SECTOR_SIZE).
+     * When flash encryption is not enabled, regular write alignment is 4 bytes.
+     */
+    size_t alignment = flash_encryption_enabled ? (erase ? FLASH_SECTOR_SIZE : 32) : 4;
 
     if (IS_ALIGNED(dest_addr, alignment) && IS_ALIGNED((uintptr_t)src, 4) && IS_ALIGNED(size, alignment)) {
         /* A single write operation is enough when all parameters are aligned */
 
+        if (flash_encryption_enabled && erase) {
+            if (bootloader_flash_erase_range(dest_addr, size) != ESP_OK) {
+                return false;
+            }
+        }
         return bootloader_flash_write(dest_addr, (void *)src, size, flash_encryption_enabled) == ESP_OK;
     }
-    BOOT_LOG_DBG("%s: forcing unaligned write dest_addr: 0x%08x src: 0x%08x size: 0x%x", __func__, (uint32_t)dest_addr, (uint32_t)src, size);
+    BOOT_LOG_DBG("%s: forcing unaligned write dest_addr: 0x%08x src: 0x%08x size: 0x%x erase: %c",
+                 __func__, (uint32_t)dest_addr, (uint32_t)src, size, erase ? 't' : 'f');
 
     const uint32_t aligned_addr = ALIGN_DOWN(dest_addr, alignment);
     const uint32_t addr_offset = ALIGN_OFFSET(dest_addr, alignment);
     uint32_t bytes_remaining = size;
-    uint8_t write_data[FLASH_BUFFER_SIZE] __attribute__((aligned(32))) = {0};
+    uint8_t write_data[FLASH_SECTOR_SIZE] __attribute__((aligned(32))) = {0};
 
     /* Perform a read operation considering an offset not aligned to 4-byte boundary */
 
@@ -265,6 +277,11 @@ static bool aligned_flash_write(size_t dest_addr, const void *src, size_t size)
         return false;
     }
 
+    if (flash_encryption_enabled && erase) {
+        if (bootloader_flash_erase_range(aligned_addr, ALIGN_UP(bytes, FLASH_SECTOR_SIZE)) != ESP_OK) {
+            return false;
+        }
+    }
     uint32_t bytes_written = bytes - addr_offset;
     memcpy(&write_data[addr_offset], src, bytes_written);
 
@@ -284,6 +301,12 @@ static bool aligned_flash_write(size_t dest_addr, const void *src, size_t size)
             return false;
         }
 
+        if (flash_encryption_enabled && erase) {
+            if (bootloader_flash_erase_range(aligned_addr + offset, ALIGN_UP(bytes, FLASH_SECTOR_SIZE)) != ESP_OK) {
+                return false;
+            }
+        }
+
         memcpy(write_data, &((uint8_t *)src)[bytes_written], bytes);
 
         if (bootloader_flash_write(aligned_addr + offset, write_data, ALIGN_UP(bytes, alignment), flash_encryption_enabled) != ESP_OK) {
@@ -301,7 +324,7 @@ static bool aligned_flash_write(size_t dest_addr, const void *src, size_t size)
 static bool aligned_flash_erase(size_t addr, size_t size)
 {
     if (IS_ALIGNED(addr, FLASH_SECTOR_SIZE) && IS_ALIGNED(size, FLASH_SECTOR_SIZE)) {
-        /* A single write operation is enough when all parameters are aligned */
+        /* A single erase operation is enough when all parameters are aligned */
 
         return bootloader_flash_erase_range(addr, size) == ESP_OK;
     }
@@ -329,13 +352,13 @@ static bool aligned_flash_erase(size_t addr, size_t size)
 
     /* Write first part of non-erased data */
     if(addr_offset > 0) {
-        if (!aligned_flash_write(aligned_addr, write_data, addr_offset)) {
+        if (!aligned_flash_write(aligned_addr, write_data, addr_offset, false)) {
             return false;
         }
     }
 
     if(bytes < sizeof(write_data)) {
-        if (!aligned_flash_write(aligned_addr + bytes, write_data + bytes, sizeof(write_data) - bytes)) {
+        if (!aligned_flash_write(aligned_addr + bytes, write_data + bytes, sizeof(write_data) - bytes, false)) {
             return false;
         }
     }
@@ -357,7 +380,7 @@ static bool aligned_flash_erase(size_t addr, size_t size)
         }
 
         if(bytes < sizeof(write_data)) {
-            if (!aligned_flash_write(aligned_addr + offset + bytes, write_data + bytes, sizeof(write_data) - bytes)) {
+            if (!aligned_flash_write(aligned_addr + offset + bytes, write_data + bytes, sizeof(write_data) - bytes, false)) {
                 return false;
             }
         }
@@ -384,9 +407,19 @@ int flash_area_write(const struct flash_area *fa, uint32_t off, const void *src,
     }
 
     const uint32_t start_addr = fa->fa_off + off;
-    BOOT_LOG_DBG("%s: Addr: 0x%08x Length: %d", __func__, (int)start_addr, (int)len);
+    bool erase = false;
+    BOOT_LOG_DBG("%s: Addr: 0x%08x Length: %d (0x%x)", __func__, (int)start_addr, (int)len, (int)len);
 
-    if (!aligned_flash_write(start_addr, src, len)) {
+#ifdef CONFIG_SECURE_FLASH_ENC_ENABLED
+    if (esp_flash_encryption_enabled()) {
+        /* Ensuring flash region has been erased before writing in order to
+         * avoid inconsistences when hardware flash encryption is enabled.
+         */
+        erase = true;
+    }
+#endif
+
+    if (!aligned_flash_write(start_addr, src, len, erase)) {
         BOOT_LOG_ERR("%s: Flash write failed", __func__);
         return -1;
     }
@@ -403,7 +436,7 @@ int flash_area_erase(const struct flash_area *fa, uint32_t off, uint32_t len)
     }
 
     const uint32_t start_addr = fa->fa_off + off;
-    BOOT_LOG_DBG("%s: Addr: 0x%08x Length: %d", __func__, (int)start_addr, (int)len);
+    BOOT_LOG_DBG("%s: Addr: 0x%08x Length: %d (0x%x)", __func__, (int)start_addr, (int)len, (int)len);
 
     if(!aligned_flash_erase(start_addr, len)) {
         BOOT_LOG_ERR("%s: Flash erase failed", __func__);
@@ -412,7 +445,38 @@ int flash_area_erase(const struct flash_area *fa, uint32_t off, uint32_t len)
 
     flush_cache(start_addr, len);
 
-#if VALIDATE_PROGRAM_OP
+#ifdef CONFIG_SECURE_FLASH_ENC_ENABLED
+    uint8_t write_data[FLASH_BUFFER_SIZE];
+    memset(write_data, flash_area_erased_val(fa), sizeof(write_data));
+    uint32_t bytes_remaining = len;
+    uint32_t offset = start_addr;
+
+    uint32_t bytes_written = MIN(sizeof(write_data), len);
+    if (esp_flash_encryption_enabled()) {
+        /* When hardware flash encryption is enabled, force expected erased
+         * value (0xFF) into flash when erasing a region.
+         *
+         * This is handled on this implementation because MCUboot's state
+         * machine relies on erased valued data (0xFF) read from a
+         * previously erased region that was not written yet, however when
+         * hardware flash encryption is enabled, the flash read always
+         * decrypts what's being read from flash, thus a region that was
+         * erased would not be read as what MCUboot expected (0xFF).
+         */
+        while (bytes_remaining != 0) {
+            if (!aligned_flash_write(offset, write_data, bytes_written, false)) {
+                BOOT_LOG_ERR("%s: Flash erase failed", __func__);
+                return -1;
+            }
+            offset += bytes_written;
+            bytes_remaining -= bytes_written;
+        }
+    }
+
+    flush_cache(start_addr, len);
+#endif
+
+#if VALIDATE_PROGRAM_OP && !defined(CONFIG_SECURE_FLASH_ENC_ENABLED)
     for (size_t i = 0; i < len; i++) {
         uint8_t *val = (void *)(start_addr + i);
         if (*val != 0xff) {

docs/readme-espressif.md

@@ -120,6 +120,22 @@ Additional configuration related to MCUboot features and slot partitioning may b
 
 2. Flash MCUboot in your device:
 
+    ---
+    ***Note***
+
+    *Prior to flashing the bootloader and/or application and booting for the first time, ensure
+    that the secondary slot and scratch area are erased. This is important because flash erased
+    value (`0xFF` in case of this port) read from the trailer registers are part of MCUboot's
+    update-state checking mechanism, thus unknown data or garbage could be potentially
+    interpreted as a valid state and lead to an unexpected behavior. Flash can be erased
+    entirely using:*
+
+    ```bash
+    esptool.py -p <PORT> erase_flash
+    ```
+
+    ---
+
     ```bash
     ninja -C build/ flash
     ```
@@ -532,10 +548,26 @@ CONFIG_SECURE_ENABLE_SECURE_ROM_DL_MODE=1
 
 ---
 
+---
+***Note***
+
+As recommended, ensure that the secondary slot and scratch area are **erased** prior to
+the first time boot. Hardware flash encryption is transparent to MCUboot and the first boot
+encryption process will encrypt the whole slots and scratch including their trailer regions,
+and as said before, erased value read from trailer registers is also an expected state of
+MCUboot update checking process. Erase flash command:
+
+```bash
+esptool.py -p <PORT> erase_flash
+```
+
+---
+
 ### [Signing the image when working with Flash Encryption](#signing-the-image-when-working-with-flash-encryption)
 
-When enabling flash encryption, it is required to signed the image using 32-byte alignment:
-`--align 32 --max-align 32`.
+When enabling flash encryption, it is required to sign the image using 32-byte alignment and also
+add the padding to fill the image up to the slot size:
+`--pad --align 32 --max-align 32`.
 
 Command example:
 
@@ -546,7 +578,9 @@ imgtool.py sign -k <YOUR_SIGNING_KEY.pem> --pad --pad-sig --align 32 --max-align
 ### [Device generated key](#device-generated-key)
 
 First ensure that the application image is able to perform encrypted read and write operations to
-the SPI Flash. Flash the bootloader and application normally:
+the SPI Flash.
+
+Flash the bootloader and application normally:
 
 ```bash
 esptool.py -p <PORT> -b 2000000 --after no_reset --chip <ESP_CHIP> write_flash --flash_mode dio --flash_size <FLASH_SIZE> --flash_freq 40m <BOOTLOADER_FLASH_OFFSET> <BOOTLOADER_BIN>

docs/release-notes.d/espressif-fix-fw-update-with-hw-flash-encryption-enabled

@@ -0,0 +1,6 @@
+- Add cache flush after write/erase operations to avoid getting invalid
+  data when these are followed by read operation.
+- Fix image wrong state after swap-scratch when hardware flash encryption
+  is enabled. When hardware flash encryption is enabled, force expected
+  erased value (0xFF) into flash when erasing a region, and also always
+  do a real erase before writing data into flash.