Sebastian Pop <sebpop....@gmail.com> writes:
> Hi,
>
> On Fri, Aug 7, 2020 at 6:18 AM Richard Sandiford <rsand...@gcc.gnu.org> wrote:
>>
>> https://gcc.gnu.org/g:5380912a17ea09a8996720fb62b1a70c16c8f9f2
>>
>> commit r9-8794-g5380912a17ea09a8996720fb62b1a70c16c8f9f2
>> Author: Richard Sandiford <richard.sandif...@arm.com>
>> Date: Fri Aug 7 12:17:37 2020 +0100
>
> could you please also apply this change to the gcc-8 branch?
OK, I'll backport it next week.
Thanks,
Richard
>
> Thanks,
> Sebastian
>
>>
>> aarch64: Clear canary value after stack_protect_test [PR96191]
>>
>> The stack_protect_test patterns were leaving the canary value in the
>> temporary register, meaning that it was often still in registers on
>> return from the function. An attacker might therefore have been
>> able to use it to defeat stack-smash protection for a later function.
>>
>> gcc/
>> PR target/96191
>> * config/aarch64/aarch64.md (stack_protect_test_<mode>): Set the
>> CC register directly, instead of a GPR. Replace the original GPR
>> destination with an extra scratch register. Zero out operand 3
>> after use.
>> (stack_protect_test): Update accordingly.
>>
>> gcc/testsuite/
>> PR target/96191
>> * gcc.target/aarch64/stack-protector-1.c: New test.
>> * gcc.target/aarch64/stack-protector-2.c: Likewise.
>>
>> (cherry picked from commit fe1a26429038d7cd17abc53f96a6f3e2639b605f)
>>
>> Diff:
>> ---
>> gcc/config/aarch64/aarch64.md | 34 ++++-----
>> .../gcc.target/aarch64/stack-protector-1.c | 89
>> ++++++++++++++++++++++
>> .../gcc.target/aarch64/stack-protector-2.c | 6 ++
>> 3 files changed, 110 insertions(+), 19 deletions(-)
>>
>> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
>> index ed8cf8ecea1..9598bac387f 100644
>> --- a/gcc/config/aarch64/aarch64.md
>> +++ b/gcc/config/aarch64/aarch64.md
>> @@ -6985,10 +6985,8 @@
>> (match_operand 2)]
>> ""
>> {
>> - rtx result;
>> machine_mode mode = GET_MODE (operands[0]);
>>
>> - result = gen_reg_rtx(mode);
>> if (aarch64_stack_protector_guard != SSP_GLOBAL)
>> {
>> /* Generate access through the system register. The
>> @@ -7013,29 +7011,27 @@
>> operands[1] = gen_rtx_MEM (mode, tmp_reg);
>> }
>> emit_insn ((mode == DImode
>> - ? gen_stack_protect_test_di
>> - : gen_stack_protect_test_si) (result,
>> - operands[0],
>> - operands[1]));
>> -
>> - if (mode == DImode)
>> - emit_jump_insn (gen_cbranchdi4 (gen_rtx_EQ (VOIDmode, result,
>> const0_rtx),
>> - result, const0_rtx, operands[2]));
>> - else
>> - emit_jump_insn (gen_cbranchsi4 (gen_rtx_EQ (VOIDmode, result,
>> const0_rtx),
>> - result, const0_rtx, operands[2]));
>> + ? gen_stack_protect_test_di
>> + : gen_stack_protect_test_si) (operands[0], operands[1]));
>> +
>> + rtx cc_reg = gen_rtx_REG (CCmode, CC_REGNUM);
>> + emit_jump_insn (gen_condjump (gen_rtx_EQ (VOIDmode, cc_reg, const0_rtx),
>> + cc_reg, operands[2]));
>> DONE;
>> })
>>
>> +;; DO NOT SPLIT THIS PATTERN. It is important for security reasons that the
>> +;; canary value does not live beyond the end of this sequence.
>> (define_insn "stack_protect_test_<mode>"
>> - [(set (match_operand:PTR 0 "register_operand" "=r")
>> - (unspec:PTR [(match_operand:PTR 1 "memory_operand" "m")
>> - (match_operand:PTR 2 "memory_operand" "m")]
>> - UNSPEC_SP_TEST))
>> + [(set (reg:CC CC_REGNUM)
>> + (unspec:CC [(match_operand:PTR 0 "memory_operand" "m")
>> + (match_operand:PTR 1 "memory_operand" "m")]
>> + UNSPEC_SP_TEST))
>> + (clobber (match_scratch:PTR 2 "=&r"))
>> (clobber (match_scratch:PTR 3 "=&r"))]
>> ""
>> - "ldr\t%<w>3, %1\;ldr\t%<w>0, %2\;eor\t%<w>0, %<w>3, %<w>0"
>> - [(set_attr "length" "12")
>> + "ldr\t%<w>2, %0\;ldr\t%<w>3, %1\;subs\t%<w>2, %<w>2, %<w>3\;mov\t%3, 0"
>> + [(set_attr "length" "16")
>> (set_attr "type" "multiple")])
>>
>> ;; Write Floating-point Control Register.
>> diff --git a/gcc/testsuite/gcc.target/aarch64/stack-protector-1.c
>> b/gcc/testsuite/gcc.target/aarch64/stack-protector-1.c
>> new file mode 100644
>> index 00000000000..73e83bc413f
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/aarch64/stack-protector-1.c
>> @@ -0,0 +1,89 @@
>> +/* { dg-do run } */
>> +/* { dg-require-effective-target fstack_protector } */
>> +/* { dg-options "-fstack-protector-all -O2" } */
>> +
>> +extern volatile long *stack_chk_guard_ptr;
>> +
>> +volatile long *
>> +get_ptr (void)
>> +{
>> + return stack_chk_guard_ptr;
>> +}
>> +
>> +void __attribute__ ((noipa))
>> +f (void)
>> +{
>> + volatile int x;
>> + x = 1;
>> + x += 1;
>> +}
>> +
>> +#define CHECK(REG) "\tcmp\tx0, " #REG "\n\tbeq\t1f\n"
>> +
>> +asm (
>> +" .pushsection .data\n"
>> +" .align 3\n"
>> +" .globl stack_chk_guard_ptr\n"
>> +"stack_chk_guard_ptr:\n"
>> +#if __ILP32__
>> +" .word __stack_chk_guard\n"
>> +#else
>> +" .xword __stack_chk_guard\n"
>> +#endif
>> +" .weak __stack_chk_guard\n"
>> +"__stack_chk_guard:\n"
>> +" .word 0xdead4321\n"
>> +" .word 0xbeef8765\n"
>> +" .text\n"
>> +" .globl main\n"
>> +" .type main, %function\n"
>> +"main:\n"
>> +" bl get_ptr\n"
>> +" str x0, [sp, #-16]!\n"
>> +" bl f\n"
>> +" str x0, [sp, #8]\n"
>> +" ldr x0, [sp]\n"
>> +#if __ILP32__
>> +" ldr w0, [x0]\n"
>> +#else
>> +" ldr x0, [x0]\n"
>> +#endif
>> + CHECK (x1)
>> + CHECK (x2)
>> + CHECK (x3)
>> + CHECK (x4)
>> + CHECK (x5)
>> + CHECK (x6)
>> + CHECK (x7)
>> + CHECK (x8)
>> + CHECK (x9)
>> + CHECK (x10)
>> + CHECK (x11)
>> + CHECK (x12)
>> + CHECK (x13)
>> + CHECK (x14)
>> + CHECK (x15)
>> + CHECK (x16)
>> + CHECK (x17)
>> + CHECK (x18)
>> + CHECK (x19)
>> + CHECK (x20)
>> + CHECK (x21)
>> + CHECK (x22)
>> + CHECK (x23)
>> + CHECK (x24)
>> + CHECK (x25)
>> + CHECK (x26)
>> + CHECK (x27)
>> + CHECK (x28)
>> + CHECK (x29)
>> + CHECK (x30)
>> +" ldr x1, [sp]\n"
>> + CHECK (x1)
>> +" mov x0, #0\n"
>> +" b exit\n"
>> +"1:\n"
>> +" b abort\n"
>> +" .size main, .-main\n"
>> +" .popsection"
>> +);
>> diff --git a/gcc/testsuite/gcc.target/aarch64/stack-protector-2.c
>> b/gcc/testsuite/gcc.target/aarch64/stack-protector-2.c
>> new file mode 100644
>> index 00000000000..266c36fdbc6
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/aarch64/stack-protector-2.c
>> @@ -0,0 +1,6 @@
>> +/* { dg-do run } */
>> +/* { dg-require-effective-target fstack_protector } */
>> +/* { dg-require-effective-target fpic } */
>> +/* { dg-options "-fstack-protector-all -O2 -fpic" } */
>> +
>> +#include "stack-protector-1.c"
