[CodeGen] Don't resolve the stack protector frame accesses until PEI

[thegameg]

Summary

Currently, stack protector loads and stores are resolved during LocalStackSlotAllocation (if the pass needs to run). When this is the case, the base register assigned to the frame access is going to be one of the vregs created during LocalStackSlotAllocation. This means that we are keeping a pointer to the stack protector slot, and we're using this pointer to load and store to it.

In case register pressure goes up, we may end up spilling this pointer to the stack, which can be a security concern.

Instead, leave it to PEI to resolve the frame accesses. In order to do that, we make all stack protector accesses go through frame index operands, then PEI will resolve this using an offset from sp/fp/bp.

[john.brawn]

llvm/test/CodeGen/Thumb/stack_guard_remat.ll

6 ↗

(On Diff #209899)

It would be better here to check that this value is being used to construct a stack offset that's being loaded from (i.e. check that we have ldr ra, LCP; add ra, SP; ldr rb, [ra]). Also it would be good to have an explicit check that the stack canary address isn't being spilled at the start of the function.

[efriedma]

It's hard for me to imagine a scenario where this actually makes stack protection substantially more effective; if you have a wild write, presumably you can use it to do something more useful than just corrupt the address of the guard. But I guess the performance cost is small.

Is it possible for register scavenging to scavenge the register containing the address of the guard after the frame index is resolved?

[thegameg]

In D64759#1588217, @efriedma wrote:

It's hard for me to imagine a scenario where this actually makes stack protection substantially more effective; if you have a wild write, presumably you can use it to do something more useful than just corrupt the address of the guard. But I guess the performance cost is small.

The only important thing I see is that it can be used to bypass the stack protector.

Is it possible for register scavenging to scavenge the register containing the address of the guard after the frame index is resolved?

I am looking into this.

[llozano]

Is there any progress on this?

[thegameg]

• Rebase
• Update tests added in r366371
• Update stack_guard_remat.ll

[ostannard]

llvm/test/CodeGen/Thumb/stack_guard_remat.ll

6 ↗

(On Diff #209899)

I think this comment still needs addressing - this just checks that a value is loaded from a constant pool. What we want to test is the sequence ldr r0, LCPI0_0; add r0, sp; ldr r0, [r0], which loads the stack protector without relying on any potentially-spilled values.

[thegameg]

In D64759#1588730, @thegameg wrote:

In D64759#1588217, @efriedma wrote:

It's hard for me to imagine a scenario where this actually makes stack protection substantially more effective; if you have a wild write, presumably you can use it to do something more useful than just corrupt the address of the guard. But I guess the performance cost is small.

The only important thing I see is that it can be used to bypass the stack protector.

Is it possible for register scavenging to scavenge the register containing the address of the guard after the frame index is resolved?

I am looking into this.

llvm/test/CodeGen/AArch64/arm64-big-stack.ll is one example for this (I reordered the instructions for readability):

_foo:                                   ; @foo
; %bb.0:                                ; %entry
	stp	x28, x27, [sp, #-32]!
	stp	x29, x30, [sp, #16]

        ; set up sp
	sub	sp, sp, #4095, lsl #12
	sub	sp, sp, #4095, lsl #12
	sub	sp, sp, #2, lsl #12
	sub	sp, sp, #16

        ; load the guard
	adrp	x8, ___stack_chk_guard@GOTPAGE
	ldr	x8, [x8, ___stack_chk_guard@GOTPAGEOFF]
	ldr	x8, [x8]

        ; compute the address of the guard's stack slot in x9
	add	x9, sp, #4095, lsl #12
	add	x9, x9, #4089, lsl #12
	add	x9, x9, #16

        ; store the guard
	str	x8, [x9, #32760]

During eliminate frame index, we have:

renamable $x8 = LOAD_STACK_GUARD :: (dereferenceable invariant load 8 from @__stack_chk_guard)
STRXui killed renamable $x8, %stack.0.StackGuardSlot, 0 :: (volatile store 8 into %stack.0.StackGuardSlot)

the offset for that is too big and there is no frame pointer, see the first two FIs:

fi#0: size=8, align=8, at location [SP-40]
fi#1: size=33554432, align=1, at location [SP-33554472]

so this becomes:

renamable $x8 = LOAD_STACK_GUARD :: (dereferenceable invariant load 8 from @__stack_chk_guard)
%0:gpr64 = ADDXri $sp, 4095, 12
%0:gpr64 = ADDXri %0:gpr64, 4089, 12
%0:gpr64 = ADDXri %0:gpr64, 16, 0
STRXui killed renamable $x8, killed %0:gpr64, 4095 :: (volatile store 8 into %stack.0.StackGuardSlot)

Then we end up after scavengeFrameVirtualRegs with:

renamable $x8 = LOAD_STACK_GUARD :: (dereferenceable invariant load 8 from @__stack_chk_guard)
$x9 = ADDXri $sp, 4095, 12
$x9 = ADDXri $x9, 4089, 12
$x9 = ADDXri $x9, 16, 0
STRXui killed renamable $x8, killed $x9, 4095 :: (volatile store 8 into %stack.0.StackGuardSlot)

I suppose the reg scavenger can also spill it if it really can't find another register, but I'm not sure we can do much about it at that point.

[thegameg]

Make stack_guard_remat.ll more explicit.

[efriedma]

I suppose the reg scavenger can also spill it if it really can't find another register, but I'm not sure we can do much about it at that point.

If there isn't any free register, the scavenger will spill a register to the emergency spill slot. That doesn't have to be any particular register; it can be basically any allocatable register. So if one or two registers are particularly sensitive, we could specifically forbid the scavenger from spilling them.

[thegameg]

In D64759#1599892, @efriedma wrote:

If there isn't any free register, the scavenger will spill a register to the emergency spill slot. That doesn't have to be any particular register; it can be basically any allocatable register. So if one or two registers are particularly sensitive, we could specifically forbid the scavenger from spilling them.

I agree, we could do that. It would require parts of the code to keep track of the register used for the guard and avoid it at all cost. Do you think this is blocking for this fix to get in?

[efriedma]

Do you think this is blocking for this fix to get in?

No; I think the changes involved would be orthogonal.

I'd like to see some documentation for what attacks we expect stack protectors to stop, and which can't be reasonably stopped with the current implementation, so we have some way to evaluate which fixes are actually useful, though. Obviously we expect that the stack protector will protect the return pointer against a simple buffer overflow of a stack buffer, but it's not clear what other attacks we can/should guard against. This fix isn't relevant for a "simple buffer overflow" scenario because the spill slot can't be placed between an overflowing buffer and the stack protector slot in the same function.

[ostannard]

This patch LGTM now, and +1 to the idea of documenting the attacks we do/don't expect to be able to defend against.

[sdesmalen]

In D64759#1599989, @thegameg wrote:

In D64759#1599892, @efriedma wrote:

If there isn't any free register, the scavenger will spill a register to the emergency spill slot. That doesn't have to be any particular register; it can be basically any allocatable register. So if one or two registers are particularly sensitive, we could specifically forbid the scavenger from spilling them.

I agree, we could do that. It would require parts of the code to keep track of the register used for the guard and avoid it at all cost. Do you think this is blocking for this fix to get in?

Is it worth, as a first step, to always enable the FP when using stack guards?

[sdesmalen]

Patch LGTM!

[thegameg]

I filed PR42764 for the missing documentation.

@efriedma, @sdesmalen, thanks for the suggestions on improving this. I filed PR42765 for follow-ups.

llvm/trunk/lib/CodeGen/LocalStackSlotAllocation.cpp

bool LocalStackSlotPass::insertFrameReferenceRegisters(MachineFunction &Fn) {
   for (int ref = 0, e = FrameReferenceInsns.size(); ref < e ; ++ref) {
     FrameRef &FR = FrameReferenceInsns[ref];
     MachineInstr &MI = *FR.getMachineInstr();
     int64_t LocalOffset = FR.getLocalOffset();
     int FrameIdx = FR.getFrameIndex();
     assert(MFI.isObjectPreAllocated(FrameIdx) &&
            "Only pre-allocated locals expected!");
 
+    // We need to keep the references to the stack protector slot through frame
+    // index operands so that it gets resolved by PEI rather than this pass.
+    // This avoids accesses to the stack protector though virtual base
+    // registers, and forces PEI to address it using fp/sp/bp.
+    if (MFI.hasStackProtectorIndex() &&
+        FrameIdx == MFI.getStackProtectorIndex())
+      continue;
+
     LLVM_DEBUG(dbgs() << "Considering: " << MI);
 
     unsigned idx = 0;
     for (unsigned f = MI.getNumOperands(); idx != f; ++idx) {
       if (!MI.getOperand(idx).isFI())
         continue;
 
       if (FrameIdx == MI.getOperand(idx).getIndex())

llvm/trunk/test/CodeGen/AArch64/stack-guard-reassign.ll

 ; RUN: llc -O0 --frame-pointer=all -mtriple=aarch64-- -o - %S/../Inputs/stack-guard-reassign.ll | FileCheck %s
 
 ; Verify that the offset assigned to the stack protector is at the top of the
 ; frame, covering the locals.
 ; CHECK-LABEL: fn:
-; CHECK:      sub x8, x29, #24
-; CHECK-NEXT: adrp x9, __stack_chk_guard
-; CHECK-NEXT: ldr x9, [x9, :lo12:__stack_chk_guard]
-; CHECK-NEXT: str x9, [x8]
+; CHECK:      adrp x8, __stack_chk_guard
+; CHECK-NEXT: ldr x8, [x8, :lo12:__stack_chk_guard]
+; CHECK-NEXT: stur x8, [x29, #-24]

llvm/trunk/test/CodeGen/ARM/stack-guard-reassign.ll

 ; RUN: llc -O0 --frame-pointer=none -mtriple=arm-- -o - %S/../Inputs/stack-guard-reassign.ll | FileCheck %s
 
 ; Verify that the offset assigned to the stack protector is at the top of the
 ; frame, covering the locals.
 ; CHECK-LABEL: fn:
 ; CHECK:      sub sp, sp, #32
 ; CHECK-NEXT: sub sp, sp, #65536
+; CHECK-NEXT: ldr r1, .LCPI0_0
+; CHECK-NEXT: ldr r2, [r1]
 ; CHECK-NEXT: add lr, sp, #65536
-; CHECK-NEXT: add r1, lr, #28
-; CHECK-NEXT: ldr r2, .LCPI0_0
-; CHECK-NEXT: ldr r3, [r2]
-; CHECK-NEXT: str r3, [r1]
+; CHECK-NEXT: str r2, [lr, #28]
 ; CHECK: .LCPI0_0:
 ; CHECK-NEXT: .long __stack_chk_guard

llvm/trunk/test/CodeGen/PowerPC/stack-guard-reassign.ll

 ; RUN: llc -O0 --frame-pointer=none -mtriple=powerpc-- -o - %S/../Inputs/stack-guard-reassign.ll | FileCheck %s
 
 ; Verify that the offset assigned to the stack protector is at the top of the
 ; frame, covering the locals.
 ; CHECK-LABEL: fn:
 ; CHECK:      mflr 0
 ; CHECK-NEXT: stw 0, 4(1)
 ; CHECK-NEXT: lis 0, -2
 ; CHECK-NEXT: ori 0, 0, 65488
 ; CHECK-NEXT: stwux 1, 1, 0
 ; CHECK-NEXT: subf 0, 0, 1
-; CHECK-NEXT: lis 4, 1
-; CHECK-NEXT: ori 4, 4, 44
-; CHECK-NEXT: add 4, 1, 4
-; CHECK-NEXT: lis 5, __stack_chk_guard@ha
-; CHECK-NEXT: lwz 6, __stack_chk_guard@l(5)
-; CHECK-NEXT: stw 6, 0(4)
+; CHECK-NEXT: lis 4, __stack_chk_guard@ha
+; CHECK-NEXT: lwz 5, __stack_chk_guard@l(4)
+; CHECK-NEXT: lis 6, 1
+; CHECK-NEXT: ori 6, 6, 44
+; CHECK-NEXT: stwx 5, 1, 6

llvm/trunk/test/CodeGen/Thumb/stack_guard_remat.ll

 ; RUN: llc < %s -mtriple=thumb-apple-darwin -relocation-model=pic -no-integrated-as | FileCheck %s -check-prefix=PIC
 ; RUN: llc < %s -mtriple=thumb-apple-darwin -relocation-model=static -no-integrated-as | FileCheck %s -check-prefix=NO-PIC  -check-prefix=STATIC
 ; RUN: llc < %s -mtriple=thumb-apple-darwin -relocation-model=dynamic-no-pic -no-integrated-as | FileCheck %s  -check-prefix=NO-PIC -check-prefix=DYNAMIC-NO-PIC
 
-;PIC:   foo2
-;PIC:   ldr [[R0:r[0-9]+]], [[LABEL0:LCPI[0-9_]+]]
-;PIC: [[LABEL1:LPC[0-9_]+]]:
-;PIC:   add [[R0]], pc
-;PIC:   ldr [[R1:r[0-9]+]], {{\[}}[[R0]]{{\]}}
-;PIC:   ldr [[R1:r[0-9]+]], {{\[}}[[R1]]{{\]}}
-
-;PIC:      [[LABEL0]]:
+;PIC:        foo2
+;PIC:        ldr [[SAVED_GUARD:r[0-9]+]], [[GUARD_STACK_OFFSET:LCPI[0-9_]+]]
+;PIC-NEXT:   add [[SAVED_GUARD]], sp
+;PIC-NEXT:   ldr [[SAVED_GUARD]], {{\[}}[[SAVED_GUARD]]{{\]}}
+;PIC-NEXT:   ldr [[ORIGINAL_GUARD:r[0-9]+]], [[ORIGINAL_GUARD_LABEL:LCPI[0-9_]+]]
+;PIC-NEXT: [[LABEL1:LPC[0-9_]+]]:
+;PIC-NEXT:   add [[ORIGINAL_GUARD]], pc
+;PIC-NEXT:   ldr [[ORIGINAL_GUARD]], {{\[}}[[ORIGINAL_GUARD]]{{\]}}
+;PIC-NEXT:   ldr [[ORIGINAL_GUARD]], {{\[}}[[ORIGINAL_GUARD]]{{\]}}
+;PIC-NEXT:   subs {{r[0-9]+}}, [[ORIGINAL_GUARD]], [[SAVED_GUARD]]
+
+;PIC:      [[GUARD_STACK_OFFSET]]:
+;PIC-NEXT:   .long 1028
+;PIC:      [[ORIGINAL_GUARD_LABEL]]:
 ;PIC-NEXT:   .long L___stack_chk_guard$non_lazy_ptr-([[LABEL1]]+4)
 
 ;NO-PIC:   foo2
-;NO-PIC:   ldr [[R0:r[0-9]+]], [[LABEL0:LCPI[0-9_]+]]
+;NO-PIC:                ldr [[SAVED_GUARD:r[0-9]+]], [[GUARD_STACK_OFFSET:LCPI[0-9_]+]]
+;NO-PIC-NEXT:           add [[SAVED_GUARD]], sp
+;NO-PIC-NEXT:           ldr [[SAVED_GUARD]], {{\[}}[[SAVED_GUARD]]{{\]}}
+;NO-PIC-NEXT:           ldr [[ORIGINAL_GUARD:r[0-9]+]], [[ORIGINAL_GUARD_LABEL:LCPI[0-9_]+]]
 ;NO-PIC-NOT: LPC
-;NO-PIC:   ldr {{r[0-9]+}}, {{\[}}[[R0]]{{\]}}
-
-;STATIC:      [[LABEL0]]:
+;NO-PIC-NEXT:           ldr [[ORIGINAL_GUARD]], {{\[}}[[ORIGINAL_GUARD]]{{\]}}
+;DYNAMIC-NO-PIC-NEXT:   ldr [[ORIGINAL_GUARD]], {{\[}}[[ORIGINAL_GUARD]]{{\]}}
+;NO-PIC-NEXT:           subs {{r[0-9]+}}, [[ORIGINAL_GUARD]], [[SAVED_GUARD]]
+
+;STATIC:      [[GUARD_STACK_OFFSET]]:
+;STATIC-NEXT:   .long 1028
+;STATIC:      [[ORIGINAL_GUARD_LABEL]]:
 ;STATIC-NEXT:   .long ___stack_chk_guard
 
-;DYNAMIC-NO-PIC:      [[LABEL0]]:
+;DYNAMIC-NO-PIC:      [[GUARD_STACK_OFFSET]]:
+;DYNAMIC-NO-PIC-NEXT:   .long 1028
+;DYNAMIC-NO-PIC:      [[ORIGINAL_GUARD_LABEL]]:
 ;DYNAMIC-NO-PIC-NEXT:   .long L___stack_chk_guard$non_lazy_ptr
 
 ; Function Attrs: nounwind ssp
 define i32 @test_stack_guard_remat() #0 {
   %a1 = alloca [256 x i32], align 4
   %1 = bitcast [256 x i32]* %a1 to i8*
   call void @llvm.lifetime.start.p0i8(i64 1024, i8* %1)
   %2 = getelementptr inbounds [256 x i32], [256 x i32]* %a1, i32 0, i32 0