Skip to content

Commit 82d852c

Browse files
author
Mariusz Borsa
committed
[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 +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
1 parent 42129de commit 82d852c

File tree

4 files changed

+167
-7
lines changed

4 files changed

+167
-7
lines changed

compiler-rt/lib/sanitizer_common/sanitizer_procmaps.h

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,8 @@ class MemoryMappedSegment {
6565
MemoryMappedSegmentData *data_;
6666
};
6767

68+
struct ImageHeader;
69+
6870
class MemoryMappingLayoutBase {
6971
public:
7072
virtual bool Next(MemoryMappedSegment *segment) { UNIMPLEMENTED(); }
@@ -75,10 +77,18 @@ class MemoryMappingLayoutBase {
7577
~MemoryMappingLayoutBase() {}
7678
};
7779

78-
class MemoryMappingLayout final : public MemoryMappingLayoutBase {
80+
class MemoryMappingLayout : public MemoryMappingLayoutBase {
7981
public:
8082
explicit MemoryMappingLayout(bool cache_enabled);
83+
84+
// This destructor cannot be virtual, as it would cause an operator new() linking
85+
// failures in hwasan test cases. However non-virtual destructors emit warnings
86+
// in macOS build, hence disabling those
87+
#pragma clang diagnostic push
88+
#pragma clang diagnostic ignored "-Wnon-virtual-dtor"
8189
~MemoryMappingLayout();
90+
#pragma clang diagnostic pop
91+
8292
virtual bool Next(MemoryMappedSegment *segment) override;
8393
virtual bool Error() const override;
8494
virtual void Reset() override;
@@ -90,10 +100,14 @@ class MemoryMappingLayout final : public MemoryMappingLayoutBase {
90100
// Adds all mapped objects into a vector.
91101
void DumpListOfModules(InternalMmapVectorNoCtor<LoadedModule> *modules);
92102

103+
protected:
104+
#if SANITIZER_APPLE
105+
virtual const ImageHeader *CurrentImageHeader();
106+
#endif
107+
MemoryMappingLayoutData data_;
108+
93109
private:
94110
void LoadFromCache();
95-
96-
MemoryMappingLayoutData data_;
97111
};
98112

99113
// Returns code range for the specified module.

compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -250,7 +250,9 @@ static bool NextSegmentLoad(MemoryMappedSegment *segment,
250250
MemoryMappedSegmentData *seg_data,
251251
MemoryMappingLayoutData *layout_data) {
252252
const char *lc = layout_data->current_load_cmd_addr;
253+
253254
layout_data->current_load_cmd_addr += ((const load_command *)lc)->cmdsize;
255+
layout_data->current_load_cmd_count--;
254256
if (((const load_command *)lc)->cmd == kLCSegment) {
255257
const SegmentCommand* sc = (const SegmentCommand *)lc;
256258
uptr base_virt_addr, addr_mask;
@@ -358,11 +360,16 @@ static bool IsModuleInstrumented(const load_command *first_lc) {
358360
return false;
359361
}
360362

363+
const ImageHeader *MemoryMappingLayout::CurrentImageHeader() {
364+
const mach_header *hdr = (data_.current_image == kDyldImageIdx)
365+
? get_dyld_hdr()
366+
: _dyld_get_image_header(data_.current_image);
367+
return (const ImageHeader *)hdr;
368+
}
369+
361370
bool MemoryMappingLayout::Next(MemoryMappedSegment *segment) {
362371
for (; data_.current_image >= kDyldImageIdx; data_.current_image--) {
363-
const mach_header *hdr = (data_.current_image == kDyldImageIdx)
364-
? get_dyld_hdr()
365-
: _dyld_get_image_header(data_.current_image);
372+
const mach_header *hdr = (const mach_header *)CurrentImageHeader();
366373
if (!hdr) continue;
367374
if (data_.current_load_cmd_count < 0) {
368375
// Set up for this image;
@@ -392,7 +399,7 @@ bool MemoryMappingLayout::Next(MemoryMappedSegment *segment) {
392399
(const load_command *)data_.current_load_cmd_addr);
393400
}
394401

395-
for (; data_.current_load_cmd_count >= 0; data_.current_load_cmd_count--) {
402+
while (data_.current_load_cmd_count > 0) {
396403
switch (data_.current_magic) {
397404
// data_.current_magic may be only one of MH_MAGIC, MH_MAGIC_64.
398405
#ifdef MH_MAGIC_64
@@ -413,6 +420,7 @@ bool MemoryMappingLayout::Next(MemoryMappedSegment *segment) {
413420
}
414421
// If we get here, no more load_cmd's in this image talk about
415422
// segments. Go on to the next image.
423+
data_.current_load_cmd_count = -1; // This will trigger loading next image
416424
}
417425
return false;
418426
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ set(SANITIZER_UNITTESTS
3535
sanitizer_posix_test.cpp
3636
sanitizer_printf_test.cpp
3737
sanitizer_procmaps_test.cpp
38+
sanitizer_procmaps_mac_test.cpp
3839
sanitizer_ring_buffer_test.cpp
3940
sanitizer_quarantine_test.cpp
4041
sanitizer_stack_store_test.cpp
Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
//===-- sanitizer_procmaps_mac_test.cpp ---------------------------------------===//
2+
//
3+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
4+
// See https://llvm.org/LICENSE.txt for license information.
5+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
6+
//
7+
//===----------------------------------------------------------------------===//
8+
//
9+
// This file is a part of ThreadSanitizer/AddressSanitizer runtime.
10+
//
11+
//===----------------------------------------------------------------------===//
12+
13+
# include "sanitizer_common/sanitizer_platform.h"
14+
15+
# if SANITIZER_APPLE
16+
17+
# include <stdlib.h>
18+
# include <string.h>
19+
# include <stdint.h>
20+
# include <stdio.h>
21+
22+
# include <vector>
23+
# include <mach-o/dyld.h>
24+
# include <mach-o/loader.h>
25+
26+
# include "gtest/gtest.h"
27+
28+
# include "sanitizer_common/sanitizer_procmaps.h"
29+
30+
namespace __sanitizer {
31+
32+
class MemoryMappingLayoutMock final : public MemoryMappingLayout {
33+
private:
34+
static constexpr uuid_command mock_uuid_command = {
35+
.cmd = LC_UUID,
36+
.cmdsize = sizeof(uuid_command),
37+
.uuid = {}
38+
};
39+
40+
static constexpr char dylib_name[] = "libclang_rt.\0\0\0"; // 8 bytes aligned, padded with zeros per loader.h
41+
static constexpr dylib_command mock_dylib_command = {
42+
.cmd = LC_LOAD_DYLIB,
43+
.cmdsize = sizeof(dylib_command) + sizeof(dylib_name),
44+
.dylib = {
45+
.name = {
46+
.offset = sizeof(dylib_command)
47+
}
48+
}
49+
};
50+
51+
static constexpr uuid_command mock_trap_command = {
52+
.cmd = LC_UUID,
53+
.cmdsize = 0x10000,
54+
.uuid = {}
55+
};
56+
57+
const char *start_load_cmd_addr;
58+
size_t sizeofcmds;
59+
std::vector<unsigned char> mock_header;
60+
61+
public:
62+
MemoryMappingLayoutMock(): MemoryMappingLayout(false) {
63+
EXPECT_EQ(mock_uuid_command.cmdsize % 8, 0u);
64+
EXPECT_EQ(mock_dylib_command.cmdsize % 8, 0u);
65+
66+
Reset();
67+
68+
#ifdef MH_MAGIC_64
69+
const struct mach_header_64 *header = (mach_header_64 *)_dyld_get_image_header(0); // Any header will do
70+
const size_t header_size = sizeof(mach_header_64);
71+
#else
72+
const struct mach_header *header = _dyld_get_image_header(0);
73+
const size_t header_size = sizeof(mach_header);
74+
#endif
75+
const size_t mock_header_size_with_extras = header_size + header->sizeofcmds +
76+
mock_uuid_command.cmdsize + mock_dylib_command.cmdsize + sizeof(uuid_command);
77+
78+
mock_header.reserve(mock_header_size_with_extras);
79+
// Copy the original header
80+
copy((unsigned char *)header,
81+
(unsigned char *)header + header_size + header->sizeofcmds,
82+
back_inserter(mock_header));
83+
// The following commands are not supposed to be processed
84+
// by the (correct) ::Next method at all, since they're not
85+
// accounted for in header->ncmds .
86+
copy((unsigned char *)&mock_uuid_command,
87+
((unsigned char *)&mock_uuid_command) + mock_uuid_command.cmdsize,
88+
back_inserter(mock_header));
89+
copy((unsigned char *)&mock_dylib_command,
90+
((unsigned char *)&mock_dylib_command) + sizeof(dylib_command), // as mock_dylib_command.cmdsize contains the following string
91+
back_inserter(mock_header));
92+
copy((unsigned char *)dylib_name,
93+
((unsigned char *)dylib_name) + sizeof(dylib_name),
94+
back_inserter(mock_header));
95+
96+
// Append a command w. huge size to have the test detect the read overrun
97+
copy((unsigned char *)&mock_trap_command,
98+
((unsigned char *)&mock_trap_command) + sizeof(uuid_command),
99+
back_inserter(mock_header));
100+
101+
start_load_cmd_addr = (const char *)(mock_header.data() + header_size);
102+
sizeofcmds = header->sizeofcmds;
103+
104+
const char *last_byte_load_cmd_addr = (start_load_cmd_addr+sizeofcmds-1);
105+
data_.current_image = -1; // So the loop in ::Next runs just once
106+
}
107+
108+
size_t SizeOfLoadCommands() {
109+
return sizeofcmds;
110+
}
111+
112+
size_t CurrentLoadCommandOffset() {
113+
size_t offset = data_.current_load_cmd_addr - start_load_cmd_addr;
114+
return offset;
115+
}
116+
117+
protected:
118+
virtual ImageHeader *CurrentImageHeader() override {
119+
return (ImageHeader *)mock_header.data();
120+
}
121+
};
122+
123+
TEST(MemoryMappingLayout, Next) {
124+
__sanitizer::MemoryMappingLayoutMock memory_mapping;
125+
__sanitizer::MemoryMappedSegment segment;
126+
size_t size = memory_mapping.SizeOfLoadCommands();
127+
while (memory_mapping.Next(&segment)) {
128+
size_t offset = memory_mapping.CurrentLoadCommandOffset();
129+
EXPECT_LE(offset, size);
130+
}
131+
size_t final_offset = memory_mapping.CurrentLoadCommandOffset();
132+
EXPECT_EQ(final_offset, size); // All commands processed, no more, no less
133+
}
134+
135+
} // namespace __sanitizer
136+
137+
# endif // SANITIZER_APPLE

0 commit comments

Comments
 (0)