Fix Mach-O bind and rebase validation errors in libObject

Summary:
llvm-objdump (via libObject) validates DYLD_INFO rebase and bind
entries against the basic structure found in the Mach-O file before
evaluating the contents of those entries. Certain malformed Mach-Os can
defeat the validation check and force llvm-objdump (libObject) to crash.

The previous logic verified a rebase or bind started in a valid Mach-O
section, but did not verify that the section wholely contained the
fixup. It also generally allows rebases or binds to start immediately
after a valid section even if that range is not itself part of a valid
section. Finally, bind and rebase opcodes that indicate more than one
fixup (apply N times...) are not completely validated: only the first
and final fixups are checked.

The previous logic also rejected certain binaries as false positives.
Some bind and rebase opcodes can modify the state machine such that the
next bind or rebase will fail. libObject will reject these opcodes as
invalid in order to be helpful and print an error message associated
with the instruction that caused the problem, even though the binary is
not actually illegal until it consumes the invalid state in the state
machine. In other words, libObject may reject a Mach-O binary that
Apple's dynamic linker may consider legal. The original version of
macho-rebase-add-addr-uleb-too-big is an example of such a binary.

I have replaced the existing checkSegAndOffset and checkCountAndSkip
functions with a single function, checkSegAndOffsets, which validates
all of the fixups realized by a DYLD_INFO opcode. checkSegAndOffsets
verifies that a Mach-O section fully contains each fixup. Every fixup
realized by an opcode is validated, and some (but not all!)
inconsistencies in the state machine are allowed until a fixup is
realized. This means that libObject may fail on an opcode that realizes
a fixup, not on the opcode that introduced the arithmetic error.

Existing test cases have been modified to reflect the changes in error
messages returned by libObject. What's more, the test case for 
macho-rebase-add-addr-uleb-too-big has been modified so that it actually
triggers the error condition; the new code in libObject considers the
original test binary "legal".

rdar://47797757

Reviewers: lhames, pete, ab

Reviewed By: pete

Subscribers: rupprecht, jdoerfert, llvm-commits

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D59574

llvm-svn: 356629

1 files changed, 62 insertions, 116 deletions

diff --git a/llvm/lib/Object/MachOObjectFile.cpp b/llvm/lib/Object/MachOObjectFile.cpp
index 456c6369d1c..710dd2161c8 100644
--- a/llvm/lib/Object/MachOObjectFile.cpp
+++ b/llvm/lib/Object/MachOObjectFile.cpp
@@ -3120,8 +3120,8 @@ void MachORebaseEntry::moveNext() {
         moveToEnd();
         return;
       }
-      error = O->RebaseEntryCheckSegAndOffset(SegmentIndex, SegmentOffset,
-                                              true);
+      error = O->RebaseEntryCheckSegAndOffsets(SegmentIndex, SegmentOffset,
+                                               PointerSize);
       if (error) {
         *E = malformedError("for REBASE_OPCODE_SET_SEGMENT_AND_OFFSET_ULEB " +
                             Twine(error) + " for opcode at: 0x" +
@@ -3145,8 +3145,8 @@ void MachORebaseEntry::moveNext() {
         moveToEnd();
         return;
       }
-      error = O->RebaseEntryCheckSegAndOffset(SegmentIndex, SegmentOffset,
-                                              true);
+      error = O->RebaseEntryCheckSegAndOffsets(SegmentIndex, SegmentOffset,
+                                               PointerSize);
       if (error) {
         *E = malformedError("for REBASE_OPCODE_ADD_ADDR_ULEB " + Twine(error) +
                             " for opcode at: 0x" +
@@ -3160,8 +3160,8 @@ void MachORebaseEntry::moveNext() {
                                        SegmentOffset) << "\n");
       break;
     case MachO::REBASE_OPCODE_ADD_ADDR_IMM_SCALED:
-      error = O->RebaseEntryCheckSegAndOffset(SegmentIndex, SegmentOffset,
-                                              true);
+      error = O->RebaseEntryCheckSegAndOffsets(SegmentIndex, SegmentOffset,
+                                               PointerSize);
       if (error) {
         *E = malformedError("for REBASE_OPCODE_ADD_ADDR_IMM_SCALED " +
                             Twine(error) + " for opcode at: 0x" +
@@ -3170,8 +3170,8 @@ void MachORebaseEntry::moveNext() {
         return;
       }
       SegmentOffset += ImmValue * PointerSize;
-      error = O->RebaseEntryCheckSegAndOffset(SegmentIndex, SegmentOffset,
-                                              false);
+      error = O->RebaseEntryCheckSegAndOffsets(SegmentIndex, SegmentOffset,
+                                               PointerSize);
       if (error) {
         *E =
             malformedError("for REBASE_OPCODE_ADD_ADDR_IMM_SCALED "
@@ -3187,15 +3187,6 @@ void MachORebaseEntry::moveNext() {
                                        SegmentOffset) << "\n");
       break;
     case MachO::REBASE_OPCODE_DO_REBASE_IMM_TIMES:
-      error = O->RebaseEntryCheckSegAndOffset(SegmentIndex, SegmentOffset,
-                                              true);
-      if (error) {
-        *E = malformedError("for REBASE_OPCODE_DO_REBASE_IMM_TIMES " +
-                            Twine(error) + " for opcode at: 0x" +
-                            Twine::utohexstr(OpcodeStart - Opcodes.begin()));
-        moveToEnd();
-        return;
-      }
       AdvanceAmount = PointerSize;
       Skip = 0;
       Count = ImmValue;
@@ -3203,8 +3194,8 @@ void MachORebaseEntry::moveNext() {
         RemainingLoopCount = ImmValue - 1;
       else
         RemainingLoopCount = 0;
-      error = O->RebaseEntryCheckCountAndSkip(Count, Skip, PointerSize,
-                                              SegmentIndex, SegmentOffset);
+      error = O->RebaseEntryCheckSegAndOffsets(SegmentIndex, SegmentOffset,
+                                               PointerSize, Count, Skip);
       if (error) {
         *E = malformedError("for REBASE_OPCODE_DO_REBASE_IMM_TIMES " +
                             Twine(error) + " for opcode at: 0x" +
@@ -3221,15 +3212,6 @@ void MachORebaseEntry::moveNext() {
                  << "\n");
       return;
     case MachO::REBASE_OPCODE_DO_REBASE_ULEB_TIMES:
-      error = O->RebaseEntryCheckSegAndOffset(SegmentIndex, SegmentOffset,
-                                              true);
-      if (error) {
-        *E = malformedError("for REBASE_OPCODE_DO_REBASE_ULEB_TIMES " +
-                            Twine(error) + " for opcode at: 0x" +
-                            Twine::utohexstr(OpcodeStart - Opcodes.begin()));
-        moveToEnd();
-        return;
-      }
       AdvanceAmount = PointerSize;
       Skip = 0;
       Count = readULEB128(&error);
@@ -3244,8 +3226,8 @@ void MachORebaseEntry::moveNext() {
         RemainingLoopCount = Count - 1;
       else
         RemainingLoopCount = 0;
-      error = O->RebaseEntryCheckCountAndSkip(Count, Skip, PointerSize,
-                                              SegmentIndex, SegmentOffset);
+      error = O->RebaseEntryCheckSegAndOffsets(SegmentIndex, SegmentOffset,
+                                               PointerSize, Count, Skip);
       if (error) {
         *E = malformedError("for REBASE_OPCODE_DO_REBASE_ULEB_TIMES " +
                             Twine(error) + " for opcode at: 0x" +
@@ -3262,15 +3244,6 @@ void MachORebaseEntry::moveNext() {
                  << "\n");
       return;
     case MachO::REBASE_OPCODE_DO_REBASE_ADD_ADDR_ULEB:
-      error = O->RebaseEntryCheckSegAndOffset(SegmentIndex, SegmentOffset,
-                                              true);
-      if (error) {
-        *E = malformedError("for REBASE_OPCODE_DO_REBASE_ADD_ADDR_ULEB " +
-                            Twine(error) + " for opcode at: 0x" +
-                            Twine::utohexstr(OpcodeStart - Opcodes.begin()));
-        moveToEnd();
-        return;
-      }
       Skip = readULEB128(&error);
       if (error) {
         *E = malformedError("for REBASE_OPCODE_DO_REBASE_ADD_ADDR_ULEB " +
@@ -3282,8 +3255,8 @@ void MachORebaseEntry::moveNext() {
       AdvanceAmount = Skip + PointerSize;
       Count = 1;
       RemainingLoopCount = 0;
-      error = O->RebaseEntryCheckCountAndSkip(Count, Skip, PointerSize,
-                                              SegmentIndex, SegmentOffset);
+      error = O->RebaseEntryCheckSegAndOffsets(SegmentIndex, SegmentOffset,
+                                               PointerSize, Count, Skip);
       if (error) {
         *E = malformedError("for REBASE_OPCODE_DO_REBASE_ADD_ADDR_ULEB " +
                             Twine(error) + " for opcode at: 0x" +
@@ -3300,16 +3273,6 @@ void MachORebaseEntry::moveNext() {
                  << "\n");
       return;
     case MachO::REBASE_OPCODE_DO_REBASE_ULEB_TIMES_SKIPPING_ULEB:
-      error = O->RebaseEntryCheckSegAndOffset(SegmentIndex, SegmentOffset,
-                                              true);
-      if (error) {
-        *E = malformedError("for REBASE_OPCODE_DO_REBASE_ULEB_TIMES_SKIPPING_"
-                            "ULEB " +
-                            Twine(error) + " for opcode at: 0x" +
-                            Twine::utohexstr(OpcodeStart - Opcodes.begin()));
-        moveToEnd();
-        return;
-      }
       Count = readULEB128(&error);
       if (error) {
         *E = malformedError("for REBASE_OPCODE_DO_REBASE_ULEB_TIMES_SKIPPING_"
@@ -3334,8 +3297,8 @@ void MachORebaseEntry::moveNext() {
       }
       AdvanceAmount = Skip + PointerSize;
 
-      error = O->RebaseEntryCheckCountAndSkip(Count, Skip, PointerSize,
-                                              SegmentIndex, SegmentOffset);
+      error = O->RebaseEntryCheckSegAndOffsets(SegmentIndex, SegmentOffset,
+                                               PointerSize, Count, Skip);
       if (error) {
         *E = malformedError("for REBASE_OPCODE_DO_REBASE_ULEB_TIMES_SKIPPING_"
                             "ULEB " +
@@ -3642,7 +3605,8 @@ void MachOBindEntry::moveNext() {
         moveToEnd();
         return;
       }
-      error = O->BindEntryCheckSegAndOffset(SegmentIndex, SegmentOffset, true);
+      error = O->BindEntryCheckSegAndOffsets(SegmentIndex, SegmentOffset,
+                                             PointerSize);
       if (error) {
         *E = malformedError("for BIND_OPCODE_SET_SEGMENT_AND_OFFSET_ULEB " +
                             Twine(error) + " for opcode at: 0x" +
@@ -3666,7 +3630,8 @@ void MachOBindEntry::moveNext() {
         moveToEnd();
         return;
       }
-      error = O->BindEntryCheckSegAndOffset(SegmentIndex, SegmentOffset, true);
+      error = O->BindEntryCheckSegAndOffsets(SegmentIndex, SegmentOffset,
+                                             PointerSize);
       if (error) {
         *E = malformedError("for BIND_OPCODE_ADD_ADDR_ULEB " + Twine(error) +
                             " for opcode at: 0x" +
@@ -3682,7 +3647,8 @@ void MachOBindEntry::moveNext() {
     case MachO::BIND_OPCODE_DO_BIND:
       AdvanceAmount = PointerSize;
       RemainingLoopCount = 0;
-      error = O->BindEntryCheckSegAndOffset(SegmentIndex, SegmentOffset, true);
+      error = O->BindEntryCheckSegAndOffsets(SegmentIndex, SegmentOffset,
+                                             PointerSize);
       if (error) {
         *E = malformedError("for BIND_OPCODE_DO_BIND " + Twine(error) +
                             " for opcode at: 0x" +
@@ -3719,7 +3685,8 @@ void MachOBindEntry::moveNext() {
         moveToEnd();
         return;
       }
-      error = O->BindEntryCheckSegAndOffset(SegmentIndex, SegmentOffset, true);
+      error = O->BindEntryCheckSegAndOffsets(SegmentIndex, SegmentOffset,
+                                             PointerSize);
       if (error) {
         *E = malformedError("for BIND_OPCODE_DO_BIND_ADD_ADDR_ULEB " +
                             Twine(error) + " for opcode at: 0x" +
@@ -3755,8 +3722,8 @@ void MachOBindEntry::moveNext() {
       // Note, this is not really an error until the next bind but make no sense
       // for a BIND_OPCODE_DO_BIND_ADD_ADDR_ULEB to not be followed by another
       // bind operation.
-      error = O->BindEntryCheckSegAndOffset(SegmentIndex, SegmentOffset +
-                                            AdvanceAmount, false);
+      error = O->BindEntryCheckSegAndOffsets(SegmentIndex, SegmentOffset +
+                                            AdvanceAmount, PointerSize);
       if (error) {
         *E = malformedError("for BIND_OPCODE_ADD_ADDR_ULEB (after adding "
                             "ULEB) " +
@@ -3782,7 +3749,8 @@ void MachOBindEntry::moveNext() {
         moveToEnd();
         return;
       }
-      error = O->BindEntryCheckSegAndOffset(SegmentIndex, SegmentOffset, true);
+      error = O->BindEntryCheckSegAndOffsets(SegmentIndex, SegmentOffset,
+                                             PointerSize);
       if (error) {
         *E = malformedError("for BIND_OPCODE_DO_BIND_ADD_ADDR_IMM_SCALED " +
                             Twine(error) + " for opcode at: 0x" +
@@ -3810,8 +3778,8 @@ void MachOBindEntry::moveNext() {
       }
       AdvanceAmount = ImmValue * PointerSize + PointerSize;
       RemainingLoopCount = 0;
-      error = O->BindEntryCheckSegAndOffset(SegmentIndex, SegmentOffset +
-                                            AdvanceAmount, false);
+      error = O->BindEntryCheckSegAndOffsets(SegmentIndex, SegmentOffset +
+                                             AdvanceAmount, PointerSize);
       if (error) {
         *E =
             malformedError("for BIND_OPCODE_DO_BIND_ADD_ADDR_IMM_SCALED "
@@ -3857,15 +3825,6 @@ void MachOBindEntry::moveNext() {
         moveToEnd();
         return;
       }
-      error = O->BindEntryCheckSegAndOffset(SegmentIndex, SegmentOffset, true);
-      if (error) {
-        *E =
-            malformedError("for BIND_OPCODE_DO_BIND_ULEB_TIMES_SKIPPING_ULEB " +
-                           Twine(error) + " for opcode at: 0x" +
-                           Twine::utohexstr(OpcodeStart - Opcodes.begin()));
-        moveToEnd();
-        return;
-      }
       if (SymbolName == StringRef()) {
         *E = malformedError(
             "for BIND_OPCODE_DO_BIND_ULEB_TIMES_SKIPPING_ULEB "
@@ -3884,8 +3843,8 @@ void MachOBindEntry::moveNext() {
         moveToEnd();
         return;
       }
-      error = O->BindEntryCheckCountAndSkip(Count, Skip, PointerSize,
-                                            SegmentIndex, SegmentOffset);
+      error = O->BindEntryCheckSegAndOffsets(SegmentIndex, SegmentOffset,
+                                             PointerSize, Count, Skip);
       if (error) {
         *E =
             malformedError("for BIND_OPCODE_DO_BIND_ULEB_TIMES_SKIPPING_ULEB " +
@@ -4008,53 +3967,40 @@ BindRebaseSegInfo::BindRebaseSegInfo(const object::MachOObjectFile *Obj) {
   MaxSegIndex = CurSegIndex;
 }
 
-// For use with a SegIndex,SegOffset pair in MachOBindEntry::moveNext() to
-// validate a MachOBindEntry or MachORebaseEntry.
-const char * BindRebaseSegInfo::checkSegAndOffset(int32_t SegIndex,
-                                                  uint64_t SegOffset,
-                                                  bool endInvalid) {
+// For use with a SegIndex, SegOffset, and PointerSize triple in
+// MachOBindEntry::moveNext() to validate a MachOBindEntry or MachORebaseEntry.
+//
+// Given a SegIndex, SegOffset, and PointerSize, verify a valid section exists
+// that fully contains a pointer at that location. Multiple fixups in a bind
+// (such as with the BIND_OPCODE_DO_BIND_ULEB_TIMES_SKIPPING_ULEB opcode) can
+// be tested via the Count and Skip parameters.
+const char * BindRebaseSegInfo::checkSegAndOffsets(int32_t SegIndex,
+                                                   uint64_t SegOffset,
+                                                   uint8_t PointerSize,
+                                                   uint32_t Count,
+                                                   uint32_t Skip) {
   if (SegIndex == -1)
     return "missing preceding *_OPCODE_SET_SEGMENT_AND_OFFSET_ULEB";
   if (SegIndex >= MaxSegIndex)
     return "bad segIndex (too large)";
-  for (const SectionInfo &SI : Sections) {
-    if (SI.SegmentIndex != SegIndex)
-      continue;
-    if (SI.OffsetInSegment > SegOffset)
-      continue;
-    if (SegOffset > (SI.OffsetInSegment + SI.Size))
-      continue;
-    if (endInvalid && SegOffset >= (SI.OffsetInSegment + SI.Size))
-      continue;
-    return nullptr;
-  }
-  return "bad segOffset, too large";
-}
-
-// For use in MachOBindEntry::moveNext() to validate a MachOBindEntry for
-// the BIND_OPCODE_DO_BIND_ULEB_TIMES_SKIPPING_ULEB opcode and for use in
-// MachORebaseEntry::moveNext() to validate a MachORebaseEntry for
-// REBASE_OPCODE_DO_*_TIMES* opcodes.  The SegIndex and SegOffset must have
-// been already checked.
-const char * BindRebaseSegInfo::checkCountAndSkip(uint32_t Count, uint32_t Skip,
-                                                  uint8_t PointerSize,
-                                                  int32_t SegIndex,
-                                                  uint64_t SegOffset) {
-  const SectionInfo &SI = findSection(SegIndex, SegOffset);
-  uint64_t addr = SI.SegmentStartAddress + SegOffset;
-  if (addr >= SI.Address + SI.Size)
-    return "bad segOffset, too large";
-  uint64_t i = 0;
-  if (Count > 1)
-    i = (Skip + PointerSize) * (Count - 1);
-  else if (Count == 1)
-    i = Skip + PointerSize;
-  if (addr + i >= SI.Address + SI.Size) {
-    // For rebase opcodes they can step from one section to another.
-    uint64_t TrailingSegOffset = (addr + i) - SI.SegmentStartAddress;
-    const char *error = checkSegAndOffset(SegIndex, TrailingSegOffset, false);
-    if (error)
-      return "bad count and skip, too large";
+  for (uint32_t i = 0; i < Count; ++i) {
+    uint32_t Start = SegOffset + i * (PointerSize + Skip);
+    uint32_t End = Start + PointerSize;
+    bool Found = false;
+    for (const SectionInfo &SI : Sections) {
+      if (SI.SegmentIndex != SegIndex)
+        continue;
+      if ((SI.OffsetInSegment<=Start) && (Start<(SI.OffsetInSegment+SI.Size))) {
+        if (End <= SI.OffsetInSegment + SI.Size) {
+          Found = true;
+          break;
+        }
+        else
+          return "bad offset, extends beyond section boundary";
+      }
+    }
+    if (!Found)
+      return "bad offset, not in section";
   }
   return nullptr;
 }