Bug 1344034 - Auto-enforce W^X for WindowsDllInterceptor hook pages. r=dmajor
authorArthur Edelstein <arthuredelstein@gmail.com>
Fri, 09 Jun 2017 14:44:00 -0400
changeset 411503 cc20d9ee0ef908d0139c521988e0ae0564e90030
parent 411502 230564c469b24b9201ae815c6af0dd1670b5ad74
child 411504 eb85e39d0383980f7d698e4bb163c14ef322c137
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdmajor
bugs1344034
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1344034 - Auto-enforce W^X for WindowsDllInterceptor hook pages. r=dmajor
xpcom/build/nsWindowsDllInterceptor.h
--- a/xpcom/build/nsWindowsDllInterceptor.h
+++ b/xpcom/build/nsWindowsDllInterceptor.h
@@ -94,16 +94,19 @@ public:
                        &mOldProtect);
     }
   }
 
   bool Protect()
   {
     mSuccess = !!VirtualProtectEx(GetCurrentProcess(), mFunc, mSize,
                                   mNewProtect, &mOldProtect);
+    if (!mSuccess) {
+      // printf("VirtualProtectEx failed! %d\n", GetLastError());
+    }
     return mSuccess;
   }
 
 private:
   void* const mFunc;
   size_t const mSize;
   DWORD const mNewProtect;
   DWORD mOldProtect;
@@ -133,17 +136,16 @@ public:
     // Restore the mov edi, edi to the beginning of each function we patched.
 
     for (int i = 0; i < mPatchedFnsLen; i++) {
       byteptr_t fn = mPatchedFns[i];
 
       // Ensure we can write to the code.
       AutoVirtualProtect protect(fn, 2, PAGE_EXECUTE_READWRITE);
       if (!protect.Protect()) {
-        // printf("VirtualProtectEx failed! %d\n", GetLastError());
         continue;
       }
 
       // mov edi, edi
       *((uint16_t*)fn) = 0xff8b;
 
       // I don't think this is actually necessary, but it can't hurt.
       FlushInstructionCache(GetCurrentProcess(),
@@ -266,17 +268,16 @@ public:
     fn = ResolveRedirectedAddress(fn);
 
     // Ensure we can read and write starting at fn - 5 (for the long jmp we're
     // going to write) and ending at fn + 2 (for the short jmp up to the long
     // jmp). These bytes may span two pages with different protection.
     AutoVirtualProtect protectBefore(fn - 5, 5, PAGE_EXECUTE_READWRITE);
     AutoVirtualProtect protectAfter(fn, 2, PAGE_EXECUTE_READWRITE);
     if (!protectBefore.Protect() || !protectAfter.Protect()) {
-      //printf ("VirtualProtectEx failed! %d\n", GetLastError());
       return false;
     }
 
     bool rv = WriteHook(fn, aHookDest, aOrigFunc);
 
     if (rv) {
       mPatchedFns[mPatchedFnsLen] = fn;
       mPatchedFnsLen++;
@@ -395,17 +396,16 @@ public:
 #else
 #error "Unknown processor type"
 #endif
       byteptr_t origBytes = *((byteptr_t*)p);
 
       // ensure we can modify the original code
       AutoVirtualProtect protect(origBytes, nBytes, PAGE_EXECUTE_READWRITE);
       if (!protect.Protect()) {
-        //printf ("VirtualProtectEx failed! %d\n", GetLastError());
         continue;
       }
 
       // Remove the hook by making the original function jump directly
       // in the trampoline.
       intptr_t dest = (intptr_t)(p + sizeof(void*));
 #if defined(_M_IX86)
       *((intptr_t*)(origBytes + 1)) =
@@ -435,38 +435,25 @@ public:
       aNumHooks = hooksPerPage;
     }
 
     mMaxHooks = aNumHooks + (hooksPerPage % aNumHooks);
 
     mHookPage = (byteptr_t)VirtualAllocEx(GetCurrentProcess(), nullptr,
                                           mMaxHooks * kHookSize,
                                           MEM_COMMIT | MEM_RESERVE,
-                                          PAGE_EXECUTE_READWRITE);
+                                          PAGE_EXECUTE_READ);
     if (!mHookPage) {
       mModule = 0;
       return;
     }
   }
 
   bool Initialized() { return !!mModule; }
 
-  void LockHooks()
-  {
-    if (!mModule) {
-      return;
-    }
-
-    DWORD op;
-    VirtualProtectEx(GetCurrentProcess(), mHookPage, mMaxHooks * kHookSize,
-                     PAGE_EXECUTE_READ, &op);
-
-    mModule = 0;
-  }
-
   bool AddHook(const char* aName, intptr_t aHookDest, void** aOrigFunc)
   {
     if (!mModule) {
       return false;
     }
 
     void* pAddr = (void*)GetProcAddress(mModule, aName);
     if (!pAddr) {
@@ -731,16 +718,22 @@ protected:
     MOZ_ASSERT(((aReg << kRegFieldShift) & kMaskReg) == (aReg << kRegFieldShift));
     return aModBits | (aReg << kRegFieldShift) | aRm;
   }
 
   void CreateTrampoline(void* aOrigFunction, intptr_t aDest, void** aOutTramp)
   {
     *aOutTramp = nullptr;
 
+    AutoVirtualProtect protectHookPage(mHookPage, mMaxHooks * kHookSize,
+                                       PAGE_EXECUTE_READWRITE);
+    if (!protectHookPage.Protect()) {
+      return;
+    }
+
     byteptr_t tramp = FindTrampolineSpace();
     if (!tramp) {
       return;
     }
 
     // We keep the address of the original function in the first bytes of
     // the trampoline buffer
     *((void**)tramp) = aOrigFunction;
@@ -1244,17 +1237,16 @@ protected:
 #endif
 
     // The trampoline is now valid.
     *aOutTramp = tramp;
 
     // ensure we can modify the original code
     AutoVirtualProtect protect(aOrigFunction, nOrigBytes, PAGE_EXECUTE_READWRITE);
     if (!protect.Protect()) {
-      //printf ("VirtualProtectEx failed! %d\n", GetLastError());
       return;
     }
 
 #if defined(_M_IX86)
     // now modify the original bytes
     origBytes[0] = 0xE9; // jmp
     *((intptr_t*)(origBytes + 1)) =
       aDest - (intptr_t)(origBytes + 5); // target displacement
@@ -1350,23 +1342,16 @@ public:
     mModuleName = aModuleName;
     mNHooks = aNumHooks;
     mNopSpacePatcher.Init(aModuleName);
 
     // Lazily initialize mDetourPatcher, since it allocates memory and we might
     // not need it.
   }
 
-  void LockHooks()
-  {
-    if (mDetourPatcher.Initialized()) {
-      mDetourPatcher.LockHooks();
-    }
-  }
-
   /**
    * Hook/detour the method aName from the DLL we set in Init so that it calls
    * aHookDest instead.  Returns the original method pointer in aOrigFunc
    * and returns true if successful.
    *
    * IMPORTANT: If you use this method, please add your case to the
    * TestDllInterceptor in order to detect future failures.  Even if this
    * succeeds now, updates to the hooked DLL could cause it to fail in