[PATCH] D119296: KCFI sanitizer

[Sami Tolvanen via Phabricator via cfe-commits]

samitolvanen added inline comments.


================
Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:121
+    if (N == Value)
+      return ~Value;
+  }
----------------
joaomoreira wrote:
> Can we use another constant blinding scheme, such as a Value++ or anything else? This way, we would prevent endbrs from being emitted in the indirect branch guards too.
> 
> Since we are using Value (prologue) and ~Value (caller/guard) for doing the checks, we also need to check if ~ENDBR was picked as a KCFIType, otherwise ENDBR will be emitted in the ibranch guards.
> Can we use another constant blinding scheme, such as a Value++ or anything else? This way, we would prevent endbrs from being emitted in the indirect branch guards too.
>
> Since we are using Value (prologue) and ~Value (caller/guard) for doing the checks, we also need to check if ~ENDBR was picked as a KCFIType, otherwise ENDBR will be emitted in the ibranch guards.

I don't mind changing this to `Value + 1`, but that actually doesn't change anything because we emit `-Value` in indirect call checks, not `~Value`. Therefore, using `~Value` works equally well here.

Specifically, this code currently emits `~Value`in the preamble and `-(~Value) == Value + 1` in the indirect call check. Switching to `Value + 1` simply reverses the order; we'll emit `Value + 1` in the preamble and `-(Value + 1) == ~Value` in the indirect call check.

However, you are right that we also need to avoid `-ENDBR` in this function. I'll fix that and clarify the comment.


================
Comment at: llvm/test/CodeGen/X86/kcfi.ll:91
+
+;; Ensure we emit ~Type for unwanted values (e.g. endbr64 == 4196274163).
+; ASM-LABEL: __cfi_f5:
----------------
joaomoreira wrote:
> We need to also ensure/test that these are not emitted in the caller/indirect branch guards.
> 
> I assume that in the current scheme (blinding with ~Value) would be unfeasible to do this, so maybe we need a different approach for masking (as suggested above).
Added a test for `-ENDBR` too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119296