Closed Bug 1328996 Opened 3 years ago Closed 3 years ago

Add support to the DLL Interceptor for RIP-relative displacements

Categories

(Core :: General, defect)

52 Branch
x86_64
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: aklotz, Assigned: handyman)

References

Details

(Whiteboard: aes+)

Attachments

(4 files, 9 obsolete files)

3.52 KB, patch
Details | Diff | Splinter Review
6.85 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
1.62 KB, patch
Details | Diff | Splinter Review
15.52 KB, patch
Details | Diff | Splinter Review
No description provided.
To be more explicit: there is a case in WindowsDllDetourPatcher::CountModRmSib that is currently unsupported on x64 and returns -1. This is because on x64 those 32-bit displacements are RIP-relative. We can easily count those bytes in the instruction, however the displacement in the affected instruction needs to be modified when copied into the trampoline in order to be relative to the new RIP.
Hey David, you've worked on the dll interceptor recently, we need to add some additional functionality to that to get accessibility turned off on windows 8 touch screen users. Can you pick this up?
Flags: needinfo?(davidp99)
Sure.  It'll probably be late next week before I get to this though.
Assignee: nobody → davidp99
Flags: needinfo?(davidp99)
(In reply to David Parks [:handyman] from comment #3)
> Sure.  It'll probably be late next week before I get to this though.

sounds good, thanks.
You won't need Windows 8 to test this, of course. In my case, I could find a reproducible case in tiptsf.dll!ProcessCaretEvents.

Note that tiptsf.dll is located at:
\Program Files\Common Files\Microsoft Shared\Ink

I have modified TestDllInterceptor to work with this. I'll attach that patch here so that you don't have to repeat that work in order to repro this.
No longer blocks: 1325676
The other patches in this bug are about running very simple tests of the patched functions to make sure that the harness e.g. doesn't immediately crash.  Its a simple sanity check.  Unfortunately, since the signature for ProcessCaretEvents isn't known, the main method this bug references isn't one of a few not included in the test.
Attachment #8833451 - Flags: review?(aklotz)
Attachment #8833451 - Attachment description: Add ability to patch 64-bit MOV instructions to the DLL interceptor → 1. Add ability to patch 64-bit MOV instructions to the DLL interceptor
A few notes:

* Obviously, the tests of these methods are not thorough but I figure if we made a mistake in the machine code, its more likely to cause a crash than not.  Since the harness takes 14 bytes, we can be fairly sure we cover the preamble of the method when we run these basic tests.
* The tests are a little wordy.  This could have been done with generics but the result would have been awfully complex.  This initial catch-up is large but adding a new method to the test won't take more than a half-dozen lines.
* The tests have caught a two harness failures (on my Win10 machine).  They are in CreateFileA and SetCapture.  I haven't fixed them because I don't believe the patched methods are ever _used_ and repairs won't be too easy.  Instead, I've added messages about them.  If we decide to go this route, I'll create a new bug for them.
* Two methods, ProcessCaretEvents and ImmSetCandidateWindow, aren't added.  As mentioned above, ProcessCaretEvents isn't tested because the method signature isn't published.  ImmSetCandidateWindow isn't tested because I don't have a good test for it (it seems to crash when given invalid parameters).  Unfortunately, this means the test doesn't check the MOV code added in patch #1.
Attachment #8833456 - Flags: review?(aklotz)
Update on SetCapture:

I added that to the DLLInterceptor test in bug 1284897.  There, I added a number of methods that were hooked in the code base but weren't added to the TestDllInterceptor.  Since I wrote that patch, the hook for that method has (apparently) been removed.  So I'll need to remove it from bug 1284897... and from this patch.  So that's one less harness failure.

OTOH, the CreateFileA stub is potentially used here:
https://dxr.mozilla.org/mozilla-central/rev/1d025ac534a6333a8170a59a95a8a3673d4028ee/dom/plugins/ipc/PluginModuleChild.cpp#1927
Whiteboard: aes+
Refreshing the patch
Attachment #8833451 - Attachment is obsolete: true
Attachment #8833451 - Flags: review?(aklotz)
Attachment #8835152 - Flags: review?(aklotz)
Refreshing the patch
Attachment #8833453 - Attachment is obsolete: true
Attachment #8833453 - Flags: review?(aklotz)
Attachment #8835154 - Flags: review?(aklotz)
Refreshing the patch and removing the SetCapture stuff as explained in comment 11.
Attachment #8833456 - Attachment is obsolete: true
Attachment #8833456 - Flags: review?(aklotz)
Attachment #8835155 - Flags: review?(aklotz)
Comment on attachment 8835152 [details] [diff] [review]
1. Add ability to patch 64-bit MOV instructions to the DLL interceptor

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

r=me when comments are addressed. Thanks!

::: xpcom/build/nsWindowsDllInterceptor.h
@@ +505,5 @@
>  
> +  // Special ModR/M codes.  These indicate operands that cannot be simply
> +  // memcpy-ed.
> +  // Operand is a 64-bit RIP-relative address.
> +  static const int MODRM_OPERAND_64 = -2;

Nit: Can you please name follow the same naming convention that are used for most other constants in this file?

@@ +521,4 @@
>    int CountModRmSib(const BYTE *aModRm, BYTE* aSubOpcode = nullptr)
>    {
>      if (!aModRm) {
> +      MOZ_ASSERT_UNREACHABLE("Missing ModRM byte");

This should just be a regular MOZ_ASSERT, I think.

@@ +899,5 @@
>            COPY_CODES(1);
>            // MOV r/m64, r64 | MOV r64, r/m64
>            int len = CountModRmSib(origBytes + nOrigBytes);
>            if (len < 0) {
> +            switch (len) {

For the sake of readability and consistency with our "Return from errors immediately" coding style, I think we should transform this switch statement into
MOZ_ASSERT(len == MODRM_OPERAND_64);
if (len != MODRM_OPERAND_64) {
  return;
}

// Remaining code goes here
Attachment #8835152 - Flags: review?(aklotz) → review+
Attachment #8835154 - Flags: review?(aklotz) → review+
Comment on attachment 8835155 [details] [diff] [review]
3. Add tests that run the intercepted DLL methods

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

r=me when comments addressed.

::: toolkit/xre/test/win/TestDllInterceptor.cpp
@@ +144,5 @@
> +// interceptor.  They check that the patched assembler preamble does
> +// something sane.  The parameter is a pointer to the patched function.
> +bool TestGetWindowInfo(void* aFunc)
> +{
> +  typedef BOOL(WINAPI *GetWindowInfoType)(HWND, PWINDOWINFO);

Can you use decltype for functions that have header declarations? Something like
auto patchedGetWindowInfo = reinterpret_cast<decltype(&GetWindowInfo)>(aFunc)

@@ +149,5 @@
> +  GetWindowInfoType patchedGetWindowInfo =
> +    reinterpret_cast<GetWindowInfoType>(aFunc);
> +  return patchedGetWindowInfo(0, 0) == FALSE;
> +}
> +bool TestSetWindowLongPtr(void* aFunc)

Nit: please add blank lines between the closing brace of the previous function and the beginning of the next function definition.

@@ +365,5 @@
> +}
> +
> +bool TestProcessCaretEvents(void* aFunc)
> +{
> +  printf("TEST-SKIPPED | WindowsDllInterceptor | "

Actually we know this, it is a WinEventProc:
https://msdn.microsoft.com/en-us/library/windows/desktop/dd373885(v=vs.85).aspx
Attachment #8835155 - Flags: review?(aklotz) → review+
(In reply to Aaron Klotz [:aklotz] from comment #17)
> Comment on attachment 8835155 [details] [diff] [review]
> Actually we know this, it is a WinEventProc:
> https://msdn.microsoft.com/en-us/library/windows/desktop/dd373885(v=vs.85).
> aspx

Its function pointer is also typedef'd in Windows headers as WINEVENTPROC, BTW.
Needinfo for myself to try this with clang-cl when it lands. (Normally I'd ask you to make sure this is green with clang-cl before pushing, but TestDllInterceptor is already busted on that platform, so the burden is on me.)
Flags: needinfo?(dmajor)
> Actually we know this, it is a WinEventProc:
> https://msdn.microsoft.com/en-us/library/windows/desktop/dd373885(v=vs.85).
> aspx

Cool.  I have no idea how you figured that out but it was quite helpful -- it allowed me to run the test and learn that I had ended up completely misinterpreting the problem.  This version of the patch sets up the trampoline correctly.  We only hit the RAX destination register case, for which there is a special opcode we can use, but I also included the non-RAX case for good measure.
Attachment #8835152 - Attachment is obsolete: true
Attachment #8838673 - Flags: review?(aklotz)
> Can you use decltype for functions that have header declarations? Something
> like
> auto patchedGetWindowInfo = reinterpret_cast<decltype(&GetWindowInfo)>(aFunc)

Quite an improvement.  Thanks!

Tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=657209ff283794a666a2302a78a9368ca2ab0674
Attachment #8835155 - Attachment is obsolete: true
Comment on attachment 8838673 [details] [diff] [review]
1. Add ability to patch 64-bit MOV instructions to the DLL interceptor

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

I'd like to see another iteration on this with respect to the REX prefixes.

::: xpcom/build/nsWindowsDllInterceptor.h
@@ +913,5 @@
> +              reinterpret_cast<int64_t*>(origBytes + nOrigBytes + 4 +
> +                                         *reinterpret_cast<int32_t*>(origBytes + nOrigBytes));
> +            nOrigBytes += 4;
> +
> +            if (reg == 0) {

Let's define constants for the registers up by the mod r/m stuff.
kRegEax = 0 ... kRegEdi = 7

@@ +916,5 @@
> +
> +            if (reg == 0) {
> +              // Destination is RAX.  Encode instruction as MOVABS with a
> +              // 64-bit absolute address as its immediate operand.
> +              tramp[nTrampBytes] = 0xa1;

Don't you need to insert a REX prefix here?

@@ +926,5 @@
> +              // The MOV must be done in two steps.  First, we MOVABS the
> +              // absolute 64-bit address into our target register.
> +              // Then, we MOV from that address into the register
> +              // using register-indirect addressing.
> +              tramp[nTrampBytes] = 0xb8 + reg;

Don't you need a REX prefix here?

@@ +933,5 @@
> +              *trampOperandPtr = absAddr;
> +              nTrampBytes += 8;
> +              tramp[nTrampBytes] = 0x48;
> +              tramp[nTrampBytes+1] = 0x8b;
> +              tramp[nTrampBytes+2] = (reg << 3) | reg;

s/3/kRegFieldShift/

... actually this is a bit too ad-hoc for my liking. Can we add something like:

inline BYTE BuildModRmByte(BYTE aModBits, BYTE aReg, BYTE aRm)
{
  MOZ_ASSERT((aRm & kMaskRm) == aRm);
  MOZ_ASSERT((aModBits & kMaskMod) == aModBits);
  MOZ_ASSERT(((aReg << kRegFieldShift) & kMaskReg) == (aReg << kRegFieldShift));
  return aModBits | (aReg << kRegFieldShift) | aRm;
}

So that we can then write this as:
tramp[nTrampBytes+2] = BuildModRmByte(kModNoRegDisp, reg, reg);

I know this is a bit more work (sorry for that), but I'm really trying to encourage "cleaning up as we go" with respect to the interceptor code.
Attachment #8838673 - Flags: review?(aklotz) → review-
Also don't forget that you will need to propagate the R and B bits from the original instruction's REX prefix as well.
This addresses all of the nits in comment 23.

> Let's define constants for the registers up by the mod r/m stuff.
> kRegEax = 0 ... kRegEdi = 7

I went with kRegAx...kRegDi just so that things would be valid in 32-bit.  (My case would actually have been kRegRax since I mean 64-bit.)

> Don't you need to insert a REX prefix here?

The ModRM byte is actually (still) copied in line 841 of the parent bug (bug 1284897)'s patch:

https://bugzilla.mozilla.org/attachment.cgi?id=8839770&action=edit

Fun with deeply nested conditionals.  Note that this only allows the bit-combinations W and WR.
Attachment #8838673 - Attachment is obsolete: true
Attachment #8840067 - Flags: review?(aklotz)
Patch refresh
Attachment #8838678 - Attachment is obsolete: true
Attachment #8840067 - Flags: review?(aklotz) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/abc0e81104d5
Add ability for 64-bit MOV instructions (those with REX.W bit set) to be patched in the DLL interceptor harness. r=aklotz
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7ae5d202106
Enable tiptsf.dll's ProcessCaretEvents DLL Interceptor test on 64-bit Windows. r=aklotz
https://hg.mozilla.org/integration/mozilla-inbound/rev/06c838c29b6f
Add tests that run the intercepted DLL methods, in order to test their harness. r=aklotz
Keywords: checkin-needed
Please request uplift to beta for these patches as well as bug 1330460.
Flags: needinfo?(davidp99)
(In reply to David Major [:dmajor] from comment #19)
> Needinfo for myself to try this with clang-cl when it lands. (Normally I'd
> ask you to make sure this is green with clang-cl before pushing, but
> TestDllInterceptor is already busted on that platform, so the burden is on
> me.)

This looks fine modulo the existing bustage, for which I've filed bug 1343149. Any interest in taking it?
Flags: needinfo?(dmajor)
(In reply to Aaron Klotz [:aklotz] from comment #31)
> Please request uplift to beta for these patches as well as bug 1330460.

Do we need this work up on 53? We have a11y + e10s disabled in release builds, and this in 54 which megres next week to aurora so I think we're ok here.
Flags: needinfo?(davidp99) → needinfo?(aklotz)
My request was mainly to give Win64 users parity with Win32 (which is up on 52), but it isn't essential.
Flags: needinfo?(aklotz)
You need to log in before you can comment on or make changes to this bug.