From 670733df9fccc35be9a8874e049bec1dbc60f2ea Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Wed, 25 Oct 2017 15:22:30 +0800 Subject: [PATCH] spi_flash: Abort on writes to dangerous regions (bootloader, partition table, app) Can be disabled or made into a failure result in kconfig if needed. --- components/spi_flash/Kconfig | 25 +++++++++++ components/spi_flash/flash_ops.c | 45 +++++++++++++++++++ .../spi_flash/test/test_out_of_bounds_write.c | 33 ++++++++++++++ tools/unit-test-app/sdkconfig.defaults | 1 + 4 files changed, 104 insertions(+) create mode 100644 components/spi_flash/test/test_out_of_bounds_write.c diff --git a/components/spi_flash/Kconfig b/components/spi_flash/Kconfig index 8b9c714820..4028cf3894 100644 --- a/components/spi_flash/Kconfig +++ b/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 diff --git a/components/spi_flash/flash_ops.c b/components/spi_flash/flash_ops.c index bbc65e4ca1..4b811d1151 100644 --- a/components/spi_flash/flash_ops.c +++ b/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 @@ out: 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; diff --git a/components/spi_flash/test/test_out_of_bounds_write.c b/components/spi_flash/test/test_out_of_bounds_write.c new file mode 100644 index 0000000000..3c9b017476 --- /dev/null +++ b/components/spi_flash/test/test_out_of_bounds_write.c @@ -0,0 +1,33 @@ +#include + +#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 diff --git a/tools/unit-test-app/sdkconfig.defaults b/tools/unit-test-app/sdkconfig.defaults index 5875356e6c..5bad913229 100644 --- a/tools/unit-test-app/sdkconfig.defaults +++ b/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