This is an archive of the discontinued LLVM Phabricator instance.

[CFI] Add CFI-icall pointer type generalization
ClosedPublic

Authored by vlad.tsyrklevich on Oct 26 2017, 8:03 PM.

Details

Summary

This change allows generalizing pointers in type signatures used for
cfi-icall by enabling the -fsanitize-cfi-icall-generalize-pointers flag.
This works by 1) emitting an additional generalized type signature
metadata node for functions and 2) llvm.type.test()ing for the
generalized type for translation units with the flag specified.

This flag is incompatible with -fsanitize-cfi-cross-dso because it would
require emitting twice as many type hashes which would increase artifact
size.

Diff Detail

Repository
rL LLVM

Event Timeline

eugenis added inline comments.Oct 27 2017, 2:22 PM
lib/CodeGen/CodeGenModule.cpp
4554 ↗(On Diff #120535)

Why append ".generalized"?

4578 ↗(On Diff #120535)

Should not this function be called CreateMetadataIdentifierGeneralized, and the other one - something else?

lib/Driver/SanitizerArgs.cpp
529 ↗(On Diff #120535)

I thought the plan was to only use non-generalized typeids in cross-dso, not to make these flags incompatible?

lib/CodeGen/CodeGenModule.cpp
4554 ↗(On Diff #120535)

To differentiate the set of generalized metadata type signatures from the generalized type signatures. (And the the string ".generalized" in particular because c++filt and llvm-cxxfilt will treat it graciously where they will continue to symbolize the string preceding .generalized and include .generalized in their output.)

4578 ↗(On Diff #120535)

Indeed.

lib/Driver/SanitizerArgs.cpp
529 ↗(On Diff #120535)

Ah, I might have misinterpreted our conversation earlier. You want to continue to allow intra-DSO type generalization without cross-DSO type generalization, is that correct? I thought it made sense to make the options exclusive because it's somewhat confusing to only support intra-DSO type generalization in that case. WDYT?

eugenis added inline comments.Oct 27 2017, 5:01 PM
lib/Driver/SanitizerArgs.cpp
529 ↗(On Diff #120535)

OK, let's keep the options exclusive

vlad.tsyrklevich marked 2 inline comments as done.
  • Address Evgeniy's comments

The size increase of including the generalized type signatures by default is 3% for a CFI-icall enabled Chrome build (3.618Gb -> 3.726Gb).

pcc edited edge metadata.Oct 30 2017, 1:28 PM

That's the size of the object files, not the size of the final executable, right?

Yes, sorry I should have specified. The final executable size is unchanged.

pcc added inline comments.Oct 30 2017, 4:36 PM
lib/CodeGen/CodeGenModule.cpp
4555 ↗(On Diff #120728)

Maybe use CVRQualifiers instead of FastQualifiers here? I think FastQualifiers is somewhat of an implementation detail.

test/CodeGen/cfi-icall-generalize.c
8 ↗(On Diff #120728)

I'd also test that you are generalizing the return type.

  • Address Peter's comments
pcc accepted this revision.Oct 30 2017, 5:56 PM

LGTM

docs/ControlFlowIntegrity.rst
222 ↗(On Diff #120917)

nit: Translation

229 ↗(On Diff #120917)

nit: separate

This revision is now accepted and ready to land.Oct 30 2017, 5:56 PM
This revision was automatically updated to reflect the committed changes.