[llvm][MachO] Fix integer truncation in rebase/bind parsing

[zixu-w]

Count and Skip should use uint64_t as they are encoded/decoded
using 64-bit ULEB128.

In *_OPCODE_DO_*_ULEB_TIMES_SKIPPING_ULEB, Skip could be encoded as
a two's complement for moving SegmentOffset backwards. Having a 32-bit
Skip truncates the encoded value and leads to a malformed AdvanceAmount
and invalid SegmentOffset that extends past valid sections.

[llvmbot]

@llvm/pr-subscribers-llvm-binary-utilities

Author: Zixu Wang (zixu-w)

Changes

Skip could use up the full 64 bits decoded by readULEB128 for some rebase/bind opcodes.
In *_OPCODE_DO_*_ULEB_TIMES_SKIPPING_ULEB, Skip could be encoded as a two's complement for moving SegmentOffset backwards. Having a 32-bit Skip truncates the encoded value and leads to a malformed AdvanceAmount and invalid SegmentOffset that extends past valid sections.

---

Full diff: https://github.com/llvm/llvm-project/pull/89337.diff

2 Files Affected:

• (modified) llvm/include/llvm/Object/MachO.h (+3-3)
• (modified) llvm/lib/Object/MachOObjectFile.cpp (+15-5)

diff --git a/llvm/include/llvm/Object/MachO.h b/llvm/include/llvm/Object/MachO.h
index 24f9954584ed5d..bf35d9c4e9905b 100644
--- a/llvm/include/llvm/Object/MachO.h
+++ b/llvm/include/llvm/Object/MachO.h
@@ -136,7 +136,7 @@ class BindRebaseSegInfo {
   // Used to check a Mach-O Bind or Rebase entry for errors when iterating.
   const char* checkSegAndOffsets(int32_t SegIndex, uint64_t SegOffset,
                                  uint8_t PointerSize, uint32_t Count=1,
-                                 uint32_t Skip=0);
+                                 uint64_t Skip=0);
   // Used with valid SegIndex/SegOffset values from checked entries.
   StringRef segmentName(int32_t SegIndex);
   StringRef sectionName(int32_t SegIndex, uint64_t SegOffset);
@@ -577,7 +577,7 @@ class MachOObjectFile : public ObjectFile {
   // This is used by MachOBindEntry::moveNext() to validate a MachOBindEntry.
   const char *BindEntryCheckSegAndOffsets(int32_t SegIndex, uint64_t SegOffset,
                                          uint8_t PointerSize, uint32_t Count=1,
-                                          uint32_t Skip=0) const {
+                                          uint64_t Skip=0) const {
     return BindRebaseSectionTable->checkSegAndOffsets(SegIndex, SegOffset,
                                                      PointerSize, Count, Skip);
   }
@@ -592,7 +592,7 @@ class MachOObjectFile : public ObjectFile {
                                             uint64_t SegOffset,
                                             uint8_t PointerSize,
                                             uint32_t Count=1,
-                                            uint32_t Skip=0) const {
+                                            uint64_t Skip=0) const {
     return BindRebaseSectionTable->checkSegAndOffsets(SegIndex, SegOffset,
                                                       PointerSize, Count, Skip);
   }
diff --git a/llvm/lib/Object/MachOObjectFile.cpp b/llvm/lib/Object/MachOObjectFile.cpp
index 1cfd0a069463e9..deeabef7e7501f 100644
--- a/llvm/lib/Object/MachOObjectFile.cpp
+++ b/llvm/lib/Object/MachOObjectFile.cpp
@@ -3515,7 +3515,12 @@ void MachORebaseEntry::moveNext() {
     uint8_t Byte = *Ptr++;
     uint8_t ImmValue = Byte & MachO::REBASE_IMMEDIATE_MASK;
     uint8_t Opcode = Byte & MachO::REBASE_OPCODE_MASK;
-    uint32_t Count, Skip;
+    uint32_t Count;
+    // For opcode REBASE_OPCODE_DO_REBASE_ULEB_TIMES_SKIPPING_ULEB, the ULEB128
+    // encoded Skip amount could be two's complements for moving SegmentOffset
+    // to a lower address. Skip should be the same integer width as
+    // SegmentOffset and AdvanceAmount to prevent truncating.
+    uint64_t Skip;
     const char *error = nullptr;
     switch (Opcode) {
     case MachO::REBASE_OPCODE_DONE:
@@ -3854,7 +3859,12 @@ void MachOBindEntry::moveNext() {
     uint8_t Opcode = Byte & MachO::BIND_OPCODE_MASK;
     int8_t SignExtended;
     const uint8_t *SymStart;
-    uint32_t Count, Skip;
+    uint32_t Count;
+    // For opcode BIND_OPCODE_DO_BIND_ULEB_TIMES_SKIPPING_ULEB, the ULEB128
+    // encoded Skip amount could be two's complements for moving SegmentOffset
+    // to a lower address. Skip should be the same integer width as
+    // SegmentOffset and AdvanceAmount to prevent truncating.
+    uint64_t Skip;
     const char *error = nullptr;
     switch (Opcode) {
     case MachO::BIND_OPCODE_DONE:
@@ -4388,14 +4398,14 @@ const char * BindRebaseSegInfo::checkSegAndOffsets(int32_t SegIndex,
                                                    uint64_t SegOffset,
                                                    uint8_t PointerSize,
                                                    uint32_t Count,
-                                                   uint32_t Skip) {
+                                                   uint64_t Skip) {
   if (SegIndex == -1)
     return "missing preceding *_OPCODE_SET_SEGMENT_AND_OFFSET_ULEB";
   if (SegIndex >= MaxSegIndex)
     return "bad segIndex (too large)";
   for (uint32_t i = 0; i < Count; ++i) {
-    uint32_t Start = SegOffset + i * (PointerSize + Skip);
-    uint32_t End = Start + PointerSize;
+    uint64_t Start = SegOffset + i * (PointerSize + Skip);
+    uint64_t End = Start + PointerSize;
     bool Found = false;
     for (const SectionInfo &SI : Sections) {
       if (SI.SegmentIndex != SegIndex)

[zixu-w]

Trying to figure out how to construct a minimum reproducer code/binary/object YAML for a test case.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[cyndyishida]

LGTM