Page MenuHomePhabricator

Add CFI integer types normalization
ClosedPublic

Authored by rcvalle on Dec 5 2022, 11:51 PM.

Details

Summary

This commit adds a new option (i.e.,
-fsanitize-cfi-icall-normalize-integers) for normalizing integer types
as vendor extended types for cross-language LLVM CFI/KCFI support with
other languages that can't represent and encode C/C++ integer types.

Specifically, integer types are encoded as their defined representations
(e.g., 8-bit signed integer, 16-bit signed integer, 32-bit signed
integer, ...) for compatibility with languages that define
explicitly-sized integer types (e.g., i8, i16, i32, ..., in Rust).

`-fsanitize-cfi-icall-normalize-integers` is compatible with
`-fsanitize-cfi-icall-generalize-pointers`.

This helps with providing cross-language CFI support with the Rust
compiler and is an alternative solution for the issue described and
alternatives proposed in the RFC
https://github.com/rust-lang/rfcs/pull/3296.

For more information about LLVM CFI/KCFI and cross-language LLVM
CFI/KCFI support for the Rust compiler, see the design document in the
tracking issue https://github.com/rust-lang/rust/issues/89653.

Diff Detail

Event Timeline

rcvalle created this revision.Dec 5 2022, 11:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 5 2022, 11:51 PM
rcvalle requested review of this revision.Dec 5 2022, 11:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 5 2022, 11:51 PM
rcvalle updated this revision to Diff 480349.Dec 5 2022, 11:57 PM
rcvalle retitled this revision from Add support for integer types notmalization to Add support for integer types normalization.

Fixed typo in commit message

ojeda added a subscriber: ojeda.Dec 6 2022, 4:19 AM

FYI, I'll still add (hopefully today) documentation for the new -fsanitize-cfi-icall-normalize-integers option and compression for these types.

rcvalle updated this revision to Diff 480700.Dec 6 2022, 4:58 PM
rcvalle retitled this revision from Add support for integer types normalization to Add CFI integer types normalization.
rcvalle edited the summary of this revision. (Show Details)

Added documentation

rcvalle updated this revision to Diff 480773.Dec 6 2022, 10:21 PM

Added compression

rcvalle updated this revision to Diff 480775.Dec 6 2022, 10:39 PM

Fixed comments

comex added a subscriber: comex.Dec 7 2022, 12:22 AM
rcvalle updated this revision to Diff 481016.Dec 7 2022, 12:38 PM

Updated tests

rcvalle updated this revision to Diff 482327.Dec 12 2022, 5:56 PM

Added ".normalized" suffix

Thanks for the patch and the work on cross language CFI support!

I wonder if we have precedent for other non-standard extensions to ItaniumMangleContextImpl? I wonder if we should perhaps have a distinct subclass to denote that this is not the standard mangling scheme. It would be nice perhaps to get this standardized in https://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangling.
https://github.com/itanium-cxx-abi/cxx-abi

cc @rjmccall @hjl.tools

pcc added a reviewer: pcc.Dec 12 2022, 6:56 PM

A high level question is whether we want to base the cross-language encoding on Itanium at all. Itanium has concepts such as substitutions that will complicate the implementation in other languages. Encoding pointee types can also lead to complications in cross-language encodings.

It may be better to consider developing a custom encoding. For the encoding being prototyped for the pointer authentication ABI on Android, the Rust side of the implementation is very simple:

https://github.com/pcc/rust/blob/d37ad119171635219ff21e054780d31024d24200/compiler/rustc_ty_utils/src/abi.rs#L396

clang/lib/AST/ItaniumMangle.cpp
2953

isInteger() will return true for enums, but only if they are complete. This would mean that code such as

void (*f)(enum E *e);

void g() {
  f(0);
}

would use a different encoding to call f depending on whether the TU completes the enum E, if pointee types are considered part of the encoding.

I elaborated on the reasons why not use a generalized encoding in the design document in the tracking issue https://github.com/rust-lang/rust/issues/89653. The tl;dr; is that it will result in less comprehensive protection by either using a generalized encoding for all C and C++ -compiled code or across the FFI boundary, and will degrade the security of the program when linking foreign Rust-compiled code into a program written in C or C++ because the program previously used a more comprehensive encoding for all its compiled code, not fixing the issue described in the design document and the RFC https://github.com/rcvalle/rfcs/blob/improve-c-types-for-cross-language-cfi/text/0000-improve-c-types-for-cross-language-cfi.md#appendix.

rcvalle added inline comments.Dec 12 2022, 7:26 PM
clang/lib/AST/ItaniumMangle.cpp
2953

Isn't isIntegerType() that does that? isInteger() definition is:

bool isInteger() const {
  return getKind() >= Bool && getKind() <= Int128;
}
pcc added a comment.Dec 12 2022, 7:52 PM

I elaborated on the reasons why not use a generalized encoding in the design document in the tracking issue https://github.com/rust-lang/rust/issues/89653. The tl;dr; is that it will result in less comprehensive protection by either using a generalized encoding for all C and C++ -compiled code or across the FFI boundary, and will degrade the security of the program when linking foreign Rust-compiled code into a program written in C or C++ because the program previously used a more comprehensive encoding for all its compiled code, not fixing the issue described in the design document and the RFC https://github.com/rcvalle/rfcs/blob/improve-c-types-for-cross-language-cfi/text/0000-improve-c-types-for-cross-language-cfi.md#appendix.

Ack.

clang/lib/AST/ItaniumMangle.cpp
2953

Ah yes, sorry, somehow I read this as a call to isIntegerType().

rcvalle updated this revision to Diff 482699.Dec 13 2022, 8:41 PM

Updated tests

rcvalle updated this revision to Diff 482708.Dec 13 2022, 9:13 PM

Added KCFI support

Thanks for the patch, Ramon. This looks like a reasonable approach to me, and just for reference, here appears to be the corresponding rustc change:

https://github.com/rust-lang/rust/pull/105452/commits/9087c336103d0fa0b465acf8dbc1e4651250fb05

@pcc did you have any other concerns about adding this option?

pcc added a comment.Thu, Jan 19, 5:00 PM

Thanks for the patch, Ramon. This looks like a reasonable approach to me, and just for reference, here appears to be the corresponding rustc change:

https://github.com/rust-lang/rust/pull/105452/commits/9087c336103d0fa0b465acf8dbc1e4651250fb05

@pcc did you have any other concerns about adding this option?

I discussed this out of band with Ramon and we agreed that the new option should be marked as experimental because the rustc implementation is not yet finalized. I think that the criteria for removing the experimental marking should be that there is a full implementation of integer normalization for Rust (or some other language) that has been tested against a large codebase that uses FFI. Aside from that I have no further concerns.

rcvalle updated this revision to Diff 490976.Fri, Jan 20, 2:28 PM

Mark as experimental

I discussed this out of band with Ramon and we agreed that the new option should be marked as experimental because the rustc implementation is not yet finalized. I think that the criteria for removing the experimental marking should be that there is a full implementation of integer normalization for Rust (or some other language) that has been tested against a large codebase that uses FFI.

Sure, that makes sense.

clang/docs/ControlFlowIntegrity.rst
241

An extra dash in the title.

rcvalle updated this revision to Diff 491013.Fri, Jan 20, 6:10 PM

Fixed typo

rcvalle marked an inline comment as done.Fri, Jan 20, 6:11 PM
rcvalle added inline comments.
clang/docs/ControlFlowIntegrity.rst
241

Fixed. Thank you!

samitolvanen accepted this revision.Wed, Jan 25, 1:29 PM
samitolvanen added a reviewer: samitolvanen.

Thanks, LGTM. @pcc, does this version look fine to you?

This revision is now accepted and ready to land.Wed, Jan 25, 1:29 PM
pcc accepted this revision.Tue, Jan 31, 10:22 AM

LGTM with nits

clang/lib/CodeGen/CodeGenModule.cpp
1731

Is the !! necessary here?

6953

Likewise

clang/test/CodeGen/cfi-icall-normalize.c
56–76

Shouldn't these all be checking for specific types? Since you're specifying a triple, the width and signedness of the integer types are fixed.

clang/test/CodeGen/cfi-icall-normalize2.c
10

Likewise; also below

rcvalle updated this revision to Diff 493732.Tue, Jan 31, 2:21 PM
rcvalle marked an inline comment as done.

Changed as per review

rcvalle marked 4 inline comments as done.Tue, Jan 31, 2:23 PM
rcvalle added inline comments.
clang/lib/CodeGen/CodeGenModule.cpp
1731

Fixed. Thank you!

6953

Fixed. Thank you!

clang/test/CodeGen/cfi-icall-normalize.c
56–76

Fixed. Thank you!

clang/test/CodeGen/cfi-icall-normalize2.c
10

Fixed. Thank you!

pcc accepted this revision.Tue, Jan 31, 2:30 PM

LGTM

This revision was automatically updated to reflect the committed changes.