[BOLT] Gadget scanner: account for BRK when searching for auth oracles #137975
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
atrosinenko
requested review from
aaupov,
asl,
ayermolo,
dcci,
jacobbramley,
kbeyls,
maksfb,
rafaelauler and
yota9
This was referenced Apr 30, 2025
|
@llvm/pr-subscribers-bolt Author: Anatoly Trosinenko (atrosinenko) ChangesAn authenticated pointer can be explicitly checked by the compiler via a Full diff: https://github.com/llvm/llvm-project/pull/137975.diff 6 Files Affected:
diff --git a/bolt/include/bolt/Core/MCPlusBuilder.h b/bolt/include/bolt/Core/MCPlusBuilder.h
index 83ad70ea97076..b0484964cfbad 100644
--- a/bolt/include/bolt/Core/MCPlusBuilder.h
+++ b/bolt/include/bolt/Core/MCPlusBuilder.h
@@ -706,6 +706,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 fab92947f6d67..20d4fdf42f03a 100644
--- a/bolt/lib/Passes/PAuthGadgetScanner.cpp
+++ b/bolt/lib/Passes/PAuthGadgetScanner.cpp
@@ -1003,6 +1003,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()) {
@@ -1108,8 +1117,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 f3c29e6ee43b9..4d11c5b206eab 100644
--- a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
+++ b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
@@ -386,10 +386,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
@@ -1745,6 +1744,25 @@ 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;
+ default:
+ 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 ecca78c80a8f5..276f11829d249 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
@@ -301,7 +301,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
@@ -629,8 +629,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
@@ -638,7 +637,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 9b10879094a03..9eb656639c191 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
@@ -732,7 +732,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
|
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-trap-instruction
branch
from
5d0aa04 to
34342bc
Compare
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-untrusted-lr-before-tail-call
branch
from
33158bc to
24d83f3
Compare
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-untrusted-lr-before-tail-call
branch
from
24d83f3 to
d20efbe
Compare
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-trap-instruction
branch
from
74bbe1e to
a9c8708
Compare
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-untrusted-lr-before-tail-call
branch
from
ec07ae0 to
a0c9617
Compare
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-trap-instruction
branch
from
a9c8708 to
3e9bfcd
Compare
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-untrusted-lr-before-tail-call
branch
from
a0c9617 to
b1cda6f
Compare
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-trap-instruction
branch
2 times, most recently
from
0ee5614 to
7297856
Compare
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-untrusted-lr-before-tail-call
branch
2 times, most recently
from
275c063 to
44f7d3d
Compare
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-trap-instruction
branch
2 times, most recently
from
1fbb916 to
66ad4fa
Compare
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-untrusted-lr-before-tail-call
branch
from
44f7d3d to
5d06789
Compare
kbeyls
reviewed
Jun 24, 2025
Comment on lines
+1756
to
+1758
| // 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(). |
There was a problem hiding this comment.
I'm not sure if it's a good idea to only consider pauthabi-specific BRK values in a "generic" AArch64-interface to test whether something is a trap. This "isTrap" function might get used by other analyses too...
I wonder if there would be a way to change the interface of isTrap to make it appropriately generic so that it could be used without confusion by other analyses too?
An example is this commit that makes the pac-ret analysis more accurate, which I guess hasn't been upstreamed yet: 5b3ed52
There was a problem hiding this comment.
In the original patch, I tried to make isTrap function reasonably usable by non-PAuth analysis by describing it as suitable for terminating the program immediately on a security violation (compared to a generic MCPlusBuilder::isBreakpoint which does not explicitly mention any security-related properties). My original reasoning for only matching well-known constants was that somebody may use brk instruction for "recoverable" break-points by installing a SIGTRAP handler, for example.
As mentioned in 5b3ed52, there was brk 0x3e8 instruction spotted in-the-wild - turned out, it is the instruction emitted by GCC for __builtin_trap(). By the way, Clang uses brk 0x1 instead. Furthermore, as discussed in GCC bug tracker, there is __builtin_debugtrap() in Clang (but not in GCC) which is not no-return (and there is even std::breakpoint proposed for C++).
Frankly speaking, I did not succeed in observing any difference between executing __builtin_trap() and __builtin_debugtrap() on AArch64 Linux - both result in SIGTRAP. But on x86_64, these are SIGILL vs. SIGTRAP and the codegen differs on both targets: for this source code
int test_trap(void) {
__builtin_trap();
return 42;
}
int test_debugtrap(void) {
__builtin_debugtrap();
return 42;
}Clang generates AArch64 assembly along the lines
test_trap:
brk #0x1
test_debugtrap:
brk #0xf000
mov w0, #42
ret
In abe2b92, I added a dedicated test file on detecting various instructions as immediately terminating the program vs. recoverable break-points. Turned out, any BRK instruction is treated as noreturn by BOLT, thus __builtin_debugtrap() is probably understood differenlty by BOLT disassembler and by LLVM backend on AArch64 - I added a FIXME on this.
There was a problem hiding this comment.
Thank you for investigating this in detail!
It is unfortunate that we have to hard-code assumptions of what the meanings are of specific immediates in the brk instruction. Really, this is some form of non-documented "ABI" now.
Anyway, I agree that what you've implemented in the current version of this patch seems to be the most reasonable way to implement this.
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-trap-instruction
branch
from
66ad4fa to
51b4bbc
Compare
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-untrusted-lr-before-tail-call
branch
2 times, most recently
from
27d75c4 to
dfa0b1a
Compare
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-trap-instruction
branch
2 times, most recently
from
150ff31 to
2192448
Compare
Base automatically changed from
users/atrosinenko/bolt-gs-untrusted-lr-before-tail-call
to
main
June 26, 2025 09:37
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-trap-instruction
branch
2 times, most recently
from
e5f2e75 to
e4a8f13
Compare
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-trap-instruction
branch
from
e4a8f13 to
abe2b92
Compare
An authenticated pointer can be explicitly checked by the compiler via a
sequence of instructions that executes BRK on failure. It is important
to recognize such BRK instruction as checking every register (as it is
expected to immediately trigger an abnormal program termination) to
prevent false positive reports about authentication oracles:
autia x2, x3
autia x0, x1
; neither x0 nor x2 are checked at this point
eor x16, x0, x0, lsl #1
tbz x16, #62, on_success ; marks x0 as checked
; end of BB: for x2 to be checked here, it must be checked in both
; successor basic blocks
on_failure:
brk 0xc470
on_success:
; x2 is checked
ldr x1, [x2] ; marks x2 as checked
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-trap-instruction
branch
from
abe2b92 to
b5f8cdb
Compare
kbeyls
approved these changes
Aug 21, 2025
| // 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 |
There was a problem hiding this comment.
If I understood correctly, this FIXME indicates that BOLT incorrectly builds a CFG when __builtin_debugtrap(), i.e. brk #0xf000 if in the binary?
If so, it seems to me this could lead to invalid code optimizations done by BOLT on such functions?
If my understanding is correct, I would suggest creating a github issue for this, so that this bug is more visible than just a FIXME in a test file...
There was a problem hiding this comment.
Documented this in #155232, thanks.
Comment on lines
+1756
to
+1758
| // 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(). |
There was a problem hiding this comment.
Thank you for investigating this in detail!
It is unfortunate that we have to hard-code assumptions of what the meanings are of specific immediates in the brk instruction. Really, this is some form of non-documented "ABI" now.
Anyway, I agree that what you've implemented in the current version of this patch seems to be the most reasonable way to implement this.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
An authenticated pointer can be explicitly checked by the compiler via a
sequence of instructions that executes BRK on failure. It is important
to recognize such BRK instruction as checking every register (as it is
expected to immediately trigger an abnormal program termination) to
prevent false positive reports about authentication oracles: