Attention is currently required from: Jakub Czapiga, Jérémy Compostella.
Nico Huber has uploaded this change for review.
[RFC] 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.
Change-Id: Ic4bd6eced638745b7e845504da74542e4220554a
Signed-off-by: Nico Huber <nico.h@gmx.de>
---
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, 26 insertions(+), 38 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/79946/1
diff --git a/src/commonlib/include/commonlib/region.h b/src/commonlib/include/commonlib/region.h
index 8de3c5d..cf31f8e 100644
--- a/src/commonlib/include/commonlib/region.h
+++ b/src/commonlib/include/commonlib/region.h
@@ -122,15 +122,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. */
@@ -154,9 +154,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 9dc6637..1a4a236 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(p))
return 0;
return 1;
@@ -22,7 +22,7 @@
static int normalize_and_ok(const struct region *outer, struct region *inner)
{
inner->offset += region_offset(outer);
- return region_end(inner) - 1 >= region_offset(inner) &&
+ return region_last(inner) >= region_offset(inner) &&
region_is_subregion(outer, inner);
}
diff --git a/src/cpu/x86/smm/smm_module_loader.c b/src/cpu/x86/smm/smm_module_loader.c
index 39b36e61..1f1127d 100644
--- a/src/cpu/x86/smm/smm_module_loader.c
+++ b/src/cpu/x86/smm/smm_module_loader.c
@@ -335,7 +335,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();
@@ -349,7 +349,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 */
@@ -425,7 +425,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 3137bfe..0f8c170 100644
--- a/src/drivers/spi/winbond.c
+++ b/src/drivers/spi/winbond.c
@@ -375,7 +375,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);
}
@@ -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_last(region) + 1 != flash->size)
return -1;
params = flash->part;
@@ -601,7 +601,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..6a7ce9e 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) + 1, VAL(6));
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);
@@ -334,7 +334,9 @@
u8 scratch[size];
int i;
struct region_device mem;
- rdev_chain_mem_rw(&mem, backing, size);
+
+ assert_true((uintptr_t)backing <= SIZE_MAX);
+ assert_int_equal(rdev_chain_mem_rw(&mem, backing, size), 0);
/* Test writing to and reading from full mapping. */
memset(backing, 0xa5, size);
diff --git a/tests/lib/region_file-test.c b/tests/lib/region_file-test.c
index 56bb8e4f..fee3b84 100644
--- a/tests/lib/region_file-test.c
+++ b/tests/lib/region_file-test.c
@@ -142,17 +142,6 @@
assert_int_equal(3, regf.slot);
}
-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));
-}
-
static void test_region_file_data(void **state)
{
/* region_device with empty data buffer */
@@ -310,9 +299,6 @@
cmocka_unit_test_setup_teardown(test_region_file_init_real_data,
setup_teardown_region_file_test,
setup_teardown_region_file_test),
- cmocka_unit_test_setup_teardown(test_region_file_init_invalid_region_device,
- setup_teardown_region_file_test,
- setup_teardown_region_file_test),
cmocka_unit_test_setup_teardown(test_region_file_data,
setup_teardown_region_file_test,
setup_teardown_region_file_test),
diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c
index 10d6501..0bb6e5d 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 9b776e6..d16e1af 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;
}
To view, visit change 79946. To unsubscribe, or for help writing mail filters, visit settings.