[compiler-rt] abbd4da - [Sanitizers] Fix read buffer overrun in scanning loader commands
Mariusz Borsa via llvm-commits
llvm-commits at lists.llvm.org
Fri Feb 3 18:43:39 PST 2023
Author: Mariusz Borsa
Date: 2023-02-03T18:43:33-08:00
New Revision: abbd4da2043856f443e3d1c8d2c7627cac93a6ac
URL: https://github.com/llvm/llvm-project/commit/abbd4da2043856f443e3d1c8d2c7627cac93a6ac
DIFF: https://github.com/llvm/llvm-project/commit/abbd4da2043856f443e3d1c8d2c7627cac93a6ac.diff
LOG: [Sanitizers] Fix read buffer overrun in scanning loader commands
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
Differential Revision: https://reviews.llvm.org/D142164
Added:
compiler-rt/lib/sanitizer_common/tests/sanitizer_procmaps_mac_test.cpp
Modified:
compiler-rt/lib/sanitizer_common/sanitizer_procmaps.h
compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp
compiler-rt/lib/sanitizer_common/tests/CMakeLists.txt
Removed:
################################################################################
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_procmaps.h b/compiler-rt/lib/sanitizer_common/sanitizer_procmaps.h
index 19bad158387c5..a3887eaac9408 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_procmaps.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_procmaps.h
@@ -65,6 +65,8 @@ class MemoryMappedSegment {
MemoryMappedSegmentData *data_;
};
+struct ImageHeader;
+
class MemoryMappingLayoutBase {
public:
virtual bool Next(MemoryMappedSegment *segment) { UNIMPLEMENTED(); }
@@ -75,10 +77,10 @@ class MemoryMappingLayoutBase {
~MemoryMappingLayoutBase() {}
};
-class MemoryMappingLayout final : public MemoryMappingLayoutBase {
+class MemoryMappingLayout : public MemoryMappingLayoutBase {
public:
explicit MemoryMappingLayout(bool cache_enabled);
- ~MemoryMappingLayout();
+ virtual ~MemoryMappingLayout();
virtual bool Next(MemoryMappedSegment *segment) override;
virtual bool Error() const override;
virtual void Reset() override;
@@ -90,10 +92,14 @@ class MemoryMappingLayout final : public MemoryMappingLayoutBase {
// 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.
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp
index 4b0e678197614..b44e016a0e5bc 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp
@@ -250,7 +250,9 @@ 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;
@@ -358,11 +360,16 @@ static bool IsModuleInstrumented(const load_command *first_lc) {
return false;
}
+const ImageHeader *MemoryMappingLayout::CurrentImageHeader() {
+ const mach_header *hdr = (data_.current_image == kDyldImageIdx)
+ ? get_dyld_hdr()
+ : _dyld_get_image_header(data_.current_image);
+ return (const ImageHeader *)hdr;
+}
+
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 mach_header *hdr = (const mach_header *)CurrentImageHeader();
if (!hdr) continue;
if (data_.current_load_cmd_count < 0) {
// Set up for this image;
@@ -392,7 +399,7 @@ bool MemoryMappingLayout::Next(MemoryMappedSegment *segment) {
(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
@@ -413,6 +420,7 @@ bool MemoryMappingLayout::Next(MemoryMappedSegment *segment) {
}
// 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;
}
diff --git a/compiler-rt/lib/sanitizer_common/tests/CMakeLists.txt b/compiler-rt/lib/sanitizer_common/tests/CMakeLists.txt
index 0c729ecee7152..2ff6d46ee10ee 100644
--- a/compiler-rt/lib/sanitizer_common/tests/CMakeLists.txt
+++ b/compiler-rt/lib/sanitizer_common/tests/CMakeLists.txt
@@ -35,6 +35,7 @@ set(SANITIZER_UNITTESTS
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
diff --git a/compiler-rt/lib/sanitizer_common/tests/sanitizer_procmaps_mac_test.cpp b/compiler-rt/lib/sanitizer_common/tests/sanitizer_procmaps_mac_test.cpp
new file mode 100644
index 0000000000000..f622bba246309
--- /dev/null
+++ b/compiler-rt/lib/sanitizer_common/tests/sanitizer_procmaps_mac_test.cpp
@@ -0,0 +1,137 @@
+//===-- 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
More information about the llvm-commits
mailing list