[AArch64][PAC] Combine signing with address materialization #130809
Conversation
|
@llvm/pr-subscribers-lld-elf @llvm/pr-subscribers-llvm-globalisel Author: Anatoly Trosinenko (atrosinenko) ChangesIn pauthtest ABI, it is common to store a pointer signed with address diversity to a heap-allocated object (namely, storing a signed pointer to VTable as part of new object construction). This patch tries to prevent introducing a signing oracle by combining pointer materialization and its (re)signing into a single pseudo instruction which is not expanded until AsmPrinter, if possible. One of the typical patterns is materializing an unsigned pointer with Another pattern is fetching a pointer to VTable from a signed GOT entry using This commit adds an instruction insertion hook for Even though the description of Full diff: https://github.com/llvm/llvm-project/pull/130809.diff 6 Files Affected:
diff --git a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
index b99b3df57b162..2b721ce47b71d 100644
--- a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
+++ b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
@@ -3339,6 +3339,78 @@ AArch64TargetLowering::EmitGetSMESaveSize(MachineInstr &MI,
MI.getOperand(0).getReg())
.addReg(AArch64::XZR);
BB->remove_instr(&MI);
+
+ return BB;
+}
+
+MachineBasicBlock *
+AArch64TargetLowering::tryRewritingPAC(MachineInstr &MI,
+ MachineBasicBlock *BB) const {
+ const TargetInstrInfo *TII = Subtarget->getInstrInfo();
+ MachineRegisterInfo &MRI = MI.getMF()->getRegInfo();
+ const DebugLoc &DL = MI.getDebugLoc();
+
+ MachineInstr *AddrInstr = nullptr;
+ int64_t Offset = 0;
+ // Try to find the address-setting instruction, accumulating the offset
+ // along the way. If any unknown pattern is found, keep everything as-is.
+ MachineOperand *CurOp = &MI.getOperand(1);
+ while (CurOp) {
+ MachineOperand *Def = MRI.getOneDef(CurOp->getReg());
+ if (!Def)
+ return BB;
+ MachineInstr *DefMI = Def->getParent();
+ assert(DefMI != nullptr);
+
+ switch (DefMI->getOpcode()) {
+ case AArch64::COPY:
+ CurOp = &DefMI->getOperand(1);
+ break;
+ case AArch64::ADDXri:
+ if (DefMI->getOperand(3).getImm() != 0) // shifts are not handled
+ return BB;
+ CurOp = &DefMI->getOperand(1);
+ Offset += DefMI->getOperand(2).getImm();
+ break;
+ case AArch64::MOVaddr:
+ case AArch64::LOADgotAUTH:
+ AddrInstr = DefMI;
+ CurOp = nullptr;
+ break;
+ default:
+ return BB;
+ }
+ }
+
+ unsigned NewOpcode = AddrInstr->getOpcode() == AArch64::LOADgotAUTH
+ ? AArch64::LOADgotPAC
+ : AArch64::MOVaddrPAC;
+ MachineOperand &AddrOp = AddrInstr->getOperand(1);
+ unsigned TargetFlags = AddrOp.getTargetFlags() & ~AArch64II::MO_PAGE;
+ Offset += AddrOp.getOffset();
+
+ // MOVaddrPAC and LOADgotPAC pseudos are expanded so that they use X16/X17
+ // internally, thus their restrictions on the register class of $AddrDisc
+ // operand are stricter than those of real PAC* instructions.
+ // If the original instruction accepts a discriminator operand, make sure
+ // it is moved out of X16/X17.
+ Register DiscReg = AArch64::XZR;
+ if (!isPACWithZeroDisc(MI.getOpcode())) {
+ DiscReg = MRI.createVirtualRegister(&AArch64::GPR64noipRegClass);
+ BuildMI(*BB, MI, DL, TII->get(AArch64::COPY), DiscReg)
+ .addReg(MI.getOperand(2).getReg());
+ }
+
+ BuildMI(*BB, MI, DL, TII->get(NewOpcode))
+ .addGlobalAddress(AddrOp.getGlobal(), Offset, TargetFlags)
+ .addImm(getKeyForPACOpcode(MI.getOpcode()))
+ .addReg(DiscReg)
+ .addImm(0);
+
+ BuildMI(*BB, MI, DL, TII->get(AArch64::COPY), MI.getOperand(0).getReg())
+ .addReg(AArch64::X16);
+
+ MI.removeFromParent();
return BB;
}
@@ -3440,6 +3512,16 @@ MachineBasicBlock *AArch64TargetLowering::EmitInstrWithCustomInserter(
return EmitZTInstr(MI, BB, AArch64::ZERO_T, /*Op0IsDef=*/true);
case AArch64::MOVT_TIZ_PSEUDO:
return EmitZTInstr(MI, BB, AArch64::MOVT_TIZ, /*Op0IsDef=*/true);
+
+ case AArch64::PACDA:
+ case AArch64::PACDB:
+ case AArch64::PACIA:
+ case AArch64::PACIB:
+ case AArch64::PACDZA:
+ case AArch64::PACDZB:
+ case AArch64::PACIZA:
+ case AArch64::PACIZB:
+ return tryRewritingPAC(MI, BB);
}
}
diff --git a/llvm/lib/Target/AArch64/AArch64ISelLowering.h b/llvm/lib/Target/AArch64/AArch64ISelLowering.h
index 1987c892ac080..1c5d8ca4f83d4 100644
--- a/llvm/lib/Target/AArch64/AArch64ISelLowering.h
+++ b/llvm/lib/Target/AArch64/AArch64ISelLowering.h
@@ -691,6 +691,8 @@ class AArch64TargetLowering : public TargetLowering {
MachineBasicBlock *BB) const;
MachineBasicBlock *EmitGetSMESaveSize(MachineInstr &MI,
MachineBasicBlock *BB) const;
+ MachineBasicBlock *tryRewritingPAC(MachineInstr &MI,
+ MachineBasicBlock *BB) const;
MachineBasicBlock *
EmitInstrWithCustomInserter(MachineInstr &MI,
diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.h b/llvm/lib/Target/AArch64/AArch64InstrInfo.h
index 3eab98139fa7c..3bd83b0af50af 100644
--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.h
+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.h
@@ -765,6 +765,39 @@ static inline unsigned getPACOpcodeForKey(AArch64PACKey::ID K, bool Zero) {
llvm_unreachable("Unhandled AArch64PACKey::ID enum");
}
+static inline AArch64PACKey::ID getKeyForPACOpcode(unsigned Opcode) {
+ switch (Opcode) {
+ case AArch64::PACDA:
+ case AArch64::PACDZA:
+ return AArch64PACKey::DA;
+ case AArch64::PACDB:
+ case AArch64::PACDZB:
+ return AArch64PACKey::DB;
+ case AArch64::PACIA:
+ case AArch64::PACIZA:
+ return AArch64PACKey::IA;
+ case AArch64::PACIB:
+ case AArch64::PACIZB:
+ return AArch64PACKey::IB;
+ }
+ llvm_unreachable("Unhandled PAC opcode");
+}
+
+static inline bool isPACWithZeroDisc(unsigned Opcode) {
+ switch (Opcode) {
+ case AArch64::PACDA:
+ case AArch64::PACDB:
+ case AArch64::PACIA:
+ case AArch64::PACIB:
+ return false;
+ case AArch64::PACDZA:
+ case AArch64::PACDZB:
+ case AArch64::PACIZA:
+ case AArch64::PACIZB:
+ return true;
+ }
+ llvm_unreachable("Unhandled PAC opcode");
+}
// struct TSFlags {
#define TSFLAG_ELEMENT_SIZE_TYPE(X) (X) // 3-bits
#define TSFLAG_DESTRUCTIVE_INST_TYPE(X) ((X) << 3) // 4-bits
diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.td b/llvm/lib/Target/AArch64/AArch64InstrInfo.td
index ee109277dfbba..9f6e91743196c 100644
--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.td
+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.td
@@ -1821,7 +1821,9 @@ let Predicates = [HasPAuth] in {
def DZB : SignAuthZero<prefix_z, 0b11, !strconcat(asm, "dzb"), op>;
}
+ let usesCustomInserter = true in
defm PAC : SignAuth<0b000, 0b010, "pac", int_ptrauth_sign>;
+
defm AUT : SignAuth<0b001, 0b011, "aut", null_frag>;
def XPACI : ClearAuth<0, "xpaci">;
diff --git a/llvm/test/CodeGen/AArch64/GlobalISel/ptrauth-constant-in-code.ll b/llvm/test/CodeGen/AArch64/GlobalISel/ptrauth-constant-in-code.ll
index 12a3448111fcb..4ef5cc7981924 100644
--- a/llvm/test/CodeGen/AArch64/GlobalISel/ptrauth-constant-in-code.ll
+++ b/llvm/test/CodeGen/AArch64/GlobalISel/ptrauth-constant-in-code.ll
@@ -78,6 +78,84 @@ define ptr @foo() {
ret ptr ptrauth (ptr @g, i32 0)
}
+;--- finalize-isel.ll
+
+; RUN: llc < finalize-isel.ll -mtriple aarch64-elf -mattr=+pauth -global-isel=1 \
+; RUN: -verify-machineinstrs -global-isel-abort=1 -stop-after=finalize-isel | \
+; RUN: FileCheck --check-prefixes=ISEL,ISEL-ELF %s
+; RUN: llc < finalize-isel.ll -mtriple arm64-apple-ios -mattr=+pauth -global-isel=1 \
+; RUN: -verify-machineinstrs -global-isel-abort=1 -stop-after=finalize-isel | \
+; RUN: FileCheck --check-prefixes=ISEL %s
+
+@const_table_local = dso_local constant [3 x ptr] [ptr null, ptr null, ptr null]
+@const_table_got = constant [3 x ptr] [ptr null, ptr null, ptr null]
+
+define void @store_signed_const_local(ptr %dest) {
+; ISEL-LABEL: name: store_signed_const_local
+; ISEL: body:
+; ISEL: %0:gpr64common = COPY $x0
+; ISEL-NEXT: %10:gpr64common = MOVaddr target-flags(aarch64-page) @const_table_local + 8, target-flags(aarch64-pageoff, aarch64-nc) @const_table_local + 8
+; ISEL-NEXT: %2:gpr64common = PAUTH_BLEND %0, 1234
+; ISEL-NEXT: %15:gpr64noip = COPY %2
+; ISEL-NEXT: MOVaddrPAC @const_table_local + 8, 2, %15, 0, implicit-def $x16, implicit-def $x17
+; ISEL-NEXT: %4:gpr64 = COPY $x16
+; ISEL-NEXT: %14:gpr64 = COPY %4
+; ISEL-NEXT: STRXui %14, %0, 0 :: (store (p0) into %ir.dest)
+; ISEL-NEXT: RET_ReallyLR
+ %dest.i = ptrtoint ptr %dest to i64
+ %discr = call i64 @llvm.ptrauth.blend(i64 %dest.i, i64 1234)
+ %signed.i = call i64 @llvm.ptrauth.sign(i64 ptrtoint (ptr getelementptr ([2 x ptr], ptr @const_table_local, i32 0, i32 1) to i64), i32 2, i64 %discr)
+ %signed.ptr = inttoptr i64 %signed.i to ptr
+ store ptr %signed.ptr, ptr %dest
+ ret void
+}
+
+define void @store_signed_const_got(ptr %dest) {
+; ISEL-ELF-LABEL: name: store_signed_const_got
+; ISEL-ELF: body:
+; ISEL-ELF: %0:gpr64common = COPY $x0
+; ISEL-ELF-NEXT: %7:gpr64common = LOADgotAUTH target-flags(aarch64-got) @const_table_got
+; ISEL-ELF-NEXT: %6:gpr64common = ADDXri %7, 8, 0
+; ISEL-ELF-NEXT: %2:gpr64common = PAUTH_BLEND %0, 1234
+; ISEL-ELF-NEXT: %12:gpr64noip = COPY %2
+; ISEL-ELF-NEXT: LOADgotPAC target-flags(aarch64-got) @const_table_got + 8, 2, %12, 0, implicit-def $x16, implicit-def $x17, implicit-def $nzcv
+; ISEL-ELF-NEXT: %4:gpr64 = COPY $x16
+; ISEL-ELF-NEXT: %10:gpr64 = COPY %4
+; ISEL-ELF-NEXT: STRXui %10, %0, 0 :: (store (p0) into %ir.dest)
+; ISEL-ELF-NEXT: RET_ReallyLR
+ %dest.i = ptrtoint ptr %dest to i64
+ %discr = call i64 @llvm.ptrauth.blend(i64 %dest.i, i64 1234)
+ %signed.i = call i64 @llvm.ptrauth.sign(i64 ptrtoint (ptr getelementptr ([2 x ptr], ptr @const_table_got, i32 0, i32 1) to i64), i32 2, i64 %discr)
+ %signed.ptr = inttoptr i64 %signed.i to ptr
+ store ptr %signed.ptr, ptr %dest
+ ret void
+}
+
+define void @store_signed_arg(ptr %dest, ptr %p) {
+; ISEL-LABEL: name: store_signed_arg
+; ISEL: body:
+; ISEL: %0:gpr64common = COPY $x0
+; ISEL-NEXT: %1:gpr64common = COPY $x1
+; ISEL-NEXT: %3:gpr64common = PAUTH_BLEND %0, 1234
+; ISEL-NEXT: %6:gpr64common = ADDXri %1, 8, 0
+; Check that no implicit defs are added to PACDA instruction.
+; ISEL-NEXT: %8:gpr64 = PACDA %6, %3{{$}}
+; ISEL-NEXT: %10:gpr64 = COPY %8
+; ISEL-NEXT: STRXui %10, %0, 0 :: (store (p0) into %ir.dest)
+; ISEL-NEXT: RET_ReallyLR
+ %dest.i = ptrtoint ptr %dest to i64
+ %discr = call i64 @llvm.ptrauth.blend(i64 %dest.i, i64 1234)
+ %p.offset = getelementptr [2 x ptr], ptr %p, i32 0, i32 1
+ %p.offset.i = ptrtoint ptr %p.offset to i64
+ %signed.i = call i64 @llvm.ptrauth.sign(i64 %p.offset.i, i32 2, i64 %discr)
+ %signed.ptr = inttoptr i64 %signed.i to ptr
+ store ptr %signed.ptr, ptr %dest
+ ret void
+}
+
+!llvm.module.flags = !{!0}
+!0 = !{i32 8, !"ptrauth-elf-got", i32 1}
+
;--- ok.ll
; RUN: llc < ok.ll -mtriple aarch64-elf -mattr=+pauth -global-isel=1 \
diff --git a/llvm/test/CodeGen/AArch64/ptrauth-constant-in-code.ll b/llvm/test/CodeGen/AArch64/ptrauth-constant-in-code.ll
index 76339a7cc5791..e59f7925b7e02 100644
--- a/llvm/test/CodeGen/AArch64/ptrauth-constant-in-code.ll
+++ b/llvm/test/CodeGen/AArch64/ptrauth-constant-in-code.ll
@@ -69,6 +69,79 @@ define ptr @foo() {
ret ptr ptrauth (ptr @g, i32 0)
}
+;--- finalize-isel.ll
+
+; RUN: llc < finalize-isel.ll -mtriple aarch64-elf -mattr=+pauth -global-isel=0 \
+; RUN: -verify-machineinstrs -stop-after=finalize-isel | FileCheck --check-prefixes=ISEL,ISEL-ELF %s
+; RUN: llc < finalize-isel.ll -mtriple arm64-apple-ios -mattr=+pauth -global-isel=0 \
+; RUN: -verify-machineinstrs -stop-after=finalize-isel | FileCheck --check-prefixes=ISEL %s
+
+@const_table_local = dso_local constant [3 x ptr] [ptr null, ptr null, ptr null]
+@const_table_got = constant [3 x ptr] [ptr null, ptr null, ptr null]
+
+define void @store_signed_const_local(ptr %dest) {
+; ISEL-LABEL: name: store_signed_const_local
+; ISEL: body:
+; ISEL: %0:gpr64common = COPY $x0
+; ISEL-NEXT: %1:gpr64common = PAUTH_BLEND %0, 1234
+; ISEL-NEXT: %2:gpr64common = MOVaddr target-flags(aarch64-page) @const_table_local + 8, target-flags(aarch64-pageoff, aarch64-nc) @const_table_local + 8
+; ISEL-NEXT: %4:gpr64noip = COPY %1
+; ISEL-NEXT: MOVaddrPAC @const_table_local + 8, 2, %4, 0, implicit-def $x16, implicit-def $x17
+; ISEL-NEXT: %3:gpr64 = COPY $x16
+; ISEL-NEXT: STRXui killed %3, %0, 0 :: (store (s64) into %ir.dest)
+; ISEL-NEXT: RET_ReallyLR
+ %dest.i = ptrtoint ptr %dest to i64
+ %discr = call i64 @llvm.ptrauth.blend(i64 %dest.i, i64 1234)
+ %signed.i = call i64 @llvm.ptrauth.sign(i64 ptrtoint (ptr getelementptr ([2 x ptr], ptr @const_table_local, i32 0, i32 1) to i64), i32 2, i64 %discr)
+ %signed.ptr = inttoptr i64 %signed.i to ptr
+ store ptr %signed.ptr, ptr %dest
+ ret void
+}
+
+define void @store_signed_const_got(ptr %dest) {
+; ISEL-ELF-LABEL: name: store_signed_const_got
+; ISEL-ELF: body:
+; ISEL-ELF: %0:gpr64common = COPY $x0
+; ISEL-ELF-NEXT: %1:gpr64common = PAUTH_BLEND %0, 1234
+; ISEL-ELF-NEXT: %2:gpr64common = LOADgotAUTH target-flags(aarch64-got) @const_table_got, implicit-def dead $x16, implicit-def dead $x17, implicit-def dead $nzcv
+; ISEL-ELF-NEXT: %3:gpr64common = ADDXri killed %2, 8, 0
+; ISEL-ELF-NEXT: %5:gpr64noip = COPY %1
+; ISEL-ELF-NEXT: LOADgotPAC target-flags(aarch64-got) @const_table_got + 8, 2, %5, 0, implicit-def $x16, implicit-def $x17, implicit-def $nzcv
+; ISEL-ELF-NEXT: %4:gpr64 = COPY $x16
+; ISEL-ELF-NEXT: STRXui killed %4, %0, 0 :: (store (s64) into %ir.dest)
+; ISEL-ELF-NEXT: RET_ReallyLR
+ %dest.i = ptrtoint ptr %dest to i64
+ %discr = call i64 @llvm.ptrauth.blend(i64 %dest.i, i64 1234)
+ %signed.i = call i64 @llvm.ptrauth.sign(i64 ptrtoint (ptr getelementptr ([2 x ptr], ptr @const_table_got, i32 0, i32 1) to i64), i32 2, i64 %discr)
+ %signed.ptr = inttoptr i64 %signed.i to ptr
+ store ptr %signed.ptr, ptr %dest
+ ret void
+}
+
+define void @store_signed_arg(ptr %dest, ptr %p) {
+; ISEL-LABEL: name: store_signed_arg
+; ISEL: body:
+; ISEL: %1:gpr64common = COPY $x1
+; ISEL-NEXT: %0:gpr64common = COPY $x0
+; ISEL-NEXT: %2:gpr64common = PAUTH_BLEND %0, 1234
+; ISEL-NEXT: %3:gpr64common = ADDXri %1, 8, 0
+; Check that no implicit defs are added to PACDA instruction.
+; ISEL-NEXT: %4:gpr64 = PACDA %3, killed %2{{$}}
+; ISEL-NEXT: STRXui killed %4, %0, 0 :: (store (s64) into %ir.dest)
+; ISEL-NEXT: RET_ReallyLR
+ %dest.i = ptrtoint ptr %dest to i64
+ %discr = call i64 @llvm.ptrauth.blend(i64 %dest.i, i64 1234)
+ %p.offset = getelementptr [2 x ptr], ptr %p, i32 0, i32 1
+ %p.offset.i = ptrtoint ptr %p.offset to i64
+ %signed.i = call i64 @llvm.ptrauth.sign(i64 %p.offset.i, i32 2, i64 %discr)
+ %signed.ptr = inttoptr i64 %signed.i to ptr
+ store ptr %signed.ptr, ptr %dest
+ ret void
+}
+
+!llvm.module.flags = !{!0}
+!0 = !{i32 8, !"ptrauth-elf-got", i32 1}
+
;--- ok.ll
; RUN: llc < ok.ll -mtriple aarch64-elf -mattr=+pauth -global-isel=0 \
|
|
Here is an example of signing oracle observed in llvm-test-suite (for I get the following (with CFI directives and most comments omitted for brevity): `main` function (without this patch)Under normal circumstances, the value of At the Machine IR level, after With this patch applied, it gets rewritten like this ( This prevents signing oracle in `main` function (with this patch) |
There was a problem hiding this comment.
Semantically, this looks reasonable to me, but I'm not really a back end/isel expert so I think @ahmedbougacha needs to look at this.
One thing I'd like to see is a test case that tries to induce an oracle using the intrinsics in C, something akin to
void* f1(void *p) {
void* authed = __ptrauth_auth(p, ....);
__asm__(""); // may be a sufficient barrier?
return __ptrauth_sign(authed, ...);
}
void* f2(void *p) {
void* authed = __ptrauth_auth(p, ....);
__asm__(""::"r"(authed)); // theoretically does not spill so no oracle
return __ptrauth_sign(authed, ...);
}
void* f3(void *p) {
void* authed = __ptrauth_auth(p, ....);
__asm__(""::"m"(authed)); // theoretically does spill so would be an oracle, what should happen?
return __ptrauth_sign(authed, ...);
}In an ideal world - out of scope for this change - I think we'd like to error out if we ever see a read from memory into an unauthenticated sign, or a spill and subsequent read of an authenticated value. Doing this robustly probably requires recording that a value was authenticated, then in clang producing a warning or error if a developer does something that forces post-auth store to memory, and for llvm/implicitly authed values increasing the keep-in-register priority for those values, and finally if they still end up being spilled giving them a custom auth schema and signing them on spill.
@ojhunt Do you suggest adding a real C source to
IIUC there are a few cases when the former is valid - such as in a dynamic loader, so an opt-out machinery will be needed (such as maybe some function attribute). |
Yes, I was thinking a
Yeah, if we were to do something like this for generic loads (not just restore from spill) we'd probably want a new "trust me I know what I'm doing" signing intrinsic that is explicitly permitted to bypass such a guard. Note I'm not suggesting anything like this for this PR, it's very much a future work idea :D |
|
@ojhunt Updated the test cases - turned out, there was actually an issue with Considering your test cases, I just realized that they should probably be handled by #130807, not by this patch. As for converting them to LLVM IR, LLVM IR for f1, f2, f3define dso_local ptr @f1(ptr noundef %p) local_unnamed_addr #0 {
entry:
%0 = ptrtoint ptr %p to i64
%1 = tail call i64 @llvm.ptrauth.auth(i64 %0, i32 2, i64 1234)
tail call void asm sideeffect "", ""() #4, !srcloc !9
%2 = tail call i64 @llvm.ptrauth.sign(i64 %1, i32 3, i64 42)
%3 = inttoptr i64 %2 to ptr
ret ptr %3
}
define dso_local ptr @f2(ptr noundef %p) local_unnamed_addr #0 {
entry:
%0 = ptrtoint ptr %p to i64
%1 = tail call i64 @llvm.ptrauth.auth(i64 %0, i32 2, i64 1234)
%2 = inttoptr i64 %1 to ptr
tail call void asm sideeffect "", "r"(ptr %2) #4, !srcloc !10
%3 = tail call i64 @llvm.ptrauth.sign(i64 %1, i32 3, i64 42)
%4 = inttoptr i64 %3 to ptr
ret ptr %4
}
define dso_local ptr @f3(ptr noundef %p) local_unnamed_addr #0 {
entry:
%authed = alloca ptr, align 8
call void @llvm.lifetime.start.p0(i64 8, ptr nonnull %authed) #4
%0 = ptrtoint ptr %p to i64
%1 = tail call i64 @llvm.ptrauth.auth(i64 %0, i32 2, i64 1234)
%2 = inttoptr i64 %1 to ptr
store ptr %2, ptr %authed, align 8, !tbaa !11
call void asm sideeffect "", "*m"(ptr nonnull elementtype(ptr) %authed) #4, !srcloc !15
%3 = load ptr, ptr %authed, align 8, !tbaa !11
%4 = ptrtoint ptr %3 to i64
%5 = call i64 @llvm.ptrauth.sign(i64 %4, i32 3, i64 42)
%6 = inttoptr i64 %5 to ptr
call void @llvm.lifetime.end.p0(i64 8, ptr nonnull %authed) #4
ret ptr %6
}
As this PR does not handle folding "standalone" auth into sign, the following code is emitted for On the branch of #130807 the result is similar, which is quite surprising, moreover On the other hand, if I replace inline asm with function call like this: void callee(void *);
void* f3(void *p) {
void* authed = __builtin_ptrauth_auth(p, 2, 1234);
callee(authed);
return __builtin_ptrauth_sign_unauthenticated(authed, 3, 42);
}... both patches behave as expected: this one calls a function and signs a potentially overwritten register: and with PR #130807, authentication is re-done after function call: |
|
Regarding this specific problem, I realized we have a downstream change that I must have lost in the upstreaming branch (my bad), that tries to address a similar problem in a way we can reason about more easily. Specifically: if, when lowering IR before ISel, we see an In general, we've tried to only do this sort of opportunistic late hardening after we tried to do all we could provide guarantees in the frontend and in IR. One good reason is, it makes these weak spots trivial to find and to generalize from, whereas doing something opportunistic will often hide problems in the common case and makes perfectly-accurate analysis tooling load-bearing. That said, the time may have come to do this as a catch-all, since we realistically won't be able to do much about the remaining representational problems (like adding an offset to a ptrauth qualified pointer). So it's a good idea to consider something like this patch. |
|
@ahmedbougacha I performed some testing of #133788 compared to this PR - it seems to eliminate mostly the same set of problems. Though, if I understand it correctly, it does not touch GlobalISel at all (though I don't know whether GlobalISel is widely used). The code base to be built by various versions of LLVM is llvm-test-suite, it was built with the same set of compilation options (optimization level is The idea is that there was detected a number of signing oracles which can be resolved by this PR together with a patch for inst-combine sent as #130807, so the LLVM versions being tested are:
For the base commit (1), 1481 signing oracles are reported (note that the same utility code is linked into many different executables, so a lot of duplicated reports are possible). The LLVM version from #133788 (2) emits only 355 signing oracles and the version from this PR (3) only emits 349 signing oracles. With #130807 cherry-picked on top of both, there are 6 and 0 signing oracles reported, correspondingly. I will provide more information on these 6 remaining oracles after a bit of debugging. |
|
@ahmedbougacha Considering the remaining signing oracles in #133788, there are six: MultiSource/Benchmarks/Prolangs-C++/employ: Materialization of Command lineMultiSource/Benchmarks/Prolangs-C++/ocean: Materialization of Command lineMultiSource/Benchmarks/Prolangs-C++/simul: Materialization of Command lineMultiSource/Benchmarks/Prolangs-C++/family: Materialization of Command lineI added some debug printing, and it turned out that while several of the affected symbols occur multiple times in calls to |
atrosinenko
force-pushed
the
pauth-isel-merge-pac
branch
from
a4071bf to
fa4f923
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
atrosinenko
force-pushed
the
pauth-isel-merge-pac
branch
from
a6ac921 to
7af97c8
Compare
atrosinenko
force-pushed
the
pauth-isel-merge-pac
branch
from
f0714a6 to
6631434
Compare
|
Ping. I have updated this PR to account for possibility of immediate modifier substitution: In the above example, the discriminator is computed as In their current state, both this PR and #133788 protect both pointer-to-be-signed as well as the immediate modifier from tampering, but #133788 still has to be updated to handle GlobalISel in addition to DAGISel. Hopefully, having a unified post-processing at the Another feature of custom inserters seems to be better handling of cross-basic-block analysis. If I understand it right, at finalize-isel stage virtual registers are in SSA form, and it should be safe to ignore basic blocks while following the chains of copy and increment/decrement instructions, as long as only virtual registers are traversed and no PHI instructions occur. @var = global i32 0
@discvar = global i64 0
define i64 @foo(i1 %cond) {
entry:
%addrdisc = load i64, ptr @discvar
%disc = call i64 @llvm.ptrauth.blend(i64 %addrdisc, i64 42)
%var.ptr.i = ptrtoint ptr @var to i64
br i1 %cond, label %next, label %exit
next:
call void asm sideeffect "nop", "r"(i64 %disc)
br label %exit
exit:
%signed = call i64 @llvm.ptrauth.sign(i64 %var.ptr.i, i32 2, i64 %disc)
ret i64 %signed
}
!llvm.module.flags = !{!0}
!0 = !{i32 8, !"ptrauth-elf-got", i32 1}here the DAGISel-based patch is unable to provide a DAGISel-based patchCustom inserter-based patch |
When changing codegen (e.g. in llvm#130809), offsets in binaries produced by LTO tests might change. We do not need to match concrete offset values, it's enough to ensure that hex values in particular places are identical.
|
Turned out, this PR crashes This patch seems to fix the issue: atrosinenko@ca1b92f. |
When changing codegen (e.g. in #130809), offsets in binaries produced by LTO tests might change. We do not need to match concrete offset values, it's enough to ensure that hex values in particular places are identical. --------- Co-authored-by: Anatoly Trosinenko <[email protected]>
…43358) When changing codegen (e.g. in llvm#130809), offsets in binaries produced by LTO tests might change. We do not need to match concrete offset values, it's enough to ensure that hex values in particular places are identical. --------- Co-authored-by: Anatoly Trosinenko <[email protected]>
…43358) When changing codegen (e.g. in llvm#130809), offsets in binaries produced by LTO tests might change. We do not need to match concrete offset values, it's enough to ensure that hex values in particular places are identical. --------- Co-authored-by: Anatoly Trosinenko <[email protected]>
In pauthtest ABI, it is common to store a pointer signed with address diversity to a heap-allocated object (namely, storing a signed pointer to VTable as part of new object construction). This patch tries to prevent introducing a signing oracle by combining pointer materialization and its (re)signing into a single pseudo instruction which is not expanded until AsmPrinter, if possible. One of the typical patterns is materializing an unsigned pointer with `MOVaddr` pseudo and then signing it with `PAC[ID][AB]` instruction, which can be moved far away from `MOVaddr` by one of the passes in the machine pipeline. As the storage address is not a `Constant` value, one cannot simply emit a `ptrauth` constant in the frontend, which would be selected into `MOVaddrPAC` pseudo. Another pattern is fetching a pointer to VTable from a signed GOT entry using `LOADgotAUTH` pseudo, authenticating and checking it, and then re-signing after adding an offset. This commit adds an instruction insertion hook for `PAC[ID][AB]` which detects the above patterns and replaces it either with `MOVaddrPAC` or `LOADgotPAC` instruction.
atrosinenko
force-pushed
the
pauth-isel-merge-pac
branch
from
ced57f0 to
b4c02ae
Compare
|
Rebased to make use of the fix in #143358 and drop ad-hoc changes to |
In pauthtest ABI, it is common to store a pointer signed with address diversity to a heap-allocated object (namely, storing a signed pointer to VTable as part of new object construction). This patch tries to prevent introducing a signing oracle by combining pointer materialization and its (re)signing into a single pseudo instruction which is not expanded until AsmPrinter, if possible. Furthermore, if the discriminator is computed as
blend(addr, imm), these components are detected and passed via separate operands of that pseudo instruction, to prevent substitution of the immediate modifier.One of the typical patterns is materializing an unsigned pointer with
MOVaddrpseudo and then signing it withPAC[ID][AB]instruction, which can be moved far away fromMOVaddrby one of the passes in the machine pipeline. As the storage address is not aConstantvalue, one cannot simply emit aptrauthconstant in the frontend, which would be selected intoMOVaddrPACpseudo.Another pattern is fetching a pointer to VTable from a signed GOT entry using
LOADgotAUTHpseudo, authenticating and checking it, and then re-signing after adding an offset.This commit adds an instruction insertion hook for
PAC[ID][AB]which detects the above patterns and replaces it either withMOVaddrPACorLOADgotPACinstruction.Even though the description of
ISD::PtrAuthGlobalAddressmentions non-constant address discriminators, this seems to be DAGISel-specific and currently unused, so I post-processPAC*instructions inAArch64TargetLowering::EmitInstrWithCustomInserternot to implement this logic twice: in DAGISel and GlobalISel.