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

[zero9178]

Summary

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.

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

[gregstoll]

Attempt to fix linting

[gregstoll]

Another attempt to fix linting

[gregstoll]

Another attempt at fixing linting

[mstorsjo]

Overall, the patch does look reasonable - but it doesn't seem to work as is.

compiler-rt/lib/interception/interception_win.cpp

741

There's no i32 type defined here - there is s32 though.

compiler-rt/lib/interception/tests/interception_win_test.cpp

447

This is a typo here, there's no FunctionPrefixNode, it should be -None right?

Even with that fixed, the new test doesn't seem to pass for me. Can you verify - that the newly added test does fail before applying the functional fix, and that it passes afterwards?

[gregstoll]

In D137788#3921370, @mstorsjo wrote:

Overall, the patch does look reasonable - but it doesn't seem to work as is.

Sorry about this! I've had trouble getting LLVM building on my machine, and was hoping the Phabricator builds would verify that the test passed. I'll work more on getting a local build going so I can address these issues.

[zero9178]

FWIW, after updating to Windows 11 today I started experiencing the same crashes with ASAN builds, remembered this patch and applied it to my local clang build. Works without issues now with the patch applied.

[mstorsjo]

In D137788#3971968, @zero9178 wrote:

FWIW, after updating to Windows 11 today I started experiencing the same crashes with ASAN builds, remembered this patch and applied it to my local clang build. Works without issues now with the patch applied.

Thanks for the data point! Are you able to look into running and fixing the tests so that they cover this issue (so that they fail before applying the functional change and succeed afterwards)?

[zero9178]

In D137788#3972045, @mstorsjo wrote:

In D137788#3971968, @zero9178 wrote:

FWIW, after updating to Windows 11 today I started experiencing the same crashes with ASAN builds, remembered this patch and applied it to my local clang build. Works without issues now with the patch applied.

Thanks for the data point! Are you able to look into running and fixing the tests so that they cover this issue (so that they fail before applying the functional change and succeed afterwards)?

Sure, I'll try to find some time for it soon.
@gregstoll would you mind if I commandeer the revision if I can fix the tests?

[gregstoll]

In D137788#3972062, @zero9178 wrote:

In D137788#3972045, @mstorsjo wrote:

In D137788#3971968, @zero9178 wrote:

FWIW, after updating to Windows 11 today I started experiencing the same crashes with ASAN builds, remembered this patch and applied it to my local clang build. Works without issues now with the patch applied.

Thanks for the data point! Are you able to look into running and fixing the tests so that they cover this issue (so that they fail before applying the functional change and succeed afterwards)?

Sure, I'll try to find some time for it soon.
@gregstoll would you mind if I commandeer the revision if I can fix the tests?

Nope, go for it! I was hoping to get back to it sometime but that probably won't happen until the new year, and I'd really just rather have it fixed 🙂

[zero9178]

Address review comments:

• Make use of s32 and sptr
• Adjust test x86 code to correctly jump to the beginning of the code
• Fix test to correctly override the relative jump

[gregstoll]

Thanks a bunch!

[zero9178]

@mstorsjo do you have any further comments on the state of the tests? I'd like to land this on Monday if possible

[mstorsjo]

LGTM, this seems to run fine for me in x86_64 mode - I didn't build and run the tests in 32 bit mode though, but I presume that you did.

compiler-rt/lib/interception/interception_win.cpp

 
 bool OverrideFunctionWithRedirectJump(
     uptr old_func, uptr new_func, uptr *orig_old_func) {
   // Check whether the first instruction is a relative jump.
   if (*(u8*)old_func != 0xE9)
     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;
   }
 
 #if SANITIZER_WINDOWS64
   // If needed, get memory space for a trampoline jump.
   uptr trampoline = AllocateMemoryForTrampoline(old_func, kDirectBranchLength);
   if (!trampoline)

compiler-rt/lib/interception/tests/interception_win_test.cpp

 const u8 kIdentityCodeWithJump[] = {
     0xE9, 0x04, 0x00, 0x00,
     0x00,                   // jmp + 4
     0xCC, 0xCC, 0xCC, 0xCC,
     0x89, 0xC8,             // mov         eax, ecx
     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
     0x8B, 0xEC,             // mov         ebp,esp
     0x8B, 0x45, 0x08,       // mov         eax,dword ptr [ebp + 8]
     0x5D,                   // pop         ebp
     0xC3,                   // ret
 };

 const u8 kIdentityCodeWithJump[] = {
     0xE9, 0x04, 0x00, 0x00,
     0x00,                   // jmp + 4
     0xCC, 0xCC, 0xCC, 0xCC,
     0x8B, 0x44, 0x24, 0x04, // mov         eax,dword ptr [esp + 4]
     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
     0x33, 0xC9,                     // xor ecx,ecx
     0xC3,                           // ret
 };
 
 const u8 kPatchableCode2[] = {
TEST(Interception, InternalGetProcAddress) {
   uptr DbgPrint_adddress = InternalGetProcAddress(ntdll_handle, "DbgPrint");
   uptr isdigit_address = InternalGetProcAddress(ntdll_handle, "isdigit");
 
   EXPECT_EQ(DbgPrint_expected, DbgPrint_adddress);
   EXPECT_EQ(isdigit_expected, isdigit_address);
   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.
   InterceptorFunctionCalled = 0;
   EXPECT_EQ(0, identity(0));
   EXPECT_EQ(42, identity(42));
   EXPECT_EQ(0, InterceptorFunctionCalled);
 
static void TestIdentityFunctionPatching(
   InterceptorFunctionCalled = 0;
   EXPECT_EQ(0, real_identity(0));
   EXPECT_EQ(42, real_identity(42));
   EXPECT_EQ(0, InterceptorFunctionCalled);
 
   TestOnlyReleaseTrampolineRegions();
 }
 
-#if !SANITIZER_WINDOWS64
+#    if !SANITIZER_WINDOWS64
 TEST(Interception, OverrideFunctionWithDetour) {
   TestOverrideFunction override = OverrideFunctionWithDetour;
   FunctionPrefixKind prefix = FunctionPrefixDetour;
   TestIdentityFunctionPatching(kIdentityCodeWithPrologue, override, prefix);
   TestIdentityFunctionPatching(kIdentityCodeWithPushPop, override, prefix);
   TestIdentityFunctionPatching(kIdentityCodeWithMov, override, prefix);
   TestIdentityFunctionPatching(kIdentityCodeWithJump, override, prefix);
 }
 #endif  // !SANITIZER_WINDOWS64
 
 TEST(Interception, OverrideFunctionWithRedirectJump) {
   TestOverrideFunction override = OverrideFunctionWithRedirectJump;
   TestIdentityFunctionPatching(kIdentityCodeWithJump, override);
+  TestIdentityFunctionPatching(kIdentityCodeWithJumpBackwards, override,
+                               FunctionPrefixNone,
+                               kIdentityCodeWithJumpBackwardsOffset);
 }
 
 TEST(Interception, OverrideFunctionWithHotPatch) {
   TestOverrideFunction override = OverrideFunctionWithHotPatch;
   FunctionPrefixKind prefix = FunctionPrefixHotPatch;
   TestIdentityFunctionPatching(kIdentityCodeWithMov, override, prefix);
 }