Attention is currently required from: Aarya.
Peter Marheine has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/82496?usp=email )
Change subject: Correct get_flash_region() to use inclusive upper bounds ......................................................................
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.
Change-Id: Ia0ca867aef33b598c2697fc264807fa5e29683ec --- M erasure_layout.c M flashrom.c M ichspi.c 3 files changed, 20 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/96/82496/1
diff --git a/erasure_layout.c b/erasure_layout.c index 1dd6b60..ef7987a 100644 --- a/erasure_layout.c +++ b/erasure_layout.c @@ -316,13 +316,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; @@ -331,7 +331,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..a8ad3f0 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,7 +1511,7 @@ 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", diff --git a/ichspi.c b/ichspi.c index bee5ec9..249e966 100644 --- a/ichspi.c +++ b/ichspi.c @@ -1451,12 +1451,12 @@ * fd_regions[i] ends before addr, constrain * region->start so that it does not overlap. */ - region->start = max(region->start, limit + 1); + region->start = max(region->start, limit); } else { /* 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;
-
Peter Marheine (Code Review)