[compiler-rt] 78c033b - [sanitizers][windows] Correctly override functions with backward jmps

[Markus Böck via llvm-commits]

Author: Markus Böck
Date: 2022-12-12T11:45:43+01:00
New Revision: 78c033b541f00d12b7e1ba2a676b02e022c9beb1

URL: https://github.com/llvm/llvm-project/commit/78c033b541f00d12b7e1ba2a676b02e022c9beb1
DIFF: https://github.com/llvm/llvm-project/commit/78c033b541f00d12b7e1ba2a676b02e022c9beb1.diff

LOG: [sanitizers][windows] Correctly override functions with backward jmps

To reproduce: Download and run the latest Firefox ASAN build (https://firefox-ci-tc.services.mozilla.com/api/index/v1/task/gecko.v2.mozilla-central.latest.firefox.win64-asan-opt/artifacts/public/build/target.zip) on Windows 11 (version 10.0.22621 Build 22621); it will crash on launch. Note that this doesn't seem to crash on another Windows 11 VM I've tried, so I'm not sure how reproducible it is across machines, but it reproduces on my machine every time.

The problem seems to be that when overriding the memset function in OverrideFunctionWithRedirectJump(), the relative_offset is stored as a uptr. Per the Intel x64 instruction set reference (https://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-software-developer-instruction-set-reference-manual-325383.pdf - warning: large PDF), on page 646 the jmp instruction (specifically the near jump flavors that start with E9, which are the ones the OverrideFunctionWithRedirectJump() considers) treats the offset as a signed displacement. This causes an incorrect value to be stored for REAL(memset) which points to uninitialized memory, and a crash the next time that gets called.

The fix is to simply treat that offset as signed. I have also added a test case.

Fixes https://github.com/llvm/llvm-project/issues/58846

Differential Revision: https://reviews.llvm.org/D137788

Added: 
    

Modified: 
    compiler-rt/lib/interception/interception_win.cpp
    compiler-rt/lib/interception/tests/interception_win_test.cpp

Removed: 
    


################################################################################
diff  --git a/compiler-rt/lib/interception/interception_win.cpp b/compiler-rt/lib/interception/interception_win.cpp
index d0db981d519cb..faaa8ee15381d 100644
--- a/compiler-rt/lib/interception/interception_win.cpp
+++ b/compiler-rt/lib/interception/interception_win.cpp
@@ -738,7 +738,7 @@ bool OverrideFunctionWithRedirectJump(
     return false;
 
   if (orig_old_func) {
-    uptr relative_offset = *(u32*)(old_func + 1);
+    sptr relative_offset = *(s32 *)(old_func + 1);
     uptr absolute_target = old_func + relative_offset + kJumpInstructionLength;
     *orig_old_func = absolute_target;
   }

diff  --git a/compiler-rt/lib/interception/tests/interception_win_test.cpp b/compiler-rt/lib/interception/tests/interception_win_test.cpp
index 084a986029691..01b8d3132c37c 100644
--- a/compiler-rt/lib/interception/tests/interception_win_test.cpp
+++ b/compiler-rt/lib/interception/tests/interception_win_test.cpp
@@ -85,7 +85,16 @@ const u8 kIdentityCodeWithJump[] = {
     0xC3,                   // ret
 };
 
-#else
+const u8 kIdentityCodeWithJumpBackwards[] = {
+    0x89, 0xC8,  // mov         eax, ecx
+    0xC3,        // ret
+    0xE9, 0xF8, 0xFF, 0xFF,
+    0xFF,  // jmp - 8
+    0xCC, 0xCC, 0xCC, 0xCC,
+};
+const u8 kIdentityCodeWithJumpBackwardsOffset = 3;
+
+#    else
 
 const u8 kIdentityCodeWithPrologue[] = {
     0x55,                   // push        ebp
@@ -134,7 +143,16 @@ const u8 kIdentityCodeWithJump[] = {
     0xC3,                   // ret
 };
 
-#endif
+const u8 kIdentityCodeWithJumpBackwards[] = {
+    0x8B, 0x44, 0x24, 0x04,  // mov         eax,dword ptr [esp + 4]
+    0xC3,                    // ret
+    0xE9, 0xF6, 0xFF, 0xFF,
+    0xFF,  // jmp - 10
+    0xCC, 0xCC, 0xCC, 0xCC,
+};
+const u8 kIdentityCodeWithJumpBackwardsOffset = 5;
+
+#    endif
 
 const u8 kPatchableCode1[] = {
     0xB8, 0x4B, 0x00, 0x00, 0x00,   // mov eax,4B
@@ -366,13 +384,14 @@ TEST(Interception, InternalGetProcAddress) {
   EXPECT_NE(DbgPrint_adddress, isdigit_address);
 }
 
-template<class T>
+template <class T>
 static void TestIdentityFunctionPatching(
-    const T &code,
-    TestOverrideFunction override,
-    FunctionPrefixKind prefix_kind = FunctionPrefixNone) {
+    const T &code, TestOverrideFunction override,
+    FunctionPrefixKind prefix_kind = FunctionPrefixNone,
+    int function_start_offset = 0) {
   uptr identity_address;
   LoadActiveCode(code, &identity_address, prefix_kind);
+  identity_address += function_start_offset;
   IdentityFunction identity = (IdentityFunction)identity_address;
 
   // Validate behavior before dynamic patching.
@@ -410,7 +429,7 @@ static void TestIdentityFunctionPatching(
   TestOnlyReleaseTrampolineRegions();
 }
 
-#if !SANITIZER_WINDOWS64
+#    if !SANITIZER_WINDOWS64
 TEST(Interception, OverrideFunctionWithDetour) {
   TestOverrideFunction override = OverrideFunctionWithDetour;
   FunctionPrefixKind prefix = FunctionPrefixDetour;
@@ -424,6 +443,9 @@ TEST(Interception, OverrideFunctionWithDetour) {
 TEST(Interception, OverrideFunctionWithRedirectJump) {
   TestOverrideFunction override = OverrideFunctionWithRedirectJump;
   TestIdentityFunctionPatching(kIdentityCodeWithJump, override);
+  TestIdentityFunctionPatching(kIdentityCodeWithJumpBackwards, override,
+                               FunctionPrefixNone,
+                               kIdentityCodeWithJumpBackwardsOffset);
 }
 
 TEST(Interception, OverrideFunctionWithHotPatch) {