This is an archive of the discontinued LLVM Phabricator instance.

Enable IBT(Indirect Branch Tracking) in JIT with CET(Control-flow Enforcement Technology)
ClosedPublic

Authored by xiangzhangllvm on Mar 26 2020, 6:47 PM.

Details

Summary

This patch comes from H.J.'s https://github.com/hjl-tools/llvm-project/commit/2bd54ce7fa9e94fcd1118b948e14d1b6fc54dfd2

This patch fix the failed llvm unit tests which running on CET machine. (e.g. ExecutionEngine/MCJIT/MCJITTests)

The reason we enable IBT at "JIT compiled with CET" is mainly that: the JIT don't know the its caller program is CET enable or not.
If JIT's caller program is non-CET, it is no problem JIT generate CET code or not.
But if JIT's caller program is CET enabled, JIT must generate CET code or it will cause Control protection exceptions.

I have test the patch at llvm-unit-test and llvm-test-suite at CET machine. It passed.
and H.J. also test it at building and running VNCserver(Virtual Network Console), it works too.
(if not apply this patch, VNCserver will crash at CET machine.)

Diff Detail

Event Timeline

xiangzhangllvm created this revision.Mar 26 2020, 6:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2020, 6:47 PM
xiangzhangllvm retitled this revision from Enable IBT(Indirect Branch Tracking) in JIT with CET to Enable IBT(Indirect Branch Tracking) in JIT with CET(Control-flow Enforcement Technology).Mar 26 2020, 6:50 PM
LuoYuanke added inline comments.Mar 26 2020, 7:33 PM
llvm/lib/Target/X86/X86TargetMachine.cpp
217

Can you add an test for it? Where to pass the JIT information to compiler?

llvm/lib/Target/X86/X86TargetMachine.h
34

What's the default value for IsJIT?

xiangzhangllvm marked 2 inline comments as done.Mar 26 2020, 8:00 PM
xiangzhangllvm added inline comments.
llvm/lib/Target/X86/X86TargetMachine.cpp
217

There is already JIT tests in llvm unit test at /llvm/unittests/ExecutionEngine/, and some examples at llvm/example.

This patch base on the build of llvm and libllvm.
To test this patch, we need to build llvm self source code with CET enabled(-fcf-protection).
JIT test mainly test runtime status (running fail/correct), not like lit test.
So current is enough to test it.

llvm/lib/Target/X86/X86TargetMachine.h
34

In the LVM code the default is false (X86TargetMachine.h), but actually There is no meaning of "default value" for IsJIT.
The value for IsJIT is decided by the if JIT is built/called in the program.
for example, if you use clang/llc to build your code, the value is false, otherwise if you use lli or directly call JIT with JIT API (e.g. MCJIT, ORCJIT) in your program the value is true.

JITBuilder will create the target for it, it will pass true for IsJIT to create X86TargetMachine object.

xiangzhangllvm marked an inline comment as done.Mar 26 2020, 8:25 PM
xiangzhangllvm added inline comments.
llvm/lib/Target/X86/X86TargetMachine.cpp
217

Yes, Usually we pass options information to JIT within the code self which JIT will handle.
But for CET, it different. because:
At runtime JIT don't know its caller program is CET enable or not.
So we do this:
If we build the LLVM code with CET enabled (e.g. use gcc -fcf-protection), the CET macro will pass to LLVM compiler. If this Macro is defined, we default enable IBT for JIT in this patch.

efriedma added inline comments.
llvm/lib/Target/X86/X86IndirectBranchTracking.cpp
124–128

This looks wrong; you can't get the address of a function with internal linkage, whether or not the JIT is involved.

That said, it's possible there something else going wrong here; the JIT defaults to the large code model, which is rarely used otherwise.

xiangzhangllvm marked an inline comment as done.Mar 26 2020, 11:05 PM
xiangzhangllvm added inline comments.
llvm/lib/Target/X86/X86IndirectBranchTracking.cpp
124–128

The isInternalLinkage will be checked in line 129, for isInternalLinkage, !MF.getFunction().hasLocalLinkage() will get true, and then we mainly check the indirect branch tracking requirement at line 130.

efriedma added inline comments.Mar 27 2020, 3:29 PM
llvm/lib/Target/X86/X86IndirectBranchTracking.cpp
124–128

The logic without your patch is, if a function has its address taken, or the symbol is visible to other modules, emit ENDBR, because it could be the target of an indirect call. Otherwise, we don't emit an ENDBR: all possible callers of the function call it directly, and direct calls will be lowered to a "call" instruction, which doesn't need ENDBR.

Your patch overrides that logic and says, if we're compiling for a JIT target, emit ENDBR anyway. That doesn't make sense: the control flow rules are the same whether or not the compiled ELF file is written to disk.

xiangzhangllvm marked an inline comment as done.Mar 28 2020, 1:21 AM
xiangzhangllvm added inline comments.
llvm/lib/Target/X86/X86IndirectBranchTracking.cpp
124–128

Make sense! here should not check isJIT, Thank you very much!

hjl.tools added inline comments.Mar 28 2020, 3:38 AM
llvm/lib/Target/X86/X86IndirectBranchTracking.cpp
124–128

As

https://bugs.llvm.org/show_bug.cgi?id=45182

shows, MF.getFunction().hasAddressTaken() isn't sufficient for IBT. Assembly dump has
landing pad address taken, but MF.getFunction().hasAddressTaken() returns false. Without

if ((isJITwithCET ||
     MF.getFunction().hasAddressTaken() ||
     !MF.getFunction().hasLocalLinkage()) &&
    !MF.getFunction().doesNoCfCheck()) {
  auto MBB = MF.begin();
  Changed |= addENDBR(*MBB, MBB->begin());
}

VNC crashes. A jitted function has to be referenced. There are 3 ways to reference a function

  1. Direct call.
  2. Indirect call.
  3. Take address.

2 and 3 are mostly likely in x86 jitted codes.

efriedma added inline comments.Mar 28 2020, 3:18 PM
llvm/lib/Target/X86/X86IndirectBranchTracking.cpp
124–128

I'm not saying there isn't a bug somewhere. I'm just saying this isn't the right way to write whatever check is necessary.

This code is checking whether all calls must be direct at the IR level. If all calls are direct at the IR level, they must all be part of the module we're currently JIT'ing. The code that lowers calls does not check IsJIT. Therefore, the code here also shouldn't use IsJIT.

LIke I said earlier, I suspect the issue is related to the large code model.

hjl.tools added inline comments.Mar 28 2020, 3:39 PM
llvm/lib/Target/X86/X86IndirectBranchTracking.cpp
124–128

I believe the same problem also exists for ia32 which doesn't have large code model.

xiangzhangllvm marked an inline comment as done.Mar 28 2020, 9:36 PM
xiangzhangllvm added inline comments.
llvm/lib/Target/X86/X86IndirectBranchTracking.cpp
149–152

landing pad address taken have been add to here now. But still need to check some other un-normal exception model, e.g SjLj exception model.

Please take a look at

https://gitlab.freedesktop.org/mesa/mesa

to see how LLVM JIT is used and extract a testcase from it.

Please take a look at

https://gitlab.freedesktop.org/mesa/mesa

to see how LLVM JIT is used and extract a testcase from it.

1, Does it in your commits? I find 2 your commits without the JIT tests.

2, Do you mean here we need add JIT test for this patch ? Check endbr in JIT generated code?

Please take a look at

https://gitlab.freedesktop.org/mesa/mesa

to see how LLVM JIT is used and extract a testcase from it.

1, Does it in your commits? I find 2 your commits without the JIT tests.

This:

https://bugs.llvm.org/show_bug.cgi?id=45013

is all I have.

2, Do you mean here we need add JIT test for this patch ? Check endbr in JIT generated code?

My changes fixed the VNC crash. But there is no testcase to show why my patches are needed.

xiangzhangllvm added a comment.EditedMar 29 2020, 7:40 PM

2, Do you mean here we need add JIT test for this patch ? Check endbr in JIT generated code?

My changes fixed the VNC crash. But there is no testcase to show why my patches are needed.

Your tests failed on CET machine
and the existing llvm unit tests also failed the on the CET machine
They are the same reason for lack of endbr* in JIT.
So I think the existing llvm unit tests is enough for this patch.

xiangzhangllvm edited the summary of this revision. (Show Details)Mar 29 2020, 7:52 PM

2, Do you mean here we need add JIT test for this patch ? Check endbr in JIT generated code?

My changes fixed the VNC crash. But there is no testcase to show why my patches are needed.

Your tests failed on CET machine
and the existing llvm unit tests also failed the on the CET machine
They are the same reason for lack of endbr* in JIT.
So I think the existing llvm unit tests is enough for this patch.

My patch has 2 parts:

  1. Check CET in JIT instead of checking -fcf-protection-branch.
  2. Always insert ENDBR in JIT with CET.

I think the first one alone will fix LLVM JIT tests on CET machine. But
it won't fix VNC. The second one is needed for VNC. I don't have
a testcase for the second patch.

If "1 Check CET in JIT instead of checking -fcf-protection-branch" can't let VNC passed. The crash must happen at "Take address of a internal function."

This is not a IBT problem only for JIT, but also the static llvm compiler, e.g. llc.
And it may unable to track these calling through function address, because these function address may be calculated out in runtime or written in a big table.

and it really seems some disaccord/unbeautiful to add "isJITwithCET" for the old patch.

I think we should change the condition from

if ((**isJITwithCET ||**
     MF.getFunction().hasAddressTaken() ||
     !MF.getFunction().hasLocalLinkage()) &&
    !MF.getFunction().doesNoCfCheck()) {
  auto MBB = MF.begin();
  Changed |= addENDBR(*MBB, MBB->begin());
}

to

if (!MF.getFunction().doesNoCfCheck()) {
  auto MBB = MF.begin();
  Changed |= addENDBR(*MBB, MBB->begin());
}

If "1 Check CET in JIT instead of checking -fcf-protection-branch" can't let VNC passed. The crash must happen at "Take address of a internal function."

This is not a IBT problem only for JIT, but also the static llvm compiler, e.g. llc.
And it may unable to track these calling through function address, because these function address may be calculated out in runtime or written in a big table.

It will be very nice to extract a testcase from Mesa. This bug may be harmless without CET,
but it is fatal with CET.

It will be very nice to extract a testcase from Mesa. This bug may be harmless without CET,
but it is fatal with CET.

OK, I'll try to extract a testcase from Mesa.

Let me first update the patch self.

Hi, efriedma, It passed. Do you want to add this file as test? The test indirect-branch-tracking.ll have already test internal function.

Yes, please add that as a test.

The current patch works, but it's emitting endbrs for small code model examples where they shouldn't be necessary. Maybe someone else can chime in on how important that is in practice.

Yes, please add that as a test.

The current patch works, but it's emitting endbrs for small code model examples where they shouldn't be necessary. Maybe someone else can chime in on how important that is in practice.

OK, It is really hard to check if a internal function will be indirectly accessed or not, here we can first "sync" with GCC.

@efriedma Put the https://bugs.llvm.org/show_bug.cgi?id=45364 test into X86/indirect-branch-tracking.ll @test5

I find your patch will not deal with internal function.
I tested GCC, It really will generate endbr for static functions:

gcc version 9.3.1 20200317 (Red Hat 9.3.1-1) (GCC)
 [xiangzh1@gnu-tgl-1 ~/tmp]$cat t.c
extern int foo2(int a);

extern int foo2(int a)
{
  return a*6;
}

static int foo3(int a)
{
  return a*7;
}

int foo(int a){
//  int (*pf3)(int) = foo;
//  int b = foo2(a) +foo3(a) + pf3(a);
  int b = foo2(a) +foo3(a);
  return b;
}
 [xiangzh1@gnu-tgl-1 ~/tmp]$
[xiangzh1@gnu-tgl-1 ~/tmp]$gcc -fcf-protection t.c -S -o t.s
[xiangzh1@gnu-tgl-1 ~/tmp]$cat t.s
        .file   "t.c"
        .text
        .globl  foo2
        .type   foo2, @function
foo2:
.LFB0:
        .cfi_startproc
        endbr64
        pushq   %rbp
        ....

        .size   foo2, .-foo2
        .type   foo3, @function
foo3:
.LFB1:
        .cfi_startproc
        endbr64
        pushq   %rbp
        .cfi_def_cfa_offset 16
      ....

I find your patch will not deal with internal function.
I tested GCC, It really will generate endbr for static functions:

gcc version 9.3.1 20200317 (Red Hat 9.3.1-1) (GCC)
 [xiangzh1@gnu-tgl-1 ~/tmp]$cat t.c
extern int foo2(int a);

extern int foo2(int a)
{
  return a*6;
}

static int foo3(int a)
{
  return a*7;
}

int foo(int a){
//  int (*pf3)(int) = foo;
//  int b = foo2(a) +foo3(a) + pf3(a);
  int b = foo2(a) +foo3(a);
  return b;
}
 [xiangzh1@gnu-tgl-1 ~/tmp]$
[xiangzh1@gnu-tgl-1 ~/tmp]$gcc -fcf-protection t.c -S -o t.s

You need -O2 to turn on optimization:

[hjl@gnu-cfl-2 tmp]$ cat y.c
__attribute__ ((noclone, noinline))
static int foo3(int a)
{
  return a*7;
}

extern int b;

void
foo(int a)
{
  b = foo3(a);
}
[hjl@gnu-cfl-2 tmp]$ gcc  -fcf-protection -O2 -S y.c
[hjl@gnu-cfl-2 tmp]$ cat y.s
	.file	"y.c"
	.text
	.p2align 4
	.type	foo3, @function
foo3:
.LFB0:
	.cfi_startproc
	leal	0(,%rdi,8), %eax
	subl	%edi, %eax
	ret
	.cfi_endproc
.LFE0:
	.size	foo3, .-foo3
	.p2align 4
	.globl	foo
	.type	foo, @function
foo:
.LFB1:
	.cfi_startproc
	endbr64
	call	foo3
	movl	%eax, b(%rip)
	ret
	.cfi_endproc
.LFE1:
	.size	foo, .-foo
	.ident	"GCC: (GNU) 9.3.1 20200317 (Red Hat 9.3.1-1)"
	.section	.note.GNU-stack,"",@progbits
	.section	.note.gnu.property,"a"
	.align 8
	.long	 1f - 0f
	.long	 4f - 1f
	.long	 5
0:
	.string	 "GNU"
1:
	.align 8
	.long	 0xc0000002
	.long	 3f - 2f
2:
	.long	 0x3
3:
	.align 8
4:
[hjl@gnu-cfl-2 tmp]$
  1. Sync with HJ's large code changes in https://github.com/hjl-tools/llvm-project/commit/6a85ba472143c91e5686e1f0faf7fea7b4f0e350
  2. Add Eli's test in https://bugs.llvm.org/show_bug.cgi?id=45364 to llc test and JIT test.

For Large code model:
The compiler is required to use the movabs instruction, as in the medium code model, even for dealing with addresses inside the text section.
Additionally, indirect branches are needed when branching to addresses whose offset from the current instruction pointer is unknown.

  1. Sync with HJ's large code changes in https://github.com/hjl-tools/llvm-project/commit/6a85ba472143c91e5686e1f0faf7fea7b4f0e350
  2. Add Eli's test in https://bugs.llvm.org/show_bug.cgi?id=45364 to llc test and JIT test.

For Large code model:
The compiler is required to use the movabs instruction, as in the medium code model, even for dealing with addresses inside the text section.
Additionally, indirect branches are needed when branching to addresses whose offset from the current instruction pointer is unknown.

It looks very confusing. Can you create a diff against master branch, not your last change?

It looks very confusing. Can you create a diff against master branch, not your last change?

Never mind. I couldn't read :-).

llvm/test/ExecutionEngine/MCJIT/cet-code-model-lager.ll
4

What does this test check? Does it fail without the fix?

xiangzhangllvm marked an inline comment as done.Apr 2 2020, 5:30 PM

It looks very confusing. Can you create a diff against master branch, not your last change?

Never mind. I couldn't read :-).

This is the whole diff against the master. :-)

llvm/test/ExecutionEngine/MCJIT/cet-code-model-lager.ll
4

lli will call JIT to compile this code. This test is same with Eli's large code model test.
This test will run fail if CET enabled JIT not generate endbr for the following 3 functions' entry in CET machine.

Look good to me.

LuoYuanke added inline comments.Apr 2 2020, 8:05 PM
llvm/test/ExecutionEngine/MCJIT/cet-code-model-lager.ll
2

It seems the same test case to the previous one. Can we merge the 2 test case into one and use different run command?

; RUN: llc -mtriple=x86_64-unknown-unknown -code-model=large < %s | FileCheck %s
; RUN: %lli -code-model=large %s > /dev/null

4

lli can check CET even on machine without CET enabled?

xiangzhangllvm marked an inline comment as done.Apr 2 2020, 8:11 PM
xiangzhangllvm added inline comments.
llvm/test/ExecutionEngine/MCJIT/cet-code-model-lager.ll
4

lli can only check CET in CET enabled machine.

The llvm/test/CodeGen/X86/indirect-branch-tracking-cm-lager.ll test will staticly check the endbr in CET/nonCET machines.

This revision is now accepted and ready to land.Apr 2 2020, 8:40 PM
This revision was automatically updated to reflect the committed changes.