fix(ecc): reject wrapped memory ranges - #75
Conversation
srpatcha
left a comment
There was a problem hiding this comment.
Approving. The evidence row in this PR claims the tests fail against the old
implementation; I checked that rather than taking it, and it holds — for both
assertions independently.
Verified
Applied on current master:
0 build errors
100% tests passed, 0 tests failed out of 19
ECC range validation tests passed
Then reverted each fix separately.
The range check. Restoring addr + len > base + size:
[FAIL] test_ecc.c:44: eos_ecc_check_region(&ctx, UINT32_MAX, 2) == -1
The init guard. Removing if (size > UINT32_MAX - base) return -1;:
[FAIL] test_ecc.c:33: eos_ecc_init(&ctx, UINT32_MAX - 0xFFu, 0x100u) == -1
Two fixes, two assertions, each failing on its own when its fix is removed. That
is the property that makes a regression test worth having, and a surprising
number of them do not manage it — one earlier in this organisation asserted on
values that were correct both before and after the bug it claimed to cover, and
another passed against unfixed code because it used a message that happened not
to trigger the defect.
The rewrite is correct
if (addr < ctx->base_addr) return -1;
uint32_t offset = addr - ctx->base_addr;
if (offset > ctx->size_bytes || len > ctx->size_bytes - offset) return -1;addr < base_addr first, so the subtraction cannot wrap; offset > size_bytes
before the second subtraction, same reason. Both are ordered so the guard
precedes the arithmetic that depends on it. The old addr + len > base + size
wrapped on both sides at once, which is why an address at UINT32_MAX slipped
through.
Rejecting a wrapped range at eos_ecc_init() is the better half of the fix.
Validating every request against a range that is itself nonsensical is a
losing position; refusing to configure it means the check downstream has
something coherent to compare against.
Same unsigned-overflow family as eos#95 (p + len > cap), eos#87
(bytes_written + len) and eBoot#76 (slot bounds). Four instances now. The safe
form is always a > limit - b with the precondition checked first, exactly as
written here.
Two details I appreciated
Using zero-length requests to exercise the boundaries, so the test never
dereferences a synthetic 32-bit address on the host — the comment says so, which
saves the next reader working out why the cases look odd.
The target is named eboot_test_ecc. #71 namespaced test targets so eos and
eBoot can be composed into one CMake project by ebuild, and there is a
configure-time guard that rejects an unprefixed one. Getting that right without
being asked means the guard never had to fire.
README test count updated 17 → 19 to match. Verified: 19 is what ctest reports.
Summary
ECC range validation used
addr + lenandbase + sizein 32-bit arithmetic.Wrapped values could therefore pass both duplicated checks, and initialization
also accepted a configured region whose exclusive end was unrepresentable.
This change validates both ranges with subtraction after ordered lower-bound
checks, removing the overflow paths before any address is converted to a
pointer.
Type of Change
Changes
base/sizepairs duringeos_ecc_init().remaining-capacity comparisons.
test_ecctarget covering null contexts, wrapped inputs,exact boundaries, and ordinary out-of-range requests.
Testing
1/1 passed19/19 passed13 passed, 1 skippedagainst the old implementation
Pre-Submission Checklist
<type>(<scope>): <description>conventionmasterRelated Issues
None.
Screenshots / Logs
Not applicable.
Additional Notes
The host regression uses zero-length or sub-word requests for synthetic 32-bit
addresses, so it exercises validation boundaries without dereferencing those
addresses. ECC access width and scrub scheduling are unchanged.