From a7e73b4158f528600ef97aca29201ddc92b3439f Mon Sep 17 00:00:00 2001 From: Richard Earnshaw Date: Tue, 27 Aug 2019 10:05:51 +0000 Subject: [PATCH] [arm/aarch64] Add comments warning that stack-protector initializer insns shouldn't be split Following the publication of https://kb.cert.org/vuls/id/129209/ I've been having a look at GCC's implementation for Arm and AArch64. I haven't identified any issues yet, but it's a bit early to be completely sure. One observation, however, is that the instruction sequence that initializes the stack canary might be vulnerable to producing a reusable value if it were ever split early. I don't think we ever would, because the memory locations involved with the stack protector are all marked volatile to ensure that the values are only loaded at the point in time when the test is intended to happen, and that also has the effect of making it unlikely that the value would be reused without reloading. Nevertheless, defence in depth is probably warranted here. So this patch just adds some comments warning that the patterns should not be split. * config/arm/arm.md (stack_protect_set_insn): Add security-related comment. * config/aarch64/aarch64.md (stack_protect_set_): Likewise. From-SVN: r274946 --- gcc/ChangeLog | 6 ++++++ gcc/config/aarch64/aarch64.md | 4 +++- gcc/config/arm/arm.md | 6 ++++-- 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 856ce684b79..b7c0fbeaa9d 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,9 @@ +2019-08-27 Richard Earnshaw + + * config/arm/arm.md (stack_protect_set_insn): Add security-related + comment. + * config/aarch64/aarch64.md (stack_protect_set_): Likewise. + 2019-08-27 Martin Liska * cgraph.c (cgraph_node::remove): Remove dead assignment before diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md index 9a07f631010..88e04df6a80 100644 --- a/gcc/config/aarch64/aarch64.md +++ b/gcc/config/aarch64/aarch64.md @@ -7016,13 +7016,15 @@ } [(set_attr "type" "mrs")]) +;; DO NOT SPLIT THIS PATTERN. It is important for security reasons that the +;; canary value does not live beyond the life of this sequence. (define_insn "stack_protect_set_" [(set (match_operand:PTR 0 "memory_operand" "=m") (unspec:PTR [(match_operand:PTR 1 "memory_operand" "m")] UNSPEC_SP_SET)) (set (match_scratch:PTR 2 "=&r") (const_int 0))] "" - "ldr\\t%2, %1\;str\\t%2, %0\;mov\t%2,0" + "ldr\\t%2, %1\;str\\t%2, %0\;mov\t%2, 0" [(set_attr "length" "12") (set_attr "type" "multiple")]) diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md index ed49c4beda1..f138d31f012 100644 --- a/gcc/config/arm/arm.md +++ b/gcc/config/arm/arm.md @@ -8208,6 +8208,8 @@ [(set_attr "arch" "t1,32")] ) +;; DO NOT SPLIT THIS INSN. It's important for security reasons that the +;; canary value does not live beyond the life of this sequence. (define_insn "*stack_protect_set_insn" [(set (match_operand:SI 0 "memory_operand" "=m,m") (unspec:SI [(mem:SI (match_operand:SI 1 "register_operand" "+&l,&r"))] @@ -8215,8 +8217,8 @@ (clobber (match_dup 1))] "" "@ - ldr\\t%1, [%1]\;str\\t%1, %0\;movs\t%1,#0 - ldr\\t%1, [%1]\;str\\t%1, %0\;mov\t%1,#0" + ldr\\t%1, [%1]\;str\\t%1, %0\;movs\t%1, #0 + ldr\\t%1, [%1]\;str\\t%1, %0\;mov\t%1, #0" [(set_attr "length" "8,12") (set_attr "conds" "clob,nocond") (set_attr "type" "multiple") -- 2.30.2