fix(storage): make flash area bounds checks overflow-safe
What the developer wrote
Authored by tychovrahe
fix(storage): make flash area bounds checks overflow-safe
Both bounds checks added an offset to a size in `uint32_t` before comparing
against the limit:
flash_area_get_address(): offset + size <= subarea_size
flash_area_write_data_padded(): offset + total_size > flash_area_get_size()
A sufficiently large second operand wraps the sum to a small value, so the
comparison passes and the function reports the range as fitting. Rewrite both
as subtractions, which cannot wrap. In `flash_area_get_address()` the
subtraction cannot underflow either, since the enclosing condition has already
established that the offset is below the sub-area size.
Neither check is reachable with such arguments today. The address lookup is
gated by `offset < subarea_size` first, so wrapping needs an almost 4 GB size
rather than a large offset, and no caller passes one - `secret_read()` is not
exposed as a syscall and every one of its call sites uses a constant or a
range-checked length. The padded write is backed by `get_sector_and_offset()`,
which re-derives the sector for every block written and fails for an offset
past the area. Both checks are nonetheless the documented bound that a future
caller would reasonably trust.
[no changelog]
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
What changed, and why it matters
This commit fixes two overflow-prone safety checks in the Trezor firmware's flash storage code. The original checks added numbers together before comparing them to a limit; with specially crafted large values that addition could silently wrap around to a small number, making an out-of-bounds access look valid. The patch rewrites the checks using subtraction, which cannot wrap in the same way. The commit message says these particular checks are not reachable from current callers, so the immediate risk is low, but the change hardens the code against future misuse.
Treat as a low-risk defensive hardening patch. No urgent response is required because the commit message states the vulnerable arguments are unreachable from current callers. Include the fix in the next firmware release and consider adding static-analysis rules or tests to catch similar overflow-prone bounds checks elsewhere in storage code.
Security signals we found
Integer overflow in bounds check (CWE-190/CWE-680)
Potential out-of-bounds flash read/write if check bypassed
Defensive hardening of documented API boundary
No changelog entry despite security-relevant hardening
Evidence from the diff
In storage/flash_area.c, two bounds checks used offset + size <= limit style comparisons in uint32_t arithmetic, which is vulnerable to integer overflow/wraparound. A large size can make offset + size wrap to a small value and pass the check. The patch converts flash_area_get_address() to size <= subarea_size - offset (safe because offset < subarea_size is already verified) and flash_area_write_data_padded() to offset > area_size || total_size > area_size - offset. These are classic overflow-safe formulations. The commit message explicitly states the vulnerable paths are not currently reachable from any caller, but the checks are the documented API boundary.
Changed components
storage/flash_area.cflash_area_get_address()flash_area_write_data_padded()Inspect captured patch +7 / −3
### storage/flash_area.c
@@ -70,8 +70,10 @@ const void *flash_area_get_address(const flash_area_t *area, uint32_t offset,
uint32_t subarea_size = flash_sector_size(first_sector, num_sectors);
// Does the requested block start in the sub-area?
if (offset < subarea_size) {
- // Does the requested block fit in the sub-area?
- if (offset + size <= subarea_size) {
+ // Does the requested block fit in the sub-area? Written as a subtraction
+ // so that a large `size` cannot wrap the sum past the limit. The
+ // subtraction cannot underflow, `offset` is below `subarea_size` here.
+ if (size <= subarea_size - offset) {
const uint8_t *ptr =
(const uint8_t *)flash_get_address(first_sector, 0, 0);
// We expect that all sectors/pages in the sub-area make
@@ -155,7 +157,9 @@ secbool __wur flash_area_write_data_padded(const flash_area_t *area,
if (data_size > total_size) {
return secfalse;
}
- if (offset + total_size > flash_area_get_size(area)) {
+ // Written as a subtraction so that the sum cannot wrap past the area size
+ const uint32_t area_size = flash_area_get_size(area);
+ if (offset > area_size || total_size > area_size - offset) {
return secfalse;
}
Why this scored 39/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.