[llvm] Check for overflows when computing load-command's addresses.

[oontvoo]

Summary

PR/56746

[alexander-shaposhnikov]

llvm/lib/Object/MachOObjectFile.cpp

202

nit: I'd probably report a separate message for the overflow case (cause it'd make debugging a tiny bit easier)

[tschuett]

llvm/lib/Object/MachOObjectFile.cpp

202

nit: could you instead use an integer with a defined bit width: uint64_t?

[oontvoo]

llvm/lib/Object/MachOObjectFile.cpp

202

nit: could you instead use an integer with a defined bit width: uint64_t?

How about uintptr_t ?

[tschuett]

Sounds good.

[oontvoo]

• updated type
• separate overflow case to different err msg to make debugging easier

[jyknight]

llvm/lib/Object/MachOObjectFile.cpp

202

long unsigned int should be uintptr_t here.
And I think the comparison should be >=?

232

Probably all of the checks here can go away, except to check that L.Ptr + L.C.cmdsize doesn't overflow.

All the range checks are done by getLoadCommandInfo anyways?

[tschuett]

llvm/lib/Object/MachOObjectFile.cpp

234

I am known to be stupid. You do saturating add with uintptr_t and claim errors on uint32_t?

Would it help to make the asserts into real errors? I like to be informed about malformed files in release builds.

[oontvoo]

addressed review comment:

• refactor code so that the overflow checks can be done in place
• remove redundant check
• fix off-by-one

[oontvoo]

llvm/lib/Object/MachOObjectFile.cpp

234

oops, sorry - forgot to update the error messages when I updated the types.
yes - can make these errors instead of asserts.

[oontvoo]

updated err msgs and removed a few asserts

[alexander-shaposhnikov]

llvm/lib/Object/MachOObjectFile.cpp

134

A slightly more general version:

template <class T, class... Ts>
std::enable_if_t<std::is_unsigned<T>::value, T>
SaturatingAdd(T X, T Y, T Z, Ts... args) {
  bool Overflowed = false;
  T XY = SaturatingAdd(X, Y, &Overflowed);
  if (Overflowed)
    return SaturatingAdd(std::numeric_limits<T>::max(), 1, args...);
  return SaturatingAdd(XY, Z, args...);
}

(p.s. if you like - i can send a small patch with this addition to MathExtras)

[oontvoo]

llvm/lib/Object/MachOObjectFile.cpp

134

sure! Thanks!

llvm/lib/Object/MachOObjectFile.cpp

 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Support/LEB128.h"
+#include "llvm/Support/MathExtras.h"
 #include "llvm/Support/MemoryBufferRef.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/SwapByteOrder.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cassert>
 #include <cstddef>
 #include <cstdint>
static T getStruct(const MachOObjectFile &O, const char *P) {
   if (O.isLittleEndian() != sys::IsLittleEndianHost)
     MachO::swapStruct(Cmd);
   return Cmd;
 }
 
 template <typename T>
 static Expected<T> getStructOrErr(const MachOObjectFile &O, const char *P) {
   // Don't read before the beginning or past the end of the file
-  if (P < O.getData().begin() || P + sizeof(T) > O.getData().end())
+  bool Overflowed = false;
+  uintptr_t Ret = llvm::SaturatingAdd<uintptr_t>(P, sizeof(T), &Overflowed);
+  if (P < O.getData().begin() || Overflowed || Ret > O.getData().end())
     return malformedError("Structure read out-of-range");
 
   T Cmd;
   memcpy(&Cmd, P, sizeof(T));
   if (O.isLittleEndian() != sys::IsLittleEndianHost)
     MachO::swapStruct(Cmd);
   return Cmd;
 }

 static StringRef parseSegmentOrSectionName(const char *P) {
   if (P[15] == 0)
     // Null terminated.
     return P;
   // Not null terminated, so this is a 16 char string.
   return StringRef(P, 16);
 }
 
 static unsigned getCPUType(const MachOObjectFile &O) {
   return O.getHeader().cputype;
 }
 
 static unsigned getCPUSubType(const MachOObjectFile &O) {
   return O.getHeader().cpusubtype;
 }
 
 static uint32_t
static uint32_t getSectionFlags(const MachOObjectFile &O,
   MachO::section Sect = O.getSection(Sec);
   return Sect.flags;
 }
 
 static Expected<MachOObjectFile::LoadCommandInfo>
 getLoadCommandInfo(const MachOObjectFile &Obj, const char *Ptr,
                    uint32_t LoadCommandIndex) {
   if (auto CmdOrErr = getStructOrErr<MachO::load_command>(Obj, Ptr)) {
-    if (CmdOrErr->cmdsize + Ptr > Obj.getData().end())
+    bool Overflowed = false;
+    uintptr_t Ret = llvm::SaturatingAdd<uintptr_t>(
+        CmdOrErr->cmdsize, reinterpret_cast<uintptr_t>(Ptr), &Overflowed);
+
+    if (Overflowed)
+      return malformedError("load command " + Twine(LoadCommandIndex) +
+                            " results in overflowed address");
+    if (Ret > reinterpret_cast<uintptr_t>(Obj.getData().end()))
       return malformedError("load command " + Twine(LoadCommandIndex) +
                             " extends past end of file");
     if (CmdOrErr->cmdsize < 8)
       return malformedError("load command " + Twine(LoadCommandIndex) +
                             " with size less than 8 bytes");
     return MachOObjectFile::LoadCommandInfo({Ptr, *CmdOrErr});
   } else
     return CmdOrErr.takeError();
getFirstLoadCommandInfo(const MachOObjectFile &Obj) {
   return getLoadCommandInfo(Obj, getPtr(Obj, HeaderSize), 0);
 }
 
 static Expected<MachOObjectFile::LoadCommandInfo>
 getNextLoadCommandInfo(const MachOObjectFile &Obj, uint32_t LoadCommandIndex,
                        const MachOObjectFile::LoadCommandInfo &L) {
   unsigned HeaderSize = Obj.is64Bit() ? sizeof(MachO::mach_header_64)
                                       : sizeof(MachO::mach_header);
-  if (L.Ptr + L.C.cmdsize + sizeof(MachO::load_command) >
-      Obj.getData().data() + HeaderSize + Obj.getHeader().sizeofcmds)
+
+  bool Overflowed = false;
+  // FIXME: Perhaps this validation on the Obj file should have been
+  // done somewhere else (earlier).
+  uint32_t LoadCmdsEnd = llvm::SaturatingAdd<uintptr_t>(
+      reinterpret_cast<uintptr_t>(Obj.getData().data()), HeaderSize,
+      &Overflowed);
+  assert(!Overflowed && "Load commands range extends past uint32_t type limit");
+  LoadCmdsEnd = llvm::SaturatingAdd<uintptr_t>(
+      LoadCmdsEnd, Obj.getHeader().sizeofcmds, &Overflowed);
+  assert(!Overflowed && "Load commands range extends past uint32_t type limit");
+
+  uint32_t PtrEnd = llvm::SaturatingAdd<uintptr_t>(
+      reinterpret_cast<uintptr_t>(L.Ptr), L.C.cmdsize, &Overflowed);
+  if (!Overflowed)
+    PtrEnd = llvm::SaturatingAdd<uintptr_t>(PtrEnd, sizeof(MachO::load_command),
+                                            &Overflowed);
+  if (Overflowed)
+    return malformedError("load command " + Twine(LoadCommandIndex + 1) +
+                          " extends past uint32_t type limit.");
+
+  if (PtrEnd > LoadCmdsEnd)
     return malformedError("load command " + Twine(LoadCommandIndex + 1) +
                           " extends past the end all load commands in the file");
   return getLoadCommandInfo(Obj, L.Ptr + L.C.cmdsize, LoadCommandIndex + 1);
 }
 
 template <typename T>
 static void parseHeader(const MachOObjectFile &Obj, T &Header,
                         Error &Err) {