From d8aae55eebc8c47703593d8c9222dd339d7f5b82 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Thu, 26 Jan 2017 18:29:18 +1100 Subject: [PATCH] Flash encryption: Temporary fix for issue with stale cache reads Seems doing certain kinds of short reads while flash encryption is enabled will return stale data. This fixes it, but is probably a little heavy-handed performance wise. --- .../include/esp_flash_encrypt.h | 15 +++++-- components/spi_flash/flash_mmap.c | 45 ++++++++++++++----- 2 files changed, 47 insertions(+), 13 deletions(-) diff --git a/components/bootloader_support/include/esp_flash_encrypt.h b/components/bootloader_support/include/esp_flash_encrypt.h index 015dea030..ff1a5f330 100644 --- a/components/bootloader_support/include/esp_flash_encrypt.h +++ b/components/bootloader_support/include/esp_flash_encrypt.h @@ -15,7 +15,8 @@ #define __ESP32_FLASH_ENCRYPT_H #include -#include +#include "esp_attr.h" +#include "esp_err.h" #include "esp_spi_flash.h" #include "soc/efuse_reg.h" @@ -30,9 +31,17 @@ * * @return true if flash encryption is enabled. */ -static inline bool esp_flash_encryption_enabled(void) { +static inline IRAM_ATTR bool esp_flash_encryption_enabled(void) { uint32_t flash_crypt_cnt = REG_GET_FIELD(EFUSE_BLK0_RDATA0_REG, EFUSE_RD_FLASH_CRYPT_CNT); - return __builtin_parity(flash_crypt_cnt) == 1; + /* __builtin_parity is in flash, so we calculate parity inline */ + bool enabled = false; + while(flash_crypt_cnt) { + if (flash_crypt_cnt & 1) { + enabled = !enabled; + } + flash_crypt_cnt >>= 1; + } + return enabled; } /* @brief Update on-device flash encryption diff --git a/components/spi_flash/flash_mmap.c b/components/spi_flash/flash_mmap.c index 837fe0cc3..f8d2e3297 100644 --- a/components/spi_flash/flash_mmap.c +++ b/components/spi_flash/flash_mmap.c @@ -28,6 +28,7 @@ #include "esp_ipc.h" #include "esp_attr.h" #include "esp_spi_flash.h" +#include "esp_flash_encrypt.h" #include "esp_log.h" #include "cache_utils.h" @@ -52,8 +53,10 @@ This ensures stale cache entries are never read after fresh calls to spi_flash_mmap(), while keeping the number of cache flushes to a minimum. + + Returns true if cache was flushed. */ -static void spi_flash_ensure_unmodified_region(size_t start_addr, size_t length); +static bool spi_flash_ensure_unmodified_region(size_t start_addr, size_t length); typedef struct mmap_entry_{ uint32_t handle; @@ -89,6 +92,7 @@ esp_err_t IRAM_ATTR spi_flash_mmap(size_t src_addr, size_t size, spi_flash_mmap_ const void** out_ptr, spi_flash_mmap_handle_t* out_handle) { esp_err_t ret; + bool did_flush, need_flush = false; mmap_entry_t* new_entry = (mmap_entry_t*) malloc(sizeof(mmap_entry_t)); if (new_entry == 0) { return ESP_ERR_NO_MEM; @@ -102,7 +106,7 @@ esp_err_t IRAM_ATTR spi_flash_mmap(size_t src_addr, size_t size, spi_flash_mmap_ spi_flash_disable_interrupts_caches_and_other_cpu(); - spi_flash_ensure_unmodified_region(src_addr, size); + did_flush = spi_flash_ensure_unmodified_region(src_addr, size); if (s_mmap_page_refcnt[0] == 0) { spi_flash_mmap_init(); @@ -159,8 +163,11 @@ esp_err_t IRAM_ATTR spi_flash_mmap(size_t src_addr, size_t size, spi_flash_mmap_ (DPORT_PRO_FLASH_MMU_TABLE[i] == entry_val && DPORT_APP_FLASH_MMU_TABLE[i] == entry_val)); if (s_mmap_page_refcnt[i] == 0) { - DPORT_PRO_FLASH_MMU_TABLE[i] = entry_val; - DPORT_APP_FLASH_MMU_TABLE[i] = entry_val; + if (DPORT_PRO_FLASH_MMU_TABLE[i] != entry_val || DPORT_APP_FLASH_MMU_TABLE[i] != entry_val) { + DPORT_PRO_FLASH_MMU_TABLE[i] = entry_val; + DPORT_APP_FLASH_MMU_TABLE[i] = entry_val; + need_flush = true; + } } ++s_mmap_page_refcnt[i]; } @@ -173,6 +180,18 @@ esp_err_t IRAM_ATTR spi_flash_mmap(size_t src_addr, size_t size, spi_flash_mmap_ *out_ptr = (void*) (region_addr + start * SPI_FLASH_MMU_PAGE_SIZE); ret = ESP_OK; } + + /* This is a temporary fix for an issue where some + encrypted cache reads may see stale data. + + Working on a long term fix that doesn't require invalidating + entire cache. + */ + if (esp_flash_encryption_enabled() && !did_flush && need_flush) { + Cache_Flush(0); + Cache_Flush(1); + } + spi_flash_enable_interrupts_caches_and_other_cpu(); if (*out_ptr == NULL) { free(new_entry); @@ -240,25 +259,29 @@ void spi_flash_mmap_dump() */ static uint32_t written_pages[256/32]; -static void update_written_pages(size_t start_addr, size_t length, bool mark); +static bool update_written_pages(size_t start_addr, size_t length, bool mark); void IRAM_ATTR spi_flash_mark_modified_region(size_t start_addr, size_t length) { update_written_pages(start_addr, length, true); } -static void IRAM_ATTR spi_flash_ensure_unmodified_region(size_t start_addr, size_t length) +static IRAM_ATTR bool spi_flash_ensure_unmodified_region(size_t start_addr, size_t length) { - update_written_pages(start_addr, length, false); + return update_written_pages(start_addr, length, false); } /* generic implementation for the previous two functions */ -static inline IRAM_ATTR void update_written_pages(size_t start_addr, size_t length, bool mark) +static inline IRAM_ATTR bool update_written_pages(size_t start_addr, size_t length, bool mark) { - for (uint32_t addr = start_addr; addr < start_addr + length; addr += SPI_FLASH_MMU_PAGE_SIZE) { + /* align start_addr & length to full MMU pages */ + uint32_t page_start_addr = start_addr & ~(SPI_FLASH_MMU_PAGE_SIZE-1); + length += (start_addr - page_start_addr); + length = (length + SPI_FLASH_MMU_PAGE_SIZE - 1) & ~(SPI_FLASH_MMU_PAGE_SIZE-1); + for (uint32_t addr = page_start_addr; addr < page_start_addr + length; addr += SPI_FLASH_MMU_PAGE_SIZE) { int page = addr / SPI_FLASH_MMU_PAGE_SIZE; if (page >= 256) { - return; /* invalid address */ + return false; /* invalid address */ } int idx = page / 32; @@ -277,6 +300,8 @@ static inline IRAM_ATTR void update_written_pages(size_t start_addr, size_t leng Cache_Flush(1); #endif bzero(written_pages, sizeof(written_pages)); + return true; } } + return false; }