From 9ec43e172122242e7999a32f9bf3a0b9e49e8e2a Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Sat, 29 Sep 2018 17:29:23 +0800 Subject: [PATCH 1/3] bootloader: verify that loaded image does not overlap bootloader code Fixes CVE-2018-18558 --- .../subproject/main/esp32.bootloader.ld | 6 +-- .../include/bootloader_util.h | 34 ++++++++++++++++ .../bootloader_support/src/esp_image_format.c | 40 ++++++++++++++++--- .../test/test_verify_image.c | 21 ++++++++++ 4 files changed, 91 insertions(+), 10 deletions(-) create mode 100644 components/bootloader_support/include/bootloader_util.h diff --git a/components/bootloader/subproject/main/esp32.bootloader.ld b/components/bootloader/subproject/main/esp32.bootloader.ld index 8113987f4..88d5b953c 100644 --- a/components/bootloader/subproject/main/esp32.bootloader.ld +++ b/components/bootloader/subproject/main/esp32.bootloader.ld @@ -30,8 +30,7 @@ SECTIONS .iram_loader.text : { . = ALIGN (16); - _stext = .; - _text_start = ABSOLUTE(.); + _loader_text_start = ABSOLUTE(.); *(.stub .gnu.warning .gnu.linkonce.literal.* .gnu.linkonce.t.*.literal .gnu.linkonce.t.*) *(.iram1 .iram1.*) /* catch stray IRAM_ATTR */ *liblog.a:(.literal .text .literal.* .text.*) @@ -51,8 +50,7 @@ SECTIONS *(.fini.literal) *(.fini) *(.gnu.version) - _text_end = ABSOLUTE(.); - _etext = .; + _loader_text_end = ABSOLUTE(.); } > iram_loader_seg .iram.text : diff --git a/components/bootloader_support/include/bootloader_util.h b/components/bootloader_support/include/bootloader_util.h new file mode 100644 index 000000000..30f8bd8d2 --- /dev/null +++ b/components/bootloader_support/include/bootloader_util.h @@ -0,0 +1,34 @@ +// Copyright 2018 Espressif Systems (Shanghai) PTE LTD +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#pragma once + +#include + +/** + * @brief Check if half-open intervals overlap + * + * @param start1 interval 1 start + * @param end1 interval 1 end + * @param start2 interval 2 start + * @param end2 interval 2 end + * @return true iff [start1; end1) overlaps [start2; end2) + */ +static inline bool bootloader_util_regions_overlap( + const intptr_t start1, const intptr_t end1, + const intptr_t start2, const intptr_t end2) +{ + return (end1 > start2 && end2 > start1) || + !(end1 <= start2 || end2 <= start1); +} diff --git a/components/bootloader_support/src/esp_image_format.c b/components/bootloader_support/src/esp_image_format.c index 87b5a94fe..0ecf6e6c9 100644 --- a/components/bootloader_support/src/esp_image_format.c +++ b/components/bootloader_support/src/esp_image_format.c @@ -23,6 +23,7 @@ #include #include #include +#include "bootloader_util.h" /* Checking signatures as part of verifying images is necessary: - Always if secure boot is enabled @@ -56,6 +57,10 @@ static const char *TAG = "esp_image"; (Means loaded code isn't executable until after the secure boot check.) */ static uint32_t ram_obfs_value[2]; + +/* Range of IRAM used by the loader, defined in ld script */ +extern int _loader_text_start; +extern int _loader_text_end; #endif /* Return true if load_addr is an address the bootloader should load into */ @@ -301,18 +306,41 @@ static esp_err_t process_segment(int index, uint32_t flash_addr, esp_image_segme (do_load)?"load":(is_mapping)?"map":""); } + +#ifdef BOOTLOADER_BUILD + /* Before loading segment, check it doesn't clobber bootloader RAM. */ if (do_load) { - /* Before loading segment, check it doesn't clobber bootloader RAM... */ - uint32_t end_addr = load_addr + data_len; - if (end_addr < 0x40000000) { + const intptr_t load_end = load_addr + data_len; + if (load_end <= (intptr_t) SOC_DIRAM_DRAM_HIGH) { + /* Writing to DRAM */ intptr_t sp = (intptr_t)get_sp(); - if (end_addr > sp - STACK_LOAD_HEADROOM) { - ESP_LOGE(TAG, "Segment %d end address 0x%08x too high (bootloader stack 0x%08x liimit 0x%08x)", - index, end_addr, sp, sp - STACK_LOAD_HEADROOM); + if (load_end > sp - STACK_LOAD_HEADROOM) { + /* Bootloader .data/.rodata/.bss is above the stack, so this + * also checks that we aren't overwriting these segments. + * + * TODO: This assumes specific arrangement of sections we have + * in the ESP32. Rewrite this in a generic way to support other + * layouts. + */ + ESP_LOGE(TAG, "Segment %d end address 0x%08x too high (bootloader stack 0x%08x limit 0x%08x)", + index, load_end, sp, sp - STACK_LOAD_HEADROOM); + return ESP_ERR_IMAGE_INVALID; + } + } else { + /* Writing to IRAM */ + const intptr_t loader_iram_start = (intptr_t) &_loader_text_start; + const intptr_t loader_iram_end = (intptr_t) &_loader_text_end; + + if (bootloader_util_regions_overlap(loader_iram_start, loader_iram_end, + load_addr, load_end)) { + ESP_LOGE(TAG, "Segment %d (0x%08x-0x%08x) overlaps bootloader IRAM (0x%08x-0x%08x)", + index, load_addr, load_end, loader_iram_start, loader_iram_end); return ESP_ERR_IMAGE_INVALID; } } } +#endif // BOOTLOADER_BUILD + #ifndef BOOTLOADER_BUILD uint32_t free_page_count = spi_flash_mmap_get_free_pages(SPI_FLASH_MMAP_DATA); ESP_LOGD(TAG, "free data page_count 0x%08x",free_page_count); diff --git a/components/bootloader_support/test/test_verify_image.c b/components/bootloader_support/test/test_verify_image.c index 7994667f3..f828518d6 100644 --- a/components/bootloader_support/test/test_verify_image.c +++ b/components/bootloader_support/test/test_verify_image.c @@ -14,6 +14,7 @@ #include "freertos/xtensa_api.h" #include "unity.h" #include "bootloader_common.h" +#include "bootloader_util.h" #include "esp_partition.h" #include "esp_ota_ops.h" #include "esp_image_format.h" @@ -92,3 +93,23 @@ TEST_CASE("Test label_search", "[bootloader_support]") check_label_search(25, "phy, 1234567890123456, nvs1", "12345678901234567", true); } + + +TEST_CASE("Test regions_overlap", "[bootloader_support]") +{ + TEST_ASSERT( bootloader_util_regions_overlap(1, 2, 1, 2) ); + + TEST_ASSERT( bootloader_util_regions_overlap(1, 2, 0, 2) ); + TEST_ASSERT( bootloader_util_regions_overlap(1, 2, 1, 3) ); + TEST_ASSERT( bootloader_util_regions_overlap(1, 2, 0, 3) ); + + TEST_ASSERT( bootloader_util_regions_overlap(0, 2, 1, 2) ); + TEST_ASSERT( bootloader_util_regions_overlap(1, 3, 1, 2) ); + TEST_ASSERT( bootloader_util_regions_overlap(0, 3, 1, 2) ); + + TEST_ASSERT( !bootloader_util_regions_overlap(2, 3, 1, 2) ); + TEST_ASSERT( !bootloader_util_regions_overlap(1, 2, 2, 3) ); + + TEST_ASSERT( !bootloader_util_regions_overlap(3, 4, 1, 2) ); + TEST_ASSERT( !bootloader_util_regions_overlap(1, 2, 3, 4) ); +} From 12e5f25a1c6d770a15e65747e0cebb2c1f370add Mon Sep 17 00:00:00 2001 From: Mahavir Jain Date: Fri, 24 Aug 2018 17:56:38 +0530 Subject: [PATCH 2/3] bootloader: keep bootloader_common code to retention region It is possible to utilize some of the routines related to otadata partition validation, after firmware image is downloaded to RAM. Hence these routines should be part of app cpu cache, so that they do not get overwritten by firmware. Signed-off-by: Mahavir Jain --- components/bootloader/subproject/main/esp32.bootloader.ld | 1 + 1 file changed, 1 insertion(+) diff --git a/components/bootloader/subproject/main/esp32.bootloader.ld b/components/bootloader/subproject/main/esp32.bootloader.ld index 88d5b953c..fb69fcb4c 100644 --- a/components/bootloader/subproject/main/esp32.bootloader.ld +++ b/components/bootloader/subproject/main/esp32.bootloader.ld @@ -35,6 +35,7 @@ SECTIONS *(.iram1 .iram1.*) /* catch stray IRAM_ATTR */ *liblog.a:(.literal .text .literal.* .text.*) *libgcc.a:(.literal .text .literal.* .text.*) + *libbootloader_support.a:bootloader_common.o(.literal .text .literal.* .text.*) *libbootloader_support.a:bootloader_flash.*(.literal .text .literal.* .text.*) *libbootloader_support.a:bootloader_random.*(.literal .text .literal.* .text.*) *libbootloader_support.a:bootloader_utility.*(.literal .text .literal.* .text.*) From 7d6b82673fcfb74cde5dfbd5ca05c1d76d0375cd Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Wed, 26 Sep 2018 17:48:50 +1000 Subject: [PATCH 3/3] bootloader: Fix crash enabling flash encryption Regression in 9c715d7946a9595bad53307cf0a141d4226d0a5a --- components/bootloader/subproject/main/esp32.bootloader.ld | 1 + 1 file changed, 1 insertion(+) diff --git a/components/bootloader/subproject/main/esp32.bootloader.ld b/components/bootloader/subproject/main/esp32.bootloader.ld index fb69fcb4c..63b6e4ff1 100644 --- a/components/bootloader/subproject/main/esp32.bootloader.ld +++ b/components/bootloader/subproject/main/esp32.bootloader.ld @@ -48,6 +48,7 @@ SECTIONS *libbootloader_support.a:secure_boot_signatures.*(.literal .text .literal.* .text.*) *libmicro-ecc.a:*.*(.literal .text .literal.* .text.*) *libspi_flash.a:*.*(.literal .text .literal.* .text.*) + *libsoc.a:rtc_wdt.*(.literal .text .literal.* .text.*) *(.fini.literal) *(.fini) *(.gnu.version)