Hi Segher, As mentionned in the ticket this was my first thought but this means making the pattern aware of all the possible way the address could be access (PIC Vs non-PIC, Arm Vs Thumb-2 Vs Thumb-1) to decide how many scratch registers are needed. I'd rather reuse the existing pattern as much as possible to make sure they are well tested. Ideally I wanted a way to mark a REG RTX so that it is never spilled and such that the mark is propagated when the register is moved to another register or propagated. But that is a bigger change so decided it should be an improvement for later but needed another solution right now.
By the way about making sure the address is not left in a register, I have a question regarding the current stack_protect_set and stack_protect_check pattern and their requirements to have register cleared afterwards: why is that necessary? Currently not all registers are cleared and the guard is available in the canari before it is overwritten anyway so I don't see how clearing the register adds any extra security. What sort of attack is it protecting against? Best regards, Thomas On 29 April 2018 at 00:33, Segher Boessenkool <seg...@kernel.crashing.org> wrote: > Hi! > > On Sat, Apr 28, 2018 at 12:32:26AM +0100, Thomas Preudhomme wrote: >> On Arm (Aarch32 and Aarch64) the stack protector's guard is accessed by >> loading its address first before loading its value from it as part of >> the stack_protect_set or stack_protect_check insn pattern. This creates >> the risk of spilling between the two. > >> diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c >> index deab929..c7ced8f 100644 >> --- a/gcc/cfgexpand.c >> +++ b/gcc/cfgexpand.c >> @@ -6156,6 +6156,10 @@ stack_protect_prologue (void) >> tree guard_decl = targetm.stack_protect_guard (); >> rtx x, y; >> >> + /* Prevent scheduling of instruction(s) between computation of the guard's >> + address and setting of the canari to avoid any spill of the guard's >> + address if computed outside the setting of the canari. */ >> + emit_insn (gen_blockage ()); >> x = expand_normal (crtl->stack_protect_guard); >> if (guard_decl) >> y = expand_normal (guard_decl); > > [ etc. ] > > Why pessimise code for all targets (quite a lot), when it does not even > fix the problem on Arm completely (or not obviously, anyway)? > > Instead, implement stack_protect_* and hide the memory accesses to the > stored canary value (and make sure its address is not left in a register > either!) > > I doubt this can be done completely target-independent, it will always > be best effort that way, aka it won't really work. > > > Segher
