Attention is currently required from: Jérémy Compostella.
Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/79905?usp=email )
Change subject: [RFC] region: Introduce region_create() functions ......................................................................
[RFC] region: Introduce region_create() functions
This introduces two new functions to create region objects. They allow us to check for integer overflows (region_create_untrusted()) or assert their absence (region_create()).
Again, FIT payload support is left out, as it doesn't use the region API (only the struct).
Change-Id: I4ae3e6274c981c9ab4fb1263c2a72fa68ef1c32b Signed-off-by: Nico Huber nico.h@gmx.de --- M src/commonlib/include/commonlib/region.h M src/cpu/x86/smm/smm_module_loader.c M src/drivers/spi/spi_flash.c M src/include/cpu/x86/smm.h M src/lib/fmap.c M util/cbfstool/cbfstool.c M util/cbfstool/cse_serger.c 7 files changed, 77 insertions(+), 46 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/79905/1
diff --git a/src/commonlib/include/commonlib/region.h b/src/commonlib/include/commonlib/region.h index 25efcc8..8de3c5d 100644 --- a/src/commonlib/include/commonlib/region.h +++ b/src/commonlib/include/commonlib/region.h @@ -94,10 +94,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 int region_create_untrusted( + struct region *r, unsigned long long offset, unsigned long long size) +{ + if (offset > SIZE_MAX || size > SIZE_MAX || offset + size - 1 < offset) + return -1; + *r = region_create(offset, size); + return 0; +}
/* Return 1 if child is subregion of parent, else 0. */ int region_is_subregion(const struct region *p, const struct region *c); @@ -123,6 +133,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_loader.c b/src/cpu/x86/smm/smm_module_loader.c index d965593..39b36e61 100644 --- a/src/cpu/x86/smm/smm_module_loader.c +++ b/src/cpu/x86/smm/smm_module_loader.c @@ -113,11 +113,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; }
@@ -425,7 +424,7 @@ 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 = @@ -433,8 +432,7 @@
if (CONFIG(STM)) { struct region stm = {}; - stm.offset = smram_top - stm_size; - stm.size = stm_size; + 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); @@ -446,10 +444,7 @@ 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;
@@ -470,10 +465,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/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/include/cpu/x86/smm.h b/src/include/cpu/x86/smm.h index dfb27cd..1c49842 100644 --- a/src/include/cpu/x86/smm.h +++ b/src/include/cpu/x86/smm.h @@ -137,7 +137,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)) + 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/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c index e11cfbc..10d6501 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) || + region_create_untrusted( + &mmap_window_table[mmap_window_table_size].host_space, + host_offset, window_size)) { + ERROR("Invalid mmap window size %zu.\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..9b776e6 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 = strtol(tok, NULL, 0);
tok = strtok(NULL, ":"); - r->size = strtol(tok, NULL, 0); + unsigned long size = strtol(tok, NULL, 0); + + if (region_create_untrusted(r, offset, size)) { + 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;