Page MenuHomePhabricator

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

Authored by thegameg on Jul 15 2019, 10:12 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

thegameg created this revision.Jul 15 2019, 10:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2019, 10:12 AM
john.brawn added inline comments.Jul 16 2019, 4:43 AM
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.

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?

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.

Is there any progress on this?

thegameg updated this revision to Diff 211332.Tue, Jul 23, 10:45 AM
  • Rebase
  • Update tests added in r366371
  • Update stack_guard_remat.ll
ostannard added inline comments.
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 marked an inline comment as done.Wed, Jul 24, 9:03 AM

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 updated this revision to Diff 211529.Wed, Jul 24, 9:31 AM
thegameg marked an inline comment as done.

Make stack_guard_remat.ll more explicit.

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.

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?

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 accepted this revision.Thu, Jul 25, 3:13 AM

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

This revision is now accepted and ready to land.Thu, Jul 25, 3:13 AM

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 accepted this revision.Thu, Jul 25, 4:56 AM

Patch LGTM!

I filed PR42764 for the missing documentation.

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

This revision was automatically updated to reflect the committed changes.