Werner Zeh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/79977?usp=email )
Change subject: tree: Refactor region_end() to return last used address of a region ......................................................................
tree: Refactor region_end() to return last used address of a region
Currently, function region_end() does return the next address after the end of a given region. This is not only confusing (as the next address after a region is not part of this region anymore) but can lead to integer overflow if the region's end reaches SIZE_MAX. This possible overflow can then lead to not catching a region overlap as reported in a bug in [1].
This patch changes now the behavior of region_end() and makes it return the last valid address of a region. In addition, the integer overflow is checked and if it would happen, SIZE_MAX is returned (which truncates the region's end to SIZE_MAX for this corner case).
And since region_end() now returns a different address, some unit-tests need adjustments.
[1]: https://ticket.coreboot.org/issues/522
Change-Id: Ide7a39af1609fed63d08832a6dcc4679a85c5027 Signed-off-by: Werner Zeh werner.zeh@siemens.com --- 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 src/include/cpu/x86/smm.h M tests/commonlib/region-test.c M util/cbfstool/cse_serger.c 7 files changed, 33 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/79977/1
diff --git a/src/commonlib/include/commonlib/region.h b/src/commonlib/include/commonlib/region.h index 25efcc8..af76026 100644 --- a/src/commonlib/include/commonlib/region.h +++ b/src/commonlib/include/commonlib/region.h @@ -114,13 +114,18 @@
static inline size_t region_end(const struct region *r) { - return region_offset(r) + region_sz(r); + if (region_sz(r) == 0) + return region_offset(r); + else if (((size_t)SIZE_MAX - region_sz(r)) < region_offset(r)) + return (size_t)SIZE_MAX; + else + 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_end(r1) >= region_offset(r2)) && + (region_offset(r1) <= region_end(r2)); }
static inline const struct region *region_device_region( diff --git a/src/commonlib/region.c b/src/commonlib/region.c index 4153f0a..3737234 100644 --- a/src/commonlib/region.c +++ b/src/commonlib/region.c @@ -7,6 +7,9 @@
int region_is_subregion(const struct region *p, const struct region *c) { + if ((region_sz(p) == 0) || (region_sz(c) == 0)) + return 0; + if (region_offset(c) < region_offset(p)) return 0;
@@ -142,8 +145,12 @@ .size = size, };
- if (!normalize_and_ok(&parent->region, &req)) + if (!normalize_and_ok(&parent->region, &req)) { + /* Make sure there are no random values in child's region. */ + child->region.offset = 0; + child->region.size = 0; return -1; + }
/* Keep track of root region device. Note the offsets are relative * to the root device. */ diff --git a/src/cpu/x86/smm/smm_module_loader.c b/src/cpu/x86/smm/smm_module_loader.c index d965593..262bb7b 100644 --- a/src/cpu/x86/smm/smm_module_loader.c +++ b/src/cpu/x86/smm/smm_module_loader.c @@ -335,8 +335,12 @@ mod_params->cbmemc_size = 0; }
- for (int i = 0; i < loader_params->num_cpus; i++) - mod_params->save_state_top[i] = region_end(&cpus[i].ss); + for (int i = 0; i < loader_params->num_cpus; i++) { + mod_params->save_state_top[i] = region_end(&cpus[i].ss) + 1; + if (mod_params->save_state_top[i] < SIZE_MAX) { + mod_params->save_state_top[i]++; + } + }
if (CONFIG(RUNTIME_CONFIGURABLE_SMM_LOGLEVEL)) mod_params->smm_log_level = mainboard_set_smm_log_level(); @@ -426,7 +430,8 @@ return -1;
const struct region smram = { .offset = smram_base, .size = smram_size }; - const uintptr_t smram_top = region_end(&smram); + const uintptr_t smram_top = region_end(&smram) < SIZE_MAX ? + region_end(&smram) + 1 : region_end(&smram);
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 3137bfe..85b3f71 100644 --- a/src/drivers/spi/winbond.c +++ b/src/drivers/spi/winbond.c @@ -503,7 +503,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_end(region) != flash->size - 1) return -1;
params = flash->part; diff --git a/src/include/cpu/x86/smm.h b/src/include/cpu/x86/smm.h index dfb27cd..2d33205 100644 --- a/src/include/cpu/x86/smm.h +++ b/src/include/cpu/x86/smm.h @@ -139,7 +139,10 @@ { const struct region r = {(uintptr_t)ptr, len};
- return smm_region_overlaps_handler(&r); + if (region_sz(r) == 0) + return false; + else + return smm_region_overlaps_handler(&r); }
/* SMM Module Loading API */ diff --git a/tests/commonlib/region-test.c b/tests/commonlib/region-test.c index 3280482..528a593 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_end(&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_end(&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_end(&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/util/cbfstool/cse_serger.c b/util/cbfstool/cse_serger.c index f53b390..b36f780 100644 --- a/util/cbfstool/cse_serger.c +++ b/util/cbfstool/cse_serger.c @@ -689,7 +689,7 @@
static int cmd_create_cse_region(void) { - size_t file_size = get_cse_region_end_offset(); + size_t file_size = get_cse_region_end_offset() + 1; struct buffer cse_buff, layout_buff;
if (fill_layout_buffer(&layout_buff))