diff mbox series

[arm,aarch64] Add comments warning that stack-protector initializer insns shouldn't be split

Message ID cea277b6-a7be-49d9-8535-e18072860e07@arm.com
State New
Headers show
Series [arm,aarch64] Add comments warning that stack-protector initializer insns shouldn't be split | expand

Commit Message

Richard Earnshaw (lists) Aug. 27, 2019, 10:07 a.m. UTC 
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.

committed to trunk.
diff mbox series

Patch

Index: gcc/config/aarch64/aarch64.md
===================================================================
--- gcc/config/aarch64/aarch64.md	(revision 274945)
+++ gcc/config/aarch64/aarch64.md	(working copy)
@@ -7016,6 +7016,8 @@ 
  }
  [(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")]
@@ -7022,7 +7024,7 @@ 
 	 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")])
 
Index: gcc/config/arm/arm.md
===================================================================
--- gcc/config/arm/arm.md	(revision 274945)
+++ gcc/config/arm/arm.md	(working copy)
@@ -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")