diff --git a/Include/Acidanthera/Library/OcAppleKernelLib.h b/Include/Acidanthera/Library/OcAppleKernelLib.h index e097f7f3f24..0c2e04281ff 100644 --- a/Include/Acidanthera/Library/OcAppleKernelLib.h +++ b/Include/Acidanthera/Library/OcAppleKernelLib.h @@ -1057,14 +1057,25 @@ KcFixupValue ( rewrite the slot in place (e.g. to translate a fileset VA into a kernel VA) or just observe it (e.g. for counting / validation). - @param[in,out] FixupLoc Pointer to the 8-byte fixup slot. + The fixup slot's containing buffer is not guaranteed to be 8-byte + aligned, so FixupLoc is typed UINT8 * to make the unaligned-access + contract explicit at the API boundary. Read or modify the 8-byte slot + via ReadUnaligned64 / WriteUnaligned64; treating FixupLoc as a UINT64 + pointer and dereferencing directly is undefined behaviour per the C + standard and may misbehave on strict-alignment hosts (some ARM + configurations) and under sanitizers (UBSan). + + @param[in,out] FixupLoc Byte address of the 8-byte fixup slot. + Access via ReadUnaligned64 / + WriteUnaligned64; do not cast and + dereference as UINT64 *. @param[in,out] VisitorContext Caller-provided context, may be NULL. **/ typedef VOID (*KC_CHAINED_FIXUP_VISIT)( - IN OUT UINT64 *FixupLoc, - IN OUT VOID *VisitorContext + IN OUT UINT8 *FixupLoc, + IN OUT VOID *VisitorContext ); /** @@ -1075,28 +1086,37 @@ VOID ARM64E_KERNEL) layouts; returns 0 for any other format without walking. - All fields read from StartsSeg are treated as untrusted input. Every - computed offset and chain step is bounds-checked against the - caller-supplied container sizes. The walker never reads or invokes - Visitor on a slot that lies outside [Buffer, Buffer + BufferSize). - A self-referencing chain is bounded by the per-page iteration cap - PageSize / sizeof (UINT64). - - @param[in] Buffer Pointer to the containing kernel - collection buffer (the same base - SegmentOffset is relative to). - @param[in] BufferSize Size of Buffer in bytes. The walker - will not access Buffer past this. - @param[in] StartsSeg MACH_DYLD_CHAINED_STARTS_IN_SEGMENT for - the segment to walk. - @param[in] StartsSegSize Size of the metadata region pointed to - by StartsSeg. The walker will not read - the StartsSeg struct or its - PageStart[] array past this. - @param[in] Visitor Callback invoked per fixup slot, or - NULL to count only. - @param[in,out] VisitorContext Opaque context forwarded to Visitor, - may be NULL. + All fields read from the per-segment metadata are treated as + untrusted input. Every computed offset and chain step is + bounds-checked against the caller-supplied container sizes. The + walker never reads or invokes Visitor on a slot that lies outside + [Buffer, Buffer + BufferSize). A self-referencing chain is bounded + by the per-page iteration cap PageSize / sizeof (UINT64). + + StartsSegBacking is not assumed to be aligned for any type. The + fixed header is read into a local properly-aligned struct via + CopyMem, and PageStart[] entries + fixup slots in Buffer are + accessed via ReadUnaligned* helpers. Buffer itself is treated as + potentially unaligned for UINT64 access at any SlotOffset. + + @param[in] Buffer Pointer to the containing kernel + collection buffer (the same base + SegmentOffset is relative to). + @param[in] BufferSize Size of Buffer in bytes. The walker + will not access Buffer past this. + @param[in] StartsSegBacking Byte pointer to the per-segment + MACH_DYLD_CHAINED_STARTS_IN_SEGMENT + metadata. Need not be aligned for + the struct; reads are unaligned-safe + and the walker never writes through + it (CONST). + @param[in] StartsSegSize Size of the metadata region pointed + to by StartsSegBacking. The walker + will not read the metadata past this. + @param[in] Visitor Callback invoked per fixup slot, or + NULL to count only. + @param[in,out] VisitorContext Opaque context forwarded to Visitor, + may be NULL. @return Count of fixup slots visited. 0 if any size precondition fails, the pointer format is unsupported, or the structure @@ -1104,12 +1124,12 @@ VOID **/ UINTN KcWalkChainedFixupsInSegment ( - IN UINT8 *Buffer, - IN UINTN BufferSize, - IN MACH_DYLD_CHAINED_STARTS_IN_SEGMENT *StartsSeg, - IN UINTN StartsSegSize, - IN KC_CHAINED_FIXUP_VISIT Visitor OPTIONAL, - IN OUT VOID *VisitorContext OPTIONAL + IN UINT8 *Buffer, + IN UINTN BufferSize, + IN CONST UINT8 *StartsSegBacking, + IN UINTN StartsSegSize, + IN KC_CHAINED_FIXUP_VISIT Visitor OPTIONAL, + IN OUT VOID *VisitorContext OPTIONAL ); /** @@ -1122,16 +1142,25 @@ KcWalkChainedFixupsInSegment ( treated as untrusted input and bounds-checked against StartsSize. Per-segment validation is performed by KcWalkChainedFixupsInSegment. + StartsBacking is not assumed to be aligned. NumSegments and each + SegInfoOffset[i] are read via ReadUnaligned32, so a chained-fixups + payload that starts at any byte alignment in a containing buffer + is handled safely. + @param[in] Buffer Pointer to the containing kernel collection buffer. @param[in] BufferSize Size of Buffer in bytes. Forwarded to KcWalkChainedFixupsInSegment. - @param[in] Starts MACH_DYLD_CHAINED_STARTS_IN_IMAGE for - the whole image. + @param[in] StartsBacking Byte pointer to the + MACH_DYLD_CHAINED_STARTS_IN_IMAGE + metadata. Need not be aligned for the + struct; reads are unaligned-safe and + the walker never writes through it + (CONST). @param[in] StartsSize Size of the chained-fixups metadata - region pointed to by Starts. Bounds - the SegInfoOffset[] array reads and - each per-segment dereference. + region pointed to by StartsBacking. + Bounds the SegInfoOffset[] array reads + and each per-segment dereference. @param[in] Visitor Callback invoked per fixup slot, or NULL to count only. @param[in,out] VisitorContext Opaque context forwarded to Visitor, @@ -1144,12 +1173,12 @@ KcWalkChainedFixupsInSegment ( **/ UINTN KcWalkChainedFixupsInImage ( - IN UINT8 *Buffer, - IN UINTN BufferSize, - IN MACH_DYLD_CHAINED_STARTS_IN_IMAGE *Starts, - IN UINTN StartsSize, - IN KC_CHAINED_FIXUP_VISIT Visitor OPTIONAL, - IN OUT VOID *VisitorContext OPTIONAL + IN UINT8 *Buffer, + IN UINTN BufferSize, + IN CONST UINT8 *StartsBacking, + IN UINTN StartsSize, + IN KC_CHAINED_FIXUP_VISIT Visitor OPTIONAL, + IN OUT VOID *VisitorContext OPTIONAL ); /** diff --git a/Library/OcAppleKernelLib/KernelCollection.c b/Library/OcAppleKernelLib/KernelCollection.c index da851a62667..b8a660126d0 100644 --- a/Library/OcAppleKernelLib/KernelCollection.c +++ b/Library/OcAppleKernelLib/KernelCollection.c @@ -838,16 +838,27 @@ KcFixupValue ( UINTN KcWalkChainedFixupsInSegment ( - IN UINT8 *Buffer, - IN UINTN BufferSize, - IN MACH_DYLD_CHAINED_STARTS_IN_SEGMENT *StartsSeg, - IN UINTN StartsSegSize, - IN KC_CHAINED_FIXUP_VISIT Visitor OPTIONAL, - IN OUT VOID *VisitorContext OPTIONAL + IN UINT8 *Buffer, + IN UINTN BufferSize, + IN CONST UINT8 *StartsSegBacking, + IN UINTN StartsSegSize, + IN KC_CHAINED_FIXUP_VISIT Visitor OPTIONAL, + IN OUT VOID *VisitorContext OPTIONAL ) { + // + // StartsSegBacking is not assumed to be aligned for the struct + // (Apple's tooling places it on UINT32-array boundaries inside + // MACH_DYLD_CHAINED_STARTS_IN_IMAGE.SegInfoOffset[], and Buffer + // slot offsets are not guaranteed UINT64-aligned). The fixed + // header is copied into StartsSegHeader (a local with natural + // alignment); PageStart[] entries are read via ReadUnaligned16, + // and fixup slots in Buffer are read via ReadUnaligned64 into + // a local UINT64 used to parse the bitfield struct. + // + MACH_DYLD_CHAINED_STARTS_IN_SEGMENT StartsSegHeader; MACH_DYLD_CHAINED_PTR_64_KERNEL_CACHE_REBASE *Fixup; - UINT64 *FixupLoc; + UINT64 RawFixup; UINT64 PageBase; UINT64 PageEnd; UINT64 SlotOffset; @@ -867,14 +878,14 @@ KcWalkChainedFixupsInSegment ( UINTN Count; ASSERT (Buffer != NULL); - ASSERT (StartsSeg != NULL); + ASSERT (StartsSegBacking != NULL); // // None of the fields below can be trusted to bound themselves; treat - // every read of StartsSeg as untrusted input from the kernel - // collection blob and validate against caller-supplied container - // sizes (BufferSize bounds the contents being walked, StartsSegSize - // bounds the metadata struct itself). + // every read of the per-segment metadata as untrusted input from the + // kernel collection blob and validate against caller-supplied + // container sizes (BufferSize bounds the contents being walked, + // StartsSegSize bounds the metadata buffer itself). // // @@ -886,10 +897,18 @@ KcWalkChainedFixupsInSegment ( return 0; } - Size = StartsSeg->Size; - PageSize = StartsSeg->PageSize; - PageCount = StartsSeg->PageCount; - PointerFormat = StartsSeg->PointerFormat; + // + // Copy the fixed header into a properly-aligned local. Direct + // dereference of (MACH_DYLD_CHAINED_STARTS_IN_SEGMENT *)StartsSegBacking + // is undefined behaviour when StartsSegBacking isn't aligned for + // UINT64 (the struct's most-restrictive field is SegmentOffset). + // + CopyMem (&StartsSegHeader, StartsSegBacking, StructHeaderSize); + + Size = StartsSegHeader.Size; + PageSize = StartsSegHeader.PageSize; + PageCount = StartsSegHeader.PageCount; + PointerFormat = StartsSegHeader.PointerFormat; // // 2. The struct's self-declared Size must not exceed StartsSegSize @@ -919,7 +938,7 @@ KcWalkChainedFixupsInSegment ( // // 5. The segment must start within the buffer. // - if (StartsSeg->SegmentOffset >= BufferSize) { + if (StartsSegHeader.SegmentOffset >= BufferSize) { return 0; } @@ -953,7 +972,16 @@ KcWalkChainedFixupsInSegment ( Count = 0; for (PageIdx = 0; PageIdx < PageCount; ++PageIdx) { - PageStart = StartsSeg->PageStart[PageIdx]; + // + // PageStart[] follows the fixed header and is naturally UINT16- + // aligned only if StartsSegBacking is. Apple's tooling does not + // guarantee 2-byte alignment of the per-segment record, so use + // ReadUnaligned16 unconditionally — it's a single-byte-mov pair + // on architectures that don't have a faster unaligned helper. + // + PageStart = ReadUnaligned16 ( + (CONST UINT16 *)(StartsSegBacking + StructHeaderSize + PageIdx * sizeof (UINT16)) + ); if (PageStart == MACH_DYLD_CHAINED_PTR_START_NONE) { continue; @@ -979,7 +1007,7 @@ KcWalkChainedFixupsInSegment ( if (BaseOverflowMulAddU64 ( (UINT64)PageIdx, (UINT64)PageSize, - StartsSeg->SegmentOffset, + StartsSegHeader.SegmentOffset, &PageBase )) { @@ -1013,18 +1041,29 @@ KcWalkChainedFixupsInSegment ( continue; } - FixupLoc = (UINT64 *)(Buffer + SlotOffset); - // // 12. Walk this page's chain with a hard upper iteration bound. A // well-formed chain cannot revisit a slot, so MaxIters slots is // a strict upper bound; we stop short on any sign of looping. // + // Buffer + SlotOffset isn't guaranteed UINT64-aligned (the + // stride-1 X86_64_KERNEL_CACHE format steps single bytes). We + // read each slot into the stack-aligned RawFixup local via + // ReadUnaligned64, then cast &RawFixup to the bitfield struct + // to extract Fixup->Next safely. + // for (IterCount = 0; IterCount < MaxIters; ++IterCount) { - Fixup = (MACH_DYLD_CHAINED_PTR_64_KERNEL_CACHE_REBASE *)FixupLoc; + RawFixup = ReadUnaligned64 ((CONST UINT64 *)(Buffer + SlotOffset)); + Fixup = (MACH_DYLD_CHAINED_PTR_64_KERNEL_CACHE_REBASE *)&RawFixup; if (Visitor != NULL) { - Visitor (FixupLoc, VisitorContext); + // + // Visitor receives the raw byte address of the slot. The typedef + // commits the contract via the UINT8 * type so visitor authors + // can't accidentally trip alignment UB by dereferencing as + // UINT64. See KC_CHAINED_FIXUP_VISIT in OcAppleKernelLib.h. + // + Visitor (Buffer + SlotOffset, VisitorContext); } ++Count; @@ -1058,7 +1097,6 @@ KcWalkChainedFixupsInSegment ( } SlotOffset = NextSlotOffset; - FixupLoc = (UINT64 *)(Buffer + SlotOffset); } } @@ -1067,24 +1105,31 @@ KcWalkChainedFixupsInSegment ( UINTN KcWalkChainedFixupsInImage ( - IN UINT8 *Buffer, - IN UINTN BufferSize, - IN MACH_DYLD_CHAINED_STARTS_IN_IMAGE *Starts, - IN UINTN StartsSize, - IN KC_CHAINED_FIXUP_VISIT Visitor OPTIONAL, - IN OUT VOID *VisitorContext OPTIONAL + IN UINT8 *Buffer, + IN UINTN BufferSize, + IN CONST UINT8 *StartsBacking, + IN UINTN StartsSize, + IN KC_CHAINED_FIXUP_VISIT Visitor OPTIONAL, + IN OUT VOID *VisitorContext OPTIONAL ) { - MACH_DYLD_CHAINED_STARTS_IN_SEGMENT *StartsSeg; - UINTN StartsSegSize; - UINT32 NumSegments; - UINT32 SegOffset; - UINT32 SegIdx; - UINTN StructHeaderSize; - UINTN Count; + UINTN StartsSegSize; + UINT32 NumSegments; + UINT32 SegOffset; + UINT32 SegIdx; + UINTN StructHeaderSize; + UINTN Count; ASSERT (Buffer != NULL); - ASSERT (Starts != NULL); + ASSERT (StartsBacking != NULL); + + // + // StartsBacking is not assumed to be aligned for the + // MACH_DYLD_CHAINED_STARTS_IN_IMAGE struct. Both NumSegments and + // each SegInfoOffset[i] are UINT32 reads off this pointer; do them + // via ReadUnaligned32 so a chained-fixups payload that lands at + // any byte alignment in its containing buffer is parsed correctly. + // // // The fixed header of MACH_DYLD_CHAINED_STARTS_IN_IMAGE is one UINT32 @@ -1095,7 +1140,7 @@ KcWalkChainedFixupsInImage ( return 0; } - NumSegments = Starts->NumSegments; + NumSegments = ReadUnaligned32 ((CONST UINT32 *)StartsBacking); // // The SegInfoOffset[NumSegments] array must fit within StartsSize. @@ -1107,7 +1152,9 @@ KcWalkChainedFixupsInImage ( Count = 0; for (SegIdx = 0; SegIdx < NumSegments; ++SegIdx) { - SegOffset = Starts->SegInfoOffset[SegIdx]; + SegOffset = ReadUnaligned32 ( + (CONST UINT32 *)(StartsBacking + StructHeaderSize + SegIdx * sizeof (UINT32)) + ); // // SegInfoOffset == 0 is the sentinel for "segment has no fixups". @@ -1127,14 +1174,11 @@ KcWalkChainedFixupsInImage ( } StartsSegSize = StartsSize - (UINTN)SegOffset; - StartsSeg = (MACH_DYLD_CHAINED_STARTS_IN_SEGMENT *)( - (UINT8 *)Starts + SegOffset - ); Count += KcWalkChainedFixupsInSegment ( Buffer, BufferSize, - StartsSeg, + StartsBacking + SegOffset, StartsSegSize, Visitor, VisitorContext diff --git a/Utilities/TestProcessKernel/ProcessKernel.c b/Utilities/TestProcessKernel/ProcessKernel.c index 829f94abdc6..006138fb4f4 100644 --- a/Utilities/TestProcessKernel/ProcessKernel.c +++ b/Utilities/TestProcessKernel/ProcessKernel.c @@ -319,8 +319,8 @@ OcGetFileSize ( STATIC VOID TestFixupVisitor ( - IN OUT UINT64 *FixupLoc, - IN OUT VOID *VisitorContext + IN OUT UINT8 *FixupLoc, + IN OUT VOID *VisitorContext ) { UINTN *VisitedAddresses; @@ -387,7 +387,7 @@ RunFixupWalkTest ( Count = KcWalkChainedFixupsInSegment ( Buffer, TEST_FIXUP_BUFFER_SZ, - StartsSeg, + StartsBuffer, sizeof (StartsBuffer), TestFixupVisitor, VisitedAddresses @@ -420,7 +420,7 @@ RunFixupWalkTest ( Count = KcWalkChainedFixupsInSegment ( Buffer, TEST_FIXUP_BUFFER_SZ, - StartsSeg, + StartsBuffer, sizeof (StartsBuffer), TestFixupVisitor, VisitedAddresses @@ -440,7 +440,7 @@ RunFixupWalkTest ( Count = KcWalkChainedFixupsInSegment ( Buffer, TEST_FIXUP_BUFFER_SZ, - StartsSeg, + StartsBuffer, sizeof (StartsBuffer), TestFixupVisitor, VisitedAddresses @@ -470,7 +470,7 @@ RunFixupWalkTest ( Count = KcWalkChainedFixupsInSegment ( Buffer, TEST_FIXUP_BUFFER_SZ, - StartsSeg, + StartsBuffer, sizeof (StartsBuffer), TestFixupVisitor, VisitedAddresses @@ -492,7 +492,7 @@ RunFixupWalkTest ( Count = KcWalkChainedFixupsInSegment ( Buffer, TEST_FIXUP_BUFFER_SZ, - StartsSeg, + StartsBuffer, sizeof (StartsBuffer), TestFixupVisitor, VisitedAddresses @@ -514,7 +514,7 @@ RunFixupWalkTest ( Count = KcWalkChainedFixupsInSegment ( Buffer, TEST_FIXUP_BUFFER_SZ, - StartsSeg, + StartsBuffer, sizeof (StartsBuffer), TestFixupVisitor, VisitedAddresses @@ -536,7 +536,7 @@ RunFixupWalkTest ( Count = KcWalkChainedFixupsInSegment ( Buffer, TEST_FIXUP_BUFFER_SZ, - StartsSeg, + StartsBuffer, sizeof (StartsBuffer), TestFixupVisitor, VisitedAddresses @@ -571,7 +571,7 @@ RunFixupWalkTest ( Count = KcWalkChainedFixupsInSegment ( Buffer, TEST_FIXUP_BUFFER_SZ, - StartsSeg, + StartsBuffer, sizeof (StartsBuffer), TestFixupVisitor, VisitedAddresses @@ -602,7 +602,7 @@ RunFixupWalkTest ( Count = KcWalkChainedFixupsInSegment ( Buffer, TEST_FIXUP_BUFFER_SZ, - StartsSeg, + StartsBuffer, sizeof (StartsBuffer), TestFixupVisitor, VisitedAddresses @@ -635,7 +635,7 @@ RunFixupWalkTest ( Count = KcWalkChainedFixupsInImage ( Buffer, TEST_FIXUP_BUFFER_SZ, - ImageStarts, + ImageStartsBuffer, sizeof (ImageStartsBuffer), TestFixupVisitor, VisitedAddresses @@ -648,6 +648,84 @@ RunFixupWalkTest ( } } + // + // (h) Alignment-safety: place the per-segment metadata at a + // deliberately misaligned byte offset inside a containing + // buffer, and confirm the walker still produces the canonical + // visit count (4) for the stride-4 chain. This exercises + // ReadUnaligned32 / ReadUnaligned16 / CopyMem on the metadata + // side and ReadUnaligned64 on the Buffer slot side. A pre-fix + // implementation that did `(struct *)(StartsSegBacking)` cast + // directly would either GP-fault on strict-alignment hosts, + // trip UBSan, or read torn fields here. + // + { + UINT8 AlignedStarts[sizeof (StartsBuffer)]; + UINT8 MisalignedContainer[sizeof (StartsBuffer) + 8]; + UINTN MisalignOffset; + + // + // Rebuild the canonical StartsBuffer at native alignment for + // copying. The slot data was overwritten by tests (e)/(f)/(g); + // restore the 4-link stride-4 layout used in the first stride-4 + // walk above. + // + Slot = (MACH_DYLD_CHAINED_PTR_64_KERNEL_CACHE_REBASE *)Buffer; + ZeroMem (Buffer, TEST_FIXUP_BUFFER_SZ); + Slot[0].Next = 4; + Slot[0].Target = 0xAAAA; + Slot[2].Next = 4; + Slot[2].Target = 0xBBBB; + Slot[4].Next = 4; + Slot[4].Target = 0xCCCC; + Slot[6].Next = 0; + Slot[6].Target = 0xDDDD; + + StartsSeg = (MACH_DYLD_CHAINED_STARTS_IN_SEGMENT *)AlignedStarts; + ZeroMem (AlignedStarts, sizeof (AlignedStarts)); + StartsSeg->Size = sizeof (AlignedStarts); + StartsSeg->PageSize = TEST_FIXUP_PAGE_SIZE; + StartsSeg->PointerFormat = MACH_DYLD_CHAINED_PTR_64_KERNEL_CACHE; + StartsSeg->SegmentOffset = 0; + StartsSeg->PageCount = TEST_FIXUP_PAGE_COUNT; + StartsSeg->PageStart[0] = 0; + + // + // Try every byte offset 1..7 — at least one of these (the odd + // ones) is misaligned for the UINT64 SegmentOffset field's + // natural alignment, and the others (2/4) for less-strict + // boundaries. All must produce identical results. + // + for (MisalignOffset = 1; MisalignOffset < 8; ++MisalignOffset) { + ZeroMem (MisalignedContainer, sizeof (MisalignedContainer)); + CopyMem (MisalignedContainer + MisalignOffset, AlignedStarts, sizeof (AlignedStarts)); + + ZeroMem (VisitedAddresses, sizeof (VisitedAddresses)); + Count = KcWalkChainedFixupsInSegment ( + Buffer, + TEST_FIXUP_BUFFER_SZ, + MisalignedContainer + MisalignOffset, + sizeof (AlignedStarts), + TestFixupVisitor, + VisitedAddresses + ); + if ((Count != 4) || (VisitedAddresses[0] != 4)) { + DEBUG (( + DEBUG_ERROR, + "[FAIL] alignment-safety at offset %u: %u fixups\n", + (UINT32)MisalignOffset, + (UINT32)Count + )); + ++FailCount; + break; + } + } + + if (MisalignOffset == 8) { + DEBUG ((DEBUG_WARN, "[OK] alignment-safe walk at byte offsets 1..7 (4 fixups each)\n")); + } + } + FreePool (Buffer); return FailCount; }