This is an archive of the discontinued LLVM Phabricator instance.

Add the ShadowCallStack pass
ClosedPublic

Authored by vlad.tsyrklevich on Mar 22 2018, 12:48 PM.

Details

Summary

The ShadowCallStack pass instruments functions marked with the
shadowcallstack attribute. The instrumented prolog saves the return
address to [gs:offset] where offset is stored and updated in [gs:0].
The instrumented epilog loads/updates the return address from [gs:0]
and checks that it matches the return address on the stack before
returning.

Diff Detail

Event Timeline

@craig.topper Just wanted to give you a heads up about this change as it's landing a pass in the X86 backend. The pass is specific to functions compiled with -fsanitize=shadow-call-stack so it shouldn't affect most users.

eugenis added inline comments.
lib/Target/X86/ShadowCallStack.cpp
96

AFAIK you can do it without a dummy instruction by using the iterator form of BuildMI and passing MBB->end() or MBB->begin() in case of the empty basic block.

pcc added inline comments.Mar 22 2018, 2:08 PM
lib/Target/X86/ShadowCallStack.cpp
90

I think this is possible if the function takes a nest parameter -- probably best to disable the protection in that case for now.

  • Skip functions with r10/r11 live-in instead of assert()ing

+Vitaly for another set of eyes.

First-pass leaf function optimization

vlad.tsyrklevich marked an inline comment as done.Mar 28 2018, 11:30 AM
vlad.tsyrklevich added inline comments.
lib/Target/X86/ShadowCallStack.cpp
90

Yup, I double checked and that looks like the case in X86FrameLowering.

96

You could use the MBB->begin() form and insert them backwards, but for the sake of readability I insert them forwards.

vitalybuka added inline comments.Mar 28 2018, 2:22 PM
lib/Target/X86/ShadowCallStack.cpp
64

Do you need this functions like members.
probably static non member should work as well

90

Why is there inconsistency reference vs pointer?

287

LeafFuncOptimization looks redundunt: LeafFuncOptimization == ( LeafFuncRegister != X86::NoRegister)

293
for (auto &MBB : Fn)
  MBB.addLiveIn(LeafFuncRegister);
324

can getFallThrough return null?

vlad.tsyrklevich marked 3 inline comments as done.
  • Address Vitaly's comments
  • Only add live-ins when a register was found
  • Add a short leaf function test case
lib/Target/X86/ShadowCallStack.cpp
293

The original loop actually skips the first basic bock: ++Fn.begin(), I'll add a comment to make that clearer.

324

I don't think so, the check at 241 demonstrates there is at least one instruction in this function so if the first MBB is empty then there has to be another MBB following it.

pcc added inline comments.Mar 28 2018, 3:56 PM
lib/Target/X86/ShadowCallStack.cpp
96

I would imagine that you could save the result of MBB->begin() and use that to insert instructions forwards, as it should always refer to what used to be the first instruction. Does that not work?

lib/Target/X86/ShadowCallStack.cpp
96

Sorry, let me be more clear. BuildMI() has two forms: one to add to the end of an MBB and one to add before a given instruction in the MBB. That means that for an empty MBB we can't insert instructions in forwards order without inserting a fake instruction to insert behind or using two different forms to insert the same instructions (one inserting behind the first instruction and the other always using the add-to-the-end form.)

pcc added inline comments.Mar 28 2018, 4:49 PM
lib/Target/X86/ShadowCallStack.cpp
96

But there is another form that takes an iterator.

http://llvm-cs.pcc.me.uk/include/llvm/CodeGen/MachineInstrBuilder.h#313

Can you pass the iterator that you previously got from MBB->begin() to it?

lib/Target/X86/ShadowCallStack.cpp
96

Huh, I didn't think this form would work because the iterator would be invalidated between MBB insertions but it looks like that's not the case for ilists so this form does work.

  • Simplify addProlog
pcc accepted this revision.Mar 29 2018, 3:13 PM

LGTM

lib/Target/X86/ShadowCallStack.cpp
95

Nit: I'd call this MBBI because it might not actually point to a MachineInstr.

122

Looks like this can be simplified to addDirectMem(BuildMI(&MBB, MBB.begin(), ...

This revision is now accepted and ready to land.Mar 29 2018, 3:13 PM
  • Address Peter's comments
This revision was automatically updated to reflect the committed changes.