[PATCH] D118052: [X86] Fix CodeGen Module Flag for -mibt-seal

[Joao Moreira via Phabricator via cfe-commits]

joaomoreira added inline comments.



Comment at: clang/test/CodeGen/X86/x86-cf-protection.c:4
 // RUN: %clang -target i386-unknown-unknown -x c -E -dM -o - 
-fcf-protection=full %s   | FileCheck %s --check-prefix=FULL
+// RUN: %clang -target i386-unknown-unknown -o - -emit-llvm -S 
-fcf-protection=branch -mibt-seal -flto %s | FileCheck %s --check-prefix=IBTSEAL
 

xiangzhangllvm wrote:
> joaomoreira wrote:
> > pengfei wrote:
> > > Is `-flto` is required?
> > Yes, we can only suppress ENDBR if we are sure the given function is not 
> > address taken in all other translation units.
> Sorry, let me make sure here. what is the "translation units" here mean? Does 
> it means another binary file (e.g. *.so , *.a)?
> Using -flto seems here want more compile units (source files) to be one 
> "translation units"?
Translation unit means a source file translated into an object file. When 
compiling the kernel, we have different .c files that are translated into 
different .o files. Each .c translated into .o is a translation unit. Because a 
function might not be address taken in the translation unit where it is defined 
but could be address taken in a different one, we need to emit ENDBRs to all 
non-local (static) functions. With LTO this changes, because we can look at all 
to-be-generated objects and be sure that a given function is not address taken 
in any of the translation units.

This optimization is kernel-specific because in user-space code non-local 
functions can be reached through the PLT of a different dynamically linked 
library (.so) or through dlsym, and this is impossible to predict in 
compilation time.

In kernel, exported symbols are implicitly address taken. This way, if a module 
tries to take the address of an exported function, this would be ok. The 
optimization will mostly rule-out non-static functions that are not exported 
from receiving an ENDBR. The numeric benefits of the optimization are shown in 
https://reviews.llvm.org/D116070


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118052

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118052: [X86] Fix CodeGen Module Flag for -mibt-seal

[Joao Moreira via Phabricator via cfe-commits]

joaomoreira added a comment.

I did track down the problem to clang/lib/Frontend/CompilerInvocation.cpp -- 
RoundTrip method. There, we can se the following statement:

  #ifndef NDEBUG
bool DoRoundTripDefault = true;
  #else
bool DoRoundTripDefault = false;
  #endif

Comment in the file says: "By default, the round-trip is enabled in assert 
builds... During round-trip, the command line arguments are parsed into a dummy 
instance of CompilerInvocation which is used to generate the command line 
arguments again. The real CompilerInvocation instance is then created by 
parsing the generated arguments, not the original ones.". Then, because the 
arguments set through this latest patch were not being generated, they would be 
missing in the dummy instance of CompilerInvocation and then not repassed into 
the real instance.

Given the above, my understanding is that there was never an actual bug that 
made ibt-seal stop working. The patch was just not tested enough and a 
confusion in different build setups blinded me against what was really 
happening.

Thank you for pushing me into looking into this @aaron.ballman. Also, with all 
this said, I'm no expert in clang/front-end, so it would be great if you could 
give an additional look into it and confirm the above.

If all is right, the patch remains correct and there are no hidden bugs. I'll 
update the patch shortly to add the test requested by @pengfei.


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

https://reviews.llvm.org/D118052

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118052: [X86] Fix CodeGen Module Flag for -mibt-seal

[Joao Moreira via Phabricator via cfe-commits]

joaomoreira updated this revision to Diff 417539.
joaomoreira edited the summary of this revision.
joaomoreira added a reviewer: nickdesaulniers.
This revision is now accepted and ready to land.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118052

Files:
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/X86/x86-cf-protection.c


Index: clang/test/CodeGen/X86/x86-cf-protection.c
===
--- clang/test/CodeGen/X86/x86-cf-protection.c
+++ clang/test/CodeGen/X86/x86-cf-protection.c
@@ -1,8 +1,17 @@
 // RUN: %clang -target i386-unknown-unknown -x c -E -dM -o - 
-fcf-protection=return %s | FileCheck %s --check-prefix=RETURN
 // RUN: %clang -target i386-unknown-unknown -x c -E -dM -o - 
-fcf-protection=branch %s | FileCheck %s --check-prefix=BRANCH
 // RUN: %clang -target i386-unknown-unknown -x c -E -dM -o - 
-fcf-protection=full %s   | FileCheck %s --check-prefix=FULL
+// RUN: %clang -target i386-unknown-unknown -o - -emit-llvm -S 
-fcf-protection=branch -mibt-seal -flto %s | FileCheck %s --check-prefix=IBTSEAL
+// RUN: %clang -target i386-unknown-unknown -o - -emit-llvm -S 
-fcf-protection=branch -flto %s | FileCheck %s --check-prefix=NOIBTSEAL
+// RUN: %clang -target i386-unknown-unknown -o - -emit-llvm -S 
-fcf-protection=branch -mibt-seal %s | FileCheck %s --check-prefix=NOLTO
 
 // RETURN: #define __CET__ 2
 // BRANCH: #define __CET__ 1
 // FULL: #define __CET__ 3
+// IBTSEAL: "cf-protection-branch", i32 1
+// IBTSEAL: "ibt-seal", i32 1
+// NOIBTSEAL: "cf-protection-branch", i32 1
+// NOIBTSEAL-NOT: "ibt-seal", i32 1
+// NOLTO: "cf-protection-branch", i32 1
+// NOLTO-NOT: "ibt-seal", i32 1
 void foo() {}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1478,6 +1478,9 @@
   else if (Opts.CFProtectionBranch)
 GenerateArg(Args, OPT_fcf_protection_EQ, "branch", SA);
 
+  if (Opts.IBTSeal)
+GenerateArg(Args, OPT_mibt_seal, SA);
+
   for (const auto &F : Opts.LinkBitcodeFiles) {
 bool Builtint = F.LinkFlags == llvm::Linker::Flags::LinkOnlyNeeded &&
 F.PropagateAttrs && F.Internalize;


Index: clang/test/CodeGen/X86/x86-cf-protection.c
===
--- clang/test/CodeGen/X86/x86-cf-protection.c
+++ clang/test/CodeGen/X86/x86-cf-protection.c
@@ -1,8 +1,17 @@
 // RUN: %clang -target i386-unknown-unknown -x c -E -dM -o - -fcf-protection=return %s | FileCheck %s --check-prefix=RETURN
 // RUN: %clang -target i386-unknown-unknown -x c -E -dM -o - -fcf-protection=branch %s | FileCheck %s --check-prefix=BRANCH
 // RUN: %clang -target i386-unknown-unknown -x c -E -dM -o - -fcf-protection=full %s   | FileCheck %s --check-prefix=FULL
+// RUN: %clang -target i386-unknown-unknown -o - -emit-llvm -S -fcf-protection=branch -mibt-seal -flto %s | FileCheck %s --check-prefix=IBTSEAL
+// RUN: %clang -target i386-unknown-unknown -o - -emit-llvm -S -fcf-protection=branch -flto %s | FileCheck %s --check-prefix=NOIBTSEAL
+// RUN: %clang -target i386-unknown-unknown -o - -emit-llvm -S -fcf-protection=branch -mibt-seal %s | FileCheck %s --check-prefix=NOLTO
 
 // RETURN: #define __CET__ 2
 // BRANCH: #define __CET__ 1
 // FULL: #define __CET__ 3
+// IBTSEAL: "cf-protection-branch", i32 1
+// IBTSEAL: "ibt-seal", i32 1
+// NOIBTSEAL: "cf-protection-branch", i32 1
+// NOIBTSEAL-NOT: "ibt-seal", i32 1
+// NOLTO: "cf-protection-branch", i32 1
+// NOLTO-NOT: "ibt-seal", i32 1
 void foo() {}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1478,6 +1478,9 @@
   else if (Opts.CFProtectionBranch)
 GenerateArg(Args, OPT_fcf_protection_EQ, "branch", SA);
 
+  if (Opts.IBTSeal)
+GenerateArg(Args, OPT_mibt_seal, SA);
+
   for (const auto &F : Opts.LinkBitcodeFiles) {
 bool Builtint = F.LinkFlags == llvm::Linker::Flags::LinkOnlyNeeded &&
 F.PropagateAttrs && F.Internalize;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122673: Add kcfi_unchecked attribute

[Joao Moreira via Phabricator via cfe-commits]

joaomoreira added a comment.

> In the previous discussion, @joaomoreira  pointed out that this is very 
> similar to `nocf_check` and proposed reusing that attribute. In an offline 
> discussion, @pcc  was concerned that an attribute may not be the right 
> approach here and suggested  a `__builtin_kcfi_unchecked(function(args))` 
> built-in function to avoid changing the type system.

I'm still thinking a bit about this/needing some time to provide a proper 
review, but just to not hold the thoughts back since this is moving.

A consideration I can foresee with extending the type system with an attribute 
is that you then tie the function pointer prototype to the attribute for 
assignments and this will later require some casting magic if you want to 
invoke functions without kcfi_unchecked through a kcfi_unchecked pointer (which 
I assume should be doable). OTOH, it would be nice to get warnings when 
assigning a kcfi_unchecked functions to pointers which will later be used in 
checked indirect calls (yet, it would be better to have explicit warning about 
the inconsistency instead of implicit type mismatching ones).

Regarding nocf_check, my understanding is that the kernel IBT support assumes 
CET/IBT to be orthogonal to KCFI -- with that said, kernel implementation is 
set to never use or allow no-track prefixes, meaning that the above situation 
never happens for IBT (since there are no nocf_check function pointers). OTOH, 
I assume that there could be situations where you want the function pointer 
call to be relaxed/coarse-grained? If yes, this would need to be done through a 
different attribute other than nocf_check, since this sets the compiler to emit 
notrack prefixes that are invalid in kernel.

With all the above, it seems to me that using a kcfi-specific builtin could be 
the more flexible option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122673

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118052: [X86] Fix CodeGen Module Flag for -mibt-seal

[Joao Moreira via Phabricator via cfe-commits]

joaomoreira updated this revision to Diff 423775.
This revision is now accepted and ready to land.

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

https://reviews.llvm.org/D118052

Files:
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/X86/x86-cf-protection.c


Index: clang/test/CodeGen/X86/x86-cf-protection.c
===
--- clang/test/CodeGen/X86/x86-cf-protection.c
+++ clang/test/CodeGen/X86/x86-cf-protection.c
@@ -1,8 +1,14 @@
 // RUN: %clang -target i386-unknown-unknown -x c -E -dM -o - 
-fcf-protection=return %s | FileCheck %s --check-prefix=RETURN
 // RUN: %clang -target i386-unknown-unknown -x c -E -dM -o - 
-fcf-protection=branch %s | FileCheck %s --check-prefix=BRANCH
 // RUN: %clang -target i386-unknown-unknown -x c -E -dM -o - 
-fcf-protection=full %s   | FileCheck %s --check-prefix=FULL
+// RUN: %clang -target i386-unknown-unknown -o - -emit-llvm -S 
-fcf-protection=branch -mibt-seal -flto %s | FileCheck %s 
--check-prefixes=CFPROT,IBTSEAL
+// RUN: %clang -target i386-unknown-unknown -o - -emit-llvm -S 
-fcf-protection=branch -flto %s | FileCheck %s --check-prefixes=CFPROT,NOIBTSEAL
+// RUN: %clang -target i386-unknown-unknown -o - -emit-llvm -S 
-fcf-protection=branch -mibt-seal %s | FileCheck %s 
--check-prefixes=CFPROT,NOIBTSEAL
 
 // RETURN: #define __CET__ 2
 // BRANCH: #define __CET__ 1
 // FULL: #define __CET__ 3
+// CFPROT: "cf-protection-branch", i32 1
+// IBTSEAL: "ibt-seal", i32 1
+// NOIBTSEAL-NOT: "ibt-seal", i32 1
 void foo() {}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1478,6 +1478,9 @@
   else if (Opts.CFProtectionBranch)
 GenerateArg(Args, OPT_fcf_protection_EQ, "branch", SA);
 
+  if (Opts.IBTSeal)
+GenerateArg(Args, OPT_mibt_seal, SA);
+
   for (const auto &F : Opts.LinkBitcodeFiles) {
 bool Builtint = F.LinkFlags == llvm::Linker::Flags::LinkOnlyNeeded &&
 F.PropagateAttrs && F.Internalize;


Index: clang/test/CodeGen/X86/x86-cf-protection.c
===
--- clang/test/CodeGen/X86/x86-cf-protection.c
+++ clang/test/CodeGen/X86/x86-cf-protection.c
@@ -1,8 +1,14 @@
 // RUN: %clang -target i386-unknown-unknown -x c -E -dM -o - -fcf-protection=return %s | FileCheck %s --check-prefix=RETURN
 // RUN: %clang -target i386-unknown-unknown -x c -E -dM -o - -fcf-protection=branch %s | FileCheck %s --check-prefix=BRANCH
 // RUN: %clang -target i386-unknown-unknown -x c -E -dM -o - -fcf-protection=full %s   | FileCheck %s --check-prefix=FULL
+// RUN: %clang -target i386-unknown-unknown -o - -emit-llvm -S -fcf-protection=branch -mibt-seal -flto %s | FileCheck %s --check-prefixes=CFPROT,IBTSEAL
+// RUN: %clang -target i386-unknown-unknown -o - -emit-llvm -S -fcf-protection=branch -flto %s | FileCheck %s --check-prefixes=CFPROT,NOIBTSEAL
+// RUN: %clang -target i386-unknown-unknown -o - -emit-llvm -S -fcf-protection=branch -mibt-seal %s | FileCheck %s --check-prefixes=CFPROT,NOIBTSEAL
 
 // RETURN: #define __CET__ 2
 // BRANCH: #define __CET__ 1
 // FULL: #define __CET__ 3
+// CFPROT: "cf-protection-branch", i32 1
+// IBTSEAL: "ibt-seal", i32 1
+// NOIBTSEAL-NOT: "ibt-seal", i32 1
 void foo() {}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1478,6 +1478,9 @@
   else if (Opts.CFProtectionBranch)
 GenerateArg(Args, OPT_fcf_protection_EQ, "branch", SA);
 
+  if (Opts.IBTSeal)
+GenerateArg(Args, OPT_mibt_seal, SA);
+
   for (const auto &F : Opts.LinkBitcodeFiles) {
 bool Builtint = F.LinkFlags == llvm::Linker::Flags::LinkOnlyNeeded &&
 F.PropagateAttrs && F.Internalize;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119296: KCFI sanitizer

[Joao Moreira via Phabricator via cfe-commits]

joaomoreira added a comment.

I played a little bit with kcfi and here are some thoughts:

- under -Os I saw functions being inlined, regardless of the source code 
calling them indirectly. In these scenarios, the KCFI check was still in place, 
even though there was not a pointer involved in the call. Although not 
semantically incorrect, it would be great to prevent the unnecessary overhead 
(see attached source/compile it with -Os and -fsanitize=kcfi).

F22842586: example.c .

- I noticed that KCFI checks are placed much before the indirect call arguments 
are properly placed in the passing registers. This makes the position of the 
check unpredictable. I assume this is a bad thing in case the kernel eventually 
decides to come up with an approach that uses alternatives CFI schemes through 
text-patching.
- On the same line as the above comment, and on a complete hypothetical 
thought, this kind of CFI approach is subject to "Time-of-Check/Time-of-Use" 
attacks, in the sense that if you have a longer sequence of instructions 
between the check and the call, the easier it is for an attacker to win the 
race condition and you are more susceptible to the attack. This shouldn't be a 
huge concern considering that the function pointer is inside a register, but I 
think it is reasonable to keep in mind that register contents may go into 
memory in the occasion of interruptions and such. This is really me being picky 
(so feel free to disregard), but I think it counts as a reason to keep checks 
and calls as close as possible.

Because of things like the above, in the past I decided to implement these 
things in the very backend of the compiler, so other optimizations would not 
break the layout nor leave dummy checks around. I find it nice to have this 
implemented as a more architecture general feature, but maybe it would be cool 
to have a finalization pass in the X86 backend just to tie things. Or maybe you 
have a better approach on how to prevent these.




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:
> 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.
Please, consider double checking that ENDBR instructions are not accidentally 
generated as tags.

Also, assuming that there will exist kcfi_unchecked calls in the source code, 
it would be great to ensure that critical instructions like clac are also not 
accidentally generated as tags.



Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:114
+  // Emit two zero bytes to avoid gadgets in llvm.kcfi.check().
+  OutStreamer->emitZeros(2);
+}

I have seen speculative mitigations using instructions that stop the 
control-flow (like int3 or hlt) as space fillers just as a 
redundant/security-in-depth scheme to hold any speculative flow that may have 
been launched from somewhere else. I think these zeros here can be replaced by 
0xcc without harm, right?


Repository:
  rG LLVM 

[PATCH] D119296: KCFI sanitizer

[Joao Moreira via Phabricator via cfe-commits]

joaomoreira added a comment.

Oh, one other tiny detail I forgot to mention. I noticed that the tag is 
pushing the functions 6 bytes forward, regardless of any prepending padding 
nops that were added to ensure 16b alignment. It would be cool to care about 
the proper function alignment and also to not emit dummy padding nops when the 
padding area can be filled with the tag itself.


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

[PATCH] D119296: KCFI sanitizer

[Joao Moreira via Phabricator via cfe-commits]

joaomoreira added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:696
+def KCFIUnchecked : Attr {
+  let Spellings = [Clang<"kcfi_unchecked">];
+  let Subjects = SubjectList<[Var, TypedefName]>;

Are you considering that perhaps one could use KCFI and X86 CET/IBT at the same 
time? If not, is there a reason for not just reusing the existing "nocf_check" 
attribute?


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

[PATCH] D118052: [X86] Fix CodeGen Module Flag for -mibt-seal

[Joao Moreira via Phabricator via cfe-commits]

joaomoreira added a comment.

I think there are no more untied knots... @pengfei, do you think this is ready 
to merge? If yes, can you please merge it? tks!


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

https://reviews.llvm.org/D118052

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119296: KCFI sanitizer

[Joao Moreira via Phabricator via cfe-commits]

joaomoreira added a subscriber: craig.topper.
joaomoreira added a comment.

> This seems like a reasonable approach, and was also the approach taken for 
> the PAuth ABI. The PAuth ABI attaches an operand bundle to the call 
> instruction and arranges for the code for the check to be generated together 
> with the call. This helps with avoiding spills of the verified function 
> pointer between the check and the call. The code isn't upstream but is 
> available on this branch: https://github.com/pcc/llvm-project/tree/apple-pac4
>
> Grep for something like `undle.*ptrauth` and you should find the relevant 
> code.

FWIIW, the way I implemented this on FineIBT 
(https://github.com/lvwr/llvm-project/tree/fineibt/kernel ) was by adding an 
attribute to the MachineInstr class and then forwarding the prototype to be 
checked whenever a call instruction was translated from IR into MachineIR. 
Then, in the back-end, the IBT pass expands the hash-related instructions from 
the MachineInstr attribute. It is simple and concise and I think it resembles 
what @pcc is suggesting (please, correct me if I'm mistaken). Of course, the 
above mentioned branch is super raw, but it gives an insight.

At first I was on the boat of KCFI being implemented in the IR level because 
being architecture agnostic is certainly a great plus. Yet, once we end up 
needing additional passes (regardless of it being in IR or in the arch-specific 
back-end) to tie things and achieve the best possible instrumentation, I start 
to think we are over-complicating the design of the feature -- if we have a 
single pass at the very end of each supported arch back-end that just traverses 
the functions and adds KCFI checks to indirect branches that have a 
"PrototypeHash" attribute set (or an special operand as also suggested by @pcc 
) we can get the best possible instrumentation while inherently dodging any 
pitfall due to unforeseen optimizations or code transformations that may happen 
between the KCFI checks placement placement and the last stage of the back-end.

I'm not an expert on LLVM's pipeline, but it just feels a little awkward and 
redundant that we need passes to fix what other passes messed up regarding a 
pass that executed before everything. Also, I don't see clearly how it could 
not be in the back-end if we want to avoid all the call argument setup in 
between the check and the call. Perhaps more people with a broader view 
regarding "CodeGen->Arch Backend" (like @craig.topper :)) could be brought into 
this discussion and share some thoughts on how to stitch it all more 
efficiently.


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

[PATCH] D119296: KCFI sanitizer

[Joao Moreira via Phabricator via cfe-commits]

joaomoreira added a comment.

> I agree that a separate pass wasn't ideal, but InstCombine seems to be full 
> of code to "fix what other passes messed up".  :)  I'm not sure if messed up 
> is the correct term though, these are checks that were necessary before 
> optimizations, but are no longer needed.

Pardon my lack of vocabulary, I didn't mean it in a disrespectful way -- what I 
meant is that having a third pass to get things in optimal shape because other 
passes moved stuff around seems to be not ideal, or at least less ideal than 
having a single pass that only puts things in their places once all the rest 
was already moved around.

It doesn't sound unreal to me thinking about a backend pass that could also 
move things around and still break the instrumentation after it is reparable, 
but this is hypothetical at this point as nothing comes to mind right away, so 
I won't push this button.


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

[PATCH] D119296: KCFI sanitizer

[Joao Moreira via Phabricator via cfe-commits]

joaomoreira added a comment.

> I looked at your code quickly and I wonder if using operand bundles would be 
> better than adding an attribute. Thoughts?

Perhaps @pcc can provide a better feedback on the hassle (or joys) behind using 
operand bundles. My insights on the attribute is that it is very simple  and 
cheap to set, get, copy... The bad side is that now you have an additional 
attribute on a frequently used class.


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

[PATCH] D119296: KCFI sanitizer

[Joao Moreira via Phabricator via cfe-commits]

joaomoreira added a comment.

In D119296#3483176 , @nickdesaulniers 
wrote:

> In D119296#3481573 , @joaomoreira 
> wrote:
>
>> I'm not an expert on LLVM's pipeline, but it just feels a little awkward and 
>> redundant that we need passes to fix what other passes messed up regarding a 
>> pass that executed before everything.
>
> I don't think so. Consider DCE; other passes leave behind garbage all the 
> time; DCE is expected to clean up after them.

Hm. Ok, if this is normalized, then I guess my point is just bike shedding, so 
I drop the argument.

FWIIW, while I would still prefer the checks to be adjacent to the indirect 
call, I can live with it. I tested the instrumentation and it looks fine on my 
end and the fixed version of the code seems to handle the dummy checks for 
good. With this said, it LGTM.


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

[PATCH] D118355: Add -mmanual-endbr switch to allow manual selection of control-flow protection

[Joao Moreira via Phabricator via cfe-commits]

joaomoreira added inline comments.



Comment at: llvm/lib/Target/X86/X86IndirectBranchTracking.cpp:156-161
   if (needsPrologueENDBR(MF, M)) {
-auto MBB = MF.begin();
-Changed |= addENDBR(*MBB, MBB->begin());
+if (!ManualENDBR || MF.getFunction().doesCfCheck()) {
+  auto MBB = MF.begin();
+  Changed |= addENDBR(*MBB, MBB->begin());
+} else {
+  // When -mmanual-endbr is in effect, the compiler does not

xiangzhangllvm wrote:
> I am a little puzzle here. Let me copy your patch description here:
> 
> ```
> >When -mmanual-endbr is set, llvm refrains from automatically adding
> >ENDBR instructions to functions' prologues, which would have been
> >automatically added by -fcf-protection=branch. Although this works
> >correctly, missing ENDBR instructions where they are actually needed
> >could lead to broken binaries, which would fail only in running time.
> ```
> I think the purpose of "-mmanual-endbr" should be "Supplementary Way" for the 
> cases which the CET can't correctly insert endbr in automatic way.
> Here (in if (needsPrologueENDBR(MF, M)) ) the automatic way will insert the 
> endbr. So I think the job for "-mmanual-endbr" should be done in parallel 
> with the old condition. Some like:
> ```
> if (ManualENDBR ){
>   do something
> }else { // automatic
>   if (needsPrologueENDBR(MF, M)) {insert endbr}
>  }
> }
> ```
I don't think the idea of -mmanual-endbr is to have a supplementary way for 
corner cases where CET misses automatic placement. In my understanding the goal 
is to set the compiler to not emit ENDBRs unless the attribute cf_check is 
used, thus providing a way to manually reduce the number of valid targets.

For reference, here is a link for -mmanual-endbr on gcc, 
https://gcc.gnu.org/legacy-ml/gcc-patches/2018-12/msg00713.html and here are 
patches on xen that use the feature (and that also mention this review): 
https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg114160.html


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118355

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87822: [FPEnv] Evaluate constant expressions under non-default rounding modes

[Joao Moreira via Phabricator via cfe-commits]

joaomoreira added a comment.

I noticed that this commit breaks MUSL 1.2.0. Here is an isolated test-case 
that illustrates the issue:

https://godbolt.org/z/6bdnEh


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87822

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119296: KCFI sanitizer

[Joao Moreira via Phabricator via cfe-commits]

joaomoreira added inline comments.



Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:121
+if (N == Value)
+  return ~Value;
+  }

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.



Comment at: llvm/test/CodeGen/X86/kcfi.ll:91
+
+;; Ensure we emit ~Type for unwanted values (e.g. endbr64 == 4196274163).
+; ASM-LABEL: __cfi_f5:

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).


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

[PATCH] D119296: KCFI sanitizer

[Joao Moreira via Phabricator via cfe-commits]

joaomoreira added inline comments.



Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:121
+if (N == Value)
+  return ~Value;
+  }

samitolvanen wrote:
> 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.
Oops, got confused with the operands, tks for clearing it up. Otherwise, 
changes LGTM.


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

[PATCH] D140363: Remove incorrectly implemented -mibt-seal

[Joao Moreira via Phabricator via cfe-commits]

joaomoreira added a comment.

FWIIW, agreed on removing this until we figure out how to make it work 
properly. Thanks for the patch @MaskRay.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140363

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140035: [X86] Prevent -mibt-seal to work together with -flto=thin

[Joao Moreira via Phabricator via cfe-commits]

joaomoreira added a comment.

Given https://reviews.llvm.org/D140363, this patch is being abandoned.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140035

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119296: KCFI sanitizer

[Joao Moreira via Phabricator via cfe-commits]

joaomoreira added inline comments.



Comment at: llvm/lib/Target/X86/X86ISelLowering.h:83
+KCFI_NT_CALL,
+KCFI_TC_RETURN,
+

I did not revise the entire patch yet. With this said, IMHO, this looks like an 
overcomplication of a simple problem. Is there a reason why you really need 
specific KCFI_ nodes instead of only embedding the hash information into an 
attribute at the Machine Instruction? Then, if hash == 0, it just means it is a 
call that doesn't need instrumentation.

This latter approach will require less code and should be easier to maintain 
compatible with other CFI approaches. If the reason is because you don't want 
to have a useless attribute for non-call instructions, then you could possibly 
have a map where you bind the call instruction with a respective hash.

Unless there is a strong reason for these, I would much better prefer the slim 
approach suggested. Either way, if there is a reason for this, I would also 
suggest that you at least don't name these as "KCFI_something", as in the 
future others might want to reuse the same structure for other CFI approaches.


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

[PATCH] D119296: KCFI sanitizer

[Joao Moreira via Phabricator via cfe-commits]

joaomoreira added inline comments.



Comment at: llvm/lib/Target/X86/X86ISelLowering.h:83
+KCFI_NT_CALL,
+KCFI_TC_RETURN,
+

samitolvanen wrote:
> joaomoreira wrote:
> > I did not revise the entire patch yet. With this said, IMHO, this looks 
> > like an overcomplication of a simple problem. Is there a reason why you 
> > really need specific KCFI_ nodes instead of only embedding the hash 
> > information into an attribute at the Machine Instruction? Then, if hash == 
> > 0, it just means it is a call that doesn't need instrumentation.
> > 
> > This latter approach will require less code and should be easier to 
> > maintain compatible with other CFI approaches. If the reason is because you 
> > don't want to have a useless attribute for non-call instructions, then you 
> > could possibly have a map where you bind the call instruction with a 
> > respective hash.
> > 
> > Unless there is a strong reason for these, I would much better prefer the 
> > slim approach suggested. Either way, if there is a reason for this, I would 
> > also suggest that you at least don't name these as "KCFI_something", as in 
> > the future others might want to reuse the same structure for other CFI 
> > approaches.
> > Is there a reason why you really need specific KCFI_ nodes instead of only 
> > embedding the hash information into an attribute at the Machine Instruction?
> 
> This implementation is similar to `CALL_RVMARKER`, `CALL_BTI` and basically 
> all other pseudo call instructions in LLVM. Is adding an attribute to 
> `MachineInstr` the preferred approach instead?
> 
> > I would also suggest that you at least don't name these as 
> > "KCFI_something", as in the future others might want to reuse the same 
> > structure for other CFI approaches.
> 
> Always happy to hear suggestions for alternative naming. Did you have 
> something in mind?
> This implementation is similar to `CALL_RVMARKER`, `CALL_BTI` and basically 
> all other pseudo call instructions in LLVM. Is adding an attribute to 
> `MachineInstr` the preferred approach instead?

My understanding is that if, every time a new mitigation or optimization comes 
in, you create a new opcode for it, it will eventually bloat to non-feasibility.

Imagine you have some mitigation like [[ 
https://www.cs.columbia.edu/~vpk/research/kguard/ | kguard  ]] being 
implemented. Now you can have calls which are KCFI checked but not KGUARD 
checked; then KCFI not-checked but KGUARD checked; then KCFI and KGUARD 
checked.; then none-checked. And then you need all these variations for tail 
calls (which imho is a first, minor, instance of the problem)...

So, in general, my understanding is that this approach works, yeah, but that in 
the long term it could become a hassle... so ideally we should use attributes 
to define these sub-specific instructions instead of opcodes.

> 
> > I would also suggest that you at least don't name these as 
> > "KCFI_something", as in the future others might want to reuse the same 
> > structure for other CFI approaches.
> 
> Always happy to hear suggestions for alternative naming. Did you have 
> something in mind?

I think switching from KCFI into CFI would already be good enough, as in the 
end these are all implementing the [[ 
https://dl.acm.org/doi/10.1145/1102120.1102165 | control-flow integrity ]] 
concept.


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

[PATCH] D140035: [X86] Prevent -mibt-seal to work together with -flto=thin

[Joao Moreira via Phabricator via cfe-commits]

joaomoreira created this revision.
joaomoreira added reviewers: samitolvanen, nickdesaulniers, xiangzhangllvm, 
pengfei, aaron.ballman, pcc, gftg85.
joaomoreira added a project: LLVM.
Herald added a subscriber: inglorion.
Herald added a project: All.
joaomoreira requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.
Herald added a project: clang.

As pointed out by @samitolvanen and then confirmed by @nickdesaulniers  here - 
https://github.com/ClangBuiltLinux/linux/issues/1737 - -flto=thin does not 
reliably know about address-taken properties across different translation 
units. This breaks the assumptions behind the -mibt-seal optimization, that 
removes ENDBR instructions from prologues of functions which are observed as 
non-address-taken.

Thus, because of the above, prevent -mibt-seal from being used together with 
-flto=thin by only enabling it if lto is Full.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D140035

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/X86/x86-cf-protection.c


Index: clang/test/CodeGen/X86/x86-cf-protection.c
===
--- clang/test/CodeGen/X86/x86-cf-protection.c
+++ clang/test/CodeGen/X86/x86-cf-protection.c
@@ -2,6 +2,7 @@
 // RUN: %clang_cc1 -E -triple i386 -dM -o - -fcf-protection=branch %s | 
FileCheck %s --check-prefix=BRANCH
 // RUN: %clang_cc1 -E -triple i386 -dM -o - -fcf-protection=full %s   | 
FileCheck %s --check-prefix=FULL
 // RUN: %clang_cc1 -emit-llvm -triple i386 -o - -fcf-protection=branch 
-mibt-seal -flto %s | FileCheck %s --check-prefixes=CFPROT,IBTSEAL
+// RUN: %clang_cc1 -emit-llvm -triple i386 -o - -fcf-protection=branch 
-mibt-seal -flto=thin %s | FileCheck %s --check-prefixes=CFPROT,NOIBTSEAL
 // RUN: %clang_cc1 -emit-llvm -triple i386 -o - -fcf-protection=branch -flto 
%s | FileCheck %s --check-prefixes=CFPROT,NOIBTSEAL
 // RUN: %clang_cc1 -emit-llvm -triple i386 -o - -fcf-protection=branch 
-mibt-seal %s | FileCheck %s --check-prefixes=CFPROT,NOIBTSEAL
 // RUN: not %clang_cc1 -emit-llvm-only -triple i386 -target-cpu pentium-mmx 
-fcf-protection=branch %s 2>&1 | FileCheck %s --check-prefix=NOCFPROT
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1853,8 +1853,11 @@
   Opts.FunctionReturnThunks = static_cast(Val);
   }
 
-  if (Opts.PrepareForLTO && Args.hasArg(OPT_mibt_seal))
+  if (Opts.PrepareForLTO &&
+  !Opts.PrepareForThinLTO &&
+  Args.hasArg(OPT_mibt_seal)) {
 Opts.IBTSeal = 1;
+  }
 
   for (auto *A :
Args.filtered(OPT_mlink_bitcode_file, OPT_mlink_builtin_bitcode)) {
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -6439,7 +6439,7 @@
 Args.MakeArgString(Twine("-fcf-protection=") + A->getValue()));
   }
 
-  if (IsUsingLTO)
+  if (LTOMode == LTOK_Full)
 Args.AddLastArg(CmdArgs, options::OPT_mibt_seal);
 
   if (Arg *A = Args.getLastArg(options::OPT_mfunction_return_EQ))


Index: clang/test/CodeGen/X86/x86-cf-protection.c
===
--- clang/test/CodeGen/X86/x86-cf-protection.c
+++ clang/test/CodeGen/X86/x86-cf-protection.c
@@ -2,6 +2,7 @@
 // RUN: %clang_cc1 -E -triple i386 -dM -o - -fcf-protection=branch %s | FileCheck %s --check-prefix=BRANCH
 // RUN: %clang_cc1 -E -triple i386 -dM -o - -fcf-protection=full %s   | FileCheck %s --check-prefix=FULL
 // RUN: %clang_cc1 -emit-llvm -triple i386 -o - -fcf-protection=branch -mibt-seal -flto %s | FileCheck %s --check-prefixes=CFPROT,IBTSEAL
+// RUN: %clang_cc1 -emit-llvm -triple i386 -o - -fcf-protection=branch -mibt-seal -flto=thin %s | FileCheck %s --check-prefixes=CFPROT,NOIBTSEAL
 // RUN: %clang_cc1 -emit-llvm -triple i386 -o - -fcf-protection=branch -flto %s | FileCheck %s --check-prefixes=CFPROT,NOIBTSEAL
 // RUN: %clang_cc1 -emit-llvm -triple i386 -o - -fcf-protection=branch -mibt-seal %s | FileCheck %s --check-prefixes=CFPROT,NOIBTSEAL
 // RUN: not %clang_cc1 -emit-llvm-only -triple i386 -target-cpu pentium-mmx -fcf-protection=branch %s 2>&1 | FileCheck %s --check-prefix=NOCFPROT
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1853,8 +1853,11 @@
   Opts.FunctionReturnThunks = static_cast(Val);
   }
 
-  if (Opts.PrepareForLTO && Args.hasArg(OPT_mibt_seal))
+  if (Opts.PrepareForLTO &&
+  !Opts.PrepareForThinLTO &&
+  Args.hasArg(OPT_mibt_seal)) {
 Opts.IBTSeal = 1;
+  }
 
   for (auto *A :
Args.filtered(OPT_mlink_bitcode_file, OPT

[PATCH] D140035: [X86] Prevent -mibt-seal to work together with -flto=thin

[Joao Moreira via Phabricator via cfe-commits]

joaomoreira added a comment.

Regarding https://github.com/ClangBuiltLinux/linux/issues/1737:

Weirdly enough, I double-tested the behavior for -flto=thin + -mibt-seal; the 
kernel did boot fine on my setup, but when dumped/grep'ed for ENDBRs, it had 
~500 less ENDBRs throughout the binary. Considering that LTO-based 
optimizations applied with -flto=thin should have also been applied with -flto 
(which means, things like dead-code elimination should be symmetrical in both 
generated binaries), and also given the confirmation that -flto=thin won't 
properly keep address-taken details across translation units, my understanding 
is that -mibt-seal must be disabled when -flto=thin. Thus, submitted the patch 
even though I was not able to reproduce the bad behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140035

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140035: [X86] Prevent -mibt-seal to work together with -flto=thin

[Joao Moreira via Phabricator via cfe-commits]

joaomoreira added a comment.

>> Weirdly enough, I double-tested the behavior for -flto=thin + -mibt-seal; 
>> the kernel did boot fine on my setup, but when dumped/grep'ed for ENDBRs, it 
>> had ~500 less ENDBRs throughout the binary
>
> Did you confirm the issue with the reproducer in the CBL bug? It would be 
> interesting to find out why you couldn't reproduce this in the kernel.

Yes, the reproducer from CBL highlights the issue. I tested it long ago and 
forgot to add the detail here, yet it should by itself suffice as a motivation 
for this fix. Thanks for bringing that up.

Regarding not being able to reproduce this in kernel -- never mind... I was 
misled by setup issues while running IBT kernels in QEMU. I managed to fix the 
setup and confirm that kernel won't boot. Thanks for pushing this bit too.

Also, FWIIW, objtool alerts about a bunch of relocations pointing to !endbr 
instructions when compiling with -flto=thin. When compiling with 
-flto+-mibt-seal, the only alert is for a data relocation to !non-endbr towards 
x86_64_start_kernel, which doesn't seem to be a concern since (already under 
the fixed setup) the kernel still doesn't trip.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140035

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140035: [X86] Prevent -mibt-seal to work together with -flto=thin

[Joao Moreira via Phabricator via cfe-commits]

joaomoreira added a comment.

In D140035#3998954 , @samitolvanen 
wrote:

> In D140035#3996066 , @joaomoreira 
> wrote:
>
>> Regarding not being able to reproduce this in kernel -- never mind... I was 
>> misled by setup issues while running IBT kernels in QEMU. I managed to fix 
>> the setup and confirm that kernel won't boot.
>
> OK, great. Thanks for double checking!
>
> The patch itself looks good to me, but I suspect ibt-seal in general has the 
> same issue as D138337  where it can drop 
> endbr instructions from `isUsedInRegularObj` symbols that are not 
> address-taken in the bitcode (e.g. functions whose address is only taken in 
> stand-alone assembly). I saw this issue only in the arm64 Linux kernel, but 
> there's always a chance a similar code pattern emerges on the x86 side at 
> some point in future too. This can obviously be worked around in the kernel, 
> but just something to keep in mind.

Ugh, yeah. I reproduced the behavior with a .s and a .C file on a similar 
scheme as the repro you wrote for the thin lto problem. I'll start looking for 
a fix once I'm back from vacation in Jan. Tks for pointing this out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140035

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118052: [X86] Fix CodeGen Module Flag for -mibt-seal

[Joao Moreira via Phabricator via cfe-commits]

joaomoreira planned changes to this revision.
joaomoreira added inline comments.
Herald added a project: All.



Comment at: clang/test/CodeGen/X86/x86-cf-protection.c:4
 // RUN: %clang -target i386-unknown-unknown -x c -E -dM -o - 
-fcf-protection=full %s   | FileCheck %s --check-prefix=FULL
+// RUN: %clang -target i386-unknown-unknown -o - -emit-llvm -S 
-fcf-protection=branch -mibt-seal -flto %s | FileCheck %s --check-prefix=IBTSEAL
 

pengfei wrote:
> Is `-flto` is required?
Yes, we can only suppress ENDBR if we are sure the given function is not 
address taken in all other translation units.



Comment at: clang/test/CodeGen/X86/x86-cf-protection.c:9-10
 // FULL: #define __CET__ 3
+// IBTSEAL: "cf-protection-branch", i32 1
+// IBTSEAL: "ibt-seal", i32 1
 void foo() {}

pengfei wrote:
> Can we add another RUN without `-mibt-seal` amd check no such flags?
Sure, I'll work on this try to track the possible bug mentioned by 
@aaron.ballman, then I'll update the diff.


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

https://reviews.llvm.org/D118052

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits