Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/79905?usp=email )
Change subject: region: Introduce region_create() functions ......................................................................
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 lord@digamma.ai Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/79905 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Felix Held felix-coreboot@felixheld.de --- M src/commonlib/include/commonlib/region.h M src/cpu/x86/smm/smm_module_handler.c M src/cpu/x86/smm/smm_module_loader.c M src/drivers/intel/fsp1_1/fsp_report.c M src/drivers/intel/fsp2_0/fspt_report.c M src/drivers/spi/spi_flash.c M src/drivers/spi/winbond.c M src/include/cpu/x86/smm.h M src/lib/fmap.c M src/soc/qualcomm/common/qclib.c M util/cbfstool/cbfstool.c M util/cbfstool/cse_serger.c 12 files changed, 95 insertions(+), 75 deletions(-)
Approvals: build bot (Jenkins): Verified Felix Held: Looks good to me, approved
diff --git a/src/commonlib/include/commonlib/region.h b/src/commonlib/include/commonlib/region.h index 25efcc8..45a3b8d 100644 --- a/src/commonlib/include/commonlib/region.h +++ b/src/commonlib/include/commonlib/region.h @@ -6,6 +6,7 @@ #include <sys/types.h> #include <stddef.h> #include <stdbool.h> +#include <commonlib/bsd/cb_err.h> #include <commonlib/bsd/helpers.h> #include <commonlib/mem_pool.h>
@@ -94,10 +95,20 @@ }, \ }
-/* 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 @@ (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 6322e59..7b4ae34 100644 --- a/src/cpu/x86/smm/smm_module_handler.c +++ b/src/cpu/x86/smm/smm_module_handler.c @@ -124,8 +124,8 @@
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 646f3bb..979fa48 100644 --- a/src/cpu/x86/smm/smm_module_loader.c +++ b/src/cpu/x86/smm/smm_module_loader.c @@ -115,11 +115,10 @@ 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 @@ 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 @@ 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 @@ 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 884218d..f5c7b2f 100644 --- a/src/drivers/intel/fsp1_1/fsp_report.c +++ b/src/drivers/intel/fsp1_1/fsp_report.c @@ -10,14 +10,11 @@
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 7fa3205..87c0863 100644 --- a/src/drivers/intel/fsp2_0/fspt_report.c +++ b/src/drivers/intel/fsp2_0/fspt_report.c @@ -10,14 +10,11 @@
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 42952df..11597c6 100644 --- a/src/drivers/spi/spi_flash.c +++ b/src/drivers/spi/spi_flash.c @@ -610,12 +610,12 @@ 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 @@ 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 @@ 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 3137bfe..3426d08 100644 --- a/src/drivers/spi/winbond.c +++ b/src/drivers/spi/winbond.c @@ -267,8 +267,7 @@ 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 @@
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 44a1086..b552d33 100644 --- a/src/include/cpu/x86/smm.h +++ b/src/include/cpu/x86/smm.h @@ -142,7 +142,10 @@ /* 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 75c5a9f..80fb0b2 100644 --- a/src/lib/fmap.c +++ b/src/lib/fmap.c @@ -199,8 +199,7 @@ 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 97b6e2d..deb047d 100644 --- a/src/soc/qualcomm/common/qclib.c +++ b/src/soc/qualcomm/common/qclib.c @@ -101,9 +101,8 @@ 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 f81c133..cc8dbb5 100644 --- a/util/cbfstool/cbfstool.c +++ b/util/cbfstool/cbfstool.c @@ -324,19 +324,25 @@ 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; }
- 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; + 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_size++; + return 0; }
@@ -377,7 +383,9 @@ 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 @@ * 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 f53b390..006a01e 100644 --- a/util/cbfstool/cse_serger.c +++ b/util/cbfstool/cse_serger.c @@ -820,15 +820,22 @@ 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 @@ 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;