1113546 - Cannot hook SetWindowLongPtrW on Firefox x64 with Windows 8.1 x64

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Makoto Kato [:m_kato]]

SetWindowLongPtrW on Windows 8.1 x64 is the following.

0:000> u user32!SetWindowLongPtrW
USER32!SetWindowLongPtrW:
00007ff8`56f34ae0 4533c9          xor     r9d,r9d
00007ff8`56f34ae3 e948ffffff      jmp     USER32!_SetWindowLongPtr (00007ff8`56f
34a30)

0:000> lmvm user32
start             end                 module name
00007ff8`56f30000 00007ff8`570a7000   USER32
    Loaded symbol image file: C:\WINDOWS\system32\USER32.dll
    Image path: C:\WINDOWS\system32\USER32.dll
    Image name: USER32.dll
    Timestamp:        Wed Oct 29 10:24:11 2014 (545041BB)
    CheckSum:         0017B9B7
    ImageSize:        00177000
    File version:     6.3.9600.17415
    Product version:  6.3.9600.17415
    File flags:       0 (Mask 3F)
    File OS:          40004 NT Win32
    File type:        2.0 Dll
    File date:        00000000.00000000
    Translations:     0409.04b0
    CompanyName:      Microsoft Corporation
    ProductName:      MicrosoftR WindowsR Operating System
    InternalName:     user32
    OriginalFilename: user32
    ProductVersion:   6.3.9600.17415
    FileVersion:      6.3.9600.17415 (winblue_r4.141028-1500)
    FileDescription:  Multi-User Windows USER API Client DLL
    LegalCopyright:   c Microsoft Corporation. All rights reserved.

But nsWindowDllInterceptor requires 13 bytes space to hook API.  If We create trampoline within 2GB address space if possible, we requires 5 bytes only even if x64.

Comment 1

[Makoto Kato [:m_kato]]

Also, CreateFileW has same issue on Windows 8 only.

0:000> u kernel32!CreateFileW
KERNEL32!CreateFileW:
000007fc`e1832e54 ff2596b71100    jmp     qword ptr [000007fc`e194e5f0]
000007fc`e1832e5a 90              nop
000007fc`e1832e5b 90              nop
000007fc`e1832e5c 90              nop
000007fc`e1832e5d 90              nop
000007fc`e1832e5e 90              nop
000007fc`e1832e5f 90              nop
KERNEL32!ReadFile:
 :
 :

Comment 2

[Makoto Kato [:m_kato]]

Attachment: fix v1

This isn't completely tested yet.

Comment 3

[Makoto Kato [:m_kato]]

Attachment: short size hook

Comment 4

[Makoto Kato [:m_kato]]

Comment on attachment 8567805 [details] [diff] [review]
short size hook

Windows 8.1 cannot hook SetWindowLongPtrA and SetWindowLongPtrW due to not enough space for hook.

ex)
0:000> u user32!SetWindowLongPtrW
USER32!SetWindowLongPtrW:
00007ffe`448a4ae0 4533c9          xor     r9d,r9d
00007ffe`448a4ae3 e948ffffff      jmp     USER32!_SetWindowLongPtr (00007ffe`448a4a30)
00007ffe`448a4ae8 b201            mov     dl,1
00007ffe`448a4aea e871feffff      call    USER32!HMValidateHandle (00007ffe`448a4960)
00007ffe`448a4aef 488bd8          mov     rbx,rax
00007ffe`448a4af2 eb82            jmp     USER32!_SetWindowLongPtr+0x42 (00007ffe`448a4a76)
00007ffe`448a4af4 cc              int     3
...

So I add minimal space hook using jmp [0fffffff2] for x64.  User32 of Windows 8/8.1 has NOP space before function, so we can use jump table.

Comment 5

[(Away)]

This seems kind of complicated. Can we just implement WindowsDllNopSpacePatcher for x64?

Comment 6

[Makoto Kato [:m_kato]]

(In reply to David Major [:dmajor] (UTC+13) from comment #5)
> This seems kind of complicated. Can we just implement
> WindowsDllNopSpacePatcher for x64?

OK, I will try it.

Comment 7

[Makoto Kato [:m_kato]]

Attachment: WindowsDllNopSpacePatcher for SetWindowLongPtrA/W

Comment 8

[Makoto Kato [:m_kato]]

Comment on attachment 8567805 [details] [diff] [review]
short size hook

># HG changeset patch
># Parent  e0cb32a0b1aa2b24db865d2cb0456861c07b430a
>Bug 1113546 - Cannot hook SetWindowLongPtrW on Firefox x64 with Windows 8.1 x64.
>
>diff --git a/xpcom/build/nsWindowsDllInterceptor.h b/xpcom/build/nsWindowsDllInterceptor.h
>--- a/xpcom/build/nsWindowsDllInterceptor.h
>+++ b/xpcom/build/nsWindowsDllInterceptor.h
>@@ -234,44 +234,63 @@ public:
>   {
>   }
> 
>   ~WindowsDllDetourPatcher()
>   {
>     int i;
>     byteptr_t p;
>     for (i = 0, p = mHookPage; i < mCurHooks; i++, p += kHookSize) {
>+      byteptr_t origBytes = *((byteptr_t*)p);
>+      byteptr_t protectAddress = origBytes;
>+
>+      if (!origBytes) {
>+        continue;
>+      }
> #if defined(_M_IX86)
>       size_t nBytes = 1 + sizeof(intptr_t);
> #elif defined(_M_X64)
>-      size_t nBytes = 2 + sizeof(intptr_t);
>+      size_t nBytes; 
>+      if (origBytes[0] == 0xff && origBytes[1] == 0x25 &&
>+          origBytes[2] == 0xf2 && origBytes[3] == 0xff &&
>+          origBytes[4] == 0xff && origBytes[5] == 0xff) {
>+        protectAddress -= 8;
>+        nBytes = 8;
>+      } else {
>+        nBytes = 2 + sizeof(intptr_t);
>+      }
> #else
> #error "Unknown processor type"
> #endif
>-      byteptr_t origBytes = *((byteptr_t*)p);
>       // ensure we can modify the original code
>       DWORD op;
>-      if (!VirtualProtectEx(GetCurrentProcess(), origBytes, nBytes,
>-                            PAGE_EXECUTE_READWRITE, &op)) {
>+      if (!VirtualProtectEx(GetCurrentProcess(), (LPVOID)protectAddress,
>+                            nBytes, PAGE_EXECUTE_READWRITE, &op)) {
>         //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)) =
>         dest - (intptr_t)(origBytes + 5); // target displacement
> #elif defined(_M_X64)
>-      *((intptr_t*)(origBytes + 2)) = dest;
>+      if (origBytes != protectAddress) {
>+        // This means that jmp table is before funtion entry
>+        *((intptr_t*)(origBytes - 8)) = dest;
>+      } else {
>+        *((intptr_t*)(origBytes + 2)) = dest;
>+      }
> #else
> #error "Unknown processor type"
> #endif
>       // restore protection; if this fails we can't really do anything about it
>-      VirtualProtectEx(GetCurrentProcess(), origBytes, nBytes, op, &op);
>+      VirtualProtectEx(GetCurrentProcess(), (LPVOID)protectAddress, nBytes,
>+                       op, &op);
>     }
>   }
> 
>   void Init(const char* aModuleName, int aNumHooks = 0)
>   {
>     if (mModule) {
>       return;
>     }
>@@ -341,16 +360,30 @@ protected:
>   const static int kPageSize = 4096;
>   const static int kHookSize = 128;
> 
>   HMODULE mModule;
>   byteptr_t mHookPage;
>   int mMaxHooks;
>   int mCurHooks;
> 
>+#if defined(_M_X64)
>+  static bool CanUseJMP32Hook(void* aOriginalFunction)
>+  {
>+    // Whether we can put jump table before function entry
>+    for (size_t offset = 1; offset <= 8; offset++) {
>+      uint8_t op = *(((uint8_t*)aOriginalFunction) - offset);
>+      if (op != 0x90 && op != 0xcc) {
>+        return false;
>+      }
>+    }
>+    return true;
>+  }
>+#endif
>+
>   void CreateTrampoline(void* aOrigFunction, intptr_t aDest, void** aOutTramp)
>   {
>     *aOutTramp = nullptr;
> 
>     byteptr_t tramp = FindTrampolineSpace();
>     if (!tramp) {
>       return;
>     }
>@@ -418,18 +451,31 @@ protected:
>         nBytes += 6;
>       } else {
>         //printf ("Unknown x86 instruction byte 0x%02x, aborting trampoline\n", origBytes[nBytes]);
>         return;
>       }
>     }
> #elif defined(_M_X64)
>     byteptr_t directJmpAddr;
>+    size_t requiredSize;
> 
>-    while (nBytes < 13) {
>+    // Some function such as SetWindowLongPtrW has no enough space to hook.
>+    // If possible, we use jump table that is function entry ahead.
>+    if (CanUseJMP32Hook(origBytes)) {
>+      // There is a space to jump table we require.
>+      // So we can use JMP [rel32] for hook
>+      requiredSize = 6;
>+    } else {
>+      // We cannot space for jump table.
>+      // We use MOV rel64 and JMP rel64
>+      requiredSize = 13;
>+    }
>+
>+    while (nBytes < requiredSize) {
> 
>       // if found JMP 32bit offset, next bytes must be NOP
>       if (pJmp32 >= 0) {
>         if (origBytes[nBytes++] != 0x90) {
>           return;
>         }
> 
>         continue;
>@@ -624,42 +670,60 @@ protected:
>     }
> #endif
> 
>     // The trampoline is now valid.
>     *aOutTramp = tramp;
> 
>     // ensure we can modify the original code
>     DWORD op;
>-    if (!VirtualProtectEx(GetCurrentProcess(), aOrigFunction, nBytes,
>-                          PAGE_EXECUTE_READWRITE, &op)) {
>+    uintptr_t protectAddress = (uintptr_t) aOrigFunction;
>+    DWORD protectSize = nBytes;
>+#if defined(_M_X64)
>+    if (requiredSize == 6) {
>+      // additional jump table
>+      protectAddress -= 8;
>+      protectSize += 8;
>+    }
>+#endif
>+    if (!VirtualProtectEx(GetCurrentProcess(), (LPVOID)protectAddress,
>+                          protectSize, PAGE_EXECUTE_READWRITE, &op)) {
>       //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
> #elif defined(_M_X64)
>-    // mov r11, address
>-    origBytes[0] = 0x49;
>-    origBytes[1] = 0xbb;
>+    if (requiredSize == 6) {
>+      // jmp [0xfffffff2]
>+      *((intptr_t*)(origBytes - 8)) = aDest;
>+      origBytes[0] = 0xff;
>+      origBytes[1] = 0x25;
>+      *((int32_t*)(origBytes + 2)) = -14;
>+    } else {
>+      // mov r11, address
>+      origBytes[0] = 0x49;
>+      origBytes[1] = 0xbb;
> 
>-    *((intptr_t*)(origBytes + 2)) = aDest;
>+      *((intptr_t*)(origBytes + 2)) = aDest;
> 
>-    // jmp r11
>-    origBytes[10] = 0x41;
>-    origBytes[11] = 0xff;
>-    origBytes[12] = 0xe3;
>+      // jmp r11
>+      origBytes[10] = 0x41;
>+      origBytes[11] = 0xff;
>+      origBytes[12] = 0xe3;
>+    }
> #endif
> 
>     // restore protection; if this fails we can't really do anything about it
>-    VirtualProtectEx(GetCurrentProcess(), aOrigFunction, nBytes, op, &op);
>+    VirtualProtectEx(GetCurrentProcess(), (LPVOID)protectAddress, protectSize,
>+                     op, &op);
>   }
> 
>   byteptr_t FindTrampolineSpace()
>   {
>     if (mCurHooks >= mMaxHooks) {
>       return 0;
>     }
>

Comment 9

[(Away)]

I'm sorry, I didn't think this through fully in comment 5. The special thing about the x86 nop space patcher is that it needs no trampoline, because |mov edi, edi| doesn't need to be restored. But x64 doesn't use such throwaway instructions. (This is even mentioned in the file comment but I failed to read it)

As a result, this new patch is still basically a detour patcher. So we might as well just keep it in the WindowsDllDetourPatcher. I'm going to go back and review the first patch instead. Again sorry for the churn.

Comment 10

[(Away)]

Comment on attachment 8567805 [details] [diff] [review]
short size hook

Review of attachment 8567805 [details] [diff] [review]:
-----------------------------------------------------------------

After seeing the alternative, I'm not so opposed to this patch anymore :)

::: xpcom/build/nsWindowsDllInterceptor.h
@@ +253,5 @@
> +          origBytes[2] == 0xf2 && origBytes[3] == 0xff &&
> +          origBytes[4] == 0xff && origBytes[5] == 0xff) {
> +        protectAddress -= 8;
> +        nBytes = 8;
> +      } else {

Please check that the bytes are 0x49 0xbb in the "else" case (in case somebody else made some changes to the function)

@@ +263,3 @@
>        // ensure we can modify the original code
>        DWORD op;
> +      if (!VirtualProtectEx(GetCurrentProcess(), (LPVOID)protectAddress,

Nit: I don't think LPVOID needs a cast.

@@ +460,3 @@
>  
> +    // Some function such as SetWindowLongPtrW has no enough space to hook.
> +    // If possible, we use jump table that is function entry ahead.

I would write this as:
// If possible, we write a jump table before the function entry point.

@@ +461,5 @@
> +    // Some function such as SetWindowLongPtrW has no enough space to hook.
> +    // If possible, we use jump table that is function entry ahead.
> +    if (CanUseJMP32Hook(origBytes)) {
> +      // There is a space to jump table we require.
> +      // So we can use JMP [rel32] for hook

To emphasize the indirect jump, I would write:
// So we can use JMP qword ptr [rel32]

@@ +465,5 @@
> +      // So we can use JMP [rel32] for hook
> +      requiredSize = 6;
> +    } else {
> +      // We cannot space for jump table.
> +      // We use MOV rel64 and JMP rel64

It's actually an absolute value, right? So maybe:
// We use MOV r11, imm64; JMP r11

Comment 11

[Cristian Baica [:cbaica], Release Desktop QA]

This issue still occurs. I ran into it while running the DLL tests for Fx67.0b3 on Windows 8 x64.

Comment 12

[Wayne Mery (:wsmwk)]

Is this a patch you wish to complete?

Comment 13

[Makoto Kato [:m_kato]]

(In reply to Wayne Mery (:wsmwk) from comment #12)

Is this a patch you wish to complete?

No, but we don't support Windows 8 .1 from 116. This fix is unnecessary.