This is an archive of the discontinued LLVM Phabricator instance.

CFI: Introduce -fsanitize=cfi-icall flag.
ClosedPublic

Authored by pcc on Aug 7 2015, 6:31 PM.

Details

Summary

This flag causes the compiler to emit bit set entries for functions as well
as runtime bitset checks at indirect call sites. Depends on the new function
bitset mechanism.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc updated this revision to Diff 31568.Aug 7 2015, 6:31 PM
pcc retitled this revision from to CFI: Introduce -fsanitize=cfi-icall flag..
pcc updated this object.
pcc added reviewers: kcc, samsonov, rsmith.
pcc added a subscriber: cfe-commits.
rsmith edited edge metadata.Aug 8 2015, 5:40 PM

This seems to have a lot of overlap with the function sanitizer. The documentation would probably benefit from some advice about when to use each and how they compare.

lib/AST/ItaniumMangle.cpp
4119–4126 ↗(On Diff #31568)

:( Making up a name here seems like it should not be necessary. Could you use a named metadata node with no name as the key for your bitset? (Weird as it is, that seems to be our representation for "internal linkage" metadata.)

samsonov edited edge metadata.Aug 10 2015, 1:23 PM

Driver changes look good.

pcc updated this revision to Diff 31871.Aug 11 2015, 3:00 PM
pcc edited edge metadata.
  • Create distinct MDNodes for types with internal linkage; replace custom bit set mangling with mangleTypeName throughout
  • Add documentation explaining the difference between this and -fsanitize=function
pcc added a comment.Aug 11 2015, 3:01 PM

The documentation would probably benefit from some advice about when to use each and how they compare.

Added.

lib/AST/ItaniumMangle.cpp
4119–4126 ↗(On Diff #31568)

Done. This also required some improvements to the bitset lowering pass (D11857) to permit non-string bitset identifiers.

pcc updated this revision to Diff 32179.Aug 14 2015, 1:52 PM

Rebase onto D12038

kcc added inline comments.Aug 24 2015, 4:14 PM
docs/ControlFlowIntegrity.rst
149 ↗(On Diff #32179)

mention that LTO is require explicitly?

pcc updated this revision to Diff 33020.Aug 24 2015, 4:31 PM
  • Address comment
docs/ControlFlowIntegrity.rst
149 ↗(On Diff #32179)

Done above.

pcc updated this revision to Diff 33175.Aug 25 2015, 9:10 PM
  • Add apology for lack of documentation
kcc accepted this revision.Sep 9 2015, 6:38 PM
kcc edited edge metadata.

LGTM with a nit

test/CodeGen/cfi-icall.c
1 ↗(On Diff #33175)

Add a comment explaining what we are testing here.
same below.

This revision is now accepted and ready to land.Sep 9 2015, 6:38 PM
This revision was automatically updated to reflect the committed changes.
pcc added inline comments.Sep 9 2015, 7:23 PM
test/CodeGen/cfi-icall.c
1 ↗(On Diff #33175)

Done