Werner Zeh has uploaded this change for review.
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))
To view, visit change 79977. To unsubscribe, or for help writing mail filters, visit settings.