Merge branch 'bugfix/protect_spiflash_regions' into 'master'

spi_flash: Abort on writes to dangerous regions (bootloader, partition table, app)

See merge request !1452

Author: projectgus

components/spi_flash/Kconfig

@@ -21,6 +21,31 @@ config SPI_FLASH_ROM_DRIVER_PATCH
         This option is needed to write to flash on ESP32-D2WD, and any configuration
         where external SPI flash is connected to non-default pins.
 
+choice SPI_FLASH_WRITING_DANGEROUS_REGIONS
+    bool  "Writing to dangerous flash regions"
+    default SPI_FLASH_WRITING_DANGEROUS_REGIONS_ABORTS
+    help
+        SPI flash APIs can optionally abort or return a failure code
+        if erasing or writing addresses that fall at the beginning
+        of flash (covering the bootloader and partition table) or that
+        overlap the app partition that contains the running app.
+
+        It is not recommended to ever write to these regions from an IDF app,
+        and this check prevents logic errors or corrupted firmware memory from
+        damaging these regions.
+
+        Note that this feature *does not* check calls to the esp_rom_xxx SPI flash
+        ROM functions. These functions should not be called directly from IDF
+        applications.
+
+config SPI_FLASH_WRITING_DANGEROUS_REGIONS_ABORTS
+     bool "Aborts"
+config SPI_FLASH_WRITING_DANGEROUS_REGIONS_FAILS
+     bool "Fails"
+config SPI_FLASH_WRITING_DANGEROUS_REGIONS_ALLOWED
+     bool "Allowed"
+endchoice
+
 endmenu
 
 

components/spi_flash/flash_ops.c

@@ -31,6 +31,8 @@
 #include "esp_spi_flash.h"
 #include "esp_log.h"
 #include "esp_clk.h"
+#include "esp_flash_partitions.h"
+#include "esp_ota_ops.h"
 #include "cache_utils.h"
 
 /* bytes erased by SPIEraseBlock() ROM function */
@@ -83,6 +85,45 @@ const DRAM_ATTR spi_flash_guard_funcs_t g_flash_guard_no_os_ops = {
 
 static const spi_flash_guard_funcs_t *s_flash_guard_ops;
 
+#ifdef CONFIG_SPI_FLASH_WRITING_DANGEROUS_REGIONS_ABORTS
+#define UNSAFE_WRITE_ADDRESS abort()
+#else
+#define UNSAFE_WRITE_ADDRESS return false
+#endif
+
+
+/* CHECK_WRITE_ADDRESS macro to fail writes which land in the
+   bootloader, partition table, or running application region.
+*/
+#if CONFIG_SPI_FLASH_WRITING_DANGEROUS_REGIONS_ALLOWED
+#define CHECK_WRITE_ADDRESS(ADDR, SIZE)
+#else /* FAILS or ABORTS */
+#define CHECK_WRITE_ADDRESS(ADDR, SIZE) do {                            \
+        if (!is_safe_write_address(ADDR, SIZE)) {                       \
+            return ESP_ERR_INVALID_ARG;                                 \
+        }                                                               \
+    } while(0)
+#endif // CONFIG_SPI_FLASH_WRITING_DANGEROUS_REGIONS_ALLOWED
+
+static __attribute__((unused)) bool is_safe_write_address(size_t addr, size_t size)
+{
+    bool result = true;
+    if (addr <= ESP_PARTITION_TABLE_OFFSET + ESP_PARTITION_TABLE_MAX_LEN) {
+        UNSAFE_WRITE_ADDRESS;
+    }
+
+    const esp_partition_t *p = esp_ota_get_running_partition();
+    if (addr >= p->address && addr < p->address + p->size) {
+        UNSAFE_WRITE_ADDRESS;
+    }
+    if (addr < p->address && addr + size > p->address) {
+        UNSAFE_WRITE_ADDRESS;
+    }
+
+    return result;
+}
+
+
 void spi_flash_init()
 {
     spi_flash_init_lock();
@@ -146,11 +187,13 @@ static esp_rom_spiflash_result_t IRAM_ATTR spi_flash_unlock()
 
 esp_err_t IRAM_ATTR spi_flash_erase_sector(size_t sec)
 {
+    CHECK_WRITE_ADDRESS(sec * SPI_FLASH_SEC_SIZE, SPI_FLASH_SEC_SIZE);
     return spi_flash_erase_range(sec * SPI_FLASH_SEC_SIZE, SPI_FLASH_SEC_SIZE);
 }
 
 esp_err_t IRAM_ATTR spi_flash_erase_range(uint32_t start_addr, uint32_t size)
 {
+    CHECK_WRITE_ADDRESS(start_addr, size);
     if (start_addr % SPI_FLASH_SEC_SIZE != 0) {
         return ESP_ERR_INVALID_ARG;
     }
@@ -187,6 +230,7 @@ esp_err_t IRAM_ATTR spi_flash_erase_range(uint32_t start_addr, uint32_t size)
 
 esp_err_t IRAM_ATTR spi_flash_write(size_t dst, const void *srcv, size_t size)
 {
+    CHECK_WRITE_ADDRESS(dst, size);
     // Out of bound writes are checked in ROM code, but we can give better
     // error code here
     if (dst + size > g_rom_flashchip.chip_size) {
@@ -281,6 +325,7 @@ esp_err_t IRAM_ATTR spi_flash_write(size_t dst, const void *srcv, size_t size)
 
 esp_err_t IRAM_ATTR spi_flash_write_encrypted(size_t dest_addr, const void *src, size_t size)
 {
+    CHECK_WRITE_ADDRESS(dest_addr, size);
     const uint8_t *ssrc = (const uint8_t *)src;
     if ((dest_addr % 16) != 0) {
         return ESP_ERR_INVALID_ARG;

components/spi_flash/test/test_out_of_bounds_write.c

@@ -0,0 +1,33 @@
+#include <string.h>
+
+#include "unity.h"
+#include "esp_spi_flash.h"
+#include "esp_ota_ops.h"
+
+#if CONFIG_SPI_FLASH_WRITING_DANGEROUS_REGIONS_ABORTS || CONFIG_SPI_FLASH_WRITING_DANGEROUS_REGIONS_FAILS
+
+static const char *data = "blah blah blah";
+
+#if CONFIG_SPI_FLASH_WRITING_DANGEROUS_REGIONS_FAILS
+#define TEST_TAGS "[spi_flash]"
+#else // ABORTS
+#define TEST_TAGS "[spi_flash][ignore]"
+#endif
+
+TEST_CASE("can't overwrite bootloader", TEST_TAGS)
+{
+    TEST_ESP_ERR(ESP_ERR_INVALID_ARG, spi_flash_write(0x1000, data, strlen(data)));
+    TEST_ESP_ERR(ESP_ERR_INVALID_ARG, spi_flash_write(0x0FF8, data, strlen(data)));
+    TEST_ESP_ERR(ESP_ERR_INVALID_ARG, spi_flash_write(0x1400, data, strlen(data)));
+    TEST_ESP_ERR(ESP_ERR_INVALID_ARG, spi_flash_erase_range(0x8000, 0x2000));
+    TEST_ESP_ERR(ESP_ERR_INVALID_ARG, spi_flash_erase_range(0x7000, 0x2000));
+}
+
+TEST_CASE("can't overwrite current running app", TEST_TAGS)
+{
+    const esp_partition_t *p = esp_ota_get_running_partition();
+    TEST_ESP_ERR(ESP_ERR_INVALID_ARG, spi_flash_write(p->address + 1024, data, strlen(data)));
+    TEST_ESP_ERR(ESP_ERR_INVALID_ARG, spi_flash_erase_range(p->address + 4096, 8192));
+}
+
+#endif // FAILS || ABORTS

tools/unit-test-app/sdkconfig.defaults

@@ -19,3 +19,4 @@ CONFIG_MBEDTLS_HARDWARE_SHA=y
 CONFIG_SPI_FLASH_ENABLE_COUNTERS=y
 CONFIG_ULP_COPROC_ENABLED=y
 CONFIG_TASK_WDT=n
+CONFIG_SPI_FLASH_WRITING_DANGEROUS_REGIONS_FAILS=y