[PATCH] D119296: KCFI sanitizer

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added inline comments.


================
Comment at: clang/lib/CodeGen/CGExpr.cpp:3168
+      -1);
+  llvm::Value *Test = Builder.CreateICmpEQ(Builder.CreateLoad(HashPtr), Hash);
+  llvm::BasicBlock *ContBB = createBasicBlock("kcfi.cont");
----------------
pcc wrote:
> samitolvanen wrote:
> > pcc wrote:
> > > We considered a scheme like this before and one problem that we discovered with comparing the hash in this way is that it can produce gadgets, e.g.
> > > ```
> > > movabs $0x0123456789abcdef, %rax
> > > cmp %rax, ...
> > > ```
> > > the `cmp`instruction ends up being a valid target address because the `movabs` instruction ends in the hash. The way we thought about solving this was to introduce a new intrinsic that would materialize the constant without these gadgets (e.g. invert the `movabs` operand and follow it by a `not`).
> > Yes, that's a concern with this approach, at least on x86_64. As the hash is more or less random, I assume you'd have to actually check that the inverted form won't have useful gadgets either, and potentially split the single `movabs` into multiple instructions if needed etc. Did you ever start work on the intrinsic or was that just an idea?
> The likelihood of the inverted operand having gadgets seems equal to that of any other piece of code having gadgets (here I'm just talking about KCFI gadgets, not any other kind of gadget). And if you're using a fixed 2-byte prefix it would be impossible for the `movabs` operand to itself be a gadget. So I don't think it would be necessary to check the inverted operand specifically for gadgets.
> 
> You might want to consider selecting the fixed prefix more carefully. It may be worth looking for a prefix that is less likely to appear in generated code (e.g. by taking a histogram of 2-byte sequences in a corpus of libraries) rather than choosing one arbitrarily.
Also the intrinsic was just an idea, we never implemented it because we ended up going with the currently implemented strategy for the CFI sanitizers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119296