[StackProtector] Clear out stack protector slot

[bwendling]

Don't leave the stack protector guard information on the stack after exiting the function. This helps to prevent information leaking.

[efriedma-quic]

How much code bloat do we expect from this? Adding an extra instruction to every function with a stack protector seems non-trivial. Do we want a flag for this?

I noticed this only updates tests for x86 and RISCV. Do we need separate fixes for targets that have target-specific stack protector support?

[efriedma-quic]

Also, have you considered a more general-purpose feature to make functions clear their stack when they return?

[rnk]

As a point of comparison, Microsoft puts the check in __security_check_cookie, and they do not zero out the stack slot with the cookie: https://gcc.godbolt.org/z/Po6fs6PnW

If we want to make our stack protection stronger, we'd should consider the technique of XOR'ing RSP or RBP into the cookie, so we don't directly store the cookie in memory.

If we care about size, we should outline this logic, particularly the conditional check. The simplest way to do that would be to make a linkonce_odr helper like __llvm_stack_protector_check that does all this.

I think zeroing out the entire frame is usually prohibitively expensive.

[bwendling]

@efriedma-quic There's a related option -ftrivial-auto-var-init for function entrance. It will have the same overall affect as this patch, but in the opposite direction. I thought this patch would be good for those not using that option. I suppose I could zero out the stack before returning and use a flag to enable that. It might be more code than a store.

This is also the first in a two-part series where next I want to zero out the register holding the stack guard value before returning.

As for the other platforms and target-specific SP support, I'll look into that.

@rnk To clarify, __llvm_stack_protector_check would act similarly to Microsoft's implementation? As for XOR'ing, I'm wary of something like that because it's still potentially visible to someone wanting to exploit the stack. Do you have an example of how we could do this?

[rnk]

Regarding XOR, LLVM already implements this for Windows, see X86TargetLowering::emitStackGuardXorFP (I can't link to it on GitHub because X86ISelLowering is 2.2MB (!) but that's another matter...), and I don't think anything stops us from reusing that logic on other platforms.

Regarding __llvm_stack_protector_check, it is an idea to optimize code size potentially at some performance cost, since it requires an extra call. In the godbolt example we generate this code sequence:

.... # regular code
  movq %fs:40, %rax
  cmpq 16(%rsp), %rax
  jne .LBB0_2
  # Normal epilogue
  addq $24, %rsp
  retq
  # extra BB to abort
.LBB0_2:
  callq __stack_chk_fail@PLT

The idea is that we could make the code more compact by loading the stack cookie into a register parameter, and then calling a helper that does the comparison, so we'd get this instead:

... # regular code
  mov 16(%rsp), %rdi
  callq __llvm_stack_protector_check
  # Normal epilogue
  addq $24, %rsp
  retq
...
__llvm_stack_protector_check:
  movq %fs:40, %rax
  cmpq %rax, %rdi
  jne .Labort
  retq
.Labort:
  subq $4, %rsp  # maintain 16 byte stack alignment
  callq __stack_chk_fail@PLT

[nickdesaulniers]

This seems related to:

KSPP/linux#29
#46338

Mind adding links to those in the commit message and PR description?

I would be curious if you could also find historical context as to why GCC does this only for arm targets (IIUC).

[nickdesaulniers]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96191 seems to be the historical context.

[bwendling]

From the feedback, I think this change may be a bit premature. As I mentioned, there's already a way to zero the stack upon entry to the function.

What I really want to do is zero out the register that held the stack guard value, like in the GCC patch @nickdesaulniers pointed out. I'm going to close this and work on the register clearing instead. (I also messed up the branch with this change. Oh boy do I love Git!)