[ASan][Win][compiler-rt] Fixes stack overflow when ntdll has mem* calls during exception handling #120110
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
compiler-rt
compiler-rt:asan
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Zack Johnson (zacklj89) ChangesThis modifies intrinsic interception behavior in x64 Windows environments to defer back to the The current behavior on newer versions of Windows will be a stack overflow by virtue of Full diff: https://github.com/llvm/llvm-project/pull/120110.diff 5 Files Affected:
diff --git a/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp b/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp
index bdf328f8920634..0e27d868dff9b7 100644
--- a/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp
+++ b/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp
@@ -20,43 +20,88 @@
#include "asan_stack.h"
#include "asan_suppressions.h"
+#if SANITIZER_WINDOWS64
+# include "asan_poisoning.h"
+# include "sanitizer_common/sanitizer_win.h"
+#endif
+
+namespace __asan {
+// On x64, the ShadowExceptionHandler is expected to handle all AVs that happen
+// as a result of uncommitted shadow memory pages. However, in programs that use
+// ntdll (a Windows-specific library that contains some memory intrinsics as
+// well as Windows-specific exception handling mechanisms) as their C Runtime,
+// or in cases where ntdll uses mem* functions inside
+// its exception handling infrastructure, ASAN can end up rethrowing a shadow
+// memory AV until a stack overflow occurs. In other words, ntdll can call back
+// into ASAN for a poisoning check, which creates infinite recursion. To remedy
+// this, we precommit the shadow memory of the address being accessed on x64 for
+// ntdll callees.
+bool ShouldReplaceIntrinsic(bool isNtdllCallee, void *addr, uptr size,
+ const void *from = nullptr) {
+#if SANITIZER_WINDOWS64
+ if (isNtdllCallee) {
+ CommitShadowMemory(reinterpret_cast<uptr>(addr), size);
+ if (from) {
+ CommitShadowMemory(reinterpret_cast<uptr>(from), size);
+ }
+ }
+#endif
+ return replace_intrin_cached;
+}
+} // namespace __asan
+
using namespace __asan;
+#if SANITIZER_WINDOWS64
+#define IS_NTDLL_CALLEE __sanitizer::IsNtdllCallee(_ReturnAddress())
+#else
+#define IS_NTDLL_CALLEE false
+#endif
+
// memcpy is called during __asan_init() from the internals of printf(...).
// We do not treat memcpy with to==from as a bug.
// See http://llvm.org/bugs/show_bug.cgi?id=11763.
-#define ASAN_MEMCPY_IMPL(ctx, to, from, size) \
- do { \
- if (LIKELY(replace_intrin_cached)) { \
- if (LIKELY(to != from)) { \
- CHECK_RANGES_OVERLAP("memcpy", to, size, from, size); \
- } \
- ASAN_READ_RANGE(ctx, from, size); \
- ASAN_WRITE_RANGE(ctx, to, size); \
- } else if (UNLIKELY(!AsanInited())) { \
- return internal_memcpy(to, from, size); \
- } \
- return REAL(memcpy)(to, from, size); \
+#define ASAN_MEMCPY_IMPL(ctx, to, from, size) \
+ do { \
+ if (!ShouldReplaceIntrinsic(IS_NTDLL_CALLEE, to, size, from)) { \
+ return REAL(memcpy)(to, from, size); \
+ } \
+ if (LIKELY(replace_intrin_cached)) { \
+ if (LIKELY(to != from)) { \
+ CHECK_RANGES_OVERLAP("memcpy", to, size, from, size); \
+ } \
+ ASAN_READ_RANGE(ctx, from, size); \
+ ASAN_WRITE_RANGE(ctx, to, size); \
+ } else if (UNLIKELY(!AsanInited())) { \
+ return internal_memcpy(to, from, size); \
+ } \
+ return REAL(memcpy)(to, from, size); \
} while (0)
// memset is called inside Printf.
-#define ASAN_MEMSET_IMPL(ctx, block, c, size) \
- do { \
- if (LIKELY(replace_intrin_cached)) { \
- ASAN_WRITE_RANGE(ctx, block, size); \
- } else if (UNLIKELY(!AsanInited())) { \
- return internal_memset(block, c, size); \
- } \
- return REAL(memset)(block, c, size); \
+#define ASAN_MEMSET_IMPL(ctx, block, c, size) \
+ do { \
+ if (!ShouldReplaceIntrinsic(IS_NTDLL_CALLEE, block, size)) { \
+ return REAL(memset)(block, c, size); \
+ } \
+ if (LIKELY(replace_intrin_cached)) { \
+ ASAN_WRITE_RANGE(ctx, block, size); \
+ } else if (UNLIKELY(!AsanInited())) { \
+ return internal_memset(block, c, size); \
+ } \
+ return REAL(memset)(block, c, size); \
} while (0)
-#define ASAN_MEMMOVE_IMPL(ctx, to, from, size) \
- do { \
- if (LIKELY(replace_intrin_cached)) { \
- ASAN_READ_RANGE(ctx, from, size); \
- ASAN_WRITE_RANGE(ctx, to, size); \
- } \
- return internal_memmove(to, from, size); \
+#define ASAN_MEMMOVE_IMPL(ctx, to, from, size) \
+ do { \
+ if (!ShouldReplaceIntrinsic(IS_NTDLL_CALLEE, to, size, from)) { \
+ return internal_memmove(to, from, size); \
+ } \
+ if (LIKELY(replace_intrin_cached)) { \
+ ASAN_READ_RANGE(ctx, from, size); \
+ ASAN_WRITE_RANGE(ctx, to, size); \
+ } \
+ return internal_memmove(to, from, size); \
} while (0)
void *__asan_memcpy(void *to, const void *from, uptr size) {
diff --git a/compiler-rt/lib/asan/asan_poisoning.h b/compiler-rt/lib/asan/asan_poisoning.h
index 600bd011f304cb..7fb12b4d828b5c 100644
--- a/compiler-rt/lib/asan/asan_poisoning.h
+++ b/compiler-rt/lib/asan/asan_poisoning.h
@@ -17,6 +17,25 @@
#include "sanitizer_common/sanitizer_flags.h"
#include "sanitizer_common/sanitizer_platform.h"
+#if SANITIZER_WINDOWS64
+# include "sanitizer_common/sanitizer_win.h"
+# include "sanitizer_common/sanitizer_win_defs.h"
+
+// These definitions are duplicated from Window.h in order to avoid conflicts
+// with other types in Windows.h.
+// These functions and types are used to manipulate the shadow memory on
+// x64 Windows.
+typedef unsigned long DWORD;
+typedef void *LPVOID;
+typedef int BOOL;
+
+constexpr DWORD MEM_COMMIT = 0x00001000;
+constexpr DWORD MEM_DECOMMIT = 0x00004000;
+constexpr DWORD PAGE_READWRITE = 0x04;
+
+extern "C" LPVOID WINAPI VirtualAlloc(LPVOID, size_t, DWORD, DWORD);
+#endif
+
namespace __asan {
// Enable/disable memory poisoning.
@@ -95,4 +114,16 @@ ALWAYS_INLINE void FastPoisonShadowPartialRightRedzone(
// [MemToShadow(p), MemToShadow(p+size)].
void FlushUnneededASanShadowMemory(uptr p, uptr size);
+// Commits the shadow memory for a range of aligned memory. This only matters
+// on 64-bit Windows where relying on pages to get paged in on access
+// violation is inefficient when we know the memory range ahead of time.
+ALWAYS_INLINE void CommitShadowMemory(uptr aligned_beg, uptr aligned_size) {
+#if SANITIZER_WINDOWS64
+ uptr shadow_beg = MEM_TO_SHADOW(aligned_beg);
+ uptr shadow_end =
+ MEM_TO_SHADOW(aligned_beg + aligned_size - ASAN_SHADOW_GRANULARITY) + 1;
+ ::VirtualAlloc((LPVOID)shadow_beg, (size_t)(shadow_end - shadow_beg),
+ MEM_COMMIT, PAGE_READWRITE);
+#endif
+}
} // namespace __asan
diff --git a/compiler-rt/lib/asan/asan_rtl.cpp b/compiler-rt/lib/asan/asan_rtl.cpp
index 19c6c210b564c5..7c88ec2426a16a 100644
--- a/compiler-rt/lib/asan/asan_rtl.cpp
+++ b/compiler-rt/lib/asan/asan_rtl.cpp
@@ -463,6 +463,10 @@ static bool AsanInitInternal() {
if (SANITIZER_START_BACKGROUND_THREAD_IN_ASAN_INTERNAL)
MaybeStartBackgroudThread();
+#if SANITIZER_WINDOWS64
+ __sanitizer::InitializeNtdllInfo();
+#endif
+
// On Linux AsanThread::ThreadStart() calls malloc() that's why asan_inited
// should be set to 1 prior to initializing the threads.
replace_intrin_cached = flags()->replace_intrin;
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_win.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_win.cpp
index fd0f989ee392bb..e3ecdd958bdfb7 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_win.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_win.cpp
@@ -1264,6 +1264,18 @@ void LogFullErrorReport(const char *buffer) {
void InitializePlatformCommonFlags(CommonFlags *cf) {}
+static MODULEINFO ntdllInfo;
+void InitializeNtdllInfo() {
+ GetModuleInformation(GetCurrentProcess(), GetModuleHandle(L"ntdll.dll"),
+ &ntdllInfo, sizeof(ntdllInfo));
+}
+
+bool IsNtdllCallee(void *calleeAddr) {
+ return (uptr)ntdllInfo.lpBaseOfDll <= (uptr)calleeAddr &&
+ ((uptr)ntdllInfo.lpBaseOfDll + (uptr)ntdllInfo.SizeOfImage) >=
+ (uptr)calleeAddr;
+}
+
} // namespace __sanitizer
#endif // _WIN32
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_win.h b/compiler-rt/lib/sanitizer_common/sanitizer_win.h
index ff8939ca5e8557..e1080f4ce86d32 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_win.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_win.h
@@ -19,6 +19,12 @@
namespace __sanitizer {
// Check based on flags if we should handle the exception.
bool IsHandledDeadlyException(DWORD exceptionCode);
+
+// Initializes module information of ntdll for referencing callee addresses
+void InitializeNtdllInfo();
+
+// Returns whether or not the callee address lies within ntdll
+bool IsNtdllCallee(void* calleeAddr);
} // namespace __sanitizer
#endif // SANITIZER_WINDOWS
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Looks like there are some formatting changes inconsistent here. I'll fix those as soon as I can. |
zmodem
reviewed
Dec 17, 2024
There was a problem hiding this comment.
Thanks for looking into this!
This sounds like an alternative to the patch Mozilla proposed in the bug report at #114793 which I'm pretty sure is the same issue: https://phabricator.services.mozilla.com/D228165
As discussed on that bug, I think the better fix is to not intercept memset or any other crt functions in ntdll at all.
The tricky part is that there are many calls to intercept functions in the runtime, and we'd need to decide which instance of those to intercept at each point.
I'll post an update on the bug.
|
I'm closing this since #120397 was merged. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This modifies intrinsic interception behavior in x64 Windows environments to defer back to the
REAL(mem*)functions during exception handling machinery or if shadow memory needs to be committed.The current behavior on newer versions of Windows will be a stack overflow by virtue of
ntdll!memsetbeing called during exception handling. This should fix this chromium issue.