Skip to content

Commit 158cde0

Browse files
authored
Merge pull request #6246 from apple/m_borsa/fix_102819707
[Sanitizers] Fix read buffer overrun in scanning loader commands
2 parents 1cc1bcb + 928defa commit 158cde0

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
@@ -34,6 +34,7 @@ set(SANITIZER_UNITTESTS
3434
sanitizer_posix_test.cpp
3535
sanitizer_printf_test.cpp
3636
sanitizer_procmaps_test.cpp
37+
sanitizer_procmaps_mac_test.cpp
3738
sanitizer_ring_buffer_test.cpp
3839
sanitizer_quarantine_test.cpp
3940
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)