Peter Marheine submitted this change.
Correct get_flash_region() to use inclusive upper bounds
get_flash_region() emits a struct flash_region, which uses chipoff_t for
the start and end addresses of a region. chipoff_t is defined as a valid
flash address, so it was wrong to be setting the end address to start +
len; this is clearly wrong in the case where there is a single region
because setting end to the flash size generates an address that is
beyond the end of the chip (due to zero-indexing).
This changes the one actual implementation of .get_region in ichspi.c to
use inclusive upper bounds, and corrects all callers of
get_flash_region() to treat the upper bounds as inclusive. Overall this
reduces complexity slightly by removing more downward adjustments by 1
than it needs to add upward adjustments.
TEST=on yaviks, `flashrom -V -x` prints equivalent messages about
"x region (0xZZZZ..0xZZZZ) is readable" before and after this
patch.
Change-Id: Ia0ca867aef33b598c2697fc264807fa5e29683ec
Signed-off-by: Peter Marheine <pmarheine@chromium.org>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/82496
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Hsuan-ting Chen <roccochen@google.com>
Reviewed-by: Anastasia Klimchuk <aklm@chromium.org>
---
M erasure_layout.c
M flashrom.c
M ichspi.c
3 files changed, 22 insertions(+), 22 deletions(-)
diff --git a/erasure_layout.c b/erasure_layout.c
index e9bfa86..d15594d 100644
--- a/erasure_layout.c
+++ b/erasure_layout.c
@@ -318,13 +318,13 @@
struct flash_region region;
get_flash_region(flashctx, addr, ®ion);
- len = min(start_addr + block_len, region.end) - addr;
+ len = min(start_addr + block_len, region.end + 1) - addr;
if (region.write_prot) {
msg_gdbg("%s: cannot erase inside %s "
"region (%#08"PRIx32"..%#08"PRIx32"), skipping range (%#08x..%#08x).\n",
__func__, region.name,
- region.start, region.end - 1,
+ region.start, region.end,
addr, addr + len - 1);
free(region.name);
continue;
@@ -333,7 +333,7 @@
msg_gdbg("%s: %s region (%#08"PRIx32"..%#08"PRIx32") is "
"writable, erasing range (%#08x..%#08x).\n",
__func__, region.name,
- region.start, region.end - 1,
+ region.start, region.end,
addr, addr + len - 1);
free(region.name);
diff --git a/flashrom.c b/flashrom.c
index a056929..dbf06cf 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -379,7 +379,7 @@
} else {
region->name = strdup("");
region->start = 0;
- region->end = flashrom_flash_getsize(flash);
+ region->end = flashrom_flash_getsize(flash) - 1;
region->read_prot = false;
region->write_prot = false;
}
@@ -388,12 +388,12 @@
int check_for_unwritable_regions(const struct flashctx *flash, unsigned int start, unsigned int len)
{
struct flash_region region;
- for (unsigned int addr = start; addr < start + len; addr = region.end) {
+ for (unsigned int addr = start; addr < start + len; addr = region.end + 1) {
get_flash_region(flash, addr, ®ion);
if (region.write_prot) {
msg_gerr("%s: cannot write/erase inside %s region (%#08"PRIx32"..%#08"PRIx32").\n",
- __func__, region.name, region.start, region.end - 1);
+ __func__, region.name, region.start, region.end);
free(region.name);
return -1;
}
@@ -596,14 +596,14 @@
struct flash_region region;
get_flash_region(flash, addr, ®ion);
- read_len = min(start + len, region.end) - addr;
+ read_len = min(start + len, region.end + 1) - addr;
uint8_t *rbuf = buf + addr - start;
if (region.read_prot) {
if (flash->flags.skip_unreadable_regions) {
msg_gdbg("%s: cannot read inside %s region (%#08"PRIx32"..%#08"PRIx32"), "
"filling (%#08x..%#08x) with erased value instead.\n",
- __func__, region.name, region.start, region.end - 1,
+ __func__, region.name, region.start, region.end,
addr, addr + read_len - 1);
free(region.name);
@@ -612,12 +612,12 @@
}
msg_gerr("%s: cannot read inside %s region (%#08"PRIx32"..%#08"PRIx32").\n",
- __func__, region.name, region.start, region.end - 1);
+ __func__, region.name, region.start, region.end);
free(region.name);
return -1;
}
msg_gdbg("%s: %s region (%#08"PRIx32"..%#08"PRIx32") is readable, reading range (%#08x..%#08x).\n",
- __func__, region.name, region.start, region.end - 1, addr, addr + read_len - 1);
+ __func__, region.name, region.start, region.end, addr, addr + read_len - 1);
free(region.name);
read_func_t *read_func = lookup_read_func_ptr(flash->chip);
@@ -665,25 +665,25 @@
for (size_t addr = start; addr < start + len; addr += read_len) {
struct flash_region region;
get_flash_region(flash, addr, ®ion);
- read_len = min(start + len, region.end) - addr;
+ read_len = min(start + len, region.end + 1) - addr;
if ((region.write_prot && flash->flags.skip_unwritable_regions) ||
(region.read_prot && flash->flags.skip_unreadable_regions)) {
msg_gdbg("%s: Skipping verification of %s region (%#08"PRIx32"..%#08"PRIx32")\n",
- __func__, region.name, region.start, region.end - 1);
+ __func__, region.name, region.start, region.end);
free(region.name);
continue;
}
if (region.read_prot) {
msg_gerr("%s: Verification imposible because %s region (%#08"PRIx32"..%#08"PRIx32") is unreadable.\n",
- __func__, region.name, region.start, region.end - 1);
+ __func__, region.name, region.start, region.end);
free(region.name);
goto out_free;
}
msg_gdbg("%s: Verifying %s region (%#08"PRIx32"..%#08"PRIx32")\n",
- __func__, region.name, region.start, region.end - 1);
+ __func__, region.name, region.start, region.end);
free(region.name);
ret = read_flash(flash, readbuf, addr, read_len);
@@ -1046,18 +1046,18 @@
struct flash_region region;
get_flash_region(flash, addr, ®ion);
- write_len = min(start + len, region.end) - addr;
+ write_len = min(start + len, region.end + 1) - addr;
const uint8_t *rbuf = buf + addr - start;
if (region.write_prot) {
msg_gdbg("%s: cannot write inside %s region (%#08"PRIx32"..%#08"PRIx32"), skipping (%#08x..%#08x).\n",
- __func__, region.name, region.start, region.end - 1, addr, addr + write_len - 1);
+ __func__, region.name, region.start, region.end, addr, addr + write_len - 1);
free(region.name);
continue;
}
msg_gdbg("%s: %s region (%#08"PRIx32"..%#08"PRIx32") is writable, writing range (%#08x..%#08x).\n",
- __func__, region.name, region.start, region.end - 1, addr, addr + write_len - 1);
+ __func__, region.name, region.start, region.end, addr, addr + write_len - 1);
write_func_t *write_func = lookup_write_func_ptr(flash->chip);
int ret = write_func(flash, rbuf, addr, write_len);
@@ -1511,17 +1511,17 @@
struct flash_region region;
get_flash_region(flashctx, addr, ®ion);
- len = min(info->erase_start + erase_len, region.end) - addr;
+ len = min(info->erase_start + erase_len, region.end + 1) - addr;
if (region.write_prot) {
msg_gdbg("%s: cannot erase inside %s region (%#08"PRIx32"..%#08"PRIx32"), skipping range (%#08x..%#08x).\n",
- __func__, region.name, region.start, region.end - 1, addr, addr + len - 1);
+ __func__, region.name, region.start, region.end, addr, addr + len - 1);
free(region.name);
continue;
}
msg_gdbg("%s: %s region (%#08"PRIx32"..%#08"PRIx32") is writable, erasing range (%#08x..%#08x).\n",
- __func__, region.name, region.start, region.end - 1, addr, addr + len - 1);
+ __func__, region.name, region.start, region.end, addr, addr + len - 1);
free(region.name);
if (erasefn(flashctx, addr, len))
diff --git a/ichspi.c b/ichspi.c
index bef43b9..cc56e0c 100644
--- a/ichspi.c
+++ b/ichspi.c
@@ -1436,7 +1436,7 @@
region->read_prot = false;
region->write_prot = false;
region->start = 0;
- region->end = flashrom_flash_getsize(flash);
+ region->end = flashrom_flash_getsize(flash) - 1;
for (ssize_t i = 0; i < nr; i++) {
uint32_t base = fd_regions[i].base;
@@ -1459,7 +1459,7 @@
/* fd_regions[i] contains addr, copy to *region. */
name = fd_regions[i].name;
region->start = base;
- region->end = limit + 1;
+ region->end = limit;
region->read_prot = (level == LOCKED) || (level == READ_PROT);
region->write_prot = (level == LOCKED) || (level == WRITE_PROT);
break;
To view, visit change 82496. To unsubscribe, or for help writing mail filters, visit settings.