procyberian/gcc
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

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

Browse source 
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
This commit is contained in:
Richard Earnshaw 2019-08-27 10:05:51 +00:00 • committed by Richard Earnshaw
parent 72bb85f8d1
commit a7e73b4158
3 changed files with 13 additions and 3 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

6
gcc/ChangeLog
View file

@ -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

4
gcc/config/aarch64/aarch64.md
View file

@ -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")])

6
gcc/config/arm/arm.md
View file

@ -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")