DRAFT: ESP32-XX hardware flash encryption issue when updating images - "bootutil layer" solution

boot/bootutil/src/bootutil_public.c

@@ -327,6 +327,11 @@ boot_write_magic(const struct flash_area *fap)
     BOOT_LOG_DBG("boot_write_magic: fa_id=%d off=0x%lx (0x%lx)",
                  flash_area_get_id(fap), (unsigned long)off,
                  (unsigned long)(flash_area_get_off(fap) + off));
+
+#ifdef MCUBOOT_FLASH_HAS_HW_ENCRYPTION
+    rc = flash_area_erase(fap, pad_off, BOOT_MAGIC_ALIGN_SIZE);
+#endif
+
     rc = flash_area_write(fap, pad_off, &magic[0], BOOT_MAGIC_ALIGN_SIZE);
 
     if (rc != 0) {
@@ -365,6 +370,10 @@ boot_write_trailer(const struct flash_area *fap, uint32_t off,
     memcpy(buf, inbuf, inlen);
     memset(&buf[inlen], erased_val, align - inlen);
 
+#ifdef MCUBOOT_FLASH_HAS_HW_ENCRYPTION
+    rc = flash_area_erase(fap, off, align);
+#endif
+
     rc = flash_area_write(fap, off, buf, align);
     if (rc != 0) {
         return BOOT_EFLASH;

boot/bootutil/src/loader.c

@@ -652,6 +652,9 @@ boot_write_status(const struct boot_loader_state *state, struct boot_status *bs)
                  flash_area_get_id(fap), (unsigned long)off,
                  (unsigned long)flash_area_get_off(fap) + off);
 
+#ifdef MCUBOOT_FLASH_HAS_HW_ENCRYPTION
+    rc = flash_area_erase(fap, off, align);
+#endif
     rc = flash_area_write(fap, off, buf, align);
     if (rc != 0) {
         rc = BOOT_EFLASH;

boot/bootutil/src/swap_misc.c

@@ -64,6 +64,40 @@ swap_erase_trailer_sectors(const struct boot_loader_state *state,
             rc = boot_erase_region(fap, off, sz, false);
             assert(rc == 0);
 
+#ifdef MCUBOOT_FLASH_HAS_HW_ENCRYPTION
+        /* MCUboot's state machine relies on erased valued data
+         * (e.g. 0xFF) readed from this erased region that could
+         * be not written before, however if the flash device has
+         * hardware flash encryption and its flash read operation
+         * always decrypts what is being read from flash, thus a
+         * region that was erased would not be read as what
+         * MCUboot expected (after erasing, the region
+         * physically contains 0xFF, but once reading it, flash
+         * controller decrypts 0xFF to something else).
+         * So this configuration force the erased value into the
+         * region after the erasing.
+         */
+#ifndef MIN
+#  define MIN(a, b)                 (((a) < (b)) ? (a) : (b))
+#endif
+
+        uint8_t write_data[FLASH_AUX_WRITE_BUFFER_SIZE];
+        memset(write_data, flash_area_erased_val(fap), sizeof(write_data));
+        uint32_t bytes_remaining = sz;
+        uint32_t offset = off;
+
+        uint32_t bytes_written = MIN(sizeof(write_data), sz);
+        while (bytes_remaining != 0) {
+            if (flash_area_write(fap, offset, write_data, bytes_written)) {
+                BOOT_LOG_ERR("%s: Force write erased value after erasing a trailer region failed", __func__);
+                rc = -1;
+                break;
+            }
+            offset += bytes_written;
+            bytes_remaining -= bytes_written;
+        }
+#endif // MCUBOOT_FLASH_HAS_HW_ENCRYPTION
+
             sector--;
             total_sz += sz;
         } while (total_sz < trailer_sz);
@@ -92,6 +126,40 @@ swap_scramble_trailer_sectors(const struct boot_loader_state *state,
         return BOOT_EFLASH;
     }
 
+#ifdef MCUBOOT_FLASH_HAS_HW_ENCRYPTION
+        /* MCUboot's state machine relies on erased valued data
+         * (e.g. 0xFF) readed from this erased region that could
+         * be not written before, however if the flash device has
+         * hardware flash encryption and its flash read operation
+         * always decrypts what is being read from flash, thus a
+         * region that was erased would not be read as what
+         * MCUboot expected (after erasing, the region
+         * physically contains 0xFF, but once reading it, flash
+         * controller decrypts 0xFF to something else).
+         * So this configuration force the erased value into the
+         * region after the erasing.
+         */
+#ifndef MIN
+#  define MIN(a, b)                 (((a) < (b)) ? (a) : (b))
+#endif
+
+        uint8_t write_data[FLASH_AUX_WRITE_BUFFER_SIZE];
+        memset(write_data, flash_area_erased_val(fap), sizeof(write_data));
+        uint32_t bytes_remaining = (flash_area_get_size(fap) - off);
+        uint32_t offset = off;
+
+        uint32_t bytes_written = MIN(sizeof(write_data), (flash_area_get_size(fap) - off));
+        while (bytes_remaining != 0) {
+            if (flash_area_write(fap, offset, write_data, bytes_written)) {
+                BOOT_LOG_ERR("%s: Force write erased value after erasing a trailer region failed", __func__);
+                rc = -1;
+                break;
+            }
+            offset += bytes_written;
+            bytes_remaining -= bytes_written;
+        }
+#endif // MCUBOOT_FLASH_HAS_HW_ENCRYPTION
+
     return 0;
 }
 

boot/bootutil/src/swap_scratch.c

@@ -943,6 +943,15 @@ swap_run(struct boot_loader_state *state, struct boot_status *bs,
         swap_idx++;
     }
 
+#ifdef MCUBOOT_FLASH_HAS_HW_ENCRYPTION
+    int rc;
+    /* Ensure that the trailer from scratch area will have
+     * unset state after the swap process finishes.
+     */
+    rc = swap_scramble_trailer_sectors(state, state->scratch.area);
+    assert(rc == 0);
+#endif // MCUBOOT_FLASH_HAS_HW_ENCRYPTION
+
 }
 #endif /* !MCUBOOT_OVERWRITE_ONLY */
 

boot/espressif/CMakeLists.txt

@@ -13,8 +13,9 @@ endif()
 
 add_definitions(-DMCUBOOT_TARGET=${MCUBOOT_TARGET})
 add_definitions(-D__ESPRESSIF__=1)
+add_definitions(-DCONFIG_MCUBOOT_ESPRESSIF=1)
 
-set(EXPECTED_IDF_HAL_VERSION "5.1.4")
+set(EXPECTED_IDF_HAL_VERSION "5.1.6")
 
 if ("${MCUBOOT_TARGET}" STREQUAL "esp32" OR
     "${MCUBOOT_TARGET}" STREQUAL "esp32s2" OR

boot/espressif/hal/include/mcuboot_config/mcuboot_config.h

@@ -33,6 +33,14 @@
 #define MCUBOOT_BOOT_MAX_ALIGN 32
 #endif
 
+#ifdef CONFIG_SECURE_FLASH_ENC_ENABLED
+#define MCUBOOT_FLASH_HAS_HW_ENCRYPTION 1
+#endif
+
+#ifdef MCUBOOT_FLASH_HAS_HW_ENCRYPTION
+#define FLASH_AUX_WRITE_BUFFER_SIZE 0x100
+#endif
+
 /*
  * Upgrade mode
  *

boot/espressif/port/esp_mcuboot.c

@@ -13,6 +13,8 @@
 
 #include "sdkconfig.h"
 #include "esp_err.h"
+#include "hal/cache_hal.h"
+#include "hal/mmu_hal.h"
 #include "bootloader_flash_priv.h"
 #include "esp_flash_encrypt.h"
 #include "mcuboot_config/mcuboot_config.h"
@@ -148,6 +150,24 @@ void flash_area_close(const struct flash_area *area)
 
 }
 
+#ifdef CONFIG_SECURE_FLASH_ENC_ENABLED
+static void flush_cache(size_t start_addr, size_t length)
+{
+#if CONFIG_IDF_TARGET_ESP32
+    Cache_Read_Disable(0);
+    Cache_Flush(0);
+    Cache_Read_Enable(0);
+#else
+    uint32_t vaddr = 0;
+
+    mmu_hal_paddr_to_vaddr(0, start_addr, MMU_TARGET_FLASH0, MMU_VADDR_DATA, &vaddr);
+    if (vaddr != NULL) {
+        cache_hal_invalidate_addr(vaddr, length);
+    }
+#endif
+}
+#endif
+
 static bool aligned_flash_read(uintptr_t addr, void *dest, size_t size)
 {
     if (IS_ALIGNED(addr, 4) && IS_ALIGNED((uintptr_t)dest, 4) && IS_ALIGNED(size, 4)) {
@@ -225,29 +245,31 @@ static bool aligned_flash_write(size_t dest_addr, const void *src, size_t size)
 #else
         bool flash_encryption_enabled = false;
 #endif
+    size_t alignment = flash_encryption_enabled ? 32 : 4;
 
-    if (IS_ALIGNED(dest_addr, 4) && IS_ALIGNED((uintptr_t)src, 4) && IS_ALIGNED(size, 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 */
 
         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);
 
-    const uint32_t aligned_addr = ALIGN_DOWN(dest_addr, 4);
-    const uint32_t addr_offset = ALIGN_OFFSET(dest_addr, 4);
+    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] = {0};
+    uint8_t write_data[FLASH_BUFFER_SIZE] __attribute__((aligned(32))) = {0};
 
     /* Perform a read operation considering an offset not aligned to 4-byte boundary */
 
     uint32_t bytes = MIN(bytes_remaining + addr_offset, sizeof(write_data));
-    if (bootloader_flash_read(aligned_addr, write_data, ALIGN_UP(bytes, 4), true) != ESP_OK) {
+    if (bootloader_flash_read(aligned_addr, write_data, ALIGN_UP(bytes, alignment), true) != ESP_OK) {
         return false;
     }
 
     uint32_t bytes_written = bytes - addr_offset;
     memcpy(&write_data[addr_offset], src, bytes_written);
 
-    if (bootloader_flash_write(aligned_addr, write_data, ALIGN_UP(bytes, 4), flash_encryption_enabled) != ESP_OK) {
+    if (bootloader_flash_write(aligned_addr, write_data, ALIGN_UP(bytes, alignment), flash_encryption_enabled) != ESP_OK) {
         return false;
     }
 
@@ -259,15 +281,87 @@ static bool aligned_flash_write(size_t dest_addr, const void *src, size_t size)
 
     while (bytes_remaining != 0) {
         bytes = MIN(bytes_remaining, sizeof(write_data));
-        if (bootloader_flash_read(aligned_addr + offset, write_data, ALIGN_UP(bytes, 4), true) != ESP_OK) {
+        if (bootloader_flash_read(aligned_addr + offset, write_data, ALIGN_UP(bytes, alignment), true) != 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, 4), flash_encryption_enabled) != ESP_OK) {
+        if (bootloader_flash_write(aligned_addr + offset, write_data, ALIGN_UP(bytes, alignment), flash_encryption_enabled) != ESP_OK) {
+            return false;
+        }
+
+        offset += bytes;
+        bytes_written += bytes;
+        bytes_remaining -= bytes;
+    }
+
+    return true;
+}
+
+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 */
+
+        return bootloader_flash_erase_range(addr, size) == ESP_OK;
+    }
+    BOOT_LOG_DBG("%s: forcing unaligned erase on sector Offset: 0x%x Length: 0x%x",
+                    __func__, (int)addr, (int)size);
+
+    const uint32_t aligned_addr = ALIGN_DOWN(addr, FLASH_SECTOR_SIZE);
+    const uint32_t addr_offset = ALIGN_OFFSET(addr, FLASH_SECTOR_SIZE);
+    uint32_t bytes_remaining = size;
+    uint8_t write_data[FLASH_SECTOR_SIZE] __attribute__((aligned(32))) = {0};
+
+    /* Perform a read operation considering an offset not aligned */
+
+    uint32_t bytes = MIN(bytes_remaining + addr_offset, sizeof(write_data));
+
+    if (bootloader_flash_read(aligned_addr, write_data, ALIGN_UP(bytes, FLASH_SECTOR_SIZE), true) != ESP_OK) {
+        return false;
+    }
+
+    if (bootloader_flash_erase_range(aligned_addr, ALIGN_UP(bytes, FLASH_SECTOR_SIZE)) != ESP_OK) {
+        return false;
+    }
+
+    uint32_t bytes_written = bytes - addr_offset;
+
+    /* Write first part of non-erased data */
+    if(addr_offset > 0) {
+        if (!aligned_flash_write(aligned_addr, write_data, addr_offset)) {
             return false;
         }
+    }
+
+    if(bytes < sizeof(write_data)) {
+        if (!aligned_flash_write(aligned_addr + bytes, write_data + bytes, sizeof(write_data) - bytes)) {
+            return false;
+        }
+    }
+
+    bytes_remaining -= bytes_written;
+
+    /* Write remaining data to Flash if any */
+
+    uint32_t offset = bytes;
+
+    while (bytes_remaining != 0) {
+        bytes = MIN(bytes_remaining, sizeof(write_data));
+        if (bootloader_flash_read(aligned_addr + offset, write_data, ALIGN_UP(bytes, FLASH_SECTOR_SIZE), true) != ESP_OK) {
+            return false;
+        }
+
+        if (bootloader_flash_erase_range(aligned_addr + offset, ALIGN_UP(bytes, FLASH_SECTOR_SIZE)) != ESP_OK) {
+            return false;
+        }
+
+        if(bytes < sizeof(write_data)) {
+            if (!aligned_flash_write(aligned_addr + offset + bytes, write_data + bytes, sizeof(write_data) - bytes)) {
+                return false;
+            }
+        }
 
         offset += bytes;
         bytes_written += bytes;
@@ -293,12 +387,15 @@ 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 success = aligned_flash_write(start_addr, src, len);
-    if (!success) {
+    if (!aligned_flash_write(start_addr, src, len)) {
         BOOT_LOG_ERR("%s: Flash write failed", __func__);
         return -1;
     }
 
+#ifdef CONFIG_SECURE_FLASH_ENC_ENABLED
+    flush_cache(start_addr, len);
+#endif
+
     return 0;
 }
 
@@ -308,19 +405,18 @@ int flash_area_erase(const struct flash_area *fa, uint32_t off, uint32_t len)
         return -1;
     }
 
-    if ((len % FLASH_SECTOR_SIZE) != 0 || (off % FLASH_SECTOR_SIZE) != 0) {
-        BOOT_LOG_ERR("%s: Not aligned on sector Offset: 0x%x Length: 0x%x",
-                     __func__, (int)off, (int)len);
-        return -1;
-    }
-
     const uint32_t start_addr = fa->fa_off + off;
     BOOT_LOG_DBG("%s: Addr: 0x%08x Length: %d", __func__, (int)start_addr, (int)len);
 
-    if (bootloader_flash_erase_range(start_addr, len) != ESP_OK) {
+    if(!aligned_flash_erase(start_addr, len)) {
         BOOT_LOG_ERR("%s: Flash erase failed", __func__);
         return -1;
     }
+
+#ifdef CONFIG_SECURE_FLASH_ENC_ENABLED
+    flush_cache(start_addr, len);
+#endif
+
 #if VALIDATE_PROGRAM_OP
     for (size_t i = 0; i < len; i++) {
         uint8_t *val = (void *)(start_addr + i);