From af0d4bce65df277b56e495892dff1c712ed76ddd Mon Sep 17 00:00:00 2001 From: Nico Huber Date: Thu, 11 Jan 2024 18:59:24 +0100 Subject: region: Introduce region_create() functions We introduce two new functions to create region objects. They allow us to check for integer overflows (region_create_untrusted()) or assert their absence (region_create()). This fixes potential overflows in region_overlap() checks in SMI handlers, where we would wrongfully report MMIO as *not* overlapping SMRAM. Also, two cases of strtol() in parse_region() (cbfstool), where the results were implicitly converted to `size_t`, are replaced with the unsigned strtoul(). FIT payload support is left out, as it doesn't use the region API (only the struct). Change-Id: I4ae3e6274c981c9ab4fb1263c2a72fa68ef1c32b Ticket: https://ticket.coreboot.org/issues/522 Found-by: Vadim Zaliva Signed-off-by: Nico Huber Reviewed-on: https://review.coreboot.org/c/coreboot/+/79905 Tested-by: build bot (Jenkins) Reviewed-by: Felix Held --- src/commonlib/include/commonlib/region.h | 24 ++++++++++++++++++++---- src/cpu/x86/smm/smm_module_handler.c | 4 ++-- src/cpu/x86/smm/smm_module_loader.c | 31 ++++++++++--------------------- src/drivers/intel/fsp1_1/fsp_report.c | 13 +++++-------- src/drivers/intel/fsp2_0/fspt_report.c | 13 +++++-------- src/drivers/spi/spi_flash.c | 12 ++++++------ src/drivers/spi/winbond.c | 7 +++---- src/include/cpu/x86/smm.h | 5 ++++- src/lib/fmap.c | 3 +-- src/soc/qualcomm/common/qclib.c | 3 +-- util/cbfstool/cbfstool.c | 27 ++++++++++++++++++--------- util/cbfstool/cse_serger.c | 28 ++++++++++++++++++++-------- 12 files changed, 95 insertions(+), 75 deletions(-) diff --git a/src/commonlib/include/commonlib/region.h b/src/commonlib/include/commonlib/region.h index 25efcc8724..45a3b8d7b7 100644 --- a/src/commonlib/include/commonlib/region.h +++ b/src/commonlib/include/commonlib/region.h @@ -6,6 +6,7 @@ #include #include #include +#include #include #include @@ -94,10 +95,20 @@ struct region_device { }, \ } -/* Helper to dynamically initialize region device. */ -void region_device_init(struct region_device *rdev, - const struct region_device_ops *ops, size_t offset, - size_t size); +static inline struct region region_create(size_t offset, size_t size) +{ + assert(offset + size - 1 >= offset); + return (struct region){ .offset = offset, .size = size }; +} + +static inline enum cb_err region_create_untrusted( + struct region *r, unsigned long long offset, unsigned long long size) +{ + if (offset > SIZE_MAX || size > SIZE_MAX || (size_t)(offset + size - 1) < offset) + return CB_ERR_ARG; + *r = (struct region){ .offset = offset, .size = size }; + return CB_SUCCESS; +} /* Return 1 if child is subregion of parent, else 0. */ int region_is_subregion(const struct region *p, const struct region *c); @@ -123,6 +134,11 @@ static inline bool region_overlap(const struct region *r1, const struct region * (region_offset(r1) < region_end(r2)); } +/* Helper to dynamically initialize region device. */ +void region_device_init(struct region_device *rdev, + const struct region_device_ops *ops, size_t offset, + size_t size); + static inline const struct region *region_device_region( const struct region_device *rdev) { diff --git a/src/cpu/x86/smm/smm_module_handler.c b/src/cpu/x86/smm/smm_module_handler.c index 6322e59beb..7b4ae34808 100644 --- a/src/cpu/x86/smm/smm_module_handler.c +++ b/src/cpu/x86/smm/smm_module_handler.c @@ -124,8 +124,8 @@ uint32_t smm_revision(void) bool smm_region_overlaps_handler(const struct region *r) { - const struct region r_smm = {smm_runtime.smbase, smm_runtime.smm_size}; - const struct region r_aseg = {SMM_BASE, SMM_DEFAULT_SIZE}; + const struct region r_smm = region_create(smm_runtime.smbase, smm_runtime.smm_size); + const struct region r_aseg = region_create(SMM_BASE, SMM_DEFAULT_SIZE); return region_overlap(&r_smm, r) || region_overlap(&r_aseg, r); } diff --git a/src/cpu/x86/smm/smm_module_loader.c b/src/cpu/x86/smm/smm_module_loader.c index 646f3bb551..979fa485ab 100644 --- a/src/cpu/x86/smm/smm_module_loader.c +++ b/src/cpu/x86/smm/smm_module_loader.c @@ -115,11 +115,10 @@ static int smm_create_map(const uintptr_t smbase, const unsigned int num_cpus, const size_t segment_number = i / cpus_per_segment; cpus[i].smbase = smbase - SMM_CODE_SEGMENT_SIZE * segment_number - needed_ss_size * (i % cpus_per_segment); - cpus[i].stub_code.offset = cpus[i].smbase + SMM_ENTRY_OFFSET; - cpus[i].stub_code.size = stub_size; - cpus[i].ss.offset = cpus[i].smbase + SMM_CODE_SEGMENT_SIZE - - params->cpu_save_state_size; - cpus[i].ss.size = params->cpu_save_state_size; + cpus[i].stub_code = region_create(cpus[i].smbase + SMM_ENTRY_OFFSET, stub_size); + cpus[i].ss = region_create( + cpus[i].smbase + SMM_CODE_SEGMENT_SIZE - params->cpu_save_state_size, + params->cpu_save_state_size); cpus[i].active = 1; } @@ -482,16 +481,14 @@ int smm_load_module(const uintptr_t smram_base, const size_t smram_size, if (rmodule_parse(&_binary_smm_start, &smi_handler)) return -1; - const struct region smram = { .offset = smram_base, .size = smram_size }; + const struct region smram = region_create(smram_base, smram_size); const uintptr_t smram_top = region_end(&smram); const size_t stm_size = CONFIG(STM) ? CONFIG_MSEG_SIZE + CONFIG_BIOS_RESOURCE_LIST_SIZE : 0; if (CONFIG(STM)) { - struct region stm = {}; - stm.offset = smram_top - stm_size; - stm.size = stm_size; + struct region stm = region_create(smram_top - stm_size, stm_size); if (append_and_check_region(smram, stm, region_list, "STM")) return -1; printk(BIOS_DEBUG, "MSEG size 0x%x\n", CONFIG_MSEG_SIZE); @@ -503,20 +500,14 @@ int smm_load_module(const uintptr_t smram_base, const size_t smram_size, const uintptr_t handler_base = ALIGN_DOWN(smram_top - stm_size - handler_size, handler_alignment); - struct region handler = { - .offset = handler_base, - .size = handler_size - }; + struct region handler = region_create(handler_base, handler_size); if (append_and_check_region(smram, handler, region_list, "HANDLER")) return -1; uintptr_t stub_segment_base; if (ENV_X86_64) { uintptr_t pt_base = install_page_table(handler_base); - struct region page_tables = { - .offset = pt_base, - .size = handler_base - pt_base, - }; + struct region page_tables = region_create(pt_base, handler_base - pt_base); if (append_and_check_region(smram, page_tables, region_list, "PAGE TABLES")) return -1; params->cr3 = pt_base; @@ -540,10 +531,8 @@ int smm_load_module(const uintptr_t smram_base, const size_t smram_size, return -1; } - struct region stacks = { - .offset = smram_base, - .size = params->num_concurrent_save_states * CONFIG_SMM_MODULE_STACK_SIZE - }; + struct region stacks = region_create(smram_base, + params->num_concurrent_save_states * CONFIG_SMM_MODULE_STACK_SIZE); printk(BIOS_DEBUG, "\n"); if (append_and_check_region(smram, stacks, region_list, "stacks")) return -1; diff --git a/src/drivers/intel/fsp1_1/fsp_report.c b/src/drivers/intel/fsp1_1/fsp_report.c index 884218d7f7..f5c7b2f05b 100644 --- a/src/drivers/intel/fsp1_1/fsp_report.c +++ b/src/drivers/intel/fsp1_1/fsp_report.c @@ -10,14 +10,11 @@ uintptr_t temp_memory_end; void report_fsp_output(void) { - const struct region fsp_car_region = { - .offset = temp_memory_start, - .size = temp_memory_end - temp_memory_start, - }; - const struct region coreboot_car_region = { - .offset = (uintptr_t)_car_region_start, - .size = (uintptr_t)_car_region_size, - }; + const struct region fsp_car_region = region_create( + temp_memory_start, temp_memory_end - temp_memory_start); + const struct region coreboot_car_region = region_create( + (uintptr_t)_car_region_start, (uintptr_t)_car_region_size); + printk(BIOS_DEBUG, "FSP: reported temp_mem region: [0x%08lx,0x%08lx)\n", temp_memory_start, temp_memory_end); if (!region_is_subregion(&fsp_car_region, &coreboot_car_region)) { diff --git a/src/drivers/intel/fsp2_0/fspt_report.c b/src/drivers/intel/fsp2_0/fspt_report.c index 7fa3205e3d..87c08637c1 100644 --- a/src/drivers/intel/fsp2_0/fspt_report.c +++ b/src/drivers/intel/fsp2_0/fspt_report.c @@ -10,14 +10,11 @@ uintptr_t temp_memory_end; void report_fspt_output(void) { - const struct region fsp_car_region = { - .offset = temp_memory_start, - .size = temp_memory_end - temp_memory_start, - }; - const struct region coreboot_car_region = { - .offset = (uintptr_t)_car_region_start, - .size = (uintptr_t)_car_region_size, - }; + const struct region fsp_car_region = region_create( + temp_memory_start, temp_memory_end - temp_memory_start); + const struct region coreboot_car_region = region_create( + (uintptr_t)_car_region_start, (uintptr_t)_car_region_size); + printk(BIOS_DEBUG, "FSP-T: reported temp_mem region: [0x%08lx,0x%08lx)\n", temp_memory_start, temp_memory_end); if (!region_is_subregion(&fsp_car_region, &coreboot_car_region)) { diff --git a/src/drivers/spi/spi_flash.c b/src/drivers/spi/spi_flash.c index 42952df531..11597c6c1d 100644 --- a/src/drivers/spi/spi_flash.c +++ b/src/drivers/spi/spi_flash.c @@ -610,12 +610,12 @@ int spi_flash_status(const struct spi_flash *flash, u8 *reg) int spi_flash_is_write_protected(const struct spi_flash *flash, const struct region *region) { - struct region flash_region = { 0 }; + struct region flash_region; if (!flash || !region) return -1; - flash_region.size = flash->size; + flash_region = region_create(0, flash->size); if (!region_is_subregion(&flash_region, region)) return -1; @@ -633,13 +633,13 @@ int spi_flash_set_write_protected(const struct spi_flash *flash, const struct region *region, const enum spi_flash_status_reg_lockdown mode) { - struct region flash_region = { 0 }; + struct region flash_region; int ret; if (!flash) return -1; - flash_region.size = flash->size; + flash_region = region_create(0, flash->size); if (!region_is_subregion(&flash_region, region)) return -1; @@ -755,12 +755,12 @@ int spi_flash_ctrlr_protect_region(const struct spi_flash *flash, const enum ctrlr_prot_type type) { const struct spi_ctrlr *ctrlr; - struct region flash_region = { 0 }; + struct region flash_region; if (!flash) return -1; - flash_region.size = flash->size; + flash_region = region_create(0, flash->size); if (!region_is_subregion(&flash_region, region)) return -1; diff --git a/src/drivers/spi/winbond.c b/src/drivers/spi/winbond.c index 3137bfe8a7..3426d08e70 100644 --- a/src/drivers/spi/winbond.c +++ b/src/drivers/spi/winbond.c @@ -267,8 +267,7 @@ static void winbond_bpbits_to_region(const size_t granularity, tb = !tb; } - out->offset = tb ? 0 : flash_size - protected_size; - out->size = protected_size; + *out = region_create(tb ? 0 : flash_size - protected_size, protected_size); } /* @@ -525,8 +524,8 @@ winbond_set_write_protection(const struct spi_flash *flash, if (region_sz(&wp_region) > flash->size / 2) { cmp = 1; - wp_region.offset = tb ? 0 : region_sz(&wp_region); - wp_region.size = flash->size - region_sz(&wp_region); + wp_region = region_create(tb ? 0 : region_sz(&wp_region), + flash->size - region_sz(&wp_region)); tb = !tb; } else { cmp = 0; diff --git a/src/include/cpu/x86/smm.h b/src/include/cpu/x86/smm.h index 44a1086e4f..b552d33b5a 100644 --- a/src/include/cpu/x86/smm.h +++ b/src/include/cpu/x86/smm.h @@ -142,7 +142,10 @@ bool smm_region_overlaps_handler(const struct region *r); /* Returns true if the memory pointed to overlaps with SMM reserved memory. */ static inline bool smm_points_to_smram(const void *ptr, const size_t len) { - const struct region r = {(uintptr_t)ptr, len}; + struct region r; + + if (region_create_untrusted(&r, (uintptr_t)ptr, len) != CB_SUCCESS) + return true; /* Play it safe and pretend it overlaps if we can't tell. */ return smm_region_overlaps_handler(&r); } diff --git a/src/lib/fmap.c b/src/lib/fmap.c index 75c5a9fb1c..80fb0b2be6 100644 --- a/src/lib/fmap.c +++ b/src/lib/fmap.c @@ -199,8 +199,7 @@ int fmap_locate_area(const char *name, struct region *ar) printk(BIOS_DEBUG, "FMAP: area %s found @ %x (%d bytes)\n", name, le32toh(area->offset), le32toh(area->size)); - ar->offset = le32toh(area->offset); - ar->size = le32toh(area->size); + *ar = region_create(le32toh(area->offset), le32toh(area->size)); rdev_munmap(&fmrd, area); diff --git a/src/soc/qualcomm/common/qclib.c b/src/soc/qualcomm/common/qclib.c index 97b6e2d4f7..deb047d45a 100644 --- a/src/soc/qualcomm/common/qclib.c +++ b/src/soc/qualcomm/common/qclib.c @@ -101,9 +101,8 @@ static void write_ddr_information(struct qclib_cb_if_table_entry *te) uint64_t ddr_size; /* Save DDR info in SRAM region to share with ramstage */ - ddr_region->offset = te->blob_address; ddr_size = te->size; - ddr_region->size = ddr_size * MiB; + *ddr_region = region_create(te->blob_address, ddr_size * MiB); /* Use DDR info to configure MMU */ qc_mmu_dram_config_post_dram_init( diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c index f81c133bf1..cc8dbb5e3c 100644 --- a/util/cbfstool/cbfstool.c +++ b/util/cbfstool/cbfstool.c @@ -324,19 +324,25 @@ struct mmap_window { static int mmap_window_table_size; static struct mmap_window mmap_window_table[MMAP_MAX_WINDOWS]; -static void add_mmap_window(size_t flash_offset, size_t host_offset, - size_t window_size) +static int add_mmap_window(unsigned long flash_offset, unsigned long host_offset, unsigned long window_size) { if (mmap_window_table_size >= MMAP_MAX_WINDOWS) { ERROR("Too many memory map windows\n"); - return; + return 1; + } + + if (region_create_untrusted( + &mmap_window_table[mmap_window_table_size].flash_space, + flash_offset, window_size) != CB_SUCCESS || + region_create_untrusted( + &mmap_window_table[mmap_window_table_size].host_space, + host_offset, window_size) != CB_SUCCESS) { + ERROR("Invalid mmap window size %lu.\n", window_size); + return 1; } - mmap_window_table[mmap_window_table_size].flash_space.offset = flash_offset; - mmap_window_table[mmap_window_table_size].host_space.offset = host_offset; - mmap_window_table[mmap_window_table_size].flash_space.size = window_size; - mmap_window_table[mmap_window_table_size].host_space.size = window_size; mmap_window_table_size++; + return 0; } @@ -377,7 +383,9 @@ static int decode_mmap_arg(char *arg) return 1; } - add_mmap_window(mmap_args.flash_base, mmap_args.mmap_base, mmap_args.mmap_size); + if (add_mmap_window(mmap_args.flash_base, mmap_args.mmap_base, mmap_args.mmap_size)) + return 1; + return 0; } @@ -403,7 +411,8 @@ static bool create_mmap_windows(void) * maximum of 16MiB. If the window is smaller than 16MiB, the SPI flash window is mapped * at the top of the host window just below 4G. */ - add_mmap_window(std_window_flash_offset, DEFAULT_DECODE_WINDOW_TOP - std_window_size, std_window_size); + if (add_mmap_window(std_window_flash_offset, DEFAULT_DECODE_WINDOW_TOP - std_window_size, std_window_size)) + return false; } else { /* * Check provided memory map diff --git a/util/cbfstool/cse_serger.c b/util/cbfstool/cse_serger.c index f53b3904af..006a01e9be 100644 --- a/util/cbfstool/cse_serger.c +++ b/util/cbfstool/cse_serger.c @@ -820,15 +820,22 @@ static int cmd_add(void) return 0; } -static void parse_region(struct region *r, char *arg) +static int parse_region(struct region *r, char *arg) { char *tok; tok = strtok(arg, ":"); - r->offset = strtol(tok, NULL, 0); + unsigned long offset = strtoul(tok, NULL, 0); tok = strtok(NULL, ":"); - r->size = strtol(tok, NULL, 0); + unsigned long size = strtoul(tok, NULL, 0); + + if (region_create_untrusted(r, offset, size) != CB_SUCCESS) { + ERROR("Invalid region: %lx:%lx\n", offset, size); + return -1; + } + + return 0; } static struct command { @@ -991,19 +998,24 @@ int main(int argc, char **argv) params.partition_type = atoi(optarg); break; case LONGOPT_BP1: - parse_region(¶ms.layout_regions[BP1], optarg); + if (parse_region(¶ms.layout_regions[BP1], optarg)) + return 1; break; case LONGOPT_BP2: - parse_region(¶ms.layout_regions[BP2], optarg); + if (parse_region(¶ms.layout_regions[BP2], optarg)) + return 1; break; case LONGOPT_BP3: - parse_region(¶ms.layout_regions[BP3], optarg); + if (parse_region(¶ms.layout_regions[BP3], optarg)) + return 1; break; case LONGOPT_BP4: - parse_region(¶ms.layout_regions[BP4], optarg); + if (parse_region(¶ms.layout_regions[BP4], optarg)) + return 1; break; case LONGOPT_DATA: - parse_region(¶ms.layout_regions[DP], optarg); + if (parse_region(¶ms.layout_regions[DP], optarg)) + return 1; break; case LONGOPT_BP1_FILE: params.layout_files[BP1] = optarg; -- cgit v1.2.3