Re: [ARM] Fix PR85434: spill of stack protector's guard address

[Thomas Preudhomme <thomas dot preudhomme at linaro dot org>]

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
<segher@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