OcAppleKernelLib: alignment-safe chained-fixup walks (follow-up to #603)#606
Open
MattJackson wants to merge 1 commit into
Open
OcAppleKernelLib: alignment-safe chained-fixup walks (follow-up to #603)#606MattJackson wants to merge 1 commit into
MattJackson wants to merge 1 commit into
Conversation
…a#603 follow-up) Address vit9696's post-merge review of acidanthera#603. Three classes of unaligned access existed in the walker: 1. `Starts + SegOffset` cast to `MACH_DYLD_CHAINED_STARTS_IN_SEGMENT *` and dereferenced (`StartsSeg->Size`, `->PageStart[i]`, etc.) on a buffer not guaranteed to be aligned for the struct. 2. `Buffer + SlotOffset` cast to `UINT64 *` and dereferenced (read of the chain-pointer slot) on a slot byte address not guaranteed to be 8-byte-aligned. 3. The same unaligned `UINT64 *` was passed to the visitor callback via the `KC_CHAINED_FIXUP_VISIT` typedef, propagating the UB contract to every future visitor implementation. Resolution: - `KcWalkChainedFixupsInSegment` / `KcWalkChainedFixupsInImage` take `CONST UINT8 *` for metadata buffers. Header is copied into a stack-aligned `MACH_DYLD_CHAINED_STARTS_IN_SEGMENT` local via `CopyMem`; PageStart[i] reads via `ReadUnaligned16`; NumSegments and SegInfoOffset[i] reads via `ReadUnaligned32`. - Chain-pointer slot read: `RawFixup = ReadUnaligned64 (...)`, then `(struct *)&RawFixup` to extract `Next` from a stack-aligned local. - Visitor typedef changed to `IN OUT UINT8 *FixupLoc`. Walker invokes with `Visitor (Buffer + SlotOffset, ...)` — no cast. Visitor authors must use `ReadUnaligned64` / `WriteUnaligned64` to access the slot; the type system enforces this at the API. API impact: the just-merged acidanthera#603 API is breaking-changed source-wise. The only in-tree consumer (`TestProcessKernel`) is updated in this commit. Tests: `--test-fixup-walk` reports all 11 [OK], exit 0: [OK] X86_64_KERNEL_CACHE walk visited 4 fixups (stride 1) [OK] 64_KERNEL_CACHE walk visited 4 fixups (stride 4) [OK] unsupported-format guard returned 0 [OK] PageCount oversize rejected [OK] Size > StartsSegSize rejected [OK] SegmentOffset out-of-buffer rejected [OK] PageStart >= PageSize rejected [OK] long-chain bounded at iteration cap (512) [OK] chain step past page-end rejected after 1 visit [OK] InImage oversize NumSegments rejected [OK] alignment-safe walk at byte offsets 1..7 (4 fixups each) The new test places the chained-starts metadata at every byte offset 1..7 inside a containing buffer and verifies the canonical 4-fixup result for each — exercises every unaligned-read site introduced above on a buffer that is not aligned for any of those types.
ecd090f to
5d3b014
Compare
5 tasks
vit9696
reviewed
May 12, 2026
| // is undefined behaviour when StartsSegBacking isn't aligned for | ||
| // UINT64 (the struct's most-restrictive field is SegmentOffset). | ||
| // | ||
| CopyMem (&StartsSegHeader, StartsSegBacking, StructHeaderSize); |
Contributor
There was a problem hiding this comment.
Ugh, why? You can just check the alignment and abort. If the pointers are not aligned, the binary is malformed anyway. Check how IS_ALIGNED macro is used.
| (*KC_CHAINED_FIXUP_VISIT)( | ||
| IN OUT UINT64 *FixupLoc, | ||
| IN OUT VOID *VisitorContext | ||
| IN OUT UINT8 *FixupLoc, |
Contributor
There was a problem hiding this comment.
Do not change typing just because of the alignment. Check alignment before casting, and then use proper types. This applies throughout PR.
| // | ||
| for (IterCount = 0; IterCount < MaxIters; ++IterCount) { | ||
| Fixup = (MACH_DYLD_CHAINED_PTR_64_KERNEL_CACHE_REBASE *)FixupLoc; | ||
| RawFixup = ReadUnaligned64 ((CONST UINT64 *)(Buffer + SlotOffset)); |
Contributor
There was a problem hiding this comment.
Same as above, though you use ReadUnaligned here and below instead of CopyMem.
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.
Follow-up to #603 addressing the alignment concerns @vit9696 raised
post-merge:
How each concern is resolved
(1)
Starts + SegOffset→MACH_DYLD_CHAINED_STARTS_IN_SEGMENT *The walker API now takes
CONST UINT8 *StartsBacking/CONST UINT8 *StartsSegBackinginstead of struct pointers. There isno struct cast on a potentially-unaligned base anywhere in the call
chain:
KcWalkChainedFixupsInImagereadsNumSegmentsand eachSegInfoOffset[i]viaReadUnaligned32(image-level metadata mayitself be unaligned in the chained-fixups payload).
KcWalkChainedFixupsInSegmentcopies the fixed header into astack-aligned
MACH_DYLD_CHAINED_STARTS_IN_SEGMENTlocal viaCopyMem, then reads all fixed fields through that aligned local.PageStart[i]entries (variable-length tail) are read viaReadUnaligned16.(2)
Buffer + SlotOffset→UINT64 *(two places)First place — internal walker read of the chain pointer:
RawFixup = ReadUnaligned64 ((CONST UINT64 *)(Buffer + SlotOffset)).Fixupthen points to&RawFixup(a stack-aligned UINT64 local), sothe bitfield-struct accesses (
Fixup->Next) deref an aligned pointer.Second place — visitor callback parameter:
KC_CHAINED_FIXUP_VISITtypedef is changed fromIN OUT UINT64 *FixupLoctoIN OUT UINT8 *FixupLoc. The walkerinvokes the visitor with
Buffer + SlotOffsetdirectly — no cast.The type system now commits the unaligned-access contract at the API
boundary so visitor authors are guided to use
ReadUnaligned64/WriteUnaligned64rather than dereferencing asUINT64 *.API changes
KcWalkChainedFixupsInSegmentandKcWalkChainedFixupsInImagetake
CONST UINT8 *for the metadata buffers (was struct pointers).KC_CHAINED_FIXUP_VISITtakesIN OUT UINT8 *FixupLoc(wasUINT64 *).These are breaking source-level changes against the just-merged #603
API. The only in-tree consumer (
TestProcessKernel) is updated inthis PR; there are no external consumers yet.
Tests
TestProcessKernel --test-fixup-walkreports all 11[OK],exit code 0, on a macOS x86_64+arm64 universal binary build:
The new
alignment-safe walk at byte offsets 1..7test places thechained-starts metadata at every byte offset 1..7 inside a containing
buffer and verifies the canonical 4-fixup count for each — exercises
ReadUnaligned32(NumSegments / SegInfoOffset[i]),ReadUnaligned16(PageStart[i]),CopyMem(fixed header), andReadUnaligned64(chain-pointer slot) on a buffer that isdeliberately not aligned for any of those types.