Page MenuHomePhabricator

[X86] Support the "ms-hotpatch" attribute.
ClosedPublic

Authored by cdavis5x on May 4 2016, 1:47 AM.

Details

Summary

Based on two patches by Michael Mueller.

This is a target attribute that causes a function marked with it to be
emitted as "hotpatchable". This particular mechanism was originally
devised by Microsoft for patching their binaries (which they are
constantly updating to stay ahead of crackers, script kiddies, and other
ne'er-do-wells on the Internet), but is now commonly abused by Windows
programs to hook API functions.

This mechanism is target-specific. For x86, a two-byte no-op instruction
is emitted at the function's entry point; the entry point must be
immediately preceded by 64 (32-bit) or 128 (64-bit) bytes of padding.
This padding is where the patch code is written. The two byte no-op is
then overwritten with a short jump into this code. The no-op is usually
a movl %edi, %edi instruction; this is used as a magic value
indicating that this is a hotpatchable function.

Diff Detail

Event Timeline

cdavis5x updated this revision to Diff 56107.May 4 2016, 1:47 AM
cdavis5x retitled this revision from to [X86] Support the "ms-hotpatch" attribute..
cdavis5x updated this object.
cdavis5x added reviewers: rnk, majnemer.
cdavis5x added a subscriber: llvm-commits.
rnk added a reviewer: sanjoy.May 4 2016, 8:30 AM
rnk requested changes to this revision.May 4 2016, 8:45 AM
rnk edited edge metadata.

This should use Sanjoy's "patchable-function" attribute:
http://reviews.llvm.org/D19046

lib/Target/X86/X86AsmPrinter.cpp
79

You should consider overriding EmitConstantPool instead, so the fill directive comes before the alignment directive:

.text
.fill 5, 1, 0xcc
.align 2
.global foo
foo:
  movl %edi, %edi
  retq

Are the hotpatching tools that we care about checking for 0xcc bytes, or will they overwrite alignment nops?

This revision now requires changes to proceed.May 4 2016, 8:45 AM
cdavis5x updated this revision to Diff 57387.May 16 2016, 1:12 PM
cdavis5x edited edge metadata.
  • Use Sanjoy's "patchable-function" attribute.
  • Override EmitConstantPool() instead of EmitFunctionEntryLabel().
  • Clarify why we're emitting 0xcc bytes (int3 instructions) instead of NOPs.
cdavis5x marked an inline comment as done.May 16 2016, 1:14 PM
cdavis5x added inline comments.
lib/Target/X86/X86AsmPrinter.cpp
79

Are the hotpatching tools that we care about checking for 0xcc bytes, or will they overwrite alignment nops?

It's not necessarily that tools care, it's that I think I'd prefer loudly crashing to getting stuck in an infinite loop if the patch forgets to jump back into the function. (I've added a comment clarifying this.)

sanjoy requested changes to this revision.May 16 2016, 1:49 PM
sanjoy edited edge metadata.
sanjoy added inline comments.
docs/LangRef.rst
1473

So on x86-64, the 2 byte instruction can be anything? May be helpful to be explicit about that.

lib/CodeGen/PatchableFunction.cpp
63

I presume you're handling the x86-64 case here? If so, how about handling both the 32bit and 64bit case here? On 32 bits, you can just emit a mov %edi, %edi instead of a PATCHABLE_OP? That way related bits stay together.

This revision now requires changes to proceed.May 16 2016, 1:49 PM
rnk added inline comments.May 17 2016, 11:06 AM
lib/CodeGen/PatchableFunction.cpp
63

+1

67

GCC will give you a -Wparentheses warning on this.

lib/Target/X86/X86ISelLowering.cpp
2574–2577

Why do we need this change? The patchable nop comes before the prologue. Surely this isn't use for some kind of on-stack-replacement.

test/CodeGen/X86/ms-hotpatch-attr.ll
5

Can you add a check for "align"? Otherwise there's no difference between the constant pool entry and the function label override.

cdavis5x marked an inline comment as done.May 17 2016, 12:36 PM
cdavis5x added inline comments.
lib/CodeGen/PatchableFunction.cpp
63

I'm not so sure about this. This looks like it's ultimately intended to be a target-independent pass. Putting target-specific code in here feels... wrong. If you really want me to do that anyway, I'll do it, but I'm just saying, I don't think that's such a good idea.

cdavis5x updated this revision to Diff 57850.May 19 2016, 1:20 PM
cdavis5x edited edge metadata.
  • Clarify that x86-64 doesn't use a special magic no-op.
  • Don't emit patch space with MSVC. The linker adds it for us there.
  • Don't force 64-bit functions to use a stack frame. That's only needed for 32-bit.
  • Test that, when we do emit the patch space, we emit it before the alignment directive.
cdavis5x marked 4 inline comments as done.May 19 2016, 1:24 PM
cdavis5x added inline comments.
docs/LangRef.rst
1473

Theoretically, but GCC seems to emit a lea (%rsp), %rsp on x86-64 (which is 8 bytes long). I don't know if we need to do this.

@rnk: What does MSVC do in that situation?

lib/Target/X86/X86ISelLowering.cpp
2574–2577

GCC seems to emit a standard push %ebp; mov %esp, %ebp prologue along with the patchable no-op. It doesn't do this for 64-bit.

Sorry for the delayed response, I thought I pressed submit, when I hadn't.

lib/CodeGen/PatchableFunction.cpp
63

You'd have to add a TTI hook or re-purposes an existing one that generates the code for you (I agree that hard-coding X86 instructions here is a bad idea).

sanjoy requested changes to this revision.Jun 17 2016, 3:14 PM
sanjoy edited edge metadata.

Marked as "needing changes" to get this off my pending queue.

This revision now requires changes to proceed.Jun 17 2016, 3:14 PM
dberris added inline comments.
docs/LangRef.rst
1484

At some point we probably have to reconcile how XRay will work with ms-hotpatch and prologue-short-redirect.

Is it absolutely required that the first instructions be specific ones, or would something like what XRay has for functions work too (like a short relative jump then a few more bytes of nops)?

cdavis5x updated this revision to Diff 62407.Jun 30 2016, 1:23 PM
cdavis5x edited edge metadata.
cdavis5x marked 2 inline comments as done.

Factor emission of the patchable instruction into a TTI hook.

cdavis5x marked an inline comment as done.Jun 30 2016, 1:48 PM
cdavis5x added inline comments.
docs/LangRef.rst
1484

tl;dr: You'll have problems on i386, but not anywhere else.

On 32-bit x86, a function marked ms-hotpatch must start with the bytes 8b ff (a mov edi, edi instruction). (Strictly speaking, it doesn't have to, but programs are abusing this to alter functionality of Windows API calls, and they use this as a sort of magic number to know if a function can be hotpatched or not, so unfortunately, it's too late to change this.) Later on, this gets replaced by a two-byte jmp short into the patch space (which is only guaranteed to be at least 5 bytes, though GCC--and therefore, this patch--gives you 64 bytes) which precedes this function. I believe the mov edi, edi must immediately be followed by a standard prologue (i.e. push ebp; mov ebp, esp), but I'm not sure about that. Based on all that, and on what you've said, I fear you may have problems getting XRay to play well with ms-hotpatch-style functions on i386. But it looks like you don't support anything other than x86-64 yet.

On any other architecture (including x64), no, it is not required (AFAICT) to be any specific instruction.

rnk accepted this revision.Jul 12 2016, 9:21 AM
rnk edited edge metadata.

lgtm

sanjoy accepted this revision.Jul 18 2016, 1:35 PM
sanjoy edited edge metadata.

lgtm

This revision is now accepted and ready to land.Jul 18 2016, 1:35 PM
cdavis5x planned changes to this revision.Jul 18 2016, 11:52 PM

Unfortunately, this patch needs revision due to a merge conflict. :\

cdavis5x updated this revision to Diff 66727.Aug 3 2016, 3:57 PM
cdavis5x edited edge metadata.

Merge with a more recent revision.

This revision is now accepted and ready to land.Aug 3 2016, 3:57 PM
This revision was automatically updated to reflect the committed changes.
cdavis5x reopened this revision.Aug 17 2016, 11:46 AM

I think I've figured out why the build broke when I landed this. (Sorry that took so long.)

When I added the emitPatchableOp() method to TargetTransformInfo, I added a dependency of Analysis on CodeGen, as well. But, I didn't record that in the build files. I didn't see any problems because I normally build with LLVM_LINK_LLVM_DYLIB=ON, so all of LLVM gets linked into a giant dylib, and everything that uses LLVM gets linked to this giant dylib. The buildbots, on the other hand, don't do this. Since the build system didn't know that Analysis now depended on CodeGen, BOOM.

I could've just added the dependency and been done with it, but this got me thinking. CodeGen already depends on Analysis. Having Analysis depend on CodeGen would produce a circular dependency. I don't think that's a good thing. I also became convinced that adding emitPatchableOp() to TargetTransformInfo constituted a layering violation. So, I moved it somewhere else--into TargetInstrInfo instead. The base TargetInstrInfo is already part of CodeGen, so no new dependencies are added by this change.

I don't want to break the build again like that. I also made an (IMO) not insignificant change to this patch. So, I'd like you all to review this patch again. (Sorry, but this is the last time, I promise!) I've verified that this will not break the build again.

This revision is now accepted and ready to land.Aug 17 2016, 11:46 AM
cdavis5x updated this revision to Diff 68393.Aug 17 2016, 11:48 AM

Fix the build error we saw in trunk.

jr added a subscriber: jr.Mar 25 2018, 2:14 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2019, 6:50 AM
Herald added a subscriber: hiraditya. · View Herald Transcript