Page MenuHomePhabricator

SSP: [3/3] cmpxchg and addrspacecast can now trigger stack protection
ClosedPublic

Authored by probinson on Sep 20 2019, 10:44 AM.

Details

Summary

cmpxchg has store-like semantics, and addrspacecast is like bitcast, as far as SSP is concerned.

The test is derived from captures.ll written by Matt Arsenault for r363169.
I tweaked it slightly, running 'llc' instead of 'opt' because StackProtector is a CodeGen pass not a Transform; I also added the llvm.memset test, which hadn't been covered before.
We could be smarter about avoiding stack canaries in the llvm.mem* case, but that's for another time.

Diff Detail

Repository
rL LLVM

Event Timeline

probinson created this revision.Sep 20 2019, 10:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 20 2019, 10:44 AM
arsenm added inline comments.Sep 20 2019, 10:47 AM
llvm/lib/CodeGen/StackProtector.cpp
204–205 ↗(On Diff #221064)

I think this should be changed to assume true on unhandled instructions in case of future additions

probinson marked an inline comment as done.Sep 23 2019, 6:30 AM
probinson added inline comments.
llvm/lib/CodeGen/StackProtector.cpp
204–205 ↗(On Diff #221064)

Hm so fail-safe; I'd need to add some more cases for things that are explicitly okay, like Load and AtomicRMW, maybe more. I'll look at that.

probinson updated this revision to Diff 221347.Sep 23 2019, 8:49 AM

HasAddressTaken now defaults to true; this means explicitly handling innocuous instructions.

  • load does not store anything.
  • atomicrmw cannot store a pointer unless it goes through ptrtoint, which we handle explicitly.
  • ret is irrelevant because the current frame goes away after the ret.

Update test to verify those instructions do not trigger a stack protector.

probinson marked an inline comment as done.Sep 23 2019, 8:50 AM
arsenm accepted this revision.Sep 27 2019, 12:03 PM

LGTM

This revision is now accepted and ready to land.Sep 27 2019, 12:03 PM
This revision was automatically updated to reflect the committed changes.