Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/79946?usp=email )
Change subject: region: Turn region_end() into an inclusive region_last() ......................................................................
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 --- M src/commonlib/include/commonlib/region.h M src/commonlib/region.c M src/cpu/x86/smm/smm_module_loader.c M src/drivers/spi/winbond.c M tests/commonlib/region-test.c M tests/lib/region_file-test.c M util/cbfstool/cbfstool.c M util/cbfstool/cse_serger.c 8 files changed, 23 insertions(+), 27 deletions(-)
Approvals: Julius Werner: Looks good to me, approved build bot (Jenkins): Verified
diff --git a/src/commonlib/include/commonlib/region.h b/src/commonlib/include/commonlib/region.h index 8471797..e066ea0 100644 --- a/src/commonlib/include/commonlib/region.h +++ b/src/commonlib/include/commonlib/region.h @@ -124,15 +124,15 @@ 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)) && - (region_offset(r1) < region_end(r2)); + return (region_last(r1) >= region_offset(r2)) && + (region_offset(r1) <= region_last(r2)); }
/* Helper to dynamically initialize region device. */ @@ -156,9 +156,9 @@ 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. */ diff --git a/src/commonlib/region.c b/src/commonlib/region.c index 4153f0a..fbc6276 100644 --- a/src/commonlib/region.c +++ b/src/commonlib/region.c @@ -10,10 +10,10 @@ 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; diff --git a/src/cpu/x86/smm/smm_module_loader.c b/src/cpu/x86/smm/smm_module_loader.c index 979fa48..c6d0753 100644 --- a/src/cpu/x86/smm/smm_module_loader.c +++ b/src/cpu/x86/smm/smm_module_loader.c @@ -341,7 +341,7 @@ }
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 print_region(const char *name, const struct region region) { printk(BIOS_DEBUG, "%-12s [0x%zx-0x%zx]\n", name, region_offset(®ion), - region_end(®ion)); + region_last(®ion)); }
/* STM + Handler + (Stub + Save state) * CONFIG_MAX_CPUS + stacks + page tables*/ @@ -482,7 +482,7 @@ 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; diff --git a/src/drivers/spi/winbond.c b/src/drivers/spi/winbond.c index 3426d08..db93ff6 100644 --- a/src/drivers/spi/winbond.c +++ b/src/drivers/spi/winbond.c @@ -374,7 +374,7 @@ }
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 @@ 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 @@ 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; } diff --git a/tests/commonlib/region-test.c b/tests/commonlib/region-test.c index 3280482..6caa12c 100644 --- a/tests/commonlib/region-test.c +++ b/tests/commonlib/region-test.c @@ -17,7 +17,7 @@ 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 @@ { 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 @@ 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);
diff --git a/tests/lib/region_file-test.c b/tests/lib/region_file-test.c index 56bb8e4f..7607aaf 100644 --- a/tests/lib/region_file-test.c +++ b/tests/lib/region_file-test.c @@ -145,12 +145,8 @@ 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); - - /* Expect fail when passing invalid region_device. */ - assert_int_equal(-1, region_file_init(®f, &bad_dev)); + assert_int_equal(rdev_chain_mem_rw(&bad_dev, NULL, 0), -1); }
static void test_region_file_data(void **state) diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c index cc8dbb5..25f4847 100644 --- a/util/cbfstool/cbfstool.c +++ b/util/cbfstool/cbfstool.c @@ -423,9 +423,9 @@ &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 @@ &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; } } diff --git a/util/cbfstool/cse_serger.c b/util/cbfstool/cse_serger.c index 006a01e..9d74a12 100644 --- a/util/cbfstool/cse_serger.c +++ b/util/cbfstool/cse_serger.c @@ -647,7 +647,7 @@ size_t end_offset;
for (size_t i = 0; i < BP_TOTAL; i++) { - end_offset = region_end(¶ms.layout_regions[i]); + end_offset = region_last(¶ms.layout_regions[i]) + 1; if (end_offset > offset) offset = end_offset; }