diff --git a/bolt/include/bolt/Core/MCPlusBuilder.h b/bolt/include/bolt/Core/MCPlusBuilder.h index f902a8c43cd1d..d79174f13f502 100644 --- a/bolt/include/bolt/Core/MCPlusBuilder.h +++ b/bolt/include/bolt/Core/MCPlusBuilder.h @@ -718,6 +718,20 @@ class MCPlusBuilder { return false; } + /// Returns true if Inst is a trap instruction. + /// + /// Tests if Inst is an instruction that immediately causes an abnormal + /// program termination, for example when a security violation is detected + /// by a compiler-inserted check. + /// + /// @note An implementation of this method should likely return false for + /// calls to library functions like abort(), as it is possible that the + /// execution state is partially attacker-controlled at this point. + virtual bool isTrap(const MCInst &Inst) const { + llvm_unreachable("not implemented"); + return false; + } + virtual bool isBreakpoint(const MCInst &Inst) const { llvm_unreachable("not implemented"); return false; diff --git a/bolt/lib/Passes/PAuthGadgetScanner.cpp b/bolt/lib/Passes/PAuthGadgetScanner.cpp index f928dd49edb25..65c84ebc8c4f4 100644 --- a/bolt/lib/Passes/PAuthGadgetScanner.cpp +++ b/bolt/lib/Passes/PAuthGadgetScanner.cpp @@ -1078,6 +1078,15 @@ class DstSafetyAnalysis { dbgs() << ")\n"; }); + // If this instruction terminates the program immediately, no + // authentication oracles are possible past this point. + if (BC.MIB->isTrap(Point)) { + LLVM_DEBUG({ traceInst(BC, "Trap instruction found", Point); }); + DstState Next(NumRegs, RegsToTrackInstsFor.getNumTrackedRegisters()); + Next.CannotEscapeUnchecked.set(); + return Next; + } + // If this instruction is reachable by the analysis, a non-empty state will // be propagated to it sooner or later. Until then, skip computeNext(). if (Cur.empty()) { @@ -1185,8 +1194,8 @@ class DataflowDstSafetyAnalysis // // A basic block without any successors, on the other hand, can be // pessimistically initialized to everything-is-unsafe: this will naturally - // handle both return and tail call instructions and is harmless for - // internal indirect branch instructions (such as computed gotos). + // handle return, trap and tail call instructions. At the same time, it is + // harmless for internal indirect branch instructions, like computed gotos. if (BB.succ_empty()) return createUnsafeState(); diff --git a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp index 973261765f951..72f95cea6fa1d 100644 --- a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp +++ b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp @@ -382,10 +382,9 @@ class AArch64MCPlusBuilder : public MCPlusBuilder { // the list of successors of this basic block as appropriate. // Any of the above code sequences assume the fall-through basic block - // is a dead-end BRK instruction (any immediate operand is accepted). + // is a dead-end trap instruction. const BinaryBasicBlock *BreakBB = BB.getFallthrough(); - if (!BreakBB || BreakBB->empty() || - BreakBB->front().getOpcode() != AArch64::BRK) + if (!BreakBB || BreakBB->empty() || !isTrap(BreakBB->front())) return std::nullopt; // Iterate over the instructions of BB in reverse order, matching opcodes @@ -1744,6 +1743,34 @@ class AArch64MCPlusBuilder : public MCPlusBuilder { Inst.addOperand(MCOperand::createImm(0)); } + bool isTrap(const MCInst &Inst) const override { + if (Inst.getOpcode() != AArch64::BRK) + return false; + // Only match the immediate values that are likely to indicate this BRK + // instruction is emitted to terminate the program immediately and not to + // be handled by a SIGTRAP handler, for example. + switch (Inst.getOperand(0).getImm()) { + case 0xc470: + case 0xc471: + case 0xc472: + case 0xc473: + // Explicit Pointer Authentication check failed, see + // AArch64AsmPrinter::emitPtrauthCheckAuthenticatedValue(). + return true; + case 0x1: + // __builtin_trap(), as emitted by Clang. + return true; + case 0x3e8: // decimal 1000 + // __builtin_trap(), as emitted by GCC. + return true; + default: + // Some constants may indicate intentionally recoverable break-points. + // This is the case at least for 0xf000, which is used by + // __builtin_debugtrap() supported by Clang. + return false; + } + } + bool isStorePair(const MCInst &Inst) const { const unsigned opcode = Inst.getOpcode(); diff --git a/bolt/test/binary-analysis/AArch64/gs-pauth-address-checks.s b/bolt/test/binary-analysis/AArch64/gs-pauth-address-checks.s index 3f982ddaf6e38..74f276197923f 100644 --- a/bolt/test/binary-analysis/AArch64/gs-pauth-address-checks.s +++ b/bolt/test/binary-analysis/AArch64/gs-pauth-address-checks.s @@ -31,7 +31,7 @@ resign_xpaci_good: xpaci x16 cmp x0, x16 b.eq 1f - brk 0x1234 + brk 0xc471 1: pacia x0, x2 ret @@ -46,7 +46,7 @@ resign_xpacd_good: xpacd x16 cmp x0, x16 b.eq 1f - brk 0x1234 + brk 0xc473 1: pacda x0, x2 ret @@ -117,7 +117,7 @@ resign_xpaci_unrelated_auth_and_check: xpaci x16 cmp x0, x16 b.eq 1f - brk 0x1234 + brk 0xc471 1: pacia x10, x2 ret @@ -139,7 +139,7 @@ resign_xpaci_wrong_pattern_1: xpaci x16 cmp x0, x16 b.eq 1f - brk 0x1234 + brk 0xc471 1: pacia x0, x2 ret @@ -157,7 +157,7 @@ resign_xpaci_wrong_pattern_2: xpaci x0 // x0 instead of x16 cmp x0, x16 b.eq 1f - brk 0x1234 + brk 0xc471 1: pacia x0, x2 ret @@ -174,7 +174,7 @@ resign_xpaci_wrong_pattern_3: xpaci x16 cmp x16, x16 // x16 instead of x0 b.eq 1f - brk 0x1234 + brk 0xc471 1: pacia x0, x2 ret @@ -191,7 +191,7 @@ resign_xpaci_wrong_pattern_4: xpaci x16 cmp x0, x0 // x0 instead of x16 b.eq 1f - brk 0x1234 + brk 0xc471 1: pacia x0, x2 ret @@ -208,7 +208,7 @@ resign_xpaci_wrong_pattern_5: mov x16, x16 // replace xpaci with a no-op instruction cmp x0, x16 b.eq 1f - brk 0x1234 + brk 0xc471 1: pacia x0, x2 ret @@ -228,7 +228,7 @@ resign_xpaclri_good: xpaclri cmp x30, x16 b.eq 1f - brk 0x1234 + brk 0xc471 1: pacia x30, x2 @@ -246,7 +246,7 @@ xpaclri_check_keeps_lr_safe: xpaclri // clobbers LR cmp x30, x16 b.eq 1f - brk 0x1234 // marks LR as trusted and safe-to-dereference + brk 0xc471 // marks LR as trusted and safe-to-dereference 1: ret // not reporting non-protected return .size xpaclri_check_keeps_lr_safe, .-xpaclri_check_keeps_lr_safe @@ -265,7 +265,7 @@ xpaclri_check_requires_safe_lr: xpaclri cmp x30, x16 b.eq 1f - brk 0x1234 + brk 0xc471 1: ret .size xpaclri_check_requires_safe_lr, .-xpaclri_check_requires_safe_lr @@ -283,7 +283,7 @@ resign_xpaclri_wrong_reg: xpaclri // ... but xpaclri still operates on x30 cmp x20, x16 b.eq 1f - brk 0x1234 + brk 0xc471 1: pacia x20, x2 @@ -303,7 +303,7 @@ resign_checked_not_authenticated: xpaci x16 cmp x0, x16 b.eq 1f - brk 0x1234 + brk 0xc471 1: pacia x0, x2 ret @@ -323,7 +323,7 @@ resign_checked_before_authenticated: xpaci x16 cmp x0, x16 b.eq 1f - brk 0x1234 + brk 0xc471 1: autib x0, x1 pacia x0, x2 @@ -339,7 +339,7 @@ resign_high_bits_tbz_good: autib x0, x1 eor x16, x0, x0, lsl #1 tbz x16, #62, 1f - brk 0x1234 + brk 0xc471 1: pacia x0, x2 ret @@ -378,7 +378,7 @@ resign_high_bits_tbz_wrong_bit: autib x0, x1 eor x16, x0, x0, lsl #1 tbz x16, #63, 1f - brk 0x1234 + brk 0xc471 1: pacia x0, x2 ret @@ -393,7 +393,7 @@ resign_high_bits_tbz_wrong_shift_amount: autib x0, x1 eor x16, x0, x0, lsl #2 tbz x16, #62, 1f - brk 0x1234 + brk 0xc471 1: pacia x0, x2 ret @@ -408,7 +408,7 @@ resign_high_bits_tbz_wrong_shift_type: autib x0, x1 eor x16, x0, x0, lsr #1 tbz x16, #62, 1f - brk 0x1234 + brk 0xc471 1: pacia x0, x2 ret @@ -423,7 +423,7 @@ resign_high_bits_tbz_wrong_pattern_1: autib x0, x1 eor x16, x0, x0, lsl #1 tbz x17, #62, 1f - brk 0x1234 + brk 0xc471 1: pacia x0, x2 ret @@ -438,7 +438,7 @@ resign_high_bits_tbz_wrong_pattern_2: autib x0, x1 eor x16, x10, x0, lsl #1 tbz x16, #62, 1f - brk 0x1234 + brk 0xc471 1: pacia x0, x2 ret @@ -453,7 +453,7 @@ resign_high_bits_tbz_wrong_pattern_3: autib x0, x1 eor x16, x0, x10, lsl #1 tbz x16, #62, 1f - brk 0x1234 + brk 0xc471 1: pacia x0, x2 ret @@ -648,7 +648,7 @@ many_checked_regs: xpacd x16 // ... cmp x2, x16 // ... b.eq 2f // end of basic block - brk 0x1234 + brk 0xc473 2: pacdza x0 pacdza x1 diff --git a/bolt/test/binary-analysis/AArch64/gs-pauth-authentication-oracles.s b/bolt/test/binary-analysis/AArch64/gs-pauth-authentication-oracles.s index c314bc7cfe5a3..f44ba21b9d484 100644 --- a/bolt/test/binary-analysis/AArch64/gs-pauth-authentication-oracles.s +++ b/bolt/test/binary-analysis/AArch64/gs-pauth-authentication-oracles.s @@ -79,7 +79,7 @@ good_explicit_check: autia x0, x1 eor x16, x0, x0, lsl #1 tbz x16, #62, 1f - brk 0x1234 + brk 0xc470 1: ret .size good_explicit_check, .-good_explicit_check @@ -373,7 +373,7 @@ good_explicit_check_multi_bb: 1: eor x16, x0, x0, lsl #1 tbz x16, #62, 2f - brk 0x1234 + brk 0xc470 2: cbz x1, 3f nop @@ -685,8 +685,7 @@ good_address_arith_nocfg: .globl good_explicit_check_unrelated_reg .type good_explicit_check_unrelated_reg,@function good_explicit_check_unrelated_reg: -// CHECK-LABEL: GS-PAUTH: authentication oracle found in function good_explicit_check_unrelated_reg, basic block {{[^,]+}}, at address - // FIXME: The below instruction is not an authentication oracle +// CHECK-NOT: good_explicit_check_unrelated_reg autia x2, x3 // One of possible execution paths after this instruction // ends at BRK below, thus BRK used as a trap instruction // should formally "check everything" not to introduce @@ -694,7 +693,7 @@ good_explicit_check_unrelated_reg: autia x0, x1 eor x16, x0, x0, lsl #1 tbz x16, #62, 1f - brk 0x1234 + brk 0xc470 1: ldr x4, [x2] // Right before this instruction X2 is checked - this // should be propagated to the basic block ending with diff --git a/bolt/test/binary-analysis/AArch64/gs-pauth-signing-oracles.s b/bolt/test/binary-analysis/AArch64/gs-pauth-signing-oracles.s index 3a4d383ec5bc6..4d4bb7b0fb251 100644 --- a/bolt/test/binary-analysis/AArch64/gs-pauth-signing-oracles.s +++ b/bolt/test/binary-analysis/AArch64/gs-pauth-signing-oracles.s @@ -57,7 +57,7 @@ good_sign_auted_checked_brk: autda x0, x2 eor x16, x0, x0, lsl #1 tbz x16, #62, 1f - brk 0x1234 + brk 0xc472 1: pacda x0, x1 ret @@ -351,7 +351,7 @@ good_sign_auted_checked_brk_multi_bb: 1: eor x16, x0, x0, lsl #1 tbz x16, #62, 2f - brk 0x1234 + brk 0xc472 2: cbz x4, 3f nop @@ -705,7 +705,7 @@ good_resign_with_increment_brk: add x0, x0, #8 eor x16, x0, x0, lsl #1 tbz x16, #62, 1f - brk 0x1234 + brk 0xc472 1: mov x2, x0 pacda x2, x1 diff --git a/bolt/test/binary-analysis/AArch64/trap-instructions.s b/bolt/test/binary-analysis/AArch64/trap-instructions.s new file mode 100644 index 0000000000000..7810b2d3c3626 --- /dev/null +++ b/bolt/test/binary-analysis/AArch64/trap-instructions.s @@ -0,0 +1,213 @@ +// RUN: %clang %cflags -march=armv8.3-a %s -o %t.exe -Wl,--emit-relocs +// RUN: llvm-bolt-binary-analysis --scanners=pauth %t.exe 2>&1 | FileCheck %s + +// Test what instructions can be used to terminate the program abnormally +// on security violation. +// +// All test cases have the same structure: +// +// cbz x0, 1f // [a], ensures [c] is never reported as unreachable +// autia x2, x3 +// cbz x1, 2f // [b] +// [instruction under test] +// 1: +// ret // [c] +// 2: +// ldr x0, [x2] +// ret +// +// This is to handle three possible cases: the instruction under test may be +// considered by BOLT as +// * trapping (and thus no-return): after being authenticated, x2 is ether +// checked by LDR (if [b] is taken) or the program is terminated +// immediately without leaking x2 (if [b] falls through to the trapping +// instruction under test). Nothing is reported. +// * non-trapping, but no-return (such as calling abort()): x2 is leaked if [b] +// falls through. Authentication oracle is reported. +// * non-trapping and falling-through (i.e. a regular instruction): +// x2 is leaked by [c]. Authentication oracle is reported. + + .text + + .globl brk_key_ia + .type brk_key_ia,@function +brk_key_ia: +// CHECK-NOT: brk_key_ia + cbz x0, 1f + autia x2, x3 + cbz x1, 2f + brk 0xc470 +1: + ret +2: + ldr x0, [x2] + ret + .size brk_key_ia, .-brk_key_ia + + .globl brk_key_ib + .type brk_key_ib,@function +brk_key_ib: +// CHECK-NOT: brk_key_ib + cbz x0, 1f + autia x2, x3 + cbz x1, 2f + brk 0xc471 +1: + ret +2: + ldr x0, [x2] + ret + .size brk_key_ib, .-brk_key_ib + + .globl brk_key_da + .type brk_key_da,@function +brk_key_da: +// CHECK-NOT: brk_key_da + cbz x0, 1f + autia x2, x3 + cbz x1, 2f + brk 0xc472 +1: + ret +2: + ldr x0, [x2] + ret + .size brk_key_da, .-brk_key_da + + .globl brk_key_db + .type brk_key_db,@function +brk_key_db: +// CHECK-NOT: brk_key_db + cbz x0, 1f + autia x2, x3 + cbz x1, 2f + brk 0xc473 +1: + ret +2: + ldr x0, [x2] + ret + .size brk_key_db, .-brk_key_db + +// The immediate operand of BRK instruction may indicate whether the instruction +// is intended to be a non-recoverable trap: for example, for this code +// +// int test_trap(void) { +// __builtin_trap(); +// return 42; +// } +// int test_debugtrap(void) { +// __builtin_debugtrap(); +// return 42; +// } +// +// Clang produces the following assembly: +// +// test_trap: +// brk #0x1 +// test_debugtrap: +// brk #0xf000 +// mov w0, #42 +// ret +// +// In GCC, __builtin_trap() uses "brk 0x3e8" (i.e. decimal 1000) and +// __builtin_debugtrap() is not supported. +// +// At the time of writing these test cases, any BRK instruction is considered +// no-return by BOLT, thus it ends its basic block and prevents falling through +// to the next BB. +// FIXME: Make BOLT handle __builtin_debugtrap() properly from the CFG point +// of view. + + .globl brk_gcc_builtin_trap + .type brk_gcc_builtin_trap,@function +brk_gcc_builtin_trap: +// CHECK-NOT: brk_gcc_builtin_trap + cbz x0, 1f + autia x2, x3 + cbz x1, 2f + brk 0x3e8 // __builtin_trap() +1: + ret +2: + ldr x0, [x2] + ret + .size brk_gcc_builtin_trap, .-brk_gcc_builtin_trap + + .globl brk_clang_builtin_trap + .type brk_clang_builtin_trap,@function +brk_clang_builtin_trap: +// CHECK-NOT: brk_clang_builtin_trap + cbz x0, 1f + autia x2, x3 + cbz x1, 2f + brk 0x1 // __builtin_trap() +1: + ret +2: + ldr x0, [x2] + ret + .size brk_clang_builtin_trap, .-brk_clang_builtin_trap + + .globl brk_clang_builtin_debugtrap + .type brk_clang_builtin_debugtrap,@function +brk_clang_builtin_debugtrap: +// CHECK-LABEL: GS-PAUTH: authentication oracle found in function brk_clang_builtin_debugtrap, basic block {{[^,]+}}, at address +// CHECK-NEXT: The instruction is {{[0-9a-f]+}}: autia x2, x3 +// CHECK-NEXT: The 0 instructions that leak the affected registers are: + cbz x0, 1f + autia x2, x3 + cbz x1, 2f + brk 0xf000 // __builtin_debugtrap() +1: + ret +2: + ldr x0, [x2] + ret + .size brk_clang_builtin_debugtrap, .-brk_clang_builtin_debugtrap + +// Conservatively assume BRK with an unknown immediate operand as not suitable +// for terminating the program on security violation. + .globl brk_unknown_imm + .type brk_unknown_imm,@function +brk_unknown_imm: +// CHECK-LABEL: GS-PAUTH: authentication oracle found in function brk_unknown_imm, basic block {{[^,]+}}, at address +// CHECK-NEXT: The instruction is {{[0-9a-f]+}}: autia x2, x3 +// CHECK-NEXT: The 0 instructions that leak the affected registers are: + cbz x0, 1f + autia x2, x3 + cbz x1, 2f + brk 0x3572 +1: + ret +2: + ldr x0, [x2] + ret + .size brk_unknown_imm, .-brk_unknown_imm + +// Conservatively assume calling the abort() function may be an unsafe way to +// terminate the program, as there is some amount of instructions that would +// be executed when the program state is already tampered with. + .globl call_abort_fn + .type call_abort_fn,@function +call_abort_fn: +// CHECK-LABEL: GS-PAUTH: authentication oracle found in function call_abort_fn, basic block {{[^,]+}}, at address +// CHECK-NEXT: The instruction is {{[0-9a-f]+}}: autia x2, x3 +// CHECK-NEXT: The 0 instructions that leak the affected registers are: + cbz x0, 1f + autia x2, x3 + cbz x1, 2f + b abort // a no-return tail call to abort() +1: + ret +2: + ldr x0, [x2] + ret + .size call_abort_fn, .-call_abort_fn + + .globl main + .type main,@function +main: + mov x0, 0 + ret + .size main, .-main