Page MenuHomePhabricator

Added control flow architecture protection Flag
ClosedPublic

Authored by oren_ben_simhon on Nov 27 2017, 12:51 AM.

Details

Summary

Instrument control flow is target independent flag that instructs the back-end to instrument control flow mechanisms.
Specifically in X86 this flag will be used to instrument Indirect Branch Tracking instructions.

Diff Detail

Repository
rC Clang

Event Timeline

craig.topper added inline comments.Nov 29 2017, 12:08 AM
lib/Driver/ToolChains/Clang.cpp
4010

This should probably have a blank line above it. Right now it kinda looks like it goes with the OpenCL code.

Implemented comments posted until 11/29 (Thanks Craig)

oren_ben_simhon marked an inline comment as done.Nov 29 2017, 6:42 AM
pcc added a subscriber: pcc.Nov 30 2017, 12:00 PM
pcc added inline comments.
include/clang/Driver/Options.td
1050

My comments about the attribute name in D40482 also apply to this flag name. GCC appears to have chosen a better name for this flag, -mcet. Can we use that name instead?

include/clang/Driver/Options.td
1050

Again, I am sorry for the late response, I was off for about a week.
GCC is only using -mcet as a super set for -mibt and -mshstk. I am planning to add it in a different review.
-mcet/-mibt/-mshstk only enables the HW technology (ISA/Intrinsics).
GCC is using -cf-protection for actually instrumenting the control flow.
I think it will be best to stay coherent with GCC (and ICC and soon MS) and use -cf-protection.
What do you think?

pcc added inline comments.Dec 7 2017, 2:01 PM
include/clang/Driver/Options.td
1050

I see. As far as I can tell, there are no released versions of GCC and ICC with the new flags, so I think there is still a possibility to change them. (MS is a different story, their flags are incompatible anyway.)

I took a closer look at what GCC does, and here are my thoughts:

  • As far as I can tell, IBT does not introduce any new instructions other than ENDBRANCH, and it doesn't seem to make sense for ENDBRANCH to be exposed via intrinsics (but rather via attributes, as you have done). That means that -mibt basically does nothing except for allowing the use of -fcf-protection.
  • SHSTK introduces ISA extensions that seem reasonable to expose via intrinsics, so it would probably be fine to have a -mshstk flag that enables the intrinsics.
  • In GCC, -fcf-protection means "emit code that is compatible with CET" and requires that the user pass -mibt or -mshstk. But the code that it emits is also compatible with non-CET processors, so it doesn't make sense to me to require the use of flags such as -mshstk which imply that SHSTK is required.

As for the name of the flag: there is nothing about the name -fcf-protection that implies hardware-based protection, so the presence of this flag could confuse users who want software-based protection. The hardware-based protection is sort of implied by the requirement to pass -mibt or -mshstk, but that is a little awkward, and I don't think it makes sense either for the reasons I mentioned. What we want is a clear way to say "I want to generate code that is compatible with (but does not require) this hardware feature", but there doesn't seem to be any precedent for how to express that. The most straightforward way to express it is to mention the name of the hardware feature in the flag.

So here are the changes that I would propose across compilers:

  • Remove the -mibt flag.
  • Change the flag that emits ENDBRANCH to something like -mcet-compatible and stop requiring the use of other flags.

Please let me know what you think.

include/clang/Driver/Options.td
1050

It is true that -mibt doesn't enable intrinsics however it is required for the case of inline asm.
The code that -cf-protection produce is not always compatible with non-CET processors.
For example, consider the required fix to the shadow stack in the case of setJmp/longJmp.
In that case, we need to use SHSTK instructions (like incssp) that are not compatible.
Would the flag -fcf-arch-protection make more sense (as it implies on HW support)?

pcc added inline comments.Dec 12 2017, 5:38 PM
include/clang/Driver/Options.td
1050

It is true that -mibt doesn't enable intrinsics however it is required for the case of inline asm.

For enabling ENDBRANCH in inline asm? Would there be any harm in allowing ENDBRANCH unconditionally?

For example, consider the required fix to the shadow stack in the case of setJmp/longJmp.
In that case, we need to use SHSTK instructions (like incssp) that are not compatible.

I don't think that is true. I looked at the code that GCC emits for setjmp and longjmp, and it appears to quite cleverly avoid the need to execute SHSTK-required instructions on older processors by taking advantage of the fact that RDSSP is a NOP on older processors.

With this program:

#include <setjmp.h>

jmp_buf env;
void f() {
  __builtin_setjmp(env);
}

void g() {
  __builtin_longjmp(env, 1);
}

I get this asm for saving the SSP:

	movl	$0, %eax
	rdsspq	%rax
	movq	%rax, env+24(%rip)

and this asm for restoring it:

	movl	$0, %eax
	rdsspq	%rax
	subq	env+24(%rip), %rax
	je	.L5
	negq	%rax
	shrq	$3, %rax
	cmpq	$255, %rax
	jbe	.L6
.L7:
	incsspq	%rax
	subq	$255, %rax
	cmpq	$255, %rax
	ja	.L7
.L6:
	incsspq	%rax
.L5:

So on a non-SHSTK processor, we will store 0 to the slot reserved for SSP during setjmp, and then compare 0 to 0 during longjmp and bypass the entire loop.

Would the flag -fcf-arch-protection make more sense (as it implies on HW support)?

It isn't ideal to me, but it does at least seem better than -fcf-protection.

Implemented some of the comments posted until 12/12 (Thanks Peter)

include/clang/Driver/Options.td
1050

You are correct, -mshstk flag should be used only for enabling the intrinsics that are not rdssp.
It shouldn't be a requirement for generating shadow stack fixes. Is that what you meant?

craig.topper edited subscribers, added: cfe-commits; removed: llvm-commits.Dec 13 2017, 9:50 AM

-mibt isn't required for inline assembly, There's no AssemblePredicate defined with HasIBT in X86InstrInfo.td. We don't normally do fine grained assembler feature enabling so I believe we should always support it.

pcc added inline comments.Dec 13 2017, 9:55 AM
include/clang/Driver/Options.td
1050

Yes, exactly.

craig.topper added inline comments.Dec 13 2017, 12:29 PM
include/clang/Driver/Options.td
1050

I think the rdssp is a nop on older Intel processors, but its hard to say for sure. Older instruction manuals(I checked a 2001 version I had on my bookshelf) from Intel don't list anything in the opcode map for those instruction encodings. There are also other vendors that have made X86 CPUs that may not have implemented them as NOPs since they were undocumented. The "cleverness" is really there for CPUs that support CET instructions, but do not have OS support enabled via CR4.

oren_ben_simhon marked an inline comment as done.Dec 14 2017, 12:09 PM

Implemented all comments posted until 12/14 (Thanks Peter)

oren_ben_simhon retitled this revision from Added Instrument Control Flow Flag to Added control flow architecture protection Flag.Dec 14 2017, 12:12 PM

-mibt is currently in discussions with other compilers, any change will be uploaded to a different review.
If you have more comments i will appreciate it.

craig.topper added inline comments.Dec 18 2017, 1:13 PM
lib/CodeGen/CGCall.cpp
1737 ↗(On Diff #127008)

If the command line option is intended to be target independent, shouldn't these generically named?

lib/CodeGen/CodeGenFunction.cpp
890–891

Why this change?

oren_ben_simhon marked an inline comment as done.Dec 19 2017, 12:44 PM
oren_ben_simhon added inline comments.
lib/CodeGen/CodeGenFunction.cpp
890–891

Apparently, some compilers do strange optimizations on bit structures (like CodeGenOptions) that cause this code to be compiled wrongly.
I encountered this issue when i added a new Code Gen Option and compiled clang in windows.
In order to overcome this issue, i changed a bit the code and the wrong optimization didn't occur.

oren_ben_simhon added a reviewer: pcc.

Implemented comments posted until 12/19 (Thanks Craig)

Are we sure we want a different command line option name from gcc? From our internal conversations with the gcc folks I thought they were suggesting that -fcf-protection could imply a software mechanism if a hardware mechanism was not available thorugh -mibt or -march?

Should we emit an error to the user if -mibt isn't available? We should be able to add virtual methods on TargetInfo that X86 can customize to check for ibt and shstk.

Can you provide more information about the miscompile on MSVC? I think we should do more to understand that, this sounds like it could be a time bomb waiting to fail somewhere else.

Are we sure we want a different command line option name from gcc? From our internal conversations with the gcc folks I thought they were suggesting that -fcf-protection could imply a software mechanism if a hardware mechanism was not available thorugh -mibt or -march?

Should we emit an error to the user if -mibt isn't available? We should be able to add virtual methods on TargetInfo that X86 can customize to check for ibt and shstk.

Can you provide more information about the miscompile on MSVC? I think we should do more to understand that, this sounds like it could be a time bomb waiting to fail somewhere else.

LLVM already has a flag for SW mechanisms "-sanitize=*". Anyway I believe that GCC and LLVM should agree first before i change it. I restored cf-protection and will create a new patch review after an agreement with GCC will be made.

I added an error message.

I currently don't have more information. After seeing the issue I workaround it. I prefer not to open MSVC miscompilation issue in this code review and investigate it on a different thread.

Implemented comments posted until 12/24 (Thanks Craig)
Moved cf-protection attributes from function attributes to module attributes.

Reverted clang-format whitespaces updates

craig.topper added inline comments.Jan 3 2018, 10:45 AM
lib/CodeGen/CodeGenModule.cpp
506

Should we still be adding the module flag if the target says its not supported?

oren_ben_simhon marked an inline comment as done.

Implemented comments posted until 01/04 (Thanks Craig)

This revision is now accepted and ready to land.Jan 4 2018, 12:20 PM
Closed by commit rC322063: Added Control Flow Protection Flag (authored by orenb, committed by ). · Explain WhyJan 9 2018, 12:55 AM
This revision was automatically updated to reflect the committed changes.
alexfh added a subscriber: alexfh.Jan 9 2018, 6:36 AM
alexfh added inline comments.
test/CodeGen/x86-cf-protection.c
1

Any reason this test runs clang with "-S" and "-emit-llvm"? Neither of those seems to be needed for the actual checks being made below.

oren_ben_simhon added inline comments.Jan 9 2018, 6:56 AM
test/CodeGen/x86-cf-protection.c
1

I agree that -emit-llvm is redundant for the test checks. Without -S the command has no output on stdout or stderr. So the run fails and doesn't execute.

alexfh added inline comments.Jan 9 2018, 7:06 AM
test/CodeGen/x86-cf-protection.c
1

The specific problem I've stumbled upon is that the test fails when the source file is on a read-only partition:

[3250/3251] Running the Clang regression tests    
llvm-lit: /src/utils/lit/lit/llvm/config.py:334: note: using clang: /build/bin/clang
FAIL: Clang :: CodeGen/x86-cf-protection.c (2485 of 11862)
******************** TEST 'Clang :: CodeGen/x86-cf-protection.c' FAILED ********************
Script:                                           
--                                                
not /build/bin/clang -cc1 -internal-isystem /build/lib/clang/7.0.0/include -nostdsysteminc -fsyntax-only -S -emit-llvm -triple i386-unknown-unknown -fcf-protection=return /src/tools/clang/test/CodeGen/x86-cf-protection.c 2>&1 | /build/bin/FileCheck /src/tools/clang/test/CodeGen/x86
-cf-protection.c --check-prefix=RETURN            
not /build/bin/clang -cc1 -internal-isystem /build/lib/clang/7.0.0/include -nostdsysteminc -fsyntax-only -S -emit-llvm -triple i386-unknown-unknown -fcf-protection=branch /src/tools/clang/test/CodeGen/x86-cf-protection.c 2>&1 | /build/bin/FileCheck /src/tools/clang/test/CodeGen/x86
-cf-protection.c --check-prefix=BRANCH            
--                                                
Exit Code: 1                                      
                                                  
Command Output (stderr):                          
--                                                
/src/tools/clang/test/CodeGen/x86-cf-protection.c:4:12: error: expected string not found in input
// RETURN: error: option 'cf-protection=return' cannot be specified without '-mshstk'
           ^                                      
<stdin>:1:1: note: scanning from here             
error: unable to open output file '': 'Read-only file system'
^                                                 
                                                  
--

Adding -o %t solves the problem (r322082).

oren_ben_simhon marked an inline comment as done.Jan 9 2018, 7:38 AM
oren_ben_simhon added inline comments.
test/CodeGen/x86-cf-protection.c
1

I see. Thanks for the fix.