[arm/aarch64] Add comments warning that stack-protector initializer insns shouldn...

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_<mode>): Likewise.

From-SVN: r274946

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 856ce684b79d9821e6d19ded40e386586b56fd45..b7c0fbeaa9de6edf7e3f53f7b62362069a176a19 100644 (file)
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,9 @@
+2019-08-27  Richard Earnshaw  <rearnsha@arm.com>
+
+       * config/arm/arm.md (stack_protect_set_insn): Add security-related
+       comment.
+       * config/aarch64/aarch64.md (stack_protect_set_<mode>): Likewise.
+
 2019-08-27  Martin Liska  <mliska@suse.cz>
 
        * cgraph.c (cgraph_node::remove): Remove dead assignment before

diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index 9a07f631010f16edc82fa6d990e61336196f17ec..88e04df6a807a290d9c245c31677717c51be0cb4 100644 (file)
--- 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_<mode>"
   [(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%<w>2, %1\;str\\t%<w>2, %0\;mov\t%<w>2,0"
+  "ldr\\t%<w>2, %1\;str\\t%<w>2, %0\;mov\t%<w>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 ed49c4beda138633a84b58fe345cf5ba99103ab7..f138d31f01293162fe0713b6bead45d1b7ab0e67 100644 (file)
--- 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")