From 5f99333cb13d9806560ef6b4712a1dc9c5ec2f51 Mon Sep 17 00:00:00 2001 From: Oliver Stannard Date: Tue, 28 Jan 2025 15:36:27 +0000 Subject: [PATCH 1/2] Add test showing bug --- .../AArch64/pauthlr-prologue-duplication.mir | 85 +++++++++++++++++++ 1 file changed, 85 insertions(+) create mode 100644 llvm/test/CodeGen/AArch64/pauthlr-prologue-duplication.mir diff --git a/llvm/test/CodeGen/AArch64/pauthlr-prologue-duplication.mir b/llvm/test/CodeGen/AArch64/pauthlr-prologue-duplication.mir new file mode 100644 index 0000000000000..a53c7a2960434 --- /dev/null +++ b/llvm/test/CodeGen/AArch64/pauthlr-prologue-duplication.mir @@ -0,0 +1,85 @@ +# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 5 +# RUN: llc -mtriple aarch64-none-elf -run-pass=block-placement -O3 -o - %s | FileCheck %s + +## Check that block-placement does not perform tail duplication on the +## PAUTH_EPILOGUE instruction. If that happened, the two prologues would use +## different addresses while calculating the return address signature, so the +## epilogue could only be correct for (at most) one of them. + +--- | + define void @test() "frame-pointer"="non-leaf" { + entry: + ret void + } + + declare void @f() +... +--- +name: test +body: | + ; CHECK-LABEL: name: test + ; CHECK: bb.0.entry: + ; CHECK-NEXT: successors: %bb.1(0x30000000), %bb.2(0x50000000) + ; CHECK-NEXT: liveins: $w0, $w1, $lr + ; CHECK-NEXT: {{ $}} + ; CHECK-NEXT: CBZW renamable $w0, %bb.1 + ; CHECK-NEXT: {{ $}} + ; CHECK-NEXT: bb.2: + ; CHECK-NEXT: successors: %bb.5(0x30000000), %bb.4(0x50000000) + ; CHECK-NEXT: liveins: $w0, $w1, $lr + ; CHECK-NEXT: {{ $}} + ; CHECK-NEXT: frame-setup PAUTH_PROLOGUE implicit-def $lr, implicit killed $lr, implicit $sp + ; CHECK-NEXT: CBZW killed renamable $w1, %bb.5 + ; CHECK-NEXT: {{ $}} + ; CHECK-NEXT: bb.4: + ; CHECK-NEXT: successors: %bb.5(0x80000000) + ; CHECK-NEXT: {{ $}} + ; CHECK-NEXT: BL @f, csr_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit-def $sp + ; CHECK-NEXT: {{ $}} + ; CHECK-NEXT: bb.5: + ; CHECK-NEXT: BL @f, csr_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit-def $sp + ; CHECK-NEXT: frame-destroy PAUTH_EPILOGUE implicit-def $lr, implicit killed $lr, implicit $sp + ; CHECK-NEXT: TCRETURNdi @f, 0, csr_aarch64_aapcs, implicit $sp + ; CHECK-NEXT: {{ $}} + ; CHECK-NEXT: bb.1: + ; CHECK-NEXT: successors: %bb.5(0x30000000), %bb.4(0x50000000) + ; CHECK-NEXT: liveins: $w1, $lr + ; CHECK-NEXT: {{ $}} + ; CHECK-NEXT: renamable $w8 = MOVZWi 1, 0 + ; CHECK-NEXT: frame-setup PAUTH_PROLOGUE implicit-def $lr, implicit killed $lr, implicit $sp + ; CHECK-NEXT: CBNZW killed renamable $w1, %bb.4 + ; CHECK-NEXT: B %bb.5 + bb.0.entry: + successors: %bb.1(0x30000000), %bb.2(0x50000000) + liveins: $w0, $w1, $lr + + CBNZW renamable $w0, %bb.2 + + bb.1: + successors: %bb.3(0x80000000) + liveins: $w1, $lr + + renamable $w8 = MOVZWi 1, 0 + B %bb.3 + + bb.2: + successors: %bb.3(0x80000000) + liveins: $w0, $w1, $lr + + bb.3: + successors: %bb.5(0x30000000), %bb.4(0x50000000) + liveins: $w1, $w8, $lr + + frame-setup PAUTH_PROLOGUE implicit-def $lr, implicit killed $lr, implicit $sp + CBZW killed renamable $w1, %bb.5 + + bb.4: + successors: %bb.5(0x80000000) + + BL @f, csr_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit-def $sp + + bb.5: + BL @f, csr_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit-def $sp + frame-destroy PAUTH_EPILOGUE implicit-def $lr, implicit killed $lr, implicit $sp + TCRETURNdi @f, 0, csr_aarch64_aapcs, implicit $sp +... From 75ed43683e982c4e29d65ba9dd2a01a196e88456 Mon Sep 17 00:00:00 2001 From: Oliver Stannard Date: Tue, 28 Jan 2025 15:41:10 +0000 Subject: [PATCH 2/2] [AArch64] PAUTH_PROLOGUE should not be duplicated with PAuthLR When using PAuthLR, the PAUTH_PROLOGUE expands into a sequence of instructions which takes the address of one of those instructions, and uses that address to compute the return address signature. If this is duplicated, there will be two different addresses used in calculating the signature, so the epilogue will only be correct for (at most) one of them. This change also restricts code generation when using v8.3-A return address signing, without PAuthLR. This isn't strictly needed, as duplicating the prologue there would be valid. We could fix this by having two copies of PAUTH_PROLOGUE, with and without isNotDuplicable, but I don't think it's worth adding the extra complexity to a security feature for that. --- llvm/lib/Target/AArch64/AArch64InstrInfo.td | 7 +++++- .../AArch64/pauthlr-prologue-duplication.mir | 23 +++++++++++-------- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.td b/llvm/lib/Target/AArch64/AArch64InstrInfo.td index d112d4f10e47d..b77246200db64 100644 --- a/llvm/lib/Target/AArch64/AArch64InstrInfo.td +++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.td @@ -1773,7 +1773,12 @@ def : InstAlias<"xpaclri", (XPACLRI), 0>; let Uses = [LR, SP], Defs = [LR] in { // Insertion point of LR signing code. -def PAUTH_PROLOGUE : Pseudo<(outs), (ins), []>, Sched<[]>; +def PAUTH_PROLOGUE : Pseudo<(outs), (ins), []>, Sched<[]> { + // When using PAuthLR, the address of one of the instructions this expands + // into is used as an input to the signature calculation, so this must not be + // duplicated. + let isNotDuplicable = 1; +} // Insertion point of LR authentication code. // The RET terminator of the containing machine basic block may be replaced // with a combined RETA(A|B) instruction when rewriting this Pseudo. diff --git a/llvm/test/CodeGen/AArch64/pauthlr-prologue-duplication.mir b/llvm/test/CodeGen/AArch64/pauthlr-prologue-duplication.mir index a53c7a2960434..5e57604263793 100644 --- a/llvm/test/CodeGen/AArch64/pauthlr-prologue-duplication.mir +++ b/llvm/test/CodeGen/AArch64/pauthlr-prologue-duplication.mir @@ -25,9 +25,21 @@ body: | ; CHECK-NEXT: CBZW renamable $w0, %bb.1 ; CHECK-NEXT: {{ $}} ; CHECK-NEXT: bb.2: - ; CHECK-NEXT: successors: %bb.5(0x30000000), %bb.4(0x50000000) + ; CHECK-NEXT: successors: %bb.3(0x80000000) ; CHECK-NEXT: liveins: $w0, $w1, $lr ; CHECK-NEXT: {{ $}} + ; CHECK-NEXT: B %bb.3 + ; CHECK-NEXT: {{ $}} + ; CHECK-NEXT: bb.1: + ; CHECK-NEXT: successors: %bb.3(0x80000000) + ; CHECK-NEXT: liveins: $w1, $lr + ; CHECK-NEXT: {{ $}} + ; CHECK-NEXT: renamable $w8 = MOVZWi 1, 0 + ; CHECK-NEXT: {{ $}} + ; CHECK-NEXT: bb.3: + ; CHECK-NEXT: successors: %bb.5(0x30000000), %bb.4(0x50000000) + ; CHECK-NEXT: liveins: $w1, $w8, $lr + ; CHECK-NEXT: {{ $}} ; CHECK-NEXT: frame-setup PAUTH_PROLOGUE implicit-def $lr, implicit killed $lr, implicit $sp ; CHECK-NEXT: CBZW killed renamable $w1, %bb.5 ; CHECK-NEXT: {{ $}} @@ -40,15 +52,6 @@ body: | ; CHECK-NEXT: BL @f, csr_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit-def $sp ; CHECK-NEXT: frame-destroy PAUTH_EPILOGUE implicit-def $lr, implicit killed $lr, implicit $sp ; CHECK-NEXT: TCRETURNdi @f, 0, csr_aarch64_aapcs, implicit $sp - ; CHECK-NEXT: {{ $}} - ; CHECK-NEXT: bb.1: - ; CHECK-NEXT: successors: %bb.5(0x30000000), %bb.4(0x50000000) - ; CHECK-NEXT: liveins: $w1, $lr - ; CHECK-NEXT: {{ $}} - ; CHECK-NEXT: renamable $w8 = MOVZWi 1, 0 - ; CHECK-NEXT: frame-setup PAUTH_PROLOGUE implicit-def $lr, implicit killed $lr, implicit $sp - ; CHECK-NEXT: CBNZW killed renamable $w1, %bb.4 - ; CHECK-NEXT: B %bb.5 bb.0.entry: successors: %bb.1(0x30000000), %bb.2(0x50000000) liveins: $w0, $w1, $lr