Attention is currently required from: Jérémy Compostella.

Nico Huber has uploaded this change for review.

View Change

[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(&params.layout_regions[BP1], optarg);
+				if (parse_region(&params.layout_regions[BP1], optarg))
+					return 1;
 				break;
 			case LONGOPT_BP2:
-				parse_region(&params.layout_regions[BP2], optarg);
+				if (parse_region(&params.layout_regions[BP2], optarg))
+					return 1;
 				break;
 			case LONGOPT_BP3:
-				parse_region(&params.layout_regions[BP3], optarg);
+				if (parse_region(&params.layout_regions[BP3], optarg))
+					return 1;
 				break;
 			case LONGOPT_BP4:
-				parse_region(&params.layout_regions[BP4], optarg);
+				if (parse_region(&params.layout_regions[BP4], optarg))
+					return 1;
 				break;
 			case LONGOPT_DATA:
-				parse_region(&params.layout_regions[DP], optarg);
+				if (parse_region(&params.layout_regions[DP], optarg))
+					return 1;
 				break;
 			case LONGOPT_BP1_FILE:
 				params.layout_files[BP1] = optarg;

To view, visit change 79905. To unsubscribe, or for help writing mail filters, visit settings.