region: Turn region_end() into an inclusive region_last()

The current region_end() implementation is susceptible to overflow
if the region is at the end of the addressable space. A common case
with the memory-mapped flash of x86 directly below the 32-bit limit.

Note: This patch also changes console output to inclusive limits.
IMO, to the better.

Change-Id: Ic4bd6eced638745b7e845504da74542e4220554a
Signed-off-by: Nico Huber <nico.h@gmx.de>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/79946
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Julius Werner <jwerner@chromium.org>

src/commonlib/include/commonlib/region.h

@@ -124,15 +124,15 @@ static inline size_t region_sz(const struct region *r)
 	return r->size;
 }
 
-static inline size_t region_end(const struct region *r)
+static inline size_t region_last(const struct region *r)
 {
-	return region_offset(r) + region_sz(r);
+	return region_offset(r) + (region_sz(r) - 1);
 }
 
 static inline bool region_overlap(const struct region *r1, const struct region *r2)
 {
-	return (region_end(r1) > region_offset(r2)) &&
+	return (region_last(r1) >= region_offset(r2)) &&
-	       (region_offset(r1) < region_end(r2));
+	       (region_offset(r1) <= region_last(r2));
 }
 
 /* Helper to dynamically initialize region device. */
@@ -156,9 +156,9 @@ static inline size_t region_device_offset(const struct region_device *rdev)
 	return region_offset(region_device_region(rdev));
 }
 
-static inline size_t region_device_end(const struct region_device *rdev)
+static inline size_t region_device_last(const struct region_device *rdev)
 {
-	return region_end(region_device_region(rdev));
+	return region_last(region_device_region(rdev));
 }
 
 /* Memory map entire region device. Same semantics as rdev_mmap() above. */

src/commonlib/region.c

@@ -10,10 +10,10 @@ int region_is_subregion(const struct region *p, const struct region *c)
 	if (region_offset(c) < region_offset(p))
 		return 0;
 
-	if (region_end(c) > region_end(p))
+	if (region_last(c) > region_last(p))
 		return 0;
 
-	if (region_end(c) < region_offset(c))
+	if (region_last(c) < region_offset(c))
 		return 0;
 
 	return 1;

src/cpu/x86/smm/smm_module_loader.c

@@ -341,7 +341,7 @@ static void setup_smihandler_params(struct smm_runtime *mod_params,
 	}
 
 	for (int i = 0; i < loader_params->num_cpus; i++)
-		mod_params->save_state_top[i] = region_end(&cpus[i].ss);
+		mod_params->save_state_top[i] = region_last(&cpus[i].ss) + 1;
 
 	if (CONFIG(RUNTIME_CONFIGURABLE_SMM_LOGLEVEL))
 		mod_params->smm_log_level = mainboard_set_smm_log_level();
@@ -371,7 +371,7 @@ static void setup_smihandler_params(struct smm_runtime *mod_params,
 static void print_region(const char *name, const struct region region)
 {
 	printk(BIOS_DEBUG, "%-12s [0x%zx-0x%zx]\n", name, region_offset(&region),
-	       region_end(&region));
+	       region_last(&region));
 }
 
 /* STM + Handler + (Stub + Save state) * CONFIG_MAX_CPUS + stacks + page tables*/
@@ -482,7 +482,7 @@ int smm_load_module(const uintptr_t smram_base, const size_t smram_size,
 		return -1;
 
 	const struct region smram = region_create(smram_base, smram_size);
-	const uintptr_t smram_top = region_end(&smram);
+	const uintptr_t smram_top = region_last(&smram) + 1;
 
 	const size_t stm_size =
 		CONFIG(STM) ? CONFIG_MSEG_SIZE + CONFIG_BIOS_RESOURCE_LIST_SIZE : 0;

src/drivers/spi/winbond.c

@@ -374,7 +374,7 @@ static int winbond_get_write_protection(const struct spi_flash *flash,
 	}
 
 	printk(BIOS_DEBUG, "WINBOND: flash protected range 0x%08zx-0x%08zx\n",
-	       region_offset(&wp_region), region_end(&wp_region));
+	       region_offset(&wp_region), region_last(&wp_region));
 
 	return region_is_subregion(&wp_region, region);
 }
@@ -502,7 +502,7 @@ winbond_set_write_protection(const struct spi_flash *flash,
 	int ret;
 
 	/* Need to touch TOP or BOTTOM */
-	if (region_offset(region) != 0 && region_end(region) != flash->size)
+	if (region_offset(region) != 0 && region_last(region) != flash->size - 1)
 		return -1;
 
 	params = flash->part;
@@ -600,7 +600,7 @@ winbond_set_write_protection(const struct spi_flash *flash,
 		return ret;
 
 	printk(BIOS_DEBUG, "WINBOND: write-protection set to range "
-	       "0x%08zx-0x%08zx\n", region_offset(region), region_end(region));
+	       "0x%08zx-0x%08zx\n", region_offset(region), region_last(region));
 
 	return ret;
 }

tests/commonlib/region-test.c

@@ -17,7 +17,7 @@ static void test_region(void **state)
 	struct region outer = {.offset = VAL(2), .size = VAL(4)};
 	assert_int_equal(region_offset(&outer), VAL(2));
 	assert_int_equal(region_sz(&outer), VAL(4));
-	assert_int_equal(region_end(&outer), VAL(6));
+	assert_int_equal(region_last(&outer), VAL(6) - 1);
 
 	struct region inner = {.offset = VAL(3), .size = VAL(2)};
 	assert_true(region_is_subregion(&outer, &inner));
@@ -118,7 +118,7 @@ static void test_rdev_basics(void **state)
 {
 	assert_int_equal(region_device_offset(&mock_rdev), 0);
 	assert_int_equal(region_device_sz(&mock_rdev), ~(size_t)0);
-	assert_int_equal(region_device_end(&mock_rdev), ~(size_t)0);
+	assert_int_equal(region_device_last(&mock_rdev), ~(size_t)0 - 1);
 }
 
 /*
@@ -254,7 +254,7 @@ static void test_rdev_chain(void **state)
 	assert_int_equal(rdev_chain(&child, &mock_rdev, child_offs, child_size), 0);
 	assert_int_equal(region_device_sz(&child), child_size);
 	assert_int_equal(region_device_offset(&child), child_offs);
-	assert_int_equal(region_device_end(&child), child_offs + child_size);
+	assert_int_equal(region_device_last(&child), child_offs + child_size - 1);
 	assert_int_equal(rdev_relative_offset(&mock_rdev, &child), child_offs);
 	assert_int_equal(rdev_relative_offset(&child, &mock_rdev), -1);
 

tests/lib/region_file-test.c

@@ -145,12 +145,8 @@ static void test_region_file_init_real_data(void **state)
 static void test_region_file_init_invalid_region_device(void **state)
 {
 	struct region_device bad_dev;
-	struct region_file regf;
 
-	rdev_chain_mem_rw(&bad_dev, NULL, 0);
+	assert_int_equal(rdev_chain_mem_rw(&bad_dev, NULL, 0), -1);
-
-	/* Expect fail when passing invalid region_device. */
-	assert_int_equal(-1, region_file_init(&regf, &bad_dev));
 }
 
 static void test_region_file_data(void **state)

util/cbfstool/cbfstool.c

@@ -423,9 +423,9 @@ static bool create_mmap_windows(void)
 						   &mmap_window_table[j].flash_space)) {
 					ERROR("Flash space windows (base=0x%zx, limit=0x%zx) and (base=0x%zx, limit=0x%zx) overlap!\n",
 					      region_offset(&mmap_window_table[i].flash_space),
-					      region_end(&mmap_window_table[i].flash_space),
+					      region_last(&mmap_window_table[i].flash_space),
 					      region_offset(&mmap_window_table[j].flash_space),
-					      region_end(&mmap_window_table[j].flash_space));
+					      region_last(&mmap_window_table[j].flash_space));
 					return false;
 				}
 
@@ -433,9 +433,9 @@ static bool create_mmap_windows(void)
 						   &mmap_window_table[j].host_space)) {
 					ERROR("Host space windows (base=0x%zx, limit=0x%zx) and (base=0x%zx, limit=0x%zx) overlap!\n",
 					      region_offset(&mmap_window_table[i].flash_space),
-					      region_end(&mmap_window_table[i].flash_space),
+					      region_last(&mmap_window_table[i].flash_space),
 					      region_offset(&mmap_window_table[j].flash_space),
-					      region_end(&mmap_window_table[j].flash_space));
+					      region_last(&mmap_window_table[j].flash_space));
 					return false;
 				}
 			}

util/cbfstool/cse_serger.c

@@ -647,7 +647,7 @@ static size_t get_cse_region_end_offset(void)
 	size_t end_offset;
 
 	for (size_t i = 0; i < BP_TOTAL; i++) {
-		end_offset = region_end(&params.layout_regions[i]);
+		end_offset = region_last(&params.layout_regions[i]) + 1;
 		if (end_offset > offset)
 			offset = end_offset;
 	}