Deviare fails to fix up branch instructions in trampoline, causing crashes

[DarkStarSword]

I've confirmed beyond a shadow of a doubt that the Deviare library we are using is definitely the source of our crashes when hooking.

Here is the VSSetSamplers function before being hooked:

00007FFFC39A90D0 45 85 C0             test        r8d,r8d
00007FFFC39A90D3 75 01                jne         00007FFFC39A90D6
00007FFFC39A90D5 C3                   ret
00007FFFC39A90D6 48 89 5C 24 08       mov         qword ptr [rsp+8],rbx
...

As you can see, the first three instructions is an early exit case if r8d (NumSamplers) is 0. The hook replaces the first five bytes of the function, which includes the jne instruction:

00007FFFC39A90D0 E9 33 B8 E7 C4       jmp         00007FFF88824908
00007FFFC39A90D5 C3                   ret
00007FFFC39A90D6 48 89 5C 24 08       mov         qword ptr [rsp+8],rbx
...

Let's examine the branch target it patched in:

00007FFF88824900 00 00 00 00 00 00 00 00                                     <- Flag to disable hook
00007FFF88824908 90                   nop                                    <- Entry point
00007FFF88824909 90                   nop
00007FFF8882490A 90                   nop
00007FFF8882490B 90                   nop
00007FFF8882490C 90                   nop
00007FFF8882490D 90                   nop
00007FFF8882490E 90                   nop
00007FFF8882490F 90                   nop
00007FFF88824910 52                   push        rdx                        <- {
00007FFF88824911 48 BA 00 49 82 88 FF 7F 00 00 mov         rdx,7FFF88824900h <-    Pointer to diable flag
00007FFF8882491B 48 F7 02 01 01 00 00 test        qword ptr [rdx],101h       <-    Test if hook is disabled
00007FFF88824922 75 0F                jne         00007FFF88824933           <-    Branch forward +16 bytes if hook is disabled
00007FFF88824924 5A                   pop         rdx                        <- }
00007FFF88824925 FF 25 00 00 00 00    jmp         qword ptr [7FFF8882492Bh]  <- Hook is enabled - jump to HookedContext::VSSetSamplers()

00007FFF8882492B 40 33 4A 99 FF 7F 00 00                                     <- Pointer to HookedContext::VSSetSamplers()

00007FFF88824933 5A                   pop         rdx                        <- Hook disabled path falls through to trampoline

That all looks fine. Now let's examine the trampoline that we call in order to call through to DirectX:

00007FFF88824934 45 85 C0             test        r8d,r8d                    <- Entry point, instructions from original function
00007FFF88824937 75 01                jne         00007FFF8882493A           <- Deviare bug, failed to fix up this instruction
00007FFF88824939 FF 25 00 00 00 00    jmp         qword ptr [7FFF8882493Fh]  <- Jump back to original function

Oh dear - the jne instruction has been included here to execute out of line, but since that is a relative branch it needed to be fixed up to point back to the correct offset in the original function, which Deviare has not done. Instead it will branch here and try to execute this garbage:

00007FFF8882493A 25 00 00 00 00       and         eax,0
00007FFF8882493F ??                   ?? ??

There's our crash.

We need to update Deviare and see if this bug has been fixed.

[DarkStarSword]

@bo3b which version of Deviare are we currently using? The sha1sum doesn't match any of the releases I see for it on github - was it built from source?

[DarkStarSword]

This bug is still present in nektra/Deviare-InProc@3b08bab

[bo3b]

Very nice find. I'm really quite surprised they would miss something so obviously necessary. Our version is years old though, so maybe it has been fixed.

To be honest, I cannot recall how I added the original files. Look at dates, it seems like my adding those files predates them open-sourcing the project, and any github releases. I vaguely recall that you could download directly from their website. I do not believe I had source code at that point. Assuming that is true, our code predates their 1.0 github release.

[bo3b]

This bug is still present in nektra/Deviare-InProc@3b08bab

Bummer.

Edit: BTW, I did a quick code inspection, and they do handle a bunch of other variants of relative addressing, like MOV, JMP, CALL instructions. No idea why they didn't just flesh out the instruction set.

[mxmauro]

Hi guys, I'll check it and add the patch as soon as possible.

EDIT: Never hit a near jump in the code being relocated before. @DarkStarSword which API did you hook for testing?

[DarkStarSword]

Thanks for looking at this :)

This one was specifically ID3D11DeviceContext::VSSetSamplers() on Windows 10 (the other variants of XXSetSamplers are similarly affected - DS, HS, GS, PS, CS).

Worth noting that differing Windows versions and patch levels change the d3d11 code, so these crashes can be OS / patch level specific - I don't think the above one occurs prior to Windows 10.

We've also seen crashes happen when hooking ID3D11DeviceContext::VSSetShaderResources() and related functions when NumViews is passed 0 on Windows 7 with KB2670838 installed (and I think that may now also affect Windows 10, possibly following the creators update).

Another variant we identified is ID3D11DeviceContext::RSSetState(), which crashes when hooked on Windows 7 without KB2670838 installed.

If you wanted to use 3DMigoto to test this hooking has to be enabled in the d3dx.ini - "hook=all" will attempt to hook all ID3D11Device and ID3D11DeviceContext APIs, and there are flags to disable hooks on these specific calls. The Witcher 3 is a good game to use while testing, since it triggers the VSSetSamplers crash on Windows 10 a few seconds after the game window appears, and allows a debugger to attach to it (we have an undocumented waitfordebugger=1 option that goes under [Logging] to attach to a running game early in 3DMigoto's init), but I expect almost any game would trigger that on Windows 10.

[mxmauro]

Commited new code

[DarkStarSword]

I've updated our copy of the library in master and can confirm that the crash I as looking at above is now resolved. I'll do a little wider smokescreen testing on the other hooks as well just to be sure, then we can change hook=recommended to leave the workarounds disabled. I might wait a release or two to actually drop the "except_" options (1.3 might be a good time) - just in case.

I think this justifies doing a 1.2.71 release before we archive the 1.2 branch, since 1.2.68 changed the way we prevent games from unbinding StereoParams and IniParams to depend on the XXSetShaderResources calls, so if anyone enables hooking and it's one of those games they would see 3DMigoto appear to be mostly non-functional. We've got other existing workarounds (ShaderRegex + command list to force rebinding on every draw call), but it's better if this "just works", because it is not at all obvious what has gone wrong.

As a bonus - this also makes the frame analysis log useful in games where we need hooking :)

[DarkStarSword]

@bo3b I'd be interested to know if you are now able to hook that IDXGIFactory::QueryInterface call you were getting crashes with in Dishonored 2, even if we don't end up using it for anything. I haven't disassembled that call to see if it was the same issue or not, but it's worth a try.

[bo3b]

Super good. Great advance for our stability. As another data point, I also tested Witcher3 on Win10, and could reproduce the crash, then swapped in the new InProc .lib files and no longer see the crash. I also then reverted the lib files and reproduced the crash again. This seems like a good fix.

I also stepped through the asm to see the fix in action, and it looks right to me. Of note there, Debug builds will never show the problem, because even for DX11 calls they don't do early exit. Also of note, if you set the bLog=true in DLLMainHook, then you'll see the Deviare logging in the VS console for the instructions being hooked, including the prior code, and the addresses of trampoline functions.

Edit: BTW, this was using 1.3 exp_single_layer.

I'll take a look at the QueryInterface crash and report back.

[bo3b]

@DarkStarSword QueryInterface crash is unrelated to the Deviare problem.

Actual patch code is:

NktHookLib: Disassembly 0x7FEF70A132C -> push rbp
NktHookLib: Disassembly 0x7FEF70A132E -> push rbx
NktHookLib: Disassembly 0x7FEF70A132F -> push rsi
NktHookLib: Disassembly 0x7FEF70A1330 -> push rdi
NktHookLib: Hook installed. Proc @ 0x7FEF70A132C -> 0x7FEEAC2F080 (Stub @ 0x7FEBCD00300) 

But... the actual problem was...

	DWORD dwOsErr = cHookMgr.Hook(&hook_id, (void**)&fnOrigQueryInterface,
		lpvtbl_CreateSwapChain(dxgiFactory), Hooked_QueryInterface, 0);

Copy/Paste bug. < cough >

[DarkStarSword]

Ok, good to know. Thanks @bo3b :)

[DarkStarSword]

Just brought up my win7 machine and uninstalled KB2670838 to look at the RSSetState crash, and can confirm that it was the same problem and is also resolved by this fix :)

[DarkStarSword]

1.2.71 is out with the updated Deviare library, and a commit removing the except_ workarounds is in the 1.3 branch, so closing this.