[Sanitizers] Fix read buffer overrun in scanning loader commands

[wrotki]

Summary

The fix only affects Darwin, but to write the test I had to modify
the MemoryMappingLayout class which is used by all OSes,
to allow for mocking of image header (this change should be NFC). Hence no [Darwin] in the subject
so I can get more eyes on it.

While looking for a memory gap to put the shadow area into, the sanitizer code
scans through the loaded images, and for each image it scans through its
loader command to determine the occupied memory ranges.

While doing so, if the 'segment load' (kLCSegment) loader comand is encountered, the command scanning function
returns success (true), but does not decrement the command list iterator counter.
The result is that the function is called again and again, with the iterator counter
now being too high. The command scanner keeps updating the loader command pointer,
by using the command size field.

If the loop counter is too high, the command pointer
lands into unintended area ( beyond <header addr>+sizeof(mac_header64)+header->sizeofcmds ),
and result depends on the random content found there.

The random content interpreted as loader command might contain a large integer value in the
cmdsize field - this value is added to the current loader command pointer,
which might now point to an inaccessible memory address. It can occasionally result
in a crash if it happens to run beyond the mapped memory segment.

Note that when the area after the loader command list
contains zeros or small integers only, the loop will end normally and the problem
will go unnoticed. So it happened until now since having a some big value
after the header area, falling into command size field is a pretty rare situation.

The fix makes sure that the iterator counter gets updated when the segment load (kLCSegment)
loader command is found too, and in the same code location so the updates will always go together.

Undo the changes in the sanitizer_procmaps_mac.cpp to see the test failing.

rdar://101161047
rdar://102819707

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_procmaps.h

75

CurrentImageHeader could be protected.

[thetruestblue]

compiler-rt/lib/sanitizer_common/tests/CMakeLists.txt

6

When enabling arm64 on Apple devices I added arm64 for Apple only a few lines below this.

if(APPLE)
  list(APPEND SANITIZER_UNITTEST_SUPPORTED_ARCH arm64)
  darwin_filter_host_archs(SANITIZER_UNITTEST_SUPPORTED_ARCH SANITIZER_UNITTEST_SUPPORTED_ARCH)
endif()

If we want to enable unit tests for arm64 universally this is the right place for the change, but I wasn't able to test that. If we want this change here, please remove the line from the apple-specific code:

list(APPEND SANITIZER_UNITTEST_SUPPORTED_ARCH arm64)

[wrotki]

Apply code review feedback

• Removed unnecessary arm64 addition - didn't notice it being added few lines below
• Made MemoryMappingLayout::CurrentImageHeader method protected

[wrotki]

compiler-rt/lib/sanitizer_common/tests/CMakeLists.txt

6

Right, didn't notice this line adding arm64 just few lines below my change. It's right to only do it for Apple builds, at least for now

[vitalybuka]

LGTM, but as it's mostly Mac, leaving to Apple reviewers.

[yln]

LGTM, with a few small nits. Thanks Mariusz!

compiler-rt/lib/sanitizer_common/sanitizer_procmaps.h

77

I think we can push the definition of this down into MemoryMappingLayout to minimize the diff.

104

The opaque return type of this and the necessary casts on both sides (implementation and user) is a bit unfortunate, but I don't see a better way to do insert this abstraction here.

compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp

247

Let's inline this into CurrentImageHeader().

compiler-rt/lib/sanitizer_common/tests/sanitizer_procmaps_mac_test.cpp

14

Copy & paste leftover? I think we can move #if SANITIZER_APPLE up here and drop this #if about Windows.

110

128–135

[wrotki]

Addressed the latest feedback from yln

[wrotki]

compiler-rt/lib/sanitizer_common/sanitizer_procmaps.h

104

Well, yeah, as you say it's unfortunate

[thakis]

It looks like this might have broken hwasan tests on Linux: http://45.33.8.238/linux/98629/step_10.txt

Please take a look and revert for now if it takes a while to fix.

[erikdesjardins]

I also bisected this as the cause of a local build failure on Linux (the same error as this buildbot: https://buildkite.com/llvm-project/llvm-main/builds/6461#01861c4f-9d9c-4781-88f7-d6ccddcb4b06/919-8848)

[yozhu]

Our testing also failed due to the same linker error as seen in the buildbot for this diff:

ld.lld: error: undefined symbol: operator delete(void*)
referenced by sanitizer_procmaps_common.cpp.o:(__sanitizer::MemoryMappingLayout::~MemoryMappingLayout())

[yln]

Thanks everyone for pointing out the build failures on Linux and reverting the change.

@wrotki
I have a hunch that the failure is related to making the MemoryMappingLayout destructor virtual (and the class non-final).

ld.lld: error: undefined symbol: operator delete(void*)
referenced by sanitizer_procmaps_common.cpp.o:(__sanitizer::MemoryMappingLayout::~MemoryMappingLayout())

Also please make sure you are available to deal with potential fallout when you land changes; especially this part of compiler-rt is very brittle and prone to fallout (so landing changes on Friday afternoon is a risky call).

[wrotki]

Thanks for reverting. Julian is right, the failure was caused by the virtual keyword being added to the destructor.

I'm going to re-land this with the destructor made non-virtual, and the warning shown for it by the macOS build disabled. I tested it by running ninja check-sanitizer on linux , which was failing with the virtual dtor.

compiler-rt/lib/sanitizer_common/sanitizer_procmaps.h

 
  private:
   friend class MemoryMappingLayout;
 
   // This field is assigned and owned by MemoryMappingLayout if needed
   MemoryMappedSegmentData *data_;
 };
 
+struct ImageHeader;
+
 class MemoryMappingLayoutBase {
  public:
   virtual bool Next(MemoryMappedSegment *segment) { UNIMPLEMENTED(); }
   virtual bool Error() const { UNIMPLEMENTED(); };
   virtual void Reset() { UNIMPLEMENTED(); }
 
  protected:
   ~MemoryMappingLayoutBase() {}
 };
 
-class MemoryMappingLayout final : public MemoryMappingLayoutBase {
+class MemoryMappingLayout : public MemoryMappingLayoutBase {
  public:
   explicit MemoryMappingLayout(bool cache_enabled);
+
+// This destructor cannot be virtual, as it would cause an operator new() linking
+// failures in hwasan test cases. However non-virtual destructors emit warnings
+// in macOS build, hence disabling those
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wnon-virtual-dtor"
   ~MemoryMappingLayout();
+#pragma clang diagnostic pop
+
   virtual bool Next(MemoryMappedSegment *segment) override;
   virtual bool Error() const override;
   virtual void Reset() override;
   // In some cases, e.g. when running under a sandbox on Linux, ASan is unable
   // to obtain the memory mappings. It should fall back to pre-cached data
   // instead of aborting.
   static void CacheMemoryMappings();
 
   // Adds all mapped objects into a vector.
   void DumpListOfModules(InternalMmapVectorNoCtor<LoadedModule> *modules);
 
+ protected:
+#if SANITIZER_APPLE
+  virtual const ImageHeader *CurrentImageHeader();
+#endif
+  MemoryMappingLayoutData data_;
+
  private:
   void LoadFromCache();
-
-  MemoryMappingLayoutData data_;
 };
 
 // Returns code range for the specified module.
 bool GetCodeRangeForFile(const char *module, uptr *start, uptr *end);
 
 bool IsDecimal(char c);
 uptr ParseDecimal(const char **p);
 bool IsHex(char c);
 uptr ParseHex(const char **p);
 
 }  // namespace __sanitizer
 
 #endif
 #endif  // SANITIZER_PROCMAPS_H

compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp

 }
 
 // Next and NextSegmentLoad were inspired by base/sysinfo.cc in
 // Google Perftools, https://github.com/gperftools/gperftools.
 
 // NextSegmentLoad scans the current image for the next segment load command
 // and returns the start and end addresses and file offset of the corresponding
 // segment.
 // Note that the segment addresses are not necessarily sorted.
 template <u32 kLCSegment, typename SegmentCommand>
 static bool NextSegmentLoad(MemoryMappedSegment *segment,
                             MemoryMappedSegmentData *seg_data,
                             MemoryMappingLayoutData *layout_data) {
   const char *lc = layout_data->current_load_cmd_addr;
+
   layout_data->current_load_cmd_addr += ((const load_command *)lc)->cmdsize;
+  layout_data->current_load_cmd_count--;
   if (((const load_command *)lc)->cmd == kLCSegment) {
     const SegmentCommand* sc = (const SegmentCommand *)lc;
     uptr base_virt_addr, addr_mask;
     if (layout_data->current_image == kDyldImageIdx) {
       base_virt_addr = (uptr)get_dyld_hdr();
       // vmaddr is masked with 0xfffff because on macOS versions < 10.12,
       // it contains an absolute address rather than an offset for dyld.
       // To make matters even more complicated, this absolute address
for (const load_command *lc = first_lc; lc->cmd != 0; lc = NextCommand(lc)) {
     dylib_name = StripModuleName(dylib_name);
     if (dylib_name != 0 && (internal_strstr(dylib_name, "libclang_rt."))) {
       return true;
     }
   }
   return false;
 }
 
-bool MemoryMappingLayout::Next(MemoryMappedSegment *segment) {
-  for (; data_.current_image >= kDyldImageIdx; data_.current_image--) {
-    const mach_header *hdr = (data_.current_image == kDyldImageIdx)
-                                 ? get_dyld_hdr()
-                                 : _dyld_get_image_header(data_.current_image);
+const ImageHeader *MemoryMappingLayout::CurrentImageHeader() {
+  return (const ImageHeader *)hdr;
+}
+
+bool MemoryMappingLayout::Next(MemoryMappedSegment *segment) {
+  for (; data_.current_image >= kDyldImageIdx; data_.current_image--) {
+    const mach_header *hdr = (const mach_header *)CurrentImageHeader();
     if (!hdr) continue;
     if (data_.current_load_cmd_count < 0) {
       // Set up for this image;
       data_.current_load_cmd_count = hdr->ncmds;
       data_.current_magic = hdr->magic;
       data_.current_filetype = hdr->filetype;
       data_.current_arch = ModuleArchFromCpuType(hdr->cputype, hdr->cpusubtype);
       switch (data_.current_magic) {
#endif
         }
       }
       FindUUID((const load_command *)data_.current_load_cmd_addr,
                data_.current_uuid);
       data_.current_instrumented = IsModuleInstrumented(
           (const load_command *)data_.current_load_cmd_addr);
     }
 
-    for (; data_.current_load_cmd_count >= 0; data_.current_load_cmd_count--) {
+    while (data_.current_load_cmd_count > 0) {
       switch (data_.current_magic) {
         // data_.current_magic may be only one of MH_MAGIC, MH_MAGIC_64.
 #ifdef MH_MAGIC_64
         case MH_MAGIC_64: {
           if (NextSegmentLoad<LC_SEGMENT_64, struct segment_command_64>(
                   segment, segment->data_, &data_))
             return true;
           break;
         }
 #endif
         case MH_MAGIC: {
           if (NextSegmentLoad<LC_SEGMENT, struct segment_command>(
                   segment, segment->data_, &data_))
             return true;
           break;
         }
       }
     }
     // If we get here, no more load_cmd's in this image talk about
     // segments.  Go on to the next image.
+    data_.current_load_cmd_count = -1; // This will trigger loading next image
   }
   return false;
 }
 
 void MemoryMappingLayout::DumpListOfModules(
     InternalMmapVectorNoCtor<LoadedModule> *modules) {
   Reset();
   InternalMmapVector<char> module_name(kMaxPathLength);

compiler-rt/lib/sanitizer_common/tests/CMakeLists.txt

 include(CompilerRTCompile)
 
 clang_compiler_add_cxx_check()
 
 # FIXME: use SANITIZER_COMMON_SUPPORTED_ARCH here
 filter_available_targets(SANITIZER_UNITTEST_SUPPORTED_ARCH x86_64 i386 mips64 mips64el riscv64 sparcv9 sparc)
 if(APPLE)
   list(APPEND SANITIZER_UNITTEST_SUPPORTED_ARCH arm64)
   darwin_filter_host_archs(SANITIZER_UNITTEST_SUPPORTED_ARCH SANITIZER_UNITTEST_SUPPORTED_ARCH)
 endif()
 
 set(SANITIZER_UNITTESTS
   sanitizer_addrhashmap_test.cpp
   sanitizer_allocator_test.cpp
set(SANITIZER_UNITTESTS
   sanitizer_list_test.cpp
   sanitizer_lzw_test.cpp
   sanitizer_mac_test.cpp
   sanitizer_mutex_test.cpp
   sanitizer_nolibc_test.cpp
   sanitizer_posix_test.cpp
   sanitizer_printf_test.cpp
   sanitizer_procmaps_test.cpp
+  sanitizer_procmaps_mac_test.cpp
   sanitizer_ring_buffer_test.cpp
   sanitizer_quarantine_test.cpp
   sanitizer_stack_store_test.cpp
   sanitizer_stackdepot_test.cpp
   sanitizer_stacktrace_printer_test.cpp
   sanitizer_stacktrace_test.cpp
   sanitizer_stoptheworld_test.cpp
   sanitizer_suppressions_test.cpp

compiler-rt/lib/sanitizer_common/tests/sanitizer_procmaps_mac_test.cpp

+//===-- sanitizer_procmaps_mac_test.cpp ---------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file is a part of ThreadSanitizer/AddressSanitizer runtime.
+//
+//===----------------------------------------------------------------------===//
+
+#  include "sanitizer_common/sanitizer_platform.h"
+
+#  if SANITIZER_APPLE
+
+#  include <stdlib.h>
+#  include <string.h>
+#  include <stdint.h>
+#  include <stdio.h>
+
+#  include <vector>
+#  include <mach-o/dyld.h>
+#  include <mach-o/loader.h>
+
+#  include "gtest/gtest.h"
+
+#  include "sanitizer_common/sanitizer_procmaps.h"
+
+namespace __sanitizer {
+
+class MemoryMappingLayoutMock final : public MemoryMappingLayout {
+private:
+  static constexpr uuid_command mock_uuid_command = {
+    .cmd = LC_UUID,
+    .cmdsize = sizeof(uuid_command),
+    .uuid = {}
+  };
+
+  static constexpr char dylib_name[] = "libclang_rt.\0\0\0"; // 8 bytes aligned, padded with zeros per loader.h
+  static constexpr dylib_command mock_dylib_command = {
+    .cmd = LC_LOAD_DYLIB,
+    .cmdsize = sizeof(dylib_command) + sizeof(dylib_name),
+    .dylib = {
+      .name = {
+        .offset = sizeof(dylib_command)
+      }
+    }
+  };
+
+  static constexpr uuid_command mock_trap_command = {
+    .cmd = LC_UUID,
+    .cmdsize = 0x10000,
+    .uuid = {}
+  };
+
+  const char *start_load_cmd_addr;
+  size_t sizeofcmds;
+  std::vector<unsigned char> mock_header;
+
+public:
+  MemoryMappingLayoutMock(): MemoryMappingLayout(false) {
+    EXPECT_EQ(mock_uuid_command.cmdsize % 8, 0u);
+    EXPECT_EQ(mock_dylib_command.cmdsize % 8, 0u);
+
+    Reset();
+
+#ifdef MH_MAGIC_64
+    const struct mach_header_64 *header = (mach_header_64 *)_dyld_get_image_header(0); // Any header will do
+    const size_t header_size = sizeof(mach_header_64);
+#else
+    const struct mach_header *header = _dyld_get_image_header(0);
+    const size_t header_size = sizeof(mach_header);
+#endif
+    const size_t mock_header_size_with_extras = header_size + header->sizeofcmds +
+      mock_uuid_command.cmdsize + mock_dylib_command.cmdsize + sizeof(uuid_command);
+
+    mock_header.reserve(mock_header_size_with_extras);
+    // Copy the original header
+    copy((unsigned char *)header,
+      (unsigned char *)header + header_size + header->sizeofcmds,
+      back_inserter(mock_header));
+    // The following commands are not supposed to be processed
+    // by the (correct) ::Next method at all, since they're not
+    // accounted for in header->ncmds .
+    copy((unsigned char *)&mock_uuid_command,
+      ((unsigned char *)&mock_uuid_command) + mock_uuid_command.cmdsize,
+      back_inserter(mock_header));
+    copy((unsigned char *)&mock_dylib_command,
+      ((unsigned char *)&mock_dylib_command) + sizeof(dylib_command), // as mock_dylib_command.cmdsize contains the following string
+      back_inserter(mock_header));
+    copy((unsigned char *)dylib_name,
+      ((unsigned char *)dylib_name) + sizeof(dylib_name),
+      back_inserter(mock_header));
+
+    // Append a command w. huge size to have the test detect the read overrun
+    copy((unsigned char *)&mock_trap_command,
+      ((unsigned char *)&mock_trap_command) + sizeof(uuid_command),
+      back_inserter(mock_header));
+
+    start_load_cmd_addr = (const char *)(mock_header.data() + header_size);
+    sizeofcmds = header->sizeofcmds;
+
+    const char *last_byte_load_cmd_addr = (start_load_cmd_addr+sizeofcmds-1);
+    data_.current_image = -1; // So the loop in ::Next runs just once
+  }
+
+  size_t SizeOfLoadCommands() {
+    return sizeofcmds;
+  }
+
+  size_t CurrentLoadCommandOffset() {
+    size_t offset = data_.current_load_cmd_addr - start_load_cmd_addr;
+    return offset;
+  }
+
+protected:
+  virtual ImageHeader *CurrentImageHeader() override {
+    return (ImageHeader *)mock_header.data();
+  }
+};
+
+TEST(MemoryMappingLayout, Next) {
+  __sanitizer::MemoryMappingLayoutMock memory_mapping;
+  __sanitizer::MemoryMappedSegment segment;
+  size_t size = memory_mapping.SizeOfLoadCommands();
+  while (memory_mapping.Next(&segment)) {
+    size_t offset = memory_mapping.CurrentLoadCommandOffset();
+    EXPECT_LE(offset, size);
+  }
+  size_t final_offset = memory_mapping.CurrentLoadCommandOffset();
+  EXPECT_EQ(final_offset, size); // All commands processed, no more, no less
+}
+
+}  // namespace __sanitizer
+
+#  endif // SANITIZER_APPLE