[PATCH] D119296: KCFI sanitizer

[Sami Tolvanen via Phabricator via cfe-commits]

samitolvanen added a comment.

In D119296#3684022 <https://reviews.llvm.org/D119296#3684022>, @joaomoreira wrote:

> Some minor suggestions/not so relevant:
>
> - Change the name of CFIType to CFIHash, as it is probably more descriptive.

My thinking here was that Type is more generic than a Hash, but I don't really have a strong opinion about this.

> - Change the KCFI.* into CFI.* where the passes/structures can be re-used by a different CFI approach. For example, X86KCFI::emitCheck can be easily re-used by other CFI schemes. Similarly, the LowerKCFI_CHECK function can also be easily re-used. Because these other approaches are hypothetical by now, this is not a big deal... but may be subject to changes in the future, in case someone comes up with new schemes.

Sure, it should be trivial for a future patch that wants to reuse this code to rename the functions and classes as needed.

> - Deny the ENDBR64/32 opcodes as valid hashes for the CFIType. There was an effort in the past to prevent it from being emitted as an immediate, thus, perhaps, we should also care. Since FineIBT may inherit hashes from KCFI, it would be great to prevent these from becoming valid indirect targets or needing to be checked.

Good point. It should be quite unlikely that we'll end up randomly generating ENDBR as a hash, but I added a helper to the X86 code to ensure we don't end up emitting these values.

> Some relevant optimizations/improvements for future work:
>
> - When LTO is used, hashes may be suppressed for non-local + non-address taken functions.
> - As suggested by Andy Cooper, add a salt attribute that allows to differentiate hashes of functions with the same prototype.

Agreed, and another request from Peter Z was to add a flag to omit ENDBR from functions with the `!kcfi_type` attribute. I think these can all be separate follow-up patches.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119296/new/

https://reviews.llvm.org/D119296