Conversation
The FreeMemoryOffset and per-file FileHeaderOffset fields are loaded from EEPROM (untrusted on a corrupted, tampered, or single-event- upset-flipped image). The previous code added them directly to BaseAddress with no sanity check; on 32-bit flight CPUs that can wrap around 2^32 and the resulting pointer ends up scribbling on arbitrary memory on the next write through FreeMemoryPointer or FileHeaderPointer. The validators check Magic, Version, and NumberOfFiles but leave the offset fields uninspected. This commit adds a wrap-check on each addition by computing the sum in uintptr_t and comparing against the base address; if the sum is below the base the offset wrapped and we reject the EEPROM with EEFS_NO_SUCH_DEVICE, same status the existing 'invalid file allocation table' path returns.
scj-hunt's per-target eefs catalog flagged this function at score 7.4 — same bug class as EEFS_LibInitFS (already fixed earlier in this PR): pointer arithmetic on BaseAddress + EEFS-loaded uint32 offset with no wrap check. MicroEEFS is the lighter-weight variant used in bootstrap code, so this lives on the same flight CPU and shares the corrupted- EEPROM threat model. Applied the identical uintptr_t-cast + 'below base' wrap-check pattern. The two EEFS_LIB_EEPROM_READ calls for FileHeader are merged into one guarded block since they use the same computed offset.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
EEFS_LibInitFS()adds the EEPROM-loadedFreeMemoryOffset/FileHeaderOffsettoBaseAddresswith no sanity check. On a 32-bit flight CPU the unsigned addition wraps when the offset is high-bits set (0xFFFFFFFFfrom a corrupted EEPROM image or SEU flip), and subsequent writes throughFreeMemoryPointer/FileHeaderPointerscribble on arbitrary memory.Closes #10.
Fix
For each pointer-arithmetic, compute the sum in
uintptr_tand compare against the base; if the sum is less than the base, the unsigned addition wrapped and we reject the EEPROM asEEFS_NO_SUCH_DEVICE— the same status the existing "invalid file allocation table" path returns.Diff
Tests
FreeMemoryOffset = 0xFFFFFFFF, mounted via the standalone driver on a 32-bit build; before the fix this corrupted heap on nextEEFS_Create; after the fix the mount fails cleanly withEEFS_NO_SUCH_DEVICE.FileHeaderOffseton a per-file entry.Scope
The same shape exists in
tools/eefstool/src/eefs_fileapi.c(the eefstool variant). Same fix applies; I left it out of this PR to keep the review small, happy to extend if you want.