Skip to content

OcAppleKernelLib: alignment-safe chained-fixup walks (follow-up to #603) #606

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MattJackson wants to merge 1 commit into
base: master
Choose a base branch
from
Open

OcAppleKernelLib: alignment-safe chained-fixup walks (follow-up to #603) #606

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 71 additions & 42 deletions Include/Acidanthera/Library/OcAppleKernelLib.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,

@vit9696 vit9696 May 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not change typing just because of the alignment. Check alignment before casting, and then use proper types. This applies throughout PR.

IN OUT VOID *VisitorContext
);

/**
Expand All @@ -1075,41 +1086,50 @@ 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
is malformed.
**/
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
);

/**
Expand All @@ -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,
Expand All @@ -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
);

/**
Expand Down
132 changes: 88 additions & 44 deletions Library/OcAppleKernelLib/KernelCollection.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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).
//

//
Expand All @@ -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);

@vit9696 vit9696 May 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Size = StartsSegHeader.Size;
PageSize = StartsSegHeader.PageSize;
PageCount = StartsSegHeader.PageCount;
PointerFormat = StartsSegHeader.PointerFormat;

//
// 2. The struct's self-declared Size must not exceed StartsSegSize
Expand Down Expand Up @@ -919,7 +938,7 @@ KcWalkChainedFixupsInSegment (
//
// 5. The segment must start within the buffer.
//
if (StartsSeg->SegmentOffset >= BufferSize) {
if (StartsSegHeader.SegmentOffset >= BufferSize) {
return 0;
}

Expand Down Expand Up @@ -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;
Expand All @@ -979,7 +1007,7 @@ KcWalkChainedFixupsInSegment (
if (BaseOverflowMulAddU64 (
(UINT64)PageIdx,
(UINT64)PageSize,
StartsSeg->SegmentOffset,
StartsSegHeader.SegmentOffset,
&PageBase
))
{
Expand Down Expand Up @@ -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));

@vit9696 vit9696 May 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, though you use ReadUnaligned here and below instead of CopyMem.

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;
Expand Down Expand Up @@ -1058,7 +1097,6 @@ KcWalkChainedFixupsInSegment (
}

SlotOffset = NextSlotOffset;
FixupLoc = (UINT64 *)(Buffer + SlotOffset);
}
}

Expand All @@ -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
Expand All @@ -1095,7 +1140,7 @@ KcWalkChainedFixupsInImage (
return 0;
}

NumSegments = Starts->NumSegments;
NumSegments = ReadUnaligned32 ((CONST UINT32 *)StartsBacking);

//
// The SegInfoOffset[NumSegments] array must fit within StartsSize.
Expand All @@ -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".
Expand All @@ -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
Expand Down
Loading
Loading