Issue when re-setting instruction pointer after hook removal in AMD64 architecture

[maxpyf]

Hello, Detours provides the very convenient capability to suspend other threads than the one doing the unhooking, And will update the instruction pointer of these suspended threads if they happen to be executing a trampoline that is being removed.

The issue I am seeing happens on AMD64 architectures in a multithreaded environment. Since the hooks are more direct in X86, I am almost certain that it cannot happen in X86. I have no idea about ARM architectures.

On AMD64 processors, the hooks jump to the following pattern “0xff 25 f2 ff ff ff”, i.e. “jmp qword [rip-0xe]”, with “rip-0xe” containing the address of the function that we wish to call.

I observe that when Rip points to the instruction above in one of the suspended threads, and the hook is removed, Detours does not update Rip in the thread context. Rip is only updated in the suspended thread context if it pointed to an instruction inside the trampoline being deleted. Here, the “jmp qword [rip-0xe]” is not inside the trampoline, so no updating of Rip takes place. But when the thread is resumed, the memory containing the instruction or address has been deleted or zeroed out by the unhooking thread --> there will be a crash.

[jaykrell]

This is a problem in a few places. See ARM64 for example. It would help if the offsets were considered as range markers. Anyway this area needs some attention and test coverage.

[chrisant996]

Is this related to #74?

https://github.com/microsoft/Detours/blob/edc8b07ae7e7325d9b9d551b46122a82665161b8/src/detours.cpp#L1760-L1780

It's an fIsRemove case, and it looks like it's intended to test whether the instruction pointer is within the trampoline code. But it looks like the math for calculating the end of the trampoline code is using sizeof(o->pTrampoline) -- shouldn't it be using o->pTrampoline->cbCode instead? That would appear to make it symmetrical with line 1779, too.

[jaykrell]

It would also maybe work to put a star on it, but yes, agreed.

[chrisant996]

Functionally, yes.

As a matter of cleanliness, a star seems less clear about the intent since it effectively also checks if the IP is in data regions. Most of the trampoline data structure is not code.

Get Outlook for Androidhttps://aka.ms/ghei36

---

From: Jay Krell [email protected] Sent: Thursday, November 5, 2020 10:22:02 PM To: microsoft/Detours [email protected] Cc: Chris Antos [email protected]; Comment [email protected] Subject: Re: [microsoft/Detours] Issue when re-setting instruction pointer after hook removal in AMD64 architecture (#3)

It would also work to put a star on it, but yes, agreed.

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/microsoft/Detours/issues/3#issuecomment-722886355, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AEFB4NYAVGQCF4FRSBAZXNLSOOIYVANCNFSM4E3PWJ2Q.