This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [ARM] Fix PR85434: spill of stack protector's guard address
- From: Thomas Preudhomme <thomas dot preudhomme at linaro dot org>
- To: Segher Boessenkool <segher at kernel dot crashing dot org>
- Cc: gcc-patches at gcc dot gnu dot org, Kyrylo Tkachov <Kyrylo dot Tkachov at arm dot com>, Ramana Radhakrishnan <ramana dot radhakrishnan at arm dot com>, Richard Earnshaw <richard dot earnshaw at arm dot com>
- Date: Wed, 2 May 2018 07:57:55 +0100
- Subject: Re: [ARM] Fix PR85434: spill of stack protector's guard address
- References: <CAKnkMGureVC-Wf09yG1x70h+LTfh+oQt_e-6nU8P8xkAi2PYAQ@mail.gmail.com> <20180428233322.GC14117@gate.crashing.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