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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
