From ae04ca7814724e0c812605efe9b8a6717868958f Mon Sep 17 00:00:00 2001 From: Toshihito Kikuchi Date: Wed, 5 Aug 2020 07:21:00 +0000 Subject: [PATCH] Bug 1655680 - Support JAE rel32 in our detour. r=handyman After the fix for bug 1642626, we need to detour `KERNELBASE!CloseHandle` instead of K32's stub, which contains `JAE rel32`. I also found a mistake in the fix for bug 1642626. When we put a conditional jump in a trampoline, we need to reverse a condition, but the JAE case mistakenly filled JAE straight. This patch corrects it to filling JB. Differential Revision: https://phabricator.services.mozilla.com/D85477 --- mozglue/misc/interceptor/PatcherDetour.h | 17 +++-- mozglue/tests/interceptor/AssemblyPayloads.h | 71 ++++++++++--------- .../tests/interceptor/TestDllInterceptor.cpp | 54 +++++++++----- 3 files changed, 86 insertions(+), 56 deletions(-) diff --git a/mozglue/misc/interceptor/PatcherDetour.h b/mozglue/misc/interceptor/PatcherDetour.h index 24844c64b9b1..99fc5696641e 100644 --- a/mozglue/misc/interceptor/PatcherDetour.h +++ b/mozglue/misc/interceptor/PatcherDetour.h @@ -644,6 +644,8 @@ class WindowsDllDetourPatcher final return !!aTramp; } + // Write an opposite conditional jump because the destination branches + // are swapped. if (aType == JumpType::Je) { // JNE RIP+14 aTramp.WriteByte(0x75); @@ -653,8 +655,8 @@ class WindowsDllDetourPatcher final aTramp.WriteByte(0x74); aTramp.WriteByte(14); } else if (aType == JumpType::Jae) { - // JAE RIP+14 - aTramp.WriteByte(0x73); + // JB RIP+14 + aTramp.WriteByte(0x72); aTramp.WriteByte(14); } @@ -1084,13 +1086,18 @@ class WindowsDllDetourPatcher final } else { COPY_CODES(nModRmSibBytes); } - } else if (*origBytes == 0x84) { - // je rel32 + } else if (*origBytes >= 0x83 && *origBytes <= 0x85) { + // 0f 83 cd JAE rel32 + // 0f 84 cd JE rel32 + // 0f 85 cd JNE rel32 + const JumpType kJumpTypes[] = {JumpType::Jae, JumpType::Je, + JumpType::Jne}; + auto jumpType = kJumpTypes[*origBytes - 0x83]; ++origBytes; --tramp; // overwrite the 0x0f we copied above if (!GenerateJump(tramp, origBytes.ReadDisp32AsAbsolute(), - JumpType::Je)) { + jumpType)) { return; } } else { diff --git a/mozglue/tests/interceptor/AssemblyPayloads.h b/mozglue/tests/interceptor/AssemblyPayloads.h index 2a212a28355f..f8fef504ecd6 100644 --- a/mozglue/tests/interceptor/AssemblyPayloads.h +++ b/mozglue/tests/interceptor/AssemblyPayloads.h @@ -11,6 +11,24 @@ #ifndef mozilla_AssemblyPayloads_h #define mozilla_AssemblyPayloads_h +#define PADDING_256_NOP \ + "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" \ + "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" \ + "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" \ + "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" \ + "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" \ + "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" \ + "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" \ + "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" \ + "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" \ + "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" \ + "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" \ + "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" \ + "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" \ + "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" \ + "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" \ + "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" + extern "C" { #if defined(__clang__) @@ -43,28 +61,32 @@ __declspec(dllexport) __attribute__((naked)) void DoubleJump() { "jmpq *%%rax;" // 0x100 bytes padding to generate jmp rel32 instead of jmp rel8 - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" + PADDING_256_NOP "label1:" "jmp label2;" : : "i"(JumpDestination)); } + +__declspec(dllexport) __attribute__((naked)) void NearJump() { + asm volatile( + "jae label3;" + "je label3;" + "jne label3;" + + "label4:" + "mov %0, %%rax;" + "jmpq *%%rax;" + + // 0x100 bytes padding to generate jae rel32 instead of jae rel8 + PADDING_256_NOP + + "label3:" + "jmp label4;" + : + : "i"(JumpDestination)); +} # elif defined(_M_IX86) constexpr uintptr_t JumpDestination = 0x7fff0000; @@ -121,22 +143,7 @@ __declspec(dllexport) __attribute__((naked)) void DoubleJump() { "jmp *%%eax;" // 0x100 bytes padding to generate jmp rel32 instead of jmp rel8 - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" - "nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;nop;" + PADDING_256_NOP "label1:" "jmp label2;" diff --git a/mozglue/tests/interceptor/TestDllInterceptor.cpp b/mozglue/tests/interceptor/TestDllInterceptor.cpp index 5176b98ccd60..98bb70658e84 100644 --- a/mozglue/tests/interceptor/TestDllInterceptor.cpp +++ b/mozglue/tests/interceptor/TestDllInterceptor.cpp @@ -706,15 +706,16 @@ bool TestShortDetour() { #endif } -template -bool TestAssemblyFunctions() { - constexpr uintptr_t NoStubAddressCheck = 0; - struct TestCase { - const char* functionName; - uintptr_t expectedStub; - explicit TestCase(const char* aFunctionName, uintptr_t aExpectedStub) - : functionName(aFunctionName), expectedStub(aExpectedStub) {} - } testCases[] = { +constexpr uintptr_t NoStubAddressCheck = 0; +struct TestCase { + const char* mFunctionName; + uintptr_t mExpectedStub; + bool mPatchedOnce; + explicit TestCase(const char* aFunctionName, uintptr_t aExpectedStub) + : mFunctionName(aFunctionName), + mExpectedStub(aExpectedStub), + mPatchedOnce(false) {} +} g_AssemblyTestCases[] = { #if defined(__clang__) // We disable these testcases because the code coverage instrumentation injects // code in a way that WindowsDllInterceptor doesn't understand. @@ -725,6 +726,10 @@ bool TestAssemblyFunctions() { TestCase("MovPushRet", JumpDestination), TestCase("MovRaxJump", JumpDestination), TestCase("DoubleJump", JumpDestination), + + // Passing NoStubAddressCheck as the following testcases return + // a trampoline address instead of the original destination. + TestCase("NearJump", NoStubAddressCheck), # elif defined(_M_IX86) // Skip the stub address check as we always generate a trampoline for x86. TestCase("PushRet", NoStubAddressCheck), @@ -736,49 +741,60 @@ bool TestAssemblyFunctions() { # endif # endif // MOZ_CODE_COVERAGE #endif // defined(__clang__) - }; +}; +template +bool TestAssemblyFunctions() { static const auto patchedFunction = []() { patched_func_called = true; }; InterceptorType interceptor; interceptor.Init("TestDllInterceptor.exe"); - for (const auto& testCase : testCases) { + for (auto& testCase : g_AssemblyTestCases) { + if (testCase.mExpectedStub == NoStubAddressCheck && testCase.mPatchedOnce) { + // For the testcases with NoStubAddressCheck, we revert a hook by + // jumping into the original stub, which is not detourable again. + continue; + } + typename InterceptorType::template FuncHookType hook; - bool result = hook.Set(interceptor, testCase.functionName, patchedFunction); + bool result = + hook.Set(interceptor, testCase.mFunctionName, patchedFunction); if (!result) { printf( "TEST-FAILED | WindowsDllInterceptor | " "Failed to detour %s.\n", - testCase.functionName); + testCase.mFunctionName); return false; } + testCase.mPatchedOnce = true; + const auto actualStub = reinterpret_cast(hook.GetStub()); - if (testCase.expectedStub != NoStubAddressCheck && - actualStub != testCase.expectedStub) { + if (testCase.mExpectedStub != NoStubAddressCheck && + actualStub != testCase.mExpectedStub) { printf( "TEST-FAILED | WindowsDllInterceptor | " "Wrong stub was backed up for %s: %zx\n", - testCase.functionName, actualStub); + testCase.mFunctionName, actualStub); return false; } patched_func_called = false; auto originalFunction = reinterpret_cast( - GetProcAddress(GetModuleHandle(nullptr), testCase.functionName)); + GetProcAddress(GetModuleHandle(nullptr), testCase.mFunctionName)); originalFunction(); if (!patched_func_called) { printf( "TEST-FAILED | WindowsDllInterceptor | " "Hook from %s was not called\n", - testCase.functionName); + testCase.mFunctionName); return false; } - printf("TEST-PASS | WindowsDllInterceptor | %s\n", testCase.functionName); + printf("TEST-PASS | WindowsDllInterceptor | %s\n", testCase.mFunctionName); } return true;