This is an archive of the discontinued LLVM Phabricator instance.

[X86] CET endbr enhance
AbandonedPublic

Authored by xiangzhangllvm on Sep 23 2020, 6:55 PM.

Details

Summary

This patch is for CET (Control-flow Enforcement Technology) enhancement.

ENDBR32 and ENDBR64 have specific opcodes:
ENDBR32: F3 0F 1E FB
ENDBR64: F3 0F 1E FA
And we want that attackers won’t find unintended ENDBR32/64 opcode matches in the binary.

Here’s an example:
If the compiler had to generate asm for the following code:
a = 0xF30F1EFA
it could, for example, generate:
mov 0xF30F1EFA, dword ptr[a]

In such a case, the binary would include a gadget that starts with a fake ENDBR64 opcode.
Therefore, we split such generation into multiple operations, let it not shows in the binary.

The goal of this patch is not to 100% remove the unintended ENDBR-IMM.
Theoretically, it can occurrence in address info, and even between 2 instructions.
In fact, All the probability of its occurrence is very small.
The idea of this patch tend to “Greatly reduce the probability of ENDBR-IMM occurrence” by handling the most comment instructions with imm32/64.

Diff Detail

Event Timeline

xiangzhangllvm created this revision.Sep 23 2020, 6:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2020, 6:55 PM
xiangzhangllvm requested review of this revision.Sep 23 2020, 6:55 PM

Should we only be doing this when CET protections are enabled?

craig.topper added inline comments.Sep 23 2020, 9:17 PM
llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
1341

This can't be an ArrayRef. The RHS is a temporary initializer_list that's lifetime ends at the end of the statement.

Use uint8_t Prefixes[] = { ... }

MaskRay added inline comments.Sep 23 2020, 11:50 PM
llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
1335

Newer function should stick with the coding standard and use lowerCase function names.

xiangzhangllvm marked 2 inline comments as done.

Done, TKS !!

craig.topper added inline comments.Sep 27 2020, 3:43 PM
llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
1349

You don't need an ArrayRef. you can just use std::begin() and std::end() on Bytes

xiangzhangllvm marked an inline comment as done.
xiangzhangllvm added inline comments.
llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
1349

Done, thank you !!

craig.topper added inline comments.Sep 27 2020, 7:45 PM
llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
1349

We shouldn't hard code the size. Its not friendly to change multiple places if the array size changes which is why I suggested std::begin/std::end

But looking again I forgot we have llvm::is_contained in STLExtras.h which would be even better here.

xiangzhangllvm marked an inline comment as done.
xiangzhangllvm marked an inline comment as done.
xiangzhangllvm added inline comments.
llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
1349

Yes, It wraps std::find, that's better, thank you !!

xiangzhangllvm marked an inline comment as done.Sep 30 2020, 5:05 PM

Hello Craig, could you help accept, TKS!

@spatel or @RKSimon i'd appreciate if you could look at this as well.

@xiangzhangllvm do we care about -O0 builds for this? Those won't always go through SelectionDAG. Only when the "fast" instruction selector find something it can't handle.

I tend not do it for -O0, most projects/libs not use -O0 to build.
This is not a bug, we just refine the CET.

spatel added a comment.Oct 1 2020, 7:30 AM

Warning: I had no idea what "CET" means before seeing this patch, so feel free to discount my feedback.
The patch title and description are not clear to me.
Is the goal of this patch to change *the compiler* binary itself? (rather than an application binary)
It would be good to pre-commit a minimal test at least so we can see exactly what asm *change* is introduced by this patch.

llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
206

Formatting: function name should be 'lowerCamelCase'

1341

Bytes -> OptionalPrefixBytes ?

1371

The function name does not describe what it actually does.
Would "obscureEndbrOpcodeImmediate" be more accurate?

1383–1384

This code structure with fallthrough is difficult to follow. Create a separate static helper function that just returns the constant operand index for a given opcode?

xiangzhangllvm edited the summary of this revision. (Show Details)
xiangzhangllvm marked an inline comment as done.
xiangzhangllvm marked 2 inline comments as done.Oct 8 2020, 2:02 AM

@spatel thank you very much for your review! and very sorry for the later update!
CET stands for Intel Control-flow Enforcement Technology, which was designed by H.J. and has already implemented in llvm and gcc. Now it can be googled out.
CET adds an Indirect Branch Tracking capability (by inserting Endbr instruction at the destination of the indirect branch) to provide software the ability to restrict COP/JOP attacks.

llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
1383–1384

I really thought using a function and refined here before commit it to phabricator, try to simplify the code I used the LLVM_FALLTHROUGH.
Here not just need 'return' constant operand index, please refer line 1412-1414.

spatel added a comment.Oct 9 2020, 8:07 AM

CET adds an Indirect Branch Tracking capability (by inserting Endbr instruction at the destination of the indirect branch) to provide software the ability to restrict COP/JOP attacks.

Ok, I understand that much. But it is still not clear to me why this patch is re-using the "-x86-indirect-branch-tracking" option. Is this obscuring of the opcode not independent of producing the instruction? I would again suggest that we pre-commit some tests, so we can see only the diffs in the codegen (and if inserting "endb64" itself is a requirement to obscure the opcode, please add comments in the test file to explain that).

CET adds an Indirect Branch Tracking capability (by inserting Endbr instruction at the destination of the indirect branch) to provide software the ability to restrict COP/JOP attacks.

Ok, I understand that much. But it is still not clear to me why this patch is re-using the "-x86-indirect-branch-tracking" option. Is this obscuring of the opcode not independent of producing the instruction? I would again suggest that we pre-commit some tests, so we can see only the diffs in the codegen (and if inserting "endb64" itself is a requirement to obscure the opcode, please add comments in the test file to explain that).

I suggested adding that the -x86-indirect-branch-tracking check because it seemed like we'd only want to do this in a binary that was expected to contain endbr opcodes. Putting things that could be confused for endbr opcodes in a binary that doesn't enable CET should be harmless.

CET adds an Indirect Branch Tracking capability (by inserting Endbr instruction at the destination of the indirect branch) to provide software the ability to restrict COP/JOP attacks.

Ok, I understand that much. But it is still not clear to me why this patch is re-using the "-x86-indirect-branch-tracking" option. Is this obscuring of the opcode not independent of producing the instruction? I would again suggest that we pre-commit some tests, so we can see only the diffs in the codegen (and if inserting "endb64" itself is a requirement to obscure the opcode, please add comments in the test file to explain that).

I suggested adding that the -x86-indirect-branch-tracking check because it seemed like we'd only want to do this in a binary that was expected to contain endbr opcodes. Putting things that could be confused for endbr opcodes in a binary that doesn't enable CET should be harmless.

Is this patch designed to change the compiler itself? Ie, we would enable this obfuscation mechanism while building clang, then use the newly built clang with the existing cl::opt flag to build an app that contains endbr instructions that are constructed in the convoluted way? We don't care if the compiler is littered with endbr itself?

CET adds an Indirect Branch Tracking capability (by inserting Endbr instruction at the destination of the indirect branch) to provide software the ability to restrict COP/JOP attacks.

Ok, I understand that much. But it is still not clear to me why this patch is re-using the "-x86-indirect-branch-tracking" option. Is this obscuring of the opcode not independent of producing the instruction? I would again suggest that we pre-commit some tests, so we can see only the diffs in the codegen (and if inserting "endb64" itself is a requirement to obscure the opcode, please add comments in the test file to explain that).

I suggested adding that the -x86-indirect-branch-tracking check because it seemed like we'd only want to do this in a binary that was expected to contain endbr opcodes. Putting things that could be confused for endbr opcodes in a binary that doesn't enable CET should be harmless.

Is this patch designed to change the compiler itself? Ie, we would enable this obfuscation mechanism while building clang, then use the newly built clang with the existing cl::opt flag to build an app that contains endbr instructions that are constructed in the convoluted way? We don't care if the compiler is littered with endbr itself?

No its not designed to change the compiler itself. It is just supposed to make it less likely that a binary containing real endbr opcodes will have bytes that look like endbr opcodes.

CET adds an Indirect Branch Tracking capability (by inserting Endbr instruction at the destination of the indirect branch) to provide software the ability to restrict COP/JOP attacks.

Ok, I understand that much. But it is still not clear to me why this patch is re-using the "-x86-indirect-branch-tracking" option. Is this obscuring of the opcode not independent of producing the instruction? I would again suggest that we pre-commit some tests, so we can see only the diffs in the codegen (and if inserting "endb64" itself is a requirement to obscure the opcode, please add comments in the test file to explain that).

I suggested adding that the -x86-indirect-branch-tracking check because it seemed like we'd only want to do this in a binary that was expected to contain endbr opcodes. Putting things that could be confused for endbr opcodes in a binary that doesn't enable CET should be harmless.

Is this patch designed to change the compiler itself? Ie, we would enable this obfuscation mechanism while building clang, then use the newly built clang with the existing cl::opt flag to build an app that contains endbr instructions that are constructed in the convoluted way? We don't care if the compiler is littered with endbr itself?

No its not designed to change the compiler itself. It is just supposed to make it less likely that a binary containing real endbr opcodes will have bytes that look like endbr opcodes.

Ok, I don't have any other questions/comments (other than it would still be helpful to reduce/pre-commit the tests).

Hello @spatel, Sorrry, I don't much understand the "reduce/pre-commit tests",
did you mean the test here "cet_endbr_imm_enhance.ll" should be merge into other existing test ?

Ok, I don't have any other questions/comments (other than it would still be helpful to reduce/pre-commit the tests).

Done, then update the tests.

craig.topper added inline comments.Oct 9 2020, 8:54 PM
llvm/test/CodeGen/X86/cet_endbr_imm_enhance.ll
2

Don't we need to test the endbr32 case?

spatel added a comment.EditedOct 10 2020, 6:16 AM

Ok, I don't have any other questions/comments (other than it would still be helpful to reduce/pre-commit the tests).

Done, then update the tests.

Thanks! That makes it easier to see how things are changing with this patch.
In addition to @craig.topper 's question about endbr32, shouldn't we have a test that checks for multiple and different prefix bytes? If I am seeing correctly, there is only 1 test checking for a single 0x2E optional prefix.

Ok, I don't have any other questions/comments (other than it would still be helpful to reduce/pre-commit the tests).

Done, then update the tests.

Thanks! That makes it easier to see how things are changing with this patch.
In addition to @craig.topper 's question about endbr32, shouldn't we have a test that checks for multiple and different prefix bytes? If I am seeing correctly, there is only 1 test checking for a single 0x2E optional prefix.

Thinking about this a bit more: if the attacker can recognize multiple different optional prefix bytes, then is there much value in complementing those bytes? They are already searching for a match from some dictionary of byte strings, so inverting the bits just means they need to increase their search by 2x?

MaskRay added inline comments.Oct 10 2020, 9:38 AM
llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
20

Which function uses llvm/CodeGen/MachineModuleInfo.h ?

1338

Please don't mix lower-case and upper-case hexadecimal literals.

1341

constexpr uint8_t OptionalPrefixBytes[]

Are these prefix bytes tested?

1344

You can use Imm >>= i; while (Imm != 0) { ... Imm >>= 8; }

1372

Add const if appropriate.
ditto below

1380

switch (Opc)

Please clang-format the patch (git diff -U0 --no-color 'HEAD^' | llvm-project/clang/tools/clang-format/clang-format-diff.py -i -p1)

1467

There is a space before //

Ok, I don't have any other questions/comments (other than it would still be helpful to reduce/pre-commit the tests).

Done, then update the tests.

Thanks! That makes it easier to see how things are changing with this patch.
In addition to @craig.topper 's question about endbr32, shouldn't we have a test that checks for multiple and different prefix bytes? If I am seeing correctly, there is only 1 test checking for a single 0x2E optional prefix.

Thinking about this a bit more: if the attacker can recognize multiple different optional prefix bytes, then is there much value in complementing those bytes? They are already searching for a match from some dictionary of byte strings, so inverting the bits just means they need to increase their search by 2x?

I'm not sure I follow. If the bytes are inverted in the binary then they are no longer useful to the attacker. The goal here is to make sure that if an attacker gains control of the register or memory location used by an indirect call or jump that they can't jump to an immediate in the binary that matches the encoding for ENDBR. As that would convince the CET protection that this a place that an indirect jump was expected to be able to jump to.

Thanks for your reviews! Please let me suspend this patch first.

duo to Craig's new patch https://reviews.llvm.org/D89178.
(Reason: Every instruction contain I32/64 targetconstant operand has a corresponding reg version.
So, we can replace the I32/64 targetconstant without checking the operation before ISel.)