[PATCH] D119296: KCFI sanitizer

[Joao Moreira via Phabricator via cfe-commits]

joaomoreira accepted this revision.
joaomoreira added a comment.

I really like this revision. It removes the redundancy of having KCFI passes both in CodeGen and in the backend; it detangles CALL instructions from KCFI by creating a new MIR instruction; it fixes alignment while still supporting the -fpatchable-function-entry option; it doesn't add hashes/gadgets through the code as it was before needed by the use of cmp instructions. With all this said, the patch LGTM.

I tested the patch compiling toy snippets and also compiling the kernel and found no issues.

Some minor suggestions/not so relevant:

- Change the name of CFIType to CFIHash, as it is probably more descriptive.
- 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.
- 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.

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.



================
Comment at: llvm/include/llvm/CodeGen/SelectionDAGNodes.h:617
 
+  uint32_t CFIType = 0;
+
----------------
I wonder if CFIHash would be a more descriptive name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119296