| Message ID | CAJK_mQ0kCcmwPkkPqsLszGW7rBxcCG+1Vn5KE6JXKNYj2fajXQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Nov 19, 2013 at 04:30:21PM +0530, Venkataramanan Kumar wrote: > This is RFC patch that adds machine descriptions to support stack > smashing protection in AArch64. Most of the testsuite changes look wrong. The fact that aarch64 gets stack protector support doesn't mean all other targets do as well. So please leave all the changes that remove native or stack_protector requirements out. Jakub
On Tue, 19 Nov 2013, Jakub Jelinek wrote: > On Tue, Nov 19, 2013 at 04:30:21PM +0530, Venkataramanan Kumar wrote: > > This is RFC patch that adds machine descriptions to support stack > > smashing protection in AArch64. > > Most of the testsuite changes look wrong. The fact that aarch64 > gets stack protector support doesn't mean all other targets do as well. > So please leave all the changes that remove native or stack_protector > requirements out. The tests for "target native" look wrong for me ("native" conditionals only make sense for special cases such as guality tests that expect to exec another tool on the host) - so I think they should be removed, but that removal needs submitting as a separate patch. I would like to see a clear description of what happens with all eight combinations of (glibc version does or doesn't support this, GCC building glibc does or doesn't support this, GCC building user program does or doesn't support this). Which of the (GCC building glibc, glibc) combinations will successfully build glibc? Will all such glibcs be binary-compatible? Will both old and new GCC work for building user programs with both old and new glibc?
Hi Joseph, On 19 November 2013 21:53, Joseph S. Myers <joseph@codesourcery.com> wrote: > On Tue, 19 Nov 2013, Jakub Jelinek wrote: > >> On Tue, Nov 19, 2013 at 04:30:21PM +0530, Venkataramanan Kumar wrote: >> > This is RFC patch that adds machine descriptions to support stack >> > smashing protection in AArch64. >> >> Most of the testsuite changes look wrong. The fact that aarch64 >> gets stack protector support doesn't mean all other targets do as well. >> So please leave all the changes that remove native or stack_protector >> requirements out. > > The tests for "target native" look wrong for me ("native" conditionals > only make sense for special cases such as guality tests that expect to > exec another tool on the host) - so I think they should be removed, but > that removal needs submitting as a separate patch. > Yes apologies for that, I will send another patch. > I would like to see a clear description of what happens with all eight > combinations of (glibc version does or doesn't support this, GCC building > glibc does or doesn't support this, GCC building user program does or > doesn't support this). Which of the (GCC building glibc, glibc) > combinations will successfully build glibc? Will all such glibcs be > binary-compatible? Will both old and new GCC work for building user > programs with both old and new glibc? Can you please clarify why we need to consider "the gcc compiler that builds the glibc" in the combinations you want me to describe. I am not able to understand that. I tested the below three GCC compiler versions for building user programs. 1) GCC trunk (without my patch) generates code for "stack protection set/test" when -fstack-protector-all is enabled. This is based on global variable "__stack_chk_guard" access, which glibc supports from version 2.4. -----snip----- adrp x0, __stack_chk_guard add x0, x0, :lo12:__stack_chk_guard ldr x0, [x0] str x0, [x29,24] -----snip----- GCC has generic code emitted for stack protection, enabled for targets where frame growth is downwards. The patch for frame growing downwards in Aarch64 is recently committed in trunk. 2) Now with the patch, GCC compiler will generate TLS based access. ----snip----- mrs x0, tpidr_el0 ldr x1, [x0,-8] str x1, [x29,24] -----snip----- I need to check if target glibc for Aarch64 supports TLS based ssp support. Currently some targets check and generate TLS based access when glibc version is >=2.4. 3) GCC linaro compiler packaged with open embedded image and which I boot in V8 foundation model as I don't have access to Aarch64 hardware yet. It will not emit any stack protection code. test.c:1:0: warning: -fstack-protector not supported for this target [enabled by default] regards, Venkat.
On Wed, 20 Nov 2013, Venkataramanan Kumar wrote: > > I would like to see a clear description of what happens with all eight > > combinations of (glibc version does or doesn't support this, GCC building > > glibc does or doesn't support this, GCC building user program does or > > doesn't support this). Which of the (GCC building glibc, glibc) > > combinations will successfully build glibc? Will all such glibcs be > > binary-compatible? Will both old and new GCC work for building user > > programs with both old and new glibc? > > Can you please clarify why we need to consider "the gcc compiler that > builds the glibc" in the combinations you want me to describe. I am > not able to understand that. Let's imagine this support goes in GCC 4.9 and the glibc support goes in glibc 2.19, whereas GCC 4.8 and glibc 2.18 are versions without this support. * Building glibc 2.18 with GCC 4.8 already works (I presume). * Suppose you use GCC 4.9 to build glibc 2.18. Does this work? If it works, it must produce a glibc binary that's ABI compatible with one built with GCC 4.8, meaning same symbols exported at same symbol versions. * Suppose you build glibc 2.19 with GCC 4.8. Does this work? If it does, then it must be ABI compatible with 2.18 (meaning the symbols exported at GLIBC_2.18 or earlier versions must be exactly the same as exported at those versions in 2.18). * Suppose you build glibc 2.19 with GCC 4.9. This needs to work and must again produce a binary compatible with the previous ones. Note: there is no current glibc support for architectures that gained TLS-based stack guards after 2.4 (meaning they need both a TLS-based scheme and backwards compatibility for binaries using __stack_chk_guard), and your glibc patch doesn't seem to add any. It looks to me like your glibc patch would have removed the __stack_chk_guard symbol and so failed ABI tests (this symbol being defined only if THREAD_SET_STACK_GUARD isn't defined) - you didn't make clear if your patch was tested with the glibc testsuite including passing the ABI tests. The normal presumption is that it's not acceptable to remove exported symbols from glibc as some application might be using them. You need to ensure that, when new glibc is built, whatever compiler it's built with, it continues to export the __stack_chk_guard symbol at version GLIBC_2.17. Furthermore, if any released GCC version would generate references to __stack_chk_guard when compiling code for AArch64 with stack protection, you need to ensure __stack_chk_guard is a normal symbol not a compat symbol so that people can continue to link against new glibc when using old GCC. If it's only a limited range of development versions of GCC that could have generated code using __stack_chk_guard because released versions didn't support stack protection on AArch64 at all, a compat symbol would probably be OK (but you should still ensure that the variable gets initialized with the correct value for any applications built with those development versions of GCC).
Hi Joseph, Thank you for the detail explanation. > You need to ensure that, when new glibc is built, whatever compiler it's > built with, it continues to export the __stack_chk_guard symbol at version > GLIBC_2.17. Furthermore, if any released GCC version would generate > references to __stack_chk_guard when compiling code for AArch64 with stack > protection, you need to ensure __stack_chk_guard is a normal symbol not a > compat symbol so that people can continue to link against new glibc when > using old GCC. If it's only a limited range of development versions of > GCC that could have generated code using __stack_chk_guard because > released versions didn't support stack protection on AArch64 at all, a > compat symbol would probably be OK (but you should still ensure that the > variable gets initialized with the correct value for any applications > built with those development versions of GCC). As you said when THREAD_SET_STACK_GUARD is set glibc does not export __stack_chk_guard. So I am planning to change the export logic by adding a new macro EXPORT_GLOBAL_STACK_GUARD and set it for Aarch64 port in glibc. ----snip---- --- a/csu/libc-start.c +++ b/csu/libc-start.c -# ifndef THREAD_SET_STACK_GUARD + +#if !defined(THREAD_SET_STACK_GUARD) || defined(EXPORT_GLOBAL_STACK_GUARD) /* Only exported for architectures that don't store the stack guard canary in thread local area. */ uintptr_t __stack_chk_guard attribute_relro; -# endif +#endif + ----snip---- I will find a better way to version that symbol as well. I will sent a RFC patch to glibc mailing list. On the GCC side, trunk GCC configure script checks and sets TARGET_LIBC_PROVIDES_SSP support when glibc is >=2.4 -----snip---- # Test for stack protector support in target C library. { $as_echo "$as_me:${as_lineno-$LINENO}: checking __stack_chk_fail in target C library" >&5 $as_echo_n "checking __stack_chk_fail in target C library... " >&6; } if test "${gcc_cv_libc_provides_ssp+set}" = set; then : $as_echo_n "(cached) " >&6 else gcc_cv_libc_provides_ssp=no case "$target" in *-*-linux* | *-*-kfreebsd*-gnu | *-*-knetbsd*-gnu) # glibc 2.4 and later provides __stack_chk_fail and # either __stack_chk_guard, or TLS access to stack guard canary. if test $glibc_version_major -gt 2 \ || ( test $glibc_version_major -eq 2 && test $glibc_version_minor -ge 4 ); then : gcc_cv_libc_provides_ssp=yes if test x$gcc_cv_libc_provides_ssp = xyes; then $as_echo "#define TARGET_LIBC_PROVIDES_SSP 1" >>confdefs.h fi ----snip----- To make GCC for AArch64 generate TLS based stack access for glibc >= 2.19 I need to introduce a new macro TARGET_LIBC_PROVIDES_TLS_SSP and check and set it for glibc >= 2.19 in GCC configure . Any better approach to this since it is specific to Aarch64? regards, Venkat. On 20 November 2013 22:38, Joseph S. Myers <joseph@codesourcery.com> wrote: > On Wed, 20 Nov 2013, Venkataramanan Kumar wrote: > >> > I would like to see a clear description of what happens with all eight >> > combinations of (glibc version does or doesn't support this, GCC building >> > glibc does or doesn't support this, GCC building user program does or >> > doesn't support this). Which of the (GCC building glibc, glibc) >> > combinations will successfully build glibc? Will all such glibcs be >> > binary-compatible? Will both old and new GCC work for building user >> > programs with both old and new glibc? >> >> Can you please clarify why we need to consider "the gcc compiler that >> builds the glibc" in the combinations you want me to describe. I am >> not able to understand that. > > Let's imagine this support goes in GCC 4.9 and the glibc support goes in > glibc 2.19, whereas GCC 4.8 and glibc 2.18 are versions without this > support. > > * Building glibc 2.18 with GCC 4.8 already works (I presume). > > * Suppose you use GCC 4.9 to build glibc 2.18. Does this work? If it > works, it must produce a glibc binary that's ABI compatible with one built > with GCC 4.8, meaning same symbols exported at same symbol versions. > > * Suppose you build glibc 2.19 with GCC 4.8. Does this work? If it does, > then it must be ABI compatible with 2.18 (meaning the symbols exported at > GLIBC_2.18 or earlier versions must be exactly the same as exported at > those versions in 2.18). > > * Suppose you build glibc 2.19 with GCC 4.9. This needs to work and must > again produce a binary compatible with the previous ones. > > Note: there is no current glibc support for architectures that gained > TLS-based stack guards after 2.4 (meaning they need both a TLS-based > scheme and backwards compatibility for binaries using __stack_chk_guard), > and your glibc patch doesn't seem to add any. It looks to me like your > glibc patch would have removed the __stack_chk_guard symbol and so failed > ABI tests (this symbol being defined only if THREAD_SET_STACK_GUARD isn't > defined) - you didn't make clear if your patch was tested with the glibc > testsuite including passing the ABI tests. The normal presumption is that > it's not acceptable to remove exported symbols from glibc as some > application might be using them. > > You need to ensure that, when new glibc is built, whatever compiler it's > built with, it continues to export the __stack_chk_guard symbol at version > GLIBC_2.17. Furthermore, if any released GCC version would generate > references to __stack_chk_guard when compiling code for AArch64 with stack > protection, you need to ensure __stack_chk_guard is a normal symbol not a > compat symbol so that people can continue to link against new glibc when > using old GCC. If it's only a limited range of development versions of > GCC that could have generated code using __stack_chk_guard because > released versions didn't support stack protection on AArch64 at all, a > compat symbol would probably be OK (but you should still ensure that the > variable gets initialized with the correct value for any applications > built with those development versions of GCC). > > -- > Joseph S. Myers > joseph@codesourcery.com
Index: gcc/testsuite/gcc.dg/pr46440.c =================================================================== --- gcc/testsuite/gcc.dg/pr46440.c (revision 204932) +++ gcc/testsuite/gcc.dg/pr46440.c (working copy) @@ -1,7 +1,6 @@ /* PR rtl-optimization/46440 */ /* { dg-do compile } */ /* { dg-options "-O -fstack-protector -fno-tree-dominator-opts -fno-tree-fre" } */ -/* { dg-require-effective-target fstack_protector } */ int i; Index: gcc/testsuite/gcc.dg/ssp-1.c =================================================================== --- gcc/testsuite/gcc.dg/ssp-1.c (revision 204932) +++ gcc/testsuite/gcc.dg/ssp-1.c (working copy) @@ -1,6 +1,4 @@ -/* { dg-do run { target native } } */ /* { dg-options "-fstack-protector" } */ -/* { dg-require-effective-target fstack_protector } */ #include <stdlib.h> Index: gcc/testsuite/gcc.dg/pr47766.c =================================================================== --- gcc/testsuite/gcc.dg/pr47766.c (revision 204932) +++ gcc/testsuite/gcc.dg/pr47766.c (working copy) @@ -1,6 +1,5 @@ /* { dg-do compile } */ /* { dg-options "-O2 -fstack-protector" } */ -/* { dg-require-effective-target fstack_protector } */ int parse_opt (int key) Index: gcc/testsuite/gcc.dg/ssp-2.c =================================================================== --- gcc/testsuite/gcc.dg/ssp-2.c (revision 204932) +++ gcc/testsuite/gcc.dg/ssp-2.c (working copy) @@ -1,7 +1,5 @@ -/* { dg-do run { target native } } */ /* { dg-options "-fstack-protector" } */ /* { dg-options "-fstack-protector -Wl,-multiply_defined,suppress" { target *-*-darwin* } } */ -/* { dg-require-effective-target fstack_protector } */ #include <stdlib.h> Index: gcc/testsuite/gcc.dg/fstack-protector-strong.c =================================================================== --- gcc/testsuite/gcc.dg/fstack-protector-strong.c (revision 204932) +++ gcc/testsuite/gcc.dg/fstack-protector-strong.c (working copy) @@ -1,6 +1,6 @@ /* Test that stack protection is done on chosen functions. */ -/* { dg-do compile { target i?86-*-* x86_64-*-* rs6000-*-* s390x-*-* } } */ +/* { dg-do compile { target i?86-*-* x86_64-*-* rs6000-*-* s390x-*-* aarch64-*-*} } */ /* { dg-options "-O2 -fstack-protector-strong" } */ #include<string.h> Index: gcc/testsuite/g++.dg/fstack-protector-strong.C =================================================================== --- gcc/testsuite/g++.dg/fstack-protector-strong.C (revision 204932) +++ gcc/testsuite/g++.dg/fstack-protector-strong.C (working copy) @@ -1,6 +1,6 @@ /* Test that stack protection is done on chosen functions. */ -/* { dg-do compile { target i?86-*-* x86_64-*-* } } */ +/* { dg-do compile { target i?86-*-* x86_64-*-* aarch64-*-* } } */ /* { dg-options "-O2 -fstack-protector-strong" } */ class A Index: gcc/config/aarch64/aarch64-linux.h =================================================================== --- gcc/config/aarch64/aarch64-linux.h (revision 204932) +++ gcc/config/aarch64/aarch64-linux.h (working copy) @@ -43,4 +43,9 @@ } \ while (0) +#ifdef TARGET_LIBC_PROVIDES_SSP +/* Aarch64 glibc provides __stack_chk_guard in [tp - 0x8]. */ +#define TARGET_THREAD_SSP_OFFSET (-1 * GET_MODE_SIZE (ptr_mode)) +#endif + #endif /* GCC_AARCH64_LINUX_H */ Index: gcc/config/aarch64/aarch64.md =================================================================== --- gcc/config/aarch64/aarch64.md (revision 204932) +++ gcc/config/aarch64/aarch64.md (working copy) @@ -99,6 +99,10 @@ UNSPEC_TLSDESC UNSPEC_USHL_2S UNSPEC_VSTRUCTDUMMY + UNSPEC_SP_SET + UNSPEC_SP_TEST + UNSPEC_SP_TLS_SET + UNSPEC_SP_TLS_TEST ]) (define_c_enum "unspecv" [ @@ -320,6 +324,7 @@ (include "../arm/cortex-a53.md") (include "../arm/cortex-a15.md") + ;; ------------------------------------------------------------------- ;; Jumps and other miscellaneous insns ;; ------------------------------------------------------------------- @@ -4181,6 +4186,82 @@ DONE; }) +;; Named patterns for stack smashing protection + +(define_expand "stack_protect_set" + [(match_operand 0 "memory_operand") + (match_operand 1 "memory_operand")] + "" +{ + enum machine_mode mode = GET_MODE (operands[0]); + +#ifdef TARGET_THREAD_SSP_OFFSET + rtx tlsreg = gen_reg_rtx (Pmode); + emit_insn (gen_aarch64_load_tp_hard (tlsreg)); + rtx addr = gen_rtx_PLUS (Pmode, tlsreg, GEN_INT (TARGET_THREAD_SSP_OFFSET)); + operands[1] = gen_rtx_MEM (Pmode, addr); +#endif + + emit_insn ((mode == DImode + ? gen_stack_protect_set_di + : gen_stack_protect_set_si) (operands[0], operands[1])); + DONE; +}) + +(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%x2, %1\;str\\t%x2, %0\;mov\t%x2,0" + [(set_attr "length" "12")]) + + +(define_expand "stack_protect_test" + [(match_operand 0 "memory_operand") + (match_operand 1 "memory_operand") + (match_operand 2)] + "" +{ + +#ifdef TARGET_THREAD_SSP_OFFSET + rtx tlsreg = gen_reg_rtx (Pmode); + emit_insn (gen_aarch64_load_tp_hard (tlsreg)); + rtx addr = gen_rtx_PLUS (Pmode, tlsreg, GEN_INT (TARGET_THREAD_SSP_OFFSET)); + operands[1] = gen_rtx_MEM (Pmode, addr); +#endif + + rtx result = gen_reg_rtx (Pmode); + + enum machine_mode mode = GET_MODE (operands[0]); + + 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])); + DONE; +}) + +(define_insn "stack_protect_test_<mode>" + [(set (match_operand:PTR 0 "register_operand") + (unspec:PTR [(match_operand:PTR 1 "memory_operand" "m") + (match_operand:PTR 2 "memory_operand" "m")] + UNSPEC_SP_TEST)) + (clobber (match_scratch:PTR 3 "=&r"))] + "" + "ldr\t%x3, %x1\;ldr\t%x0, %x2\;eor\t%x0, %x3, %x0" + [(set_attr "length" "12")]) + + ;; AdvSIMD Stuff (include "aarch64-simd.md")
Hi Maintainers, This is RFC patch that adds machine descriptions to support stack smashing protection in AArch64. I have written a very simple patch that prints "stack set" and "stack test" as template of instructions. I had 2 assumptions. 1) For "stack_protect_set" and "stack_protect_test", I used "memory_operand" as predicate. GCC pushes the memory operand in a register much earlier during expand phase before these patterns are invoked. So assuming that I will get a memory operand "__stack_chk_gaurd" in a register when we are not using TLS based stack guard. 2) For the TLS case, assuming stack guard value will be stored at "-8" offset from "tp" GCC generates below code for stack set. mrs x0, tpidr_el0 ldr x1, [x0,-8] str x1, [x29,24] mov x1,0 I submitted Glibc patches some time before https://sourceware.org/ml/libc-ports/2013-08/msg00044.html. There are few regressions, the pthread_cancel tests in glibc fails I am currently debugging :(. GCC with the patch generates below code for stack test ldr x1, [x29,24] ldr x0, [x0,-8] eor x0, x1, x0 cbnz x0, .L4 ................................. ...................................... .L4: bl __stack_chk_f I generate "eor" since it has 2 purpose one for checking equality, and two for clearing the canary loaded register. Request your feedback to shape this into a better patch. regards, Venkat.