This is an archive of the discontinued LLVM Phabricator instance.

cfi-icall: Allow the jump table to be optionally made non-canonical.
ClosedPublic

Authored by pcc on Aug 1 2019, 7:27 PM.

Details

Summary

The default behavior of Clang's indirect function call checker will replace
the address of each CFI-checked function in the output file's symbol table
with the address of a jump table entry which will pass CFI checks. We refer
to this as making the jump table canonical. This property allows code that
was not compiled with `-fsanitize=cfi-icall` to take a CFI-valid address
of a function, but it comes with a couple of caveats that are especially
relevant for users of cross-DSO CFI:

  • There is a performance and code size overhead associated with each exported function, because each such function must have an associated jump table entry, which must be emitted even in the common case where the function is never address-taken anywhere in the program, and must be used even for direct calls between DSOs, in addition to the PLT overhead.
  • There is no good way to take a CFI-valid address of a function written in assembly or a language not supported by Clang. The reason is that the code generator would need to insert a jump table in order to form a CFI-valid address for assembly functions, but there is no way in general for the code generator to determine the language of the function. This may be possible with LTO in the intra-DSO case, but in the cross-DSO case the only information available is the function declaration. One possible solution is to add a C wrapper for each assembly function, but these wrappers can present a significant maintenance burden for heavy users of assembly in addition to adding runtime overhead.

For these reasons, we provide the option of making the jump table non-canonical
with the flag `-fno-sanitize-cfi-canonical-jump-tables`. When the jump
table is made non-canonical, symbol table entries point directly to the
function body. Any instances of a function's address being taken in C will
be replaced with a jump table address.

This scheme does have its own caveats, however. It does end up breaking
function address equality more aggressively than the default behavior,
especially in cross-DSO mode which normally preserves function address
equality entirely.

Furthermore, it is occasionally necessary for code not compiled with
`-fsanitize=cfi-icall` to take a function address that is valid
for CFI. For example, this is necessary when a function's address
is taken by assembly code and then called by CFI-checking C code. The
`__attribute__((cfi_jump_table_canonical))` attribute may be used to make
the jump table entry of a specific function canonical so that the external
code will end up taking a address for the function that will pass CFI checks.

Fixes PR41972.

Event Timeline

pcc created this revision.Aug 1 2019, 7:27 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 1 2019, 7:27 PM
kees removed a subscriber: kees.Aug 7 2019, 10:40 AM
kees added a subscriber: kees.
kees added a comment.Aug 9 2019, 12:59 AM

Just FYI, I can confirm a happily running arm64 kernel with CFI enabled built with this patch series. The C wrappers aren't needed and CFI is still triggering on mismatches:

[  106.656280] lkdtm: Performing direct entry CFI_FORWARD_PROTO
[  106.657307] lkdtm: Calling matched prototype ...
[  106.657432] lkdtm: Calling mismatched prototype ...
[  106.658216] ------------[ cut here ]------------
[  106.659084] CFI failure (target: lkdtm_increment_int$53641d38e2dc4a151b75cbe816cbb86b.cfi_jt+0x0/0x4):
[  106.671576] WARNING: CPU: 1 PID: 2716 at kernel/cfi.c:29 __cfi_check_fail+0x44/0x4c
pcc added a reviewer: eugenis.Aug 9 2019, 8:54 AM

Thanks for the confirmation Kees.

eugenis added inline comments.Aug 9 2019, 11:11 AM
clang/test/CodeGen/cfi-icall-canonical-jump-tables.c
16

would it be more natural to spell it "cfi_canonical_jump_table" ?
No strong feelings one way or the other.

clang/test/SemaCXX/attr-cfi-jump-table-canonical.cpp
3 ↗(On Diff #212954)

Test the new attribute on function declaration.

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
328

Is this correct? Looks like it would cause function definitions with non-canonical jump tables to be exported as declarations. This could be desirable for LowerTypeTests, but could it affect other users negatively?

llvm/test/Transforms/ThinLTOBitcodeWriter/cfi-functions-canonical-jump-tables.ll
5

Does this test check exported functions metadata? Could you add a check for the metadata name, or at least a comment to make this easier to understand in the future?

pcc marked 6 inline comments as done.Aug 9 2019, 1:16 PM
pcc added inline comments.
clang/test/CodeGen/cfi-icall-canonical-jump-tables.c
16

Me neither. In the end it's probably a little better to be consistent with the flag name, so I've renamed it.

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
328

http://llvm-cs.pcc.me.uk/?q=%22cfi.functions%22
This metadata is only used by LowerTypeTests to decide how to create the jump table (and by CrossDSOCFI, but it doesn't look at this field). So I don't think it should affect anything else.

llvm/test/Transforms/ThinLTOBitcodeWriter/cfi-functions-canonical-jump-tables.ll
5

Yes, it's for exported functions. I've added a comment here.

pcc updated this revision to Diff 214434.Aug 9 2019, 1:16 PM
pcc marked 2 inline comments as done.
  • Address review comments
eugenis accepted this revision.Aug 9 2019, 3:09 PM

LGTM

This revision is now accepted and ready to land.Aug 9 2019, 3:09 PM
This revision was automatically updated to reflect the committed changes.