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?
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"
