public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 03/14 v7] Do not stack-protect ifunc resolvers.
  2016-06-07 11:06 --enable-stack-protector for glibc, v7 Nix
@ 2016-06-07 11:06 ` Nix
  2016-06-24 13:21   ` Florian Weimer
  2016-06-07 11:06 ` [PATCH 08/14 v6] Work even with compilers hacked to enable -fstack-protector by default Nix
                   ` (13 subsequent siblings)
  14 siblings, 1 reply; 44+ messages in thread
From: Nix @ 2016-06-07 11:06 UTC (permalink / raw)
  To: libc-alpha; +Cc: fweimer, Nick Alcock

From: Nick Alcock <nick.alcock@oracle.com>

When dynamically linking, ifunc resolvers are called before TLS is
initialized, so they cannot be safely stack-protected.

We avoid disabling stack-protection on large numbers of files by
using __attribute__ ((__optimize__ ("-fno-stack-protector")))
to turn it off just for the resolvers themselves.  (We provide
the attribute even when statically linking, because we will later
use it elsewhere too.)

v4: New.
v5: Comment fix.
v6: Don't check for __attribute__((__optimize__)).
v7: Tiny context adjustments for revisions in earlier patches.

	* config.h.in (HAVE_CC_NO_STACK_PROTECTOR): New macro.
	* include/libc-symbols.h (inhibit_stack_protector): New macro.
	(libc_ifunc): Use it.
	(libm_ifunc): Likewise.
	* elf/ifuncdep2.c (foo1_ifunc): Add inhibit_stack_protector.
	(foo2_ifunc): Likewise.
	(foo3_ifunc): Likewise.
	* elf/ifuncmain6pie.c (foo_ifunc): Likewise.
	* elf/ifuncmain7.c (foo_ifunc): Likewise.
	* elf/ifuncmod1.c (foo_ifunc): Likewise.
	(foo_hidden_ifunc): Likewise.
	(foo_protected_ifunc): Likewise.
	* elf/ifuncmod5.c (foo_ifunc): Likewise.
	(foo_hidden_ifunc): Likewise.
	(foo_protected_ifunc): Likewise.
	* sysdeps/x86_64/ifuncmod8.c (foo_ifunc): Likewise.
	* nptl/pt-fork.c (fork_resolve): Likewise.
	* nptl/pt-longjmp.c (longjmp_resolve): Likewise.
	* nptl/pt-system.c (system_resolve): Likewise.
	* nptl/pt-vfork.c (vfork_resolve): Likewise.
	* rt/clock-compat.c [HAVE_IFUNC] (COMPAT_REDIRECT): Likewise.
	* sysdeps/generic/ifunc-sel.h (ifunc_sel): Likewise.
	(ifunc_one): Likewise.
	* sysdeps/nacl/nacl_interface_query.c [SHARED]
	(nacl_interface_query_ifunc): Likewise.
	* sysdeps/powerpc/ifunc-sel.h (ifunc_sel): Likewise.
	(ifunc_one): Likewise.
	* sysdeps/unix/make-syscalls.sh: Likewise.
	* sysdeps/unix/sysv/linux/powerpc/gettimeofday.c
	(gettimeofday_ifunc): Likewise.
	* sysdeps/unix/sysv/linux/powerpc/time.c (time_ifunc): Likewise.
	* sysdeps/unix/sysv/linux/x86/gettimeofday.c (gettimeofday_ifunc):
	Likewise.
	* sysdeps/unix/sysv/linux/x86/time.c (time_ifunc): Likewise.
	* sysdeps/unix/sysv/linux/x86_64/x32/getcpu.c (getcpu_ifunc):
	Likewise.
---
 config.h.in                                    |  4 ++++
 configure.ac                                   |  1 +
 elf/ifuncdep2.c                                |  3 +++
 elf/ifuncmain6pie.c                            |  1 +
 elf/ifuncmain7.c                               |  1 +
 elf/ifuncmod1.c                                |  3 +++
 elf/ifuncmod5.c                                |  3 +++
 include/libc-symbols.h                         | 18 ++++++++++++++++--
 nptl/pt-fork.c                                 |  1 +
 nptl/pt-longjmp.c                              |  1 +
 nptl/pt-system.c                               |  1 +
 nptl/pt-vfork.c                                |  1 +
 rt/clock-compat.c                              |  4 +++-
 sysdeps/generic/ifunc-sel.h                    |  2 ++
 sysdeps/nacl/nacl_interface_query.c            |  1 +
 sysdeps/powerpc/ifunc-sel.h                    |  2 ++
 sysdeps/unix/make-syscalls.sh                  |  1 +
 sysdeps/unix/sysv/linux/powerpc/gettimeofday.c |  1 +
 sysdeps/unix/sysv/linux/powerpc/time.c         |  1 +
 sysdeps/unix/sysv/linux/x86/gettimeofday.c     |  1 +
 sysdeps/unix/sysv/linux/x86/time.c             |  1 +
 sysdeps/unix/sysv/linux/x86_64/x32/getcpu.c    |  1 +
 sysdeps/x86_64/ifuncmod8.c                     |  1 +
 23 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/config.h.in b/config.h.in
index 990d5e1..612e18e 100644
--- a/config.h.in
+++ b/config.h.in
@@ -43,6 +43,10 @@
 /* Define if compiler accepts -ftree-loop-distribute-patterns.  */
 #undef  HAVE_CC_INHIBIT_LOOP_TO_LIBCALL
 
+/* Define if compiler accepts -fno-stack-protector in an
+   __attribute__((__optimize__)).  */
+#undef	HAVE_CC_NO_STACK_PROTECTOR
+
 /* The level of stack protection in use for glibc as a whole.  */
 #undef	STACK_PROTECTOR_LEVEL
 
diff --git a/configure.ac b/configure.ac
index 11982fd..389044c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -636,6 +636,7 @@ stack_protector=
 no_stack_protector=
 if test "$libc_cv_ssp" = yes; then
   no_stack_protector="-fno-stack-protector"
+  AC_DEFINE(HAVE_CC_NO_STACK_PROTECTOR)
 fi
 
 if test "$enable_stack_protector" = yes && test "$libc_cv_ssp" = yes; then
diff --git a/elf/ifuncdep2.c b/elf/ifuncdep2.c
index 6e66d31..d87d61d 100644
--- a/elf/ifuncdep2.c
+++ b/elf/ifuncdep2.c
@@ -32,6 +32,7 @@ void * foo1_ifunc (void) __asm__ ("foo1");
 __asm__(".type foo1, %gnu_indirect_function");
 
 void *
+inhibit_stack_protector
 foo1_ifunc (void)
 {
   return ifunc_sel (one, minus_one, zero);
@@ -41,6 +42,7 @@ void * foo2_ifunc (void) __asm__ ("foo2");
 __asm__(".type foo2, %gnu_indirect_function");
 
 void *
+inhibit_stack_protector
 foo2_ifunc (void)
 {
   return ifunc_sel (minus_one, one, zero);
@@ -50,6 +52,7 @@ void * foo3_ifunc (void) __asm__ ("foo3");
 __asm__(".type foo3, %gnu_indirect_function");
 
 void *
+inhibit_stack_protector
 foo3_ifunc (void)
 {
   return ifunc_sel (one, zero, minus_one);
diff --git a/elf/ifuncmain6pie.c b/elf/ifuncmain6pie.c
index 8478d4c..04faeb8 100644
--- a/elf/ifuncmain6pie.c
+++ b/elf/ifuncmain6pie.c
@@ -21,6 +21,7 @@ void * foo_ifunc (void) __asm__ ("foo");
 __asm__(".type foo, %gnu_indirect_function");
 
 void *
+inhibit_stack_protector
 foo_ifunc (void)
 {
   return ifunc_one (one);
diff --git a/elf/ifuncmain7.c b/elf/ifuncmain7.c
index 617a596..1e8f7ea 100644
--- a/elf/ifuncmain7.c
+++ b/elf/ifuncmain7.c
@@ -20,6 +20,7 @@ __asm__(".type foo, %gnu_indirect_function");
 
 static void *
 __attribute__ ((used))
+inhibit_stack_protector
 foo_ifunc (void)
 {
   return ifunc_one (one);
diff --git a/elf/ifuncmod1.c b/elf/ifuncmod1.c
index 0b61380..f0bf5fb 100644
--- a/elf/ifuncmod1.c
+++ b/elf/ifuncmod1.c
@@ -36,6 +36,7 @@ void * foo_ifunc (void) __asm__ ("foo");
 __asm__(".type foo, %gnu_indirect_function");
 
 void *
+inhibit_stack_protector
 foo_ifunc (void)
 {
   return ifunc_sel (one, minus_one, zero);
@@ -45,6 +46,7 @@ void * foo_hidden_ifunc (void) __asm__ ("foo_hidden");
 __asm__(".type foo_hidden, %gnu_indirect_function");
 
 void *
+inhibit_stack_protector
 foo_hidden_ifunc (void)
 {
   return ifunc_sel (minus_one, one, zero);
@@ -54,6 +56,7 @@ void * foo_protected_ifunc (void) __asm__ ("foo_protected");
 __asm__(".type foo_protected, %gnu_indirect_function");
 
 void *
+inhibit_stack_protector
 foo_protected_ifunc (void)
 {
   return ifunc_sel (one, zero, minus_one);
diff --git a/elf/ifuncmod5.c b/elf/ifuncmod5.c
index 0e65a63..5a95780 100644
--- a/elf/ifuncmod5.c
+++ b/elf/ifuncmod5.c
@@ -31,6 +31,7 @@ void * foo_ifunc (void) __asm__ ("foo");
 __asm__(".type foo, %gnu_indirect_function");
 
 void *
+inhibit_stack_protector
 foo_ifunc (void)
 {
   return ifunc_sel (one, minus_one, zero);
@@ -40,6 +41,7 @@ void * foo_hidden_ifunc (void) __asm__ ("foo_hidden");
 __asm__(".type foo_hidden, %gnu_indirect_function");
 
 void *
+inhibit_stack_protector
 foo_hidden_ifunc (void)
 {
   return ifunc_sel (minus_one, one, zero);
@@ -49,6 +51,7 @@ void * foo_protected_ifunc (void) __asm__ ("foo_protected");
 __asm__(".type foo_protected, %gnu_indirect_function");
 
 void *
+inhibit_stack_protector
 foo_protected_ifunc (void)
 {
   return ifunc_sel (one, zero, minus_one);
diff --git a/include/libc-symbols.h b/include/libc-symbols.h
index 4548e09..531092c 100644
--- a/include/libc-symbols.h
+++ b/include/libc-symbols.h
@@ -314,6 +314,16 @@ for linking")
 
 #define attribute_relro __attribute__ ((section (".data.rel.ro")))
 
+
+/* Used to disable stack protection in sensitive places, like ifunc
+   resolvers and early static TLS init.  */
+#ifdef HAVE_CC_NO_STACK_PROTECTOR
+# define inhibit_stack_protector \
+    __attribute__ ((__optimize__ ("-fno-stack-protector")))
+#else
+# define inhibit_stack_protector
+#endif
+
 /* The following macros are used for PLT bypassing within libc.so
    (and if needed other libraries similarly).
    First of all, you need to have the function prototyped somewhere,
@@ -716,7 +726,9 @@ for linking")
 /* Marker used for indirection function symbols.  */
 #define libc_ifunc(name, expr)						\
   extern void *name##_ifunc (void) __asm__ (#name);			\
-  void *name##_ifunc (void)						\
+  void *								\
+  inhibit_stack_protector						\
+  name##_ifunc (void)							\
   {									\
     INIT_ARCH ();							\
     __typeof (name) *res = expr;					\
@@ -728,7 +740,9 @@ for linking")
    which will, if necessary, initialize the data first.  */
 #define libm_ifunc(name, expr)						\
   extern void *name##_ifunc (void) __asm__ (#name);			\
-  void *name##_ifunc (void)						\
+  void *								\
+  inhibit_stack_protector						\
+  name##_ifunc (void)							\
   {									\
     __typeof (name) *res = expr;					\
     return res;								\
diff --git a/nptl/pt-fork.c b/nptl/pt-fork.c
index b65d6b4..4178af8 100644
--- a/nptl/pt-fork.c
+++ b/nptl/pt-fork.c
@@ -34,6 +34,7 @@
 
 static __typeof (fork) *
 __attribute__ ((used))
+inhibit_stack_protector
 fork_resolve (void)
 {
   return &__libc_fork;
diff --git a/nptl/pt-longjmp.c b/nptl/pt-longjmp.c
index a1cc286..8a33cb4 100644
--- a/nptl/pt-longjmp.c
+++ b/nptl/pt-longjmp.c
@@ -34,6 +34,7 @@
 
 static __typeof (longjmp) *
 __attribute__ ((used))
+inhibit_stack_protector
 longjmp_resolve (void)
 {
   return &__libc_longjmp;
diff --git a/nptl/pt-system.c b/nptl/pt-system.c
index 56f2a89..a481ab3 100644
--- a/nptl/pt-system.c
+++ b/nptl/pt-system.c
@@ -34,6 +34,7 @@
 
 static __typeof (system) *
 __attribute__ ((used))
+inhibit_stack_protector
 system_resolve (void)
 {
   return &__libc_system;
diff --git a/nptl/pt-vfork.c b/nptl/pt-vfork.c
index 8f4be0c..8f3c2c0 100644
--- a/nptl/pt-vfork.c
+++ b/nptl/pt-vfork.c
@@ -48,6 +48,7 @@ extern __typeof (vfork) __libc_vfork;   /* Defined in libc.  */
 
 static __typeof (vfork) *
 __attribute__ ((used))
+inhibit_stack_protector
 vfork_resolve (void)
 {
   return &__libc_vfork;
diff --git a/rt/clock-compat.c b/rt/clock-compat.c
index dc69e4a..b0fdd8b 100644
--- a/rt/clock-compat.c
+++ b/rt/clock-compat.c
@@ -30,7 +30,9 @@
 #if HAVE_IFUNC
 # define COMPAT_REDIRECT(name, proto, arglist)				      \
   __typeof (name) *name##_ifunc (void) asm (#name);			      \
-  __typeof (name) *name##_ifunc (void)					      \
+  __typeof (name) *							      \
+  inhibit_stack_protector						      \
+  name##_ifunc (void)							      \
   {									      \
     return &__##name;							      \
   }									      \
diff --git a/sysdeps/generic/ifunc-sel.h b/sysdeps/generic/ifunc-sel.h
index 6a27b69..1fff405 100644
--- a/sysdeps/generic/ifunc-sel.h
+++ b/sysdeps/generic/ifunc-sel.h
@@ -5,6 +5,7 @@
 extern int global;
 
 static inline void *
+inhibit_stack_protector
 ifunc_sel (int (*f1) (void), int (*f2) (void), int (*f3) (void))
 {
  switch (global)
@@ -19,6 +20,7 @@ ifunc_sel (int (*f1) (void), int (*f2) (void), int (*f3) (void))
 }
 
 static inline void *
+inhibit_stack_protector
 ifunc_one (int (*f1) (void))
 {
   return f1;
diff --git a/sysdeps/nacl/nacl_interface_query.c b/sysdeps/nacl/nacl_interface_query.c
index adf1dd4..dbaa88b 100644
--- a/sysdeps/nacl/nacl_interface_query.c
+++ b/sysdeps/nacl/nacl_interface_query.c
@@ -29,6 +29,7 @@ extern TYPE_nacl_irt_query nacl_interface_query_ifunc (void)
   asm ("nacl_interface_query");
 
 TYPE_nacl_irt_query
+inhibit_stack_protector
 nacl_interface_query_ifunc (void)
 {
   return &__nacl_irt_query;
diff --git a/sysdeps/powerpc/ifunc-sel.h b/sysdeps/powerpc/ifunc-sel.h
index 526d8ed..598b125 100644
--- a/sysdeps/powerpc/ifunc-sel.h
+++ b/sysdeps/powerpc/ifunc-sel.h
@@ -5,6 +5,7 @@
 extern int global;
 
 static inline void *
+inhibit_stack_protector
 ifunc_sel (int (*f1) (void), int (*f2) (void), int (*f3) (void))
 {
   register void *ret __asm__ ("r3");
@@ -30,6 +31,7 @@ ifunc_sel (int (*f1) (void), int (*f2) (void), int (*f3) (void))
 }
 
 static inline void *
+inhibit_stack_protector
 ifunc_one (int (*f1) (void))
 {
   register void *ret __asm__ ("r3");
diff --git a/sysdeps/unix/make-syscalls.sh b/sysdeps/unix/make-syscalls.sh
index 58d165e..123553c 100644
--- a/sysdeps/unix/make-syscalls.sh
+++ b/sysdeps/unix/make-syscalls.sh
@@ -287,6 +287,7 @@ while read file srcfile caller syscall args strong weak; do
 	(echo '#include <dl-vdso.h>'; \\
 	 echo 'extern void *${strong}_ifunc (void) __asm ("${strong}");'; \\
 	 echo 'void *'; \\
+	 echo 'inhibit_stack_protector'; \\
 	 echo '${strong}_ifunc (void)'; \\
 	 echo '{'; \\
 	 echo '  PREPARE_VERSION_KNOWN (symver, ${vdso_symver});'; \\
diff --git a/sysdeps/unix/sysv/linux/powerpc/gettimeofday.c b/sysdeps/unix/sysv/linux/powerpc/gettimeofday.c
index 25a4e7c..a8b6cfa 100644
--- a/sysdeps/unix/sysv/linux/powerpc/gettimeofday.c
+++ b/sysdeps/unix/sysv/linux/powerpc/gettimeofday.c
@@ -33,6 +33,7 @@ __gettimeofday_syscall (struct timeval *tv, struct timezone *tz)
 }
 
 void *
+inhibit_stack_protector
 gettimeofday_ifunc (void)
 {
   PREPARE_VERSION (linux2615, "LINUX_2.6.15", 123718565);
diff --git a/sysdeps/unix/sysv/linux/powerpc/time.c b/sysdeps/unix/sysv/linux/powerpc/time.c
index 7973419..4b89b38 100644
--- a/sysdeps/unix/sysv/linux/powerpc/time.c
+++ b/sysdeps/unix/sysv/linux/powerpc/time.c
@@ -43,6 +43,7 @@ time_syscall (time_t *t)
 }
 
 void *
+inhibit_stack_protector
 time_ifunc (void)
 {
   PREPARE_VERSION (linux2615, "LINUX_2.6.15", 123718565);
diff --git a/sysdeps/unix/sysv/linux/x86/gettimeofday.c b/sysdeps/unix/sysv/linux/x86/gettimeofday.c
index 36f7c26..e05ad53 100644
--- a/sysdeps/unix/sysv/linux/x86/gettimeofday.c
+++ b/sysdeps/unix/sysv/linux/x86/gettimeofday.c
@@ -32,6 +32,7 @@ __gettimeofday_syscall (struct timeval *tv, struct timezone *tz)
 void *gettimeofday_ifunc (void) __asm__ ("__gettimeofday");
 
 void *
+inhibit_stack_protector
 gettimeofday_ifunc (void)
 {
   PREPARE_VERSION_KNOWN (linux26, LINUX_2_6);
diff --git a/sysdeps/unix/sysv/linux/x86/time.c b/sysdeps/unix/sysv/linux/x86/time.c
index f5f7f91..c5bd8dc 100644
--- a/sysdeps/unix/sysv/linux/x86/time.c
+++ b/sysdeps/unix/sysv/linux/x86/time.c
@@ -33,6 +33,7 @@ __time_syscall (time_t *t)
 void *time_ifunc (void) __asm__ ("time");
 
 void *
+inhibit_stack_protector
 time_ifunc (void)
 {
   PREPARE_VERSION_KNOWN (linux26, LINUX_2_6);
diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/getcpu.c b/sysdeps/unix/sysv/linux/x86_64/x32/getcpu.c
index cbac4b3..8436f9d 100644
--- a/sysdeps/unix/sysv/linux/x86_64/x32/getcpu.c
+++ b/sysdeps/unix/sysv/linux/x86_64/x32/getcpu.c
@@ -21,6 +21,7 @@
 void *getcpu_ifunc (void) __asm__ ("__getcpu");
 
 void *
+inhibit_stack_protector
 getcpu_ifunc (void)
 {
   PREPARE_VERSION (linux26, "LINUX_2.6", 61765110);
diff --git a/sysdeps/x86_64/ifuncmod8.c b/sysdeps/x86_64/ifuncmod8.c
index c004367..7c06562 100644
--- a/sysdeps/x86_64/ifuncmod8.c
+++ b/sysdeps/x86_64/ifuncmod8.c
@@ -28,6 +28,7 @@ foo_impl (float x)
 }
 
 void *
+inhibit_stack_protector
 foo_ifunc (void)
 {
   __m128i xmm = _mm_set1_epi32 (-1);
-- 
2.8.2.202.g98588b6

^ permalink raw reply	[flat|nested] 44+ messages in thread

* [PATCH 08/14 v6] Work even with compilers hacked to enable -fstack-protector by default.
  2016-06-07 11:06 --enable-stack-protector for glibc, v7 Nix
  2016-06-07 11:06 ` [PATCH 03/14 v7] Do not stack-protect ifunc resolvers Nix
@ 2016-06-07 11:06 ` Nix
  2016-06-24 14:32   ` Florian Weimer
  2016-06-07 11:06 ` [PATCH 05/14 v6] Open-code the memcpy() at static TLS initialization time Nix
                   ` (12 subsequent siblings)
  14 siblings, 1 reply; 44+ messages in thread
From: Nix @ 2016-06-07 11:06 UTC (permalink / raw)
  To: libc-alpha; +Cc: fweimer, Nick Alcock

From: Nick Alcock <nick.alcock@oracle.com>

With all the machinery we just added, we can easily arrange to work even
when the compiler passes in -fstack-protector automatically: all the
necessary bits of glibc are always compiled with -fno-stack-protector
now.

So tear out the check in configure, and add appropriate calls to
-fno-stack-protector in tests that need them (largely those that use
-nostdlib), since we don't yet have a __stack_chk_fail() that those
tests can rely upon.  (GCC often provides one, but we cannot rely on
this, especially not when bootstrapping.)

v2: No longer pass in -lssp to anything.
v5: Remove accidentally duplicated $(no_ssp)s.
v6: Small revisions following Mike Frysinger's review.

	* configure.ac: Add check for unsupported stack-protection level.
	(libc_cv_predef_stack_protector): Remove.
	(no_ssp): New variable.
	(libc_cv_ld_gnu_indirect_function): Use it.
	(libc_cv_asm_set_directive): Likewise.
	(libc_cv_protected_data): Likewise.
	(libc_cv_z_combreloc): Likewise.
	(libc_cv_hashstyle): Likewise.
	(libc_cv_has_glob_dat): Likewise.
	(libc_cv_output_format): Likewise.
	(libc_cv_ehdr_start): Likewise.
	* aclocal.m4 (LIBC_TRY_LINK_STATIC): Likewise.
	(LIBC_LINKER_FEATURE): Likewise.
	(LIBC_COMPILER_BUILTIN_INLINED): Likewise.
---
 aclocal.m4   |  6 +++---
 configure.ac | 70 ++++++++++++++++++------------------------------------------
 2 files changed, 24 insertions(+), 52 deletions(-)

diff --git a/aclocal.m4 b/aclocal.m4
index 3d64f77..6902155 100644
--- a/aclocal.m4
+++ b/aclocal.m4
@@ -141,7 +141,7 @@ int _start (void) { return 0; }
 int __start (void) { return 0; }
 $1
 EOF
-AS_IF([AC_TRY_COMMAND([${CC-cc} $CFLAGS $CPPFLAGS $LDFLAGS -o conftest
+AS_IF([AC_TRY_COMMAND([${CC-cc} $CFLAGS $CPPFLAGS $LDFLAGS $no_ssp -o conftest
 		       conftest.c -static -nostartfiles -nostdlib
 		       1>&AS_MESSAGE_LOG_FD])],
       [$2], [$3])
@@ -226,7 +226,7 @@ if test x"$gnu_ld" = x"yes"; then
     cat > conftest.c <<EOF
 int _start (void) { return 42; }
 EOF
-    if AC_TRY_COMMAND([${CC-cc} $CFLAGS $CPPFLAGS $LDFLAGS
+    if AC_TRY_COMMAND([${CC-cc} $CFLAGS $CPPFLAGS $LDFLAGS $no_ssp
 				$2 -nostdlib -nostartfiles
 				-fPIC -shared -o conftest.so conftest.c
 				1>&AS_MESSAGE_LOG_FD])
@@ -268,7 +268,7 @@ libc_compiler_builtin_inlined=no
 cat > conftest.c <<EOF
 int _start (void) { $2 return 0; }
 EOF
-if ! AC_TRY_COMMAND([${CC-cc} $CFLAGS $CPPFLAGS $LDFLAGS
+if ! AC_TRY_COMMAND([${CC-cc} $CFLAGS $CPPFLAGS $LDFLAGS $no_ssp
 		     $3 -nostdlib -nostartfiles
 		     -S conftest.c -o - | fgrep "$1"
 		     1>&AS_MESSAGE_LOG_FD])
diff --git a/configure.ac b/configure.ac
index 389044c..7630dbd 100644
--- a/configure.ac
+++ b/configure.ac
@@ -653,6 +653,18 @@ AC_SUBST(libc_cv_ssp)
 AC_SUBST(stack_protector)
 AC_SUBST(no_stack_protector)
 
+if test -n "$stack_protector"; then
+  dnl Don't run configure tests with stack-protection on, to avoid problems with
+  dnl bootstrapping.
+  no_ssp=-fno-stack-protector
+else
+  no_ssp=
+
+  if test "$enable_stack_protector" != no; then
+    AC_MSG_ERROR([--enable-stack-protector=$enable_stack_protector specified, but specified level of stack protection is not supported by the compiler.])
+  fi
+fi
+
 # For the multi-arch option we need support in the assembler & linker.
 AC_CACHE_CHECK([for assembler and linker STT_GNU_IFUNC support],
 	       libc_cv_ld_gnu_indirect_function, [dnl
@@ -672,7 +684,7 @@ __start:
 EOF
 libc_cv_ld_gnu_indirect_function=no
 if ${CC-cc} $CFLAGS $CPPFLAGS $LDFLAGS \
-	    -nostartfiles -nostdlib \
+	    -nostartfiles -nostdlib $no_ssp \
 	    -o conftest conftest.S 1>&AS_MESSAGE_LOG_FD 2>&AS_MESSAGE_LOG_FD; then
   # Do a link to see if the backend supports IFUNC relocs.
   $READELF -r conftest 1>&AS_MESSAGE_LOG_FD
@@ -1141,7 +1153,7 @@ extern int glibc_conftest_frobozz;
 void _start() { glibc_conftest_frobozz = 1; }
 EOF
 if ${CC-cc} $CFLAGS $CPPFLAGS $LDFLAGS \
-	    -nostartfiles -nostdlib \
+	    -nostartfiles -nostdlib $no_ssp \
 	    -o conftest conftest.s conftest1.c 1>&AS_MESSAGE_LOG_FD 2>&AS_MESSAGE_LOG_FD; then
   libc_cv_asm_set_directive=yes
 else
@@ -1158,12 +1170,12 @@ AC_CACHE_CHECK(linker support for protected data symbol,
 		int bar __attribute__ ((visibility ("protected"))) = 1;
 EOF
 		libc_cv_protected_data=no
-		if AC_TRY_COMMAND(${CC-cc} -nostdlib -nostartfiles -fPIC -shared conftest.c -o conftest.so); then
+		if AC_TRY_COMMAND(${CC-cc} -nostdlib -nostartfiles $no_ssp -fPIC -shared conftest.c -o conftest.so); then
 		  cat > conftest.c <<EOF
 		  extern int bar;
 		  int main (void) { return bar; }
 EOF
-		  if AC_TRY_COMMAND(${CC-cc} -nostdlib -nostartfiles conftest.c -o conftest conftest.so); then
+		  if AC_TRY_COMMAND(${CC-cc} -nostdlib -nostartfiles $no_ssp conftest.c -o conftest conftest.so); then
 		    libc_cv_protected_data=yes
 		  fi
 		fi
@@ -1285,7 +1297,7 @@ extern int mumble;
 int foo (void) { return bar (mumble); }
 EOF
 if AC_TRY_COMMAND([${CC-cc} $CFLAGS $CPPFLAGS $LDFLAGS
-			-fPIC -shared -o conftest.so conftest.c
+			-fPIC -shared $no_ssp -o conftest.so conftest.c
 			-nostdlib -nostartfiles
 			-Wl,-z,combreloc 1>&AS_MESSAGE_LOG_FD])
 then
@@ -1323,7 +1335,7 @@ AC_CACHE_CHECK(for --hash-style option,
 cat > conftest.c <<EOF
 int _start (void) { return 42; }
 EOF
-if AC_TRY_COMMAND([${CC-cc} $CFLAGS $CPPFLAGS $LDFLAGS
+if AC_TRY_COMMAND([${CC-cc} $CFLAGS $CPPFLAGS $LDFLAGS $no_ssp
 			    -fPIC -shared -o conftest.so conftest.c
 			    -Wl,--hash-style=both -nostdlib 1>&AS_MESSAGE_LOG_FD])
 then
@@ -1395,7 +1407,7 @@ int foo (void) { return mumble; }
 EOF
 if AC_TRY_COMMAND([${CC-cc} $CFLAGS $CPPFLAGS $LDFLAGS
 			-fPIC -shared -o conftest.so conftest.c
-			-nostdlib -nostartfiles
+			-nostdlib -nostartfiles $no_ssp
 			1>&AS_MESSAGE_LOG_FD])
 then
 dnl look for GLOB_DAT relocation.
@@ -1412,7 +1424,7 @@ AC_SUBST(libc_cv_has_glob_dat)
 
 AC_CACHE_CHECK(linker output format, libc_cv_output_format, [dnl
 if libc_cv_output_format=`
-${CC-cc} -nostartfiles -nostdlib -Wl,--print-output-format 2>&AS_MESSAGE_LOG_FD`
+${CC-cc} -nostartfiles -nostdlib $no_ssp -Wl,--print-output-format 2>&AS_MESSAGE_LOG_FD`
 then
   :
 else
@@ -1611,46 +1623,6 @@ if test $libc_cv_predef_fortify_source = yes; then
 fi
 AC_SUBST(CPPUNDEFS)
 
-dnl Check for silly hacked compilers inserting -fstack-protector.
-dnl This breaks badly for the early startup code we compile, since
-dnl the compiled code can refer to a magic machine-dependent location
-dnl for the canary value before we have sufficient setup for that to
-dnl work.  It's also questionable to build all of libc with this flag
-dnl even when you're doing that for most applications you build, since
-dnl libc's code is so heavily-used and performance-sensitive.  If we
-dnl ever really want to make that work, it should be enabled explicitly
-dnl in the libc build, not inherited from implicit compiler settings.
-AC_CACHE_CHECK([whether $CC implicitly enables -fstack-protector],
-	       libc_cv_predef_stack_protector, [
-AC_TRY_COMPILE([extern void foobar (char *);],
-	       [char large_array[2048]; foobar (large_array);], [
-libc_undefs=`$NM -u conftest.o |
-  LC_ALL=C $AWK '$1 == "U" { print $2 | "sort -u"; next } { exit(1) }' \
-    2>&AS_MESSAGE_LOG_FD` || {
-  AC_MSG_ERROR([confusing output from $NM -u])
-}
-echo >&AS_MESSAGE_LOG_FD "libc_undefs='$libc_undefs'"
-# On some architectures, there are architecture-specific undefined
-# symbols (resolved by the linker), so filter out unknown symbols.
-# This will fail to produce the correct result if the compiler
-# defaults to -fstack-protector but this produces an undefined symbol
-# other than __stack_chk_fail.  However, compilers like that have not
-# been encountered in practice.
-libc_undefs=`echo "$libc_undefs" | egrep '^(foobar|__stack_chk_fail)$'`
-case "$libc_undefs" in
-foobar) libc_cv_predef_stack_protector=no ;;
-'__stack_chk_fail
-foobar') libc_cv_predef_stack_protector=yes ;;
-*) AC_MSG_ERROR([unexpected symbols in test: $libc_undefs]) ;;
-esac],
-	       [AC_MSG_ERROR([test compilation failed])])
-])
-libc_extra_cflags=
-if test $libc_cv_predef_stack_protector = yes; then
-  libc_extra_cflags="$libc_extra_cflags -fno-stack-protector"
-fi
-libc_extra_cppflags=
-
 # Some linkers on some architectures support __ehdr_start but with
 # bugs.  Make sure usage of it does not create relocations in the
 # output (as the linker should resolve them all for us).
@@ -1660,7 +1632,7 @@ old_CFLAGS="$CFLAGS"
 old_LDFLAGS="$LDFLAGS"
 old_LIBS="$LIBS"
 CFLAGS="$CFLAGS -fPIC"
-LDFLAGS="$LDFLAGS -nostdlib -nostartfiles -shared"
+LDFLAGS="$LDFLAGS -nostdlib -nostartfiles -shared $no_ssp"
 LIBS=
 AC_LINK_IFELSE([AC_LANG_SOURCE([
 typedef struct {
-- 
2.8.2.202.g98588b6

^ permalink raw reply	[flat|nested] 44+ messages in thread

* --enable-stack-protector for glibc, v7
@ 2016-06-07 11:06 Nix
  2016-06-07 11:06 ` [PATCH 03/14 v7] Do not stack-protect ifunc resolvers Nix
                   ` (14 more replies)
  0 siblings, 15 replies; 44+ messages in thread
From: Nix @ 2016-06-07 11:06 UTC (permalink / raw)
  To: libc-alpha; +Cc: fweimer

This is version 7 of the stack-protected glibc patch, incorporating all review
comments to date (unless I missed some).

It's not rebased and is still against glibc head as of a few months ago,
a5df3210a641c17, though I have also tested it with HEAD as of last week.  Patches
that have been merged upstream have been dropped, and cherry-picked back in when
testing.  (However, after I tested, Florian's patch f06f3f05 was merged, which
clashes with patch 3, the ifunc resolver protection patch, because it drops
an ifunc resolver.  Fixing this clash is trivial, but will obviously require
me to rebase the patch series, so perhaps the person doing the patch
application would rather do that.)

Tested with these flag combinations on {i686,x86_64)-pc-linux-gnu:

--enable-omitfp --enable-stack-protector=all
--enable-stack-protector
--enable-stack-protector=strong
--enable-stack-protector=all
--enable-stackguard-randomization --enable-stack-protector=all
--enable-omitfp --enable-stackguard-randomization --enable-stack-protector
--enable-omitfp --enable-stackguard-randomization --enable-stack-protector=strong
--enable-omitfp --enable-stackguard-randomization --enable-stack-protector=all
--disable-stack-protector
--enable-stack-protector=no

Tested with with these flag combinations on sparc{32,64}-pc-linux-gnu:

--enable-stack-protector
--enable-stack-protector=strong
--enable-stackguard-randomization --enable-stack-protector=strong
--enable-stackguard-randomization --enable-stack-protector=all
--disable-stack-protector

Tested with these flag combinations on armv7l-unknown-linux-gnueabihf (it
happened to have GCC 4.8, so -strong wasn't available):

--enable-stackguard-randomization --enable-stack-protector
--enable-stackguard-randomization --enable-stack-protector=all --enable-omitfp
--disable-stack-protector

No failures are observed that are not also observed on an unpatched glibc with
the same flag combinations.

On the copyright assignment front, I am informed that Oracle has a blanket
assignment on file for glibc work, so I don't need to do anything. (Patch 11 is
in Adhemerval's name, but obviously there's no assignment problem there either.)

Overview of changes in this posting:

 - Dropped "Allow overriding of CFLAGS as well as CPPFLAGS for rtld." and
   "x86, pthread_cond_*wait: Do not depend on %eax not being clobbered":
   merged upstream.

 - Report the argument value used in --enable-stack-protector on error; fix
   quoting.  [Review comment from Mike Frysinger.]

 - Comment on the reason for some $(no-stack-protector)isms.

 - Stack-protect sigreturn.c, and say why stack-protecting sigreturn handlers
   stubs is necessary.  [Review comment from David Miller.]

^ permalink raw reply	[flat|nested] 44+ messages in thread

* [PATCH 05/14 v6] Open-code the memcpy() at static TLS initialization time.
  2016-06-07 11:06 --enable-stack-protector for glibc, v7 Nix
  2016-06-07 11:06 ` [PATCH 03/14 v7] Do not stack-protect ifunc resolvers Nix
  2016-06-07 11:06 ` [PATCH 08/14 v6] Work even with compilers hacked to enable -fstack-protector by default Nix
@ 2016-06-07 11:06 ` Nix
  2016-06-24 12:42   ` Florian Weimer
  2016-06-07 11:25 ` [PATCH 06/14 v3] Compile the entire dynamic linker with -fno-stack-protector Nix
                   ` (11 subsequent siblings)
  14 siblings, 1 reply; 44+ messages in thread
From: Nix @ 2016-06-07 11:06 UTC (permalink / raw)
  To: libc-alpha; +Cc: fweimer, Nick Alcock

From: Nick Alcock <nick.alcock@oracle.com>

This one is a bit nasty.  Now that we are initializing TLS earlier for
the stack canary's sake, existing memcpy() implementations become
problematic.  We can use the multiarch implementations, but they might
not always be present, and even if they are present they might not always
be in assembler, so might be compiled with stack-protection.  We cannot
use posix/memcpy.c without marking both it and */wordcopy.c as non-stack-
protected, which for memcpy() of all things seems like a seriously bad
idea: if any function in glibc should be stack-protected, it's memcpy()
(though stack-protecting the many optimized assembly versions is not done
in this patch series).

So we have two real options: hack up the guts of posix/memcpy.c and
*/wordcopy.c so that they can be #included (renamed and declared static)
inside libc-tls.c, or simply open-code the memcpy().  For simplicity's
sake, this patch open-codes it, on the grounds that static binaries are
relatively rare and quasi-deprecated anyway, and static binaries with
large TLS sections are yet rarer, and not worth the complexity of hacking
up all the arch-dependent wordcopy files.  If the arch provides an inline
assembler memcpy() implementation, we can use that in preference, for
speed; also, of course, if stack protection is not enabled at all, we
can still use a normal memcpy() as before.

(This was not revealed when testing on x86 because on that platform
GCC was open-coding the memcpy() for us.)

v2: New, lets us remove the memcpy() -fno-stack-protection, which wasn't
    enough in any case.
v4: Add an inhibit_loop_to_libcall to prevent GCC from turning the loop
    back into a memcpy() again.  Wrap long lines.
v6: Only do this if stack-protected.  Use the inline assembler
    ARCH_memcpy if available.

	* csu/libc-tls.c (__libc_setup_tls): Add inhibit_loop_to_libcall
	to avoid calls to potentially ifunced or stack-protected memcpy.
	Optionally open-code the TLS-initialization memcpy.
---
 csu/libc-tls.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/csu/libc-tls.c b/csu/libc-tls.c
index 3d67a64..4d81113 100644
--- a/csu/libc-tls.c
+++ b/csu/libc-tls.c
@@ -102,6 +102,7 @@ init_static_tls (size_t memsz, size_t align)
 }
 
 void
+inhibit_loop_to_libcall
 __libc_setup_tls (size_t tcbsize, size_t tcbalign)
 {
   void *tlsblock;
@@ -176,8 +177,23 @@ __libc_setup_tls (size_t tcbsize, size_t tcbalign)
 # error "Either TLS_TCB_AT_TP or TLS_DTV_AT_TP must be defined"
 #endif
   _dl_static_dtv[2].pointer.is_static = true;
-  /* sbrk gives us zero'd memory, so we don't need to clear the remainder.  */
+
+  /* sbrk gives us zero'd memory, so we don't need to clear the remainder.
+
+     When stack-protecting, use inlined asm implementation if available:
+     otherwise, copy by hand, because memcpy() is stack-protected and is often
+     multiarch too.  */
+
+#if defined _HAVE_STRING_ARCH_memcpy || !defined STACK_PROTECTOR_LEVEL
   memcpy (_dl_static_dtv[2].pointer.val, initimage, filesz);
+#else
+  char *dst = (char *) _dl_static_dtv[2].pointer.val;
+  char *src = (char *) initimage;
+  size_t i;
+
+  for (i = 0; i < filesz; dst++, src++, i++)
+    *dst = *src;
+#endif
 
   /* Install the pointer to the dtv.  */
 
-- 
2.8.2.202.g98588b6

^ permalink raw reply	[flat|nested] 44+ messages in thread

* [PATCH 14/14 v5] Enable -fstack-protector=* when requested by configure.
  2016-06-07 11:06 --enable-stack-protector for glibc, v7 Nix
                   ` (3 preceding siblings ...)
  2016-06-07 11:25 ` [PATCH 06/14 v3] Compile the entire dynamic linker with -fno-stack-protector Nix
@ 2016-06-07 11:25 ` Nix
  2016-06-24 14:47   ` Florian Weimer
  2016-06-07 11:25 ` [PATCH 10/14 v6] De-PLTize __stack_chk_fail internal calls within libc.so Nix
                   ` (9 subsequent siblings)
  14 siblings, 1 reply; 44+ messages in thread
From: Nix @ 2016-06-07 11:25 UTC (permalink / raw)
  To: libc-alpha; +Cc: fweimer, Nick Alcock

From: Nick Alcock <nick.alcock@oracle.com>

This finally turns on all the machinery added in previous commits.

v3: Wrap long lines.
v5: Shuffle to the end.

	* Makeconfig (+stack-protector): New variable.
	(+cflags): Use it.
---
 Makeconfig | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/Makeconfig b/Makeconfig
index 87a22e8..efcfdad 100644
--- a/Makeconfig
+++ b/Makeconfig
@@ -762,6 +762,11 @@ endif
 # disable any optimization that assume default rounding mode.
 +math-flags = -frounding-math
 
+# We might want to compile with some stack-protection flag.
+ifneq ($(stack-protector),)
++stack-protector=$(stack-protector)
+endif
+
 # This is the program that generates makefile dependencies from C source files.
 # The -MP flag tells GCC >= 3.2 (which we now require) to produce dummy
 # targets for headers so that removed headers don't break the build.
@@ -821,7 +826,8 @@ ifeq	"$(strip $(+cflags))" ""
 +cflags	:= $(default_cflags)
 endif	# $(+cflags) == ""
 
-+cflags += $(cflags-cpu) $(+gccwarn) $(+merge-constants) $(+math-flags)
++cflags += $(cflags-cpu) $(+gccwarn) $(+merge-constants) $(+math-flags) \
+	   $(+stack-protector)
 +gcc-nowarn := -w
 
 # Don't duplicate options if we inherited variables from the parent.
-- 
2.8.2.202.g98588b6

^ permalink raw reply	[flat|nested] 44+ messages in thread

* [PATCH 10/14 v6] De-PLTize __stack_chk_fail internal calls within libc.so.
  2016-06-07 11:06 --enable-stack-protector for glibc, v7 Nix
                   ` (4 preceding siblings ...)
  2016-06-07 11:25 ` [PATCH 14/14 v5] Enable -fstack-protector=* when requested by configure Nix
@ 2016-06-07 11:25 ` Nix
  2016-06-24 14:37   ` Florian Weimer
  2016-06-07 11:26 ` [PATCH 07/14 v6] Prevent the rtld mapfile computation from dragging in __stack_chk_fail* Nix
                   ` (8 subsequent siblings)
  14 siblings, 1 reply; 44+ messages in thread
From: Nix @ 2016-06-07 11:25 UTC (permalink / raw)
  To: libc-alpha; +Cc: fweimer, Adhemerval Zanella

From: Adhemerval Zanella <adhemerval.zanella@linaro.org>

We use the same assembler-macro trick we use to de-PLTize
compiler-generated libcalls to memcpy and memset to redirect
__stack_chk_fail to __stack_chk_fail_local.

v5: New.
v6: Only do it within the shared library: with __stack_chk_fail_local
    in libc_pic.a now we don't need to worry about calls from inside
    other routines in libc_nonshared.a any more.

	* sysdeps/generic/symbol-hacks.h (__stack_chk_fail): Add internal
	alias.
---
 sysdeps/generic/symbol-hacks.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/sysdeps/generic/symbol-hacks.h b/sysdeps/generic/symbol-hacks.h
index ce576c9..fd3d2de 100644
--- a/sysdeps/generic/symbol-hacks.h
+++ b/sysdeps/generic/symbol-hacks.h
@@ -5,3 +5,9 @@ asm ("memmove = __GI_memmove");
 asm ("memset = __GI_memset");
 asm ("memcpy = __GI_memcpy");
 #endif
+
+/* -fstack-protector generates calls to __stack_chk_fail, which need
+   similar adjustments to avoid going through the PLT.  */
+#if !defined __ASSEMBLER__ && IS_IN (libc) && defined SHARED
+asm ("__stack_chk_fail = __stack_chk_fail_local");
+#endif
-- 
2.8.2.202.g98588b6

^ permalink raw reply	[flat|nested] 44+ messages in thread

* [PATCH 06/14 v3] Compile the entire dynamic linker with -fno-stack-protector.
  2016-06-07 11:06 --enable-stack-protector for glibc, v7 Nix
                   ` (2 preceding siblings ...)
  2016-06-07 11:06 ` [PATCH 05/14 v6] Open-code the memcpy() at static TLS initialization time Nix
@ 2016-06-07 11:25 ` Nix
  2016-06-24 13:13   ` Florian Weimer
  2016-06-07 11:25 ` [PATCH 14/14 v5] Enable -fstack-protector=* when requested by configure Nix
                   ` (10 subsequent siblings)
  14 siblings, 1 reply; 44+ messages in thread
From: Nix @ 2016-06-07 11:25 UTC (permalink / raw)
  To: libc-alpha; +Cc: fweimer, Nick Alcock

From: Nick Alcock <nick.alcock@oracle.com>

Also compile corresponding routines in the static libc.a with the same
flag.

v3: Use $(no-stack-protector).
    Introduce $(elide-stack-protector) and use it to reduce redundancy.
    Bring all the elisions together textually.

	* elf/Makefile (elide-stack-protector): New.
	(CFLAGS-.os): Use it, eliding $(all-rtld-routines).
	(CFLAGS-.oX): Likewise, eliding $(elide-routines.os).
	(rtld-CFLAGS): Likewise.
---
 elf/Makefile   | 13 +++++++++++++
 elf/rtld-Rules |  2 ++
 2 files changed, 15 insertions(+)

diff --git a/elf/Makefile b/elf/Makefile
index 63a5355..0037cca 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -50,6 +50,19 @@ CFLAGS-dl-runtime.c = -fexceptions -fasynchronous-unwind-tables
 CFLAGS-dl-lookup.c = -fexceptions -fasynchronous-unwind-tables
 CFLAGS-dl-iterate-phdr.c = $(uses-callbacks)
 
+# Compile rtld itself without stack protection.
+# Also compile all routines in the static library that are elided from
+# the shared libc because they are in ld.so the same way.
+
+define elide-stack-protector
+$(if $(filter $(@F),$(patsubst %,%$(1),$(2))), $(no-stack-protector))
+endef
+
+CFLAGS-.o += $(call elide-stack-protector,.o,$(elide-routines.os))
+CFLAGS-.op += $(call elide-stack-protector,.op,$(elide-routines.os))
+CFLAGS-.og += $(call elide-stack-protector,.og,$(elide-routines.os))
+CFLAGS-.os += $(call elide-stack-protector,.os,$(all-rtld-routines))
+
 ifeq ($(unwind-find-fde),yes)
 routines += unwind-dw2-fde-glibc
 shared-only-routines += unwind-dw2-fde-glibc
diff --git a/elf/rtld-Rules b/elf/rtld-Rules
index 94ca39b..1c8593e 100644
--- a/elf/rtld-Rules
+++ b/elf/rtld-Rules
@@ -144,4 +144,6 @@ cpp-srcs-left := $(rtld-modules:%.os=%)
 lib := rtld
 include $(patsubst %,$(..)cppflags-iterator.mk,$(cpp-srcs-left))
 
+rtld-CFLAGS := $(no-stack-protector)
+
 endif
-- 
2.8.2.202.g98588b6

^ permalink raw reply	[flat|nested] 44+ messages in thread

* [PATCH 12/14] Drop explicit stack-protection of pieces of the system.
  2016-06-07 11:06 --enable-stack-protector for glibc, v7 Nix
                   ` (8 preceding siblings ...)
  2016-06-07 11:26 ` [PATCH 09/14 v6] Add stack_chk_fail_local to libc.so Nix
@ 2016-06-07 11:26 ` Nix
  2016-06-24 13:25   ` Florian Weimer
  2016-06-07 11:26 ` [PATCH 13/14 v7] Do not stack-protect sigreturn stubs Nix
                   ` (4 subsequent siblings)
  14 siblings, 1 reply; 44+ messages in thread
From: Nix @ 2016-06-07 11:26 UTC (permalink / raw)
  To: libc-alpha; +Cc: fweimer, Nick Alcock

From: Nick Alcock <nick.alcock@oracle.com>

This is probably a bad idea: maybe we want to stack-protect some parts
of the system even when ! --enable-stack-protector.  I can easily adjust
the patch to do that (though it'll mean introducing a new variable
analogous to $(stack-protector) but not controlled by the configure
flag.)

But if we wanted to value consistency over security, and use the same
stack-protection configure flag to control everything, this is how we'd
do it!

("Always include at least one patch with something obviously wrong with
it.")

	* login/Makefile (pt_chown-cflags): Remove.
	* nscd/Makefile (CFLAGS-nscd): Likewise.
	* resolv/Makefile (CFLAGS-libresolv): Likewise.
---
 login/Makefile  | 1 -
 nscd/Makefile   | 1 -
 resolv/Makefile | 1 -
 3 files changed, 3 deletions(-)

diff --git a/login/Makefile b/login/Makefile
index 9ff36d6..1a6161c 100644
--- a/login/Makefile
+++ b/login/Makefile
@@ -58,7 +58,6 @@ CFLAGS-getpt.c = -fexceptions
 ifeq (yesyes,$(have-fpie)$(build-shared))
 pt_chown-cflags += $(pie-ccflag)
 endif
-pt_chown-cflags += $(stack-protector)
 ifeq (yes,$(have-libcap))
 libcap = -lcap
 endif
diff --git a/nscd/Makefile b/nscd/Makefile
index 50bad32..bfd72d5 100644
--- a/nscd/Makefile
+++ b/nscd/Makefile
@@ -84,7 +84,6 @@ CPPFLAGS-nscd += -D_FORTIFY_SOURCE=2
 ifeq (yesyes,$(have-fpie)$(build-shared))
 CFLAGS-nscd += $(pie-ccflag)
 endif
-CFLAGS-nscd += $(stack-protector)
 
 ifeq (yesyes,$(have-fpie)$(build-shared))
 LDFLAGS-nscd = -Wl,-z,now
diff --git a/resolv/Makefile b/resolv/Makefile
index 8be41d3..0395b1a 100644
--- a/resolv/Makefile
+++ b/resolv/Makefile
@@ -90,7 +90,6 @@ CPPFLAGS += -Dgethostbyname=res_gethostbyname \
 	    -Dgetnetbyname=res_getnetbyname \
 	    -Dgetnetbyaddr=res_getnetbyaddr
 
-CFLAGS-libresolv += $(stack-protector)
 CFLAGS-res_hconf.c = -fexceptions
 
 # The BIND code elicits some harmless warnings.
-- 
2.8.2.202.g98588b6

^ permalink raw reply	[flat|nested] 44+ messages in thread

* [PATCH 13/14 v7] Do not stack-protect sigreturn stubs.
  2016-06-07 11:06 --enable-stack-protector for glibc, v7 Nix
                   ` (9 preceding siblings ...)
  2016-06-07 11:26 ` [PATCH 12/14] Drop explicit stack-protection of pieces of the system Nix
@ 2016-06-07 11:26 ` Nix
  2016-06-24 13:52   ` Florian Weimer
  2016-06-07 11:27 ` [PATCH 02/14 v6] Initialize the stack guard earlier when linking statically Nix
                   ` (3 subsequent siblings)
  14 siblings, 1 reply; 44+ messages in thread
From: Nix @ 2016-06-07 11:26 UTC (permalink / raw)
  To: libc-alpha; +Cc: fweimer, Nick Alcock

From: Nick Alcock <nick.alcock@oracle.com>

These are called from the kernel with the stack at a carefully-
chosen location so that the stack frame can be restored: they must not
move the stack pointer lest garbage be restored into the registers.

We explicitly inhibit protection for SPARC and for signal/sigreturn.c:
other arches either define their sigreturn stubs in .S files, or (i386,
x86_64, mips) use macros expanding to top-level asm blocks and explicit
labels in the text section to mock up a "function" without telling the
compiler that one is there at all.

v2: New.
v3: Use $(no-stack-protector).
v4: Use inhibit_stack_protector.
v7: Add sigreturn.c.

	* signal/Makefile (CFLAGS-sigreturn.c): Use
	$(no-stack-protector).
	* sysdeps/unix/sysv/linux/sparc/sparc64/sigaction.c:
	(__rt_sigreturn_stub): Use inhibit_stack_protector.
	* sysdeps/unix/sysv/linux/sparc/sparc32/sigaction.c
	(__rt_sigreturn_stub): Likewise.
	(__sigreturn_stub): Likewise.
---
 signal/Makefile                                   | 2 ++
 sysdeps/unix/sysv/linux/sparc/sparc32/sigaction.c | 8 ++++++--
 sysdeps/unix/sysv/linux/sparc/sparc64/sigaction.c | 4 +++-
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/signal/Makefile b/signal/Makefile
index 9d29ff4..ccd6f51 100644
--- a/signal/Makefile
+++ b/signal/Makefile
@@ -48,3 +48,5 @@ CFLAGS-sigsuspend.c = -fexceptions -fasynchronous-unwind-tables
 CFLAGS-sigtimedwait.c = -fexceptions -fasynchronous-unwind-tables
 CFLAGS-sigwait.c = -fexceptions -fasynchronous-unwind-tables
 CFLAGS-sigwaitinfo.c = -fexceptions -fasynchronous-unwind-tables
+
+CFLAGS-sigreturn.c = $(no-stack-protector)
diff --git a/sysdeps/unix/sysv/linux/sparc/sparc32/sigaction.c b/sysdeps/unix/sysv/linux/sparc/sparc32/sigaction.c
index 5aa3c35..b75142f 100644
--- a/sysdeps/unix/sysv/linux/sparc/sparc32/sigaction.c
+++ b/sysdeps/unix/sysv/linux/sparc/sparc32/sigaction.c
@@ -65,7 +65,9 @@ libc_hidden_def (__libc_sigaction)
 #include <nptl/sigaction.c>
 
 
-static void
+static
+inhibit_stack_protector
+void
 __rt_sigreturn_stub (void)
 {
   __asm__ ("mov %0, %%g1\n\t"
@@ -74,7 +76,9 @@ __rt_sigreturn_stub (void)
 	   : "i" (__NR_rt_sigreturn));
 }
 
-static void
+static
+inhibit_stack_protector
+void
 __sigreturn_stub (void)
 {
   __asm__ ("mov %0, %%g1\n\t"
diff --git a/sysdeps/unix/sysv/linux/sparc/sparc64/sigaction.c b/sysdeps/unix/sysv/linux/sparc/sparc64/sigaction.c
index 50c444c..058c011 100644
--- a/sysdeps/unix/sysv/linux/sparc/sparc64/sigaction.c
+++ b/sysdeps/unix/sysv/linux/sparc/sparc64/sigaction.c
@@ -66,7 +66,9 @@ libc_hidden_def (__libc_sigaction)
 #include <nptl/sigaction.c>
 
 
-static void
+static
+inhibit_stack_protector
+void
 __rt_sigreturn_stub (void)
 {
   __asm__ ("mov %0, %%g1\n\t"
-- 
2.8.2.202.g98588b6

^ permalink raw reply	[flat|nested] 44+ messages in thread

* [PATCH 07/14 v6] Prevent the rtld mapfile computation from dragging in __stack_chk_fail*.
  2016-06-07 11:06 --enable-stack-protector for glibc, v7 Nix
                   ` (5 preceding siblings ...)
  2016-06-07 11:25 ` [PATCH 10/14 v6] De-PLTize __stack_chk_fail internal calls within libc.so Nix
@ 2016-06-07 11:26 ` Nix
  2016-06-07 11:39   ` Andreas Schwab
  2016-06-24 13:03   ` Florian Weimer
  2016-06-07 11:26 ` [PATCH 11/14 v3] Link various tests with -fno-stack-protector Nix
                   ` (7 subsequent siblings)
  14 siblings, 2 replies; 44+ messages in thread
From: Nix @ 2016-06-07 11:26 UTC (permalink / raw)
  To: libc-alpha; +Cc: fweimer, Nick Alcock

From: Nick Alcock <nick.alcock@oracle.com>

The previous commit prevented rtld itself from being built with
-fstack-protector, but this is not quite enough.  We identify which
objects belong in rtld via a test link and analysis of the resulting
mapfile.  That link is necessarily done against objects that are
stack-protected, so drags in __stack_chk_fail_local, __stack_chk_fail,
and all the libc and libio code they use.

To stop this happening, use --defsym in the test librtld.map-production
link to force the linker to predefine these two symbols (to 0, but it
could be to anything).  (In a real link, this would of course be
catastrophic, but these object files are never used for anything else.)

v2: New.
v6: Dummy out stack_chk_fail_local too.

	* elf/Makefile (dummy-stack-chk-fail): New.
	($(objpfx)librtld.map): Use it.
---
 elf/Makefile | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/elf/Makefile b/elf/Makefile
index 0037cca..d1e29a58 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -349,9 +349,20 @@ $(objpfx)dl-allobjs.os: $(all-rtld-routines:%=$(objpfx)%.os)
 # are compiled with special flags, and puts these modules into rtld-libc.a
 # for us.  Then we do the real link using rtld-libc.a instead of libc_pic.a.
 
+# If the compiler can do SSP, build the mapfile with dummy __stack_chk_fail
+# and __stack_chk_fail_local symbols defined, to prevent the real things
+# being dragged into rtld even though rtld is never built with stack-protection.
+
+ifeq ($(have-ssp),yes)
+dummy-stack-chk-fail := -Wl,--defsym='__stack_chk_fail=0' \
+			-Wl,--defsym='__stack_chk_fail_local=0'
+else
+dummy-stack-chk-fail :=
+endif
+
 $(objpfx)librtld.map: $(objpfx)dl-allobjs.os $(common-objpfx)libc_pic.a
 	@-rm -f $@T
-	$(reloc-link) -o $@.o '-Wl,-(' $^ -lgcc '-Wl,-)' -Wl,-Map,$@T
+	$(reloc-link) -o $@.o $(dummy-stack-chk-fail) '-Wl,-(' $^ -lgcc '-Wl,-)' -Wl,-Map,$@T
 	rm -f $@.o
 	mv -f $@T $@
 
-- 
2.8.2.202.g98588b6

^ permalink raw reply	[flat|nested] 44+ messages in thread

* [PATCH 09/14 v6] Add stack_chk_fail_local to libc.so.
  2016-06-07 11:06 --enable-stack-protector for glibc, v7 Nix
                   ` (7 preceding siblings ...)
  2016-06-07 11:26 ` [PATCH 11/14 v3] Link various tests with -fno-stack-protector Nix
@ 2016-06-07 11:26 ` Nix
  2016-06-24 14:39   ` Florian Weimer
  2016-06-07 11:26 ` [PATCH 12/14] Drop explicit stack-protection of pieces of the system Nix
                   ` (5 subsequent siblings)
  14 siblings, 1 reply; 44+ messages in thread
From: Nix @ 2016-06-07 11:26 UTC (permalink / raw)
  To: libc-alpha; +Cc: fweimer, Nick Alcock

From: Nick Alcock <nick.alcock@oracle.com>

This is required by the next commit, which routes all
__stack_chk_fail() calls in libc.so via this function to avoid
the PLT.  It has be duplicated in libc.so and libc_nonshared.a
because its entire reason for existence is to be hidden and avoid
the PLT, so the copy in libc.so is not visible from elsewhere.

Also stop all the variants of __stack_chk_fail from being stack-
protected: this makes no sense and risks recursion.

v5: Better explanation.  Add no-stack-protection of
    __stack_chk_fail_local etc.
v6: Rework as suggested by Andreas: make a shared-only version of
    stack_chk_fail_local.c rather than linking libc_nonshared into
    libc.

	* debug/libc-stack_chk_fail_local.c: New file.
	* debug/Makefile (routines): Add it.
	(shared-only-routines): Likewise.
	(CFLAGS-stack_chk_fail.c): Use $(no-stack-protector).
	(CFLAGS-stack_chk_fail_local.c): Likewise.
	(CFLAGS-libc-stack_chk_fail_local.c): Likewise.
---
 debug/Makefile                    | 14 +++++++++++++-
 debug/libc-stack_chk_fail_local.c |  3 +++
 2 files changed, 16 insertions(+), 1 deletion(-)
 create mode 100644 debug/libc-stack_chk_fail_local.c

diff --git a/debug/Makefile b/debug/Makefile
index 6b5f31e..27da081 100644
--- a/debug/Makefile
+++ b/debug/Makefile
@@ -48,9 +48,21 @@ routines  = backtrace backtracesyms backtracesymsfd noophooks \
 	    vdprintf_chk obprintf_chk \
 	    longjmp_chk ____longjmp_chk \
 	    fdelt_chk poll_chk ppoll_chk \
-	    stack_chk_fail fortify_fail \
+	    stack_chk_fail fortify_fail libc-stack_chk_fail_local \
 	    $(static-only-routines)
+
+# stack_chk_fail_local must be non-PIC, thus static-only, but we also
+# want an identical thunk hidden in libc.so to avoid going via the PLT.
+
 static-only-routines := warning-nop stack_chk_fail_local
+shared-only-routines += libc-stack_chk_fail_local
+
+# Building the stack-protector failure routines with stack protection
+# makes no sense.
+
+CFLAGS-stack_chk_fail.c = $(no-stack-protector)
+CFLAGS-stack_chk_fail_local.c = $(no-stack-protector)
+CFLAGS-libc-stack_chk_fail_local.c = $(no-stack-protector)
 
 CFLAGS-backtrace.c = -fno-omit-frame-pointer
 CFLAGS-sprintf_chk.c = $(libio-mtsafe)
diff --git a/debug/libc-stack_chk_fail_local.c b/debug/libc-stack_chk_fail_local.c
new file mode 100644
index 0000000..73da970
--- /dev/null
+++ b/debug/libc-stack_chk_fail_local.c
@@ -0,0 +1,3 @@
+/* This goes into the shared libc.  */
+
+#include <stack_chk_fail_local.c>
-- 
2.8.2.202.g98588b6

^ permalink raw reply	[flat|nested] 44+ messages in thread

* [PATCH 11/14 v3] Link various tests with -fno-stack-protector.
  2016-06-07 11:06 --enable-stack-protector for glibc, v7 Nix
                   ` (6 preceding siblings ...)
  2016-06-07 11:26 ` [PATCH 07/14 v6] Prevent the rtld mapfile computation from dragging in __stack_chk_fail* Nix
@ 2016-06-07 11:26 ` Nix
  2016-06-24 14:40   ` Florian Weimer
  2016-06-07 11:26 ` [PATCH 09/14 v6] Add stack_chk_fail_local to libc.so Nix
                   ` (6 subsequent siblings)
  14 siblings, 1 reply; 44+ messages in thread
From: Nix @ 2016-06-07 11:26 UTC (permalink / raw)
  To: libc-alpha; +Cc: fweimer, Nick Alcock

From: Nick Alcock <nick.alcock@oracle.com>

These tests do not link with libc, so cannot see __stack_chk_fail().

v3: Use $(no-stack-protector).

	* elf/Makefile (CFLAGS-filtmod1.c): Use $(no-stack-protector) for
	non-libc-linking testcase.
	(CFLAGS-filtmod2.c): Likewise.
	* stdlib/Makefile (CFLAGS-tst-putenvmod.c): Likewise.
	* sysdeps/x86_64/Makefile (CFLAGS-tst-quad1pie.c): Likewise.
	(CFLAGS-tst-quad2pie.c): Likewise.
---
 elf/Makefile            | 4 ++++
 stdlib/Makefile         | 3 +++
 sysdeps/x86_64/Makefile | 3 +++
 3 files changed, 10 insertions(+)

diff --git a/elf/Makefile b/elf/Makefile
index d1e29a58..185731e 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -752,6 +752,10 @@ $(objpfx)filtmod1.so: $(objpfx)filtmod1.os $(objpfx)filtmod2.so
 		  $< -Wl,-F,$(objpfx)filtmod2.so
 $(objpfx)filter: $(objpfx)filtmod1.so
 
+# These do not link against libc.
+CFLAGS-filtmod1.c = $(no-stack-protector)
+CFLAGS-filtmod2.c = $(no-stack-protector)
+
 $(objpfx)unload: $(libdl)
 $(objpfx)unload.out: $(objpfx)unloadmod.so
 
diff --git a/stdlib/Makefile b/stdlib/Makefile
index 26fe67a..d601b87 100644
--- a/stdlib/Makefile
+++ b/stdlib/Makefile
@@ -164,6 +164,9 @@ LDFLAGS-tst-putenv = $(no-as-needed)
 
 $(objpfx)tst-putenvmod.so: $(objpfx)tst-putenvmod.os $(link-libc-deps)
 	$(build-module)
+# This is not only not in libc, it's not even linked with it.
+CFLAGS-tst-putenvmod.c += $(no-stack-protector)
+
 libof-tst-putenvmod = extramodules
 
 $(objpfx)bug-getcontext: $(libm)
diff --git a/sysdeps/x86_64/Makefile b/sysdeps/x86_64/Makefile
index 67ed5ba..6caa74a 100644
--- a/sysdeps/x86_64/Makefile
+++ b/sysdeps/x86_64/Makefile
@@ -40,6 +40,9 @@ quad-pie-test += tst-quad1pie tst-quad2pie
 tests += $(quad-pie-test)
 tests-pie += $(quad-pie-test)
 
+CFLAGS-tst-quad1pie.c = $(no-stack-protector)
+CFLAGS-tst-quad2pie.c = $(no-stack-protector)
+
 $(objpfx)tst-quad1pie: $(objpfx)tst-quadmod1pie.o
 $(objpfx)tst-quad2pie: $(objpfx)tst-quadmod2pie.o
 
-- 
2.8.2.202.g98588b6

^ permalink raw reply	[flat|nested] 44+ messages in thread

* [PATCH 01/14 v7] Configury support for --enable-stack-protector.
  2016-06-07 11:06 --enable-stack-protector for glibc, v7 Nix
                   ` (11 preceding siblings ...)
  2016-06-07 11:27 ` [PATCH 02/14 v6] Initialize the stack guard earlier when linking statically Nix
@ 2016-06-07 11:27 ` Nix
  2016-06-24 14:50   ` Florian Weimer
  2016-06-07 11:27 ` [PATCH 04/14 v7] Mark all machinery needed in early static-link init as -fno-stack-protector Nix
  2016-06-24 15:01 ` --enable-stack-protector for glibc, v7 Florian Weimer
  14 siblings, 1 reply; 44+ messages in thread
From: Nix @ 2016-06-07 11:27 UTC (permalink / raw)
  To: libc-alpha; +Cc: fweimer, Nick Alcock

From: Nick Alcock <nick.alcock@oracle.com>

This adds =all and =strong, with obvious semantics, and with a rather
arbitrarily-chosen default off, which we might well want to change to
something stronger once this patch has been tested by people other than
me.

We don't validate the value of the option yet: that's in a later patch.
Nor do we use it for anything at this stage.

We differentiate between 'the compiler understands -fstack-protector'
and 'the user wanted -fstack-protector' so that we can pass
-fno-stack-protector in appropriate places even if the user didn't want
to turn on -fstack-protector for other parts.  (This helps us overcome
another existing limitation, that glibc doesn't work with GCCs hacked
to pass in -fstack-protector by default.)

We might want to add another configuration option to turn on
-fstack-protector for nscd and other network-facing operations by
default, but for now I've stuck with one option to control everything.

v2: documentation in install.texi; better description of the option.
    INSTALL regenerated.
v3: Substitute in no_stack_protector.
v6: Small quoting/spacing revisions following Mike Frysinger's review.
    Add STACK_PROTECTOR_LEVEL.
v7: Quoting changes. Report --enable-stack-protector argument values
    on error.

	[BZ #7065]
	* configure.ac (libc_cv_ssp): Move up.
	(libc_cv_ssp_strong): Likewise.
	(libc_cv_ssp_all): New.
	(stack_protector): Augment, adding -fstack-protector-all.
	(no_stack_protector): New.
	(STACK_PROTECTOR_LEVEL): New.
	(AC_ARG_ENABLE(stack-protector)): New configure flag.
	* manual/install.texi (--enable-stack-protector): Document it.
	* config.h.in (STACK_PROTECTOR_LEVEL): New macro.
	* INSTALL: Regenerate.
---
 INSTALL             | 39 ++++++++++++++++++-----------
 config.h.in         |  3 +++
 configure.ac        | 70 ++++++++++++++++++++++++++++++++++++++---------------
 manual/install.texi | 12 +++++++++
 4 files changed, 90 insertions(+), 34 deletions(-)

diff --git a/INSTALL b/INSTALL
index a0d0c05..3d55585 100644
--- a/INSTALL
+++ b/INSTALL
@@ -141,20 +141,31 @@ will be used, and CFLAGS sets optimization options for the compiler.
 '--enable-lock-elision=yes'
      Enable lock elision for pthread mutexes by default.
 
-'--enable-pt_chown'
-     The file 'pt_chown' is a helper binary for 'grantpt' (*note
-     Pseudo-Terminals: Allocation.) that is installed setuid root to fix
-     up pseudo-terminal ownership.  It is not built by default because
-     systems using the Linux kernel are commonly built with the 'devpts'
-     filesystem enabled and mounted at '/dev/pts', which manages
-     pseudo-terminal ownership automatically.  By using
-     '--enable-pt_chown', you may build 'pt_chown' and install it setuid
-     and owned by 'root'.  The use of 'pt_chown' introduces additional
-     security risks to the system and you should enable it only if you
-     understand and accept those risks.
-
-'--disable-werror'
-     By default, the GNU C Library is built with '-Werror'.  If you wish
+`--enable-stack-protector'
+`--enable-stack-protector=strong'
+`--enable-stack-protector=all'
+     Compile the C library and all other parts of the glibc package
+     (including the threading and math libraries, NSS modules, and
+     transliteration modules) using the GCC `-fstack-protector',
+     `-fstack-protector-strong' or `-fstack-protector-all' options to
+     detect stack overruns.  Only the dynamic linker and a small number
+     of routines called directly from assembler are excluded from this
+     protection.
+
+`--enable-pt_chown'
+     The file `pt_chown' is a helper binary for `grantpt' (*note
+     Pseudo-Terminals: Allocation.) that is installed setuid root to
+     fix up pseudo-terminal ownership.  It is not built by default
+     because systems using the Linux kernel are commonly built with the
+     `devpts' filesystem enabled and mounted at `/dev/pts', which
+     manages pseudo-terminal ownership automatically.  By using
+     `--enable-pt_chown', you may build `pt_chown' and install it
+     setuid and owned by `root'.  The use of `pt_chown' introduces
+     additional security risks to the system and you should enable it
+     only if you understand and accept those risks.
+
+`--disable-werror'
+     By default, the GNU C Library is built with `-Werror'.  If you wish
      to build without this option (for example, if building with a newer
      version of GCC than this version of the GNU C Library was tested
      with, so new warnings cause the build with '-Werror' to fail), you
diff --git a/config.h.in b/config.h.in
index ec9c8bc..990d5e1 100644
--- a/config.h.in
+++ b/config.h.in
@@ -43,6 +43,9 @@
 /* Define if compiler accepts -ftree-loop-distribute-patterns.  */
 #undef  HAVE_CC_INHIBIT_LOOP_TO_LIBCALL
 
+/* The level of stack protection in use for glibc as a whole.  */
+#undef	STACK_PROTECTOR_LEVEL
+
 /* Define if the regparm attribute shall be used for local functions
    (gcc on ix86 only).  */
 #undef	USE_REGPARMS
diff --git a/configure.ac b/configure.ac
index 3c766b7..11982fd 100644
--- a/configure.ac
+++ b/configure.ac
@@ -232,6 +232,18 @@ AC_ARG_ENABLE([bind-now],
 	      [bindnow=no])
 AC_SUBST(bindnow)
 
+dnl Build glibc with -fstack-protector, -fstack-protector-all, or
+dnl -fstack-protector-strong.
+AC_ARG_ENABLE([stack-protector],
+	      AC_HELP_STRING([--enable-stack-protector=@<:@yes|no|all|strong@:>@],
+			     [Use -fstack-protector[-all|-strong] to detect glibc buffer overflows]),
+	      [enable_stack_protector=$enableval],
+	      [enable_stack_protector=no])
+case "$enable_stack_protector" in
+all|yes|no|strong) ;;
+*) AC_MSG_ERROR([Not a valid argument for --enable-stack-protector: \"$enable_stack_protector\"]);;
+esac
+
 dnl On some platforms we cannot use dynamic loading.  We must provide
 dnl static NSS modules.
 AC_ARG_ENABLE([static-nss],
@@ -602,6 +614,44 @@ fi
 test -n "$base_machine" || base_machine=$machine
 AC_SUBST(base_machine)
 
+AC_CACHE_CHECK(for -fstack-protector, libc_cv_ssp, [dnl
+LIBC_TRY_CC_OPTION([$CFLAGS $CPPFLAGS -Werror -fstack-protector],
+		   [libc_cv_ssp=yes],
+		   [libc_cv_ssp=no])
+])
+
+AC_CACHE_CHECK(for -fstack-protector-strong, libc_cv_ssp_strong, [dnl
+LIBC_TRY_CC_OPTION([$CFLAGS $CPPFLAGS -Werror -fstack-protector-strong],
+		   [libc_cv_ssp_strong=yes],
+		   [libc_cv_ssp_strong=no])
+])
+
+AC_CACHE_CHECK(for -fstack-protector-all, libc_cv_ssp_all, [dnl
+LIBC_TRY_CC_OPTION([$CFLAGS $CPPFLAGS -Werror -fstack-protector-all],
+		   [libc_cv_ssp_all=yes],
+		   [libc_cv_ssp_all=no])
+])
+
+stack_protector=
+no_stack_protector=
+if test "$libc_cv_ssp" = yes; then
+  no_stack_protector="-fno-stack-protector"
+fi
+
+if test "$enable_stack_protector" = yes && test "$libc_cv_ssp" = yes; then
+  stack_protector="-fstack-protector"
+  AC_DEFINE(STACK_PROTECTOR_LEVEL, 1)
+elif test "$enable_stack_protector" = all && test "$libc_cv_ssp_all" = yes; then
+  stack_protector="-fstack-protector-all"
+  AC_DEFINE(STACK_PROTECTOR_LEVEL, 2)
+elif test "$enable_stack_protector" = strong && test "$libc_cv_ssp_strong" = yes; then
+  stack_protector="-fstack-protector-strong"
+  AC_DEFINE(STACK_PROTECTOR_LEVEL, 3)
+fi
+AC_SUBST(libc_cv_ssp)
+AC_SUBST(stack_protector)
+AC_SUBST(no_stack_protector)
+
 # For the multi-arch option we need support in the assembler & linker.
 AC_CACHE_CHECK([for assembler and linker STT_GNU_IFUNC support],
 	       libc_cv_ld_gnu_indirect_function, [dnl
@@ -1389,26 +1439,6 @@ else
 fi
 AC_SUBST(fno_unit_at_a_time)
 
-AC_CACHE_CHECK(for -fstack-protector, libc_cv_ssp, [dnl
-LIBC_TRY_CC_OPTION([$CFLAGS $CPPFLAGS -Werror -fstack-protector],
-		   [libc_cv_ssp=yes],
-		   [libc_cv_ssp=no])
-])
-
-AC_CACHE_CHECK(for -fstack-protector-strong, libc_cv_ssp_strong, [dnl
-LIBC_TRY_CC_OPTION([$CFLAGS $CPPFLAGS -Werror -fstack-protector-strong],
-		   [libc_cv_ssp_strong=yes],
-		   [libc_cv_ssp_strong=no])
-])
-
-stack_protector=
-if test "$libc_cv_ssp_strong" = "yes"; then
-  stack_protector="-fstack-protector-strong"
-elif test "$libc_cv_ssp" = "yes"; then
-  stack_protector="-fstack-protector"
-fi
-AC_SUBST(stack_protector)
-
 AC_CACHE_CHECK(whether cc puts quotes around section names,
 	       libc_cv_have_section_quotes,
 	       [cat > conftest.c <<EOF
diff --git a/manual/install.texi b/manual/install.texi
index b329950..e05424a 100644
--- a/manual/install.texi
+++ b/manual/install.texi
@@ -170,6 +170,18 @@ time.  Consult the @file{timezone} subdirectory for more details.
 @item --enable-lock-elision=yes
 Enable lock elision for pthread mutexes by default.
 
+@item --enable-stack-protector
+@itemx --enable-stack-protector=strong
+@itemx --enable-stack-protector=all
+Compile the C library and all other parts of the glibc package
+(including the threading and math libraries, NSS modules, and
+transliteration modules) using the GCC @option{-fstack-protector},
+@option{-fstack-protector-strong} or @option{-fstack-protector-all}
+options to detect stack overruns.  Only the dynamic linker and a small
+number of routines called directly from assembler are excluded from this
+protection.
+
+
 @pindex pt_chown
 @findex grantpt
 @item --enable-pt_chown
-- 
2.8.2.202.g98588b6

^ permalink raw reply	[flat|nested] 44+ messages in thread

* [PATCH 02/14 v6] Initialize the stack guard earlier when linking statically.
  2016-06-07 11:06 --enable-stack-protector for glibc, v7 Nix
                   ` (10 preceding siblings ...)
  2016-06-07 11:26 ` [PATCH 13/14 v7] Do not stack-protect sigreturn stubs Nix
@ 2016-06-07 11:27 ` Nix
  2016-06-24 14:18   ` Florian Weimer
  2016-06-07 11:27 ` [PATCH 01/14 v7] Configury support for --enable-stack-protector Nix
                   ` (2 subsequent siblings)
  14 siblings, 1 reply; 44+ messages in thread
From: Nix @ 2016-06-07 11:27 UTC (permalink / raw)
  To: libc-alpha; +Cc: fweimer, Nick Alcock

From: Nick Alcock <nick.alcock@oracle.com>

The address of the stack canary is stored in a per-thread variable,
which means that we must ensure that the TLS area is intialized before
calling any -fstack-protector'ed functions.  For dynamically linked
applications, we ensure this (in a later patch) by disabling
-fstack-protector for the whole dynamic linker, but for static
applications the AT_ENTRY address is called directly by the kernel, so
we must deal with the problem differently.

So split out the part of pthread initialization that sets up the TCB
(and, more generally, the TLS area) into a separate function (twice --
there is one implementation in libpthread.a, and another outside it for
programs that do not link with libpthread), then call it at
initialization time.  Call that, and move the stack guard initialization
above the DL_SYSDEP_OSCHECK hook, which if set will probably call
functions which are stack-protected (it does on Linux and NaCL too).
We also move apply_irel() up, so that we can still safely call functions
that require ifuncs while in __pthread_initialize_tcb_internal()
(though if stack-protection is enabled we still have to avoid calling
functions that are not stack-protected at this stage).

v2: describe why we don't move apply_irel() up, and the consequences.
v6: We can safely move apply_irel() up now.

	* nptl/nptl-init.c (__pthread_initialize_tcb_internal): New
	function, split out from...
	(__pthread_initialize_minimal_internal): ... here.
	* csu/libc-start.c (LIBC_START_MAIN): Call it.  Move stack canary
	and apply_irel() initialization up.
---
 csu/libc-start.c | 26 +++++++++++++++-----------
 csu/libc-tls.c   |  8 ++++++++
 nptl/nptl-init.c | 11 +++++++----
 3 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/csu/libc-start.c b/csu/libc-start.c
index f4aa01a..c0a8b7a 100644
--- a/csu/libc-start.c
+++ b/csu/libc-start.c
@@ -33,6 +33,7 @@ extern int __libc_multiple_libcs;
 #ifndef SHARED
 # include <dl-osinfo.h>
 extern void __pthread_initialize_minimal (void);
+extern void __pthread_initialize_tcb_internal (void);
 # ifndef THREAD_SET_STACK_GUARD
 /* Only exported for architectures that don't store the stack guard canary
    in thread local area.  */
@@ -178,6 +179,20 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
         }
     }
 
+  /* Perform IREL{,A} relocations.  */
+  apply_irel ();
+
+  /* The stack guard goes into the TCB.  */
+  __pthread_initialize_tcb_internal ();
+
+  /* Set up the stack checker's canary.  */
+  uintptr_t stack_chk_guard = _dl_setup_stack_chk_guard (_dl_random);
+# ifdef THREAD_SET_STACK_GUARD
+  THREAD_SET_STACK_GUARD (stack_chk_guard);
+# else
+  __stack_chk_guard = stack_chk_guard;
+# endif
+
 # ifdef DL_SYSDEP_OSCHECK
   if (!__libc_multiple_libcs)
     {
@@ -187,22 +202,11 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
     }
 # endif
 
-  /* Perform IREL{,A} relocations.  */
-  apply_irel ();
-
   /* Initialize the thread library at least a bit since the libgcc
      functions are using thread functions if these are available and
      we need to setup errno.  */
   __pthread_initialize_minimal ();
 
-  /* Set up the stack checker's canary.  */
-  uintptr_t stack_chk_guard = _dl_setup_stack_chk_guard (_dl_random);
-# ifdef THREAD_SET_STACK_GUARD
-  THREAD_SET_STACK_GUARD (stack_chk_guard);
-# else
-  __stack_chk_guard = stack_chk_guard;
-# endif
-
   /* Set up the pointer guard value.  */
   uintptr_t pointer_chk_guard = _dl_setup_pointer_guard (_dl_random,
 							 stack_chk_guard);
diff --git a/csu/libc-tls.c b/csu/libc-tls.c
index d6425e0..3d67a64 100644
--- a/csu/libc-tls.c
+++ b/csu/libc-tls.c
@@ -241,5 +241,13 @@ void
 __attribute__ ((weak))
 __pthread_initialize_minimal (void)
 {
+}
+
+/* This is the minimal initialization function used when libpthread is
+   not used.  */
+void
+__attribute__ ((weak))
+__pthread_initialize_tcb_internal (void)
+{
   __libc_setup_tls (TLS_INIT_TCB_SIZE, TLS_INIT_TCB_ALIGN);
 }
diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
index bdbdfed..a4626be 100644
--- a/nptl/nptl-init.c
+++ b/nptl/nptl-init.c
@@ -296,21 +296,24 @@ extern void **__libc_dl_error_tsd (void) __attribute__ ((const));
 /* This can be set by the debugger before initialization is complete.  */
 static bool __nptl_initial_report_events __attribute_used__;
 
+#ifndef SHARED
 void
-__pthread_initialize_minimal_internal (void)
+__pthread_initialize_tcb_internal (void)
 {
-#ifndef SHARED
   /* Unlike in the dynamically linked case the dynamic linker has not
      taken care of initializing the TLS data structures.  */
   __libc_setup_tls (TLS_TCB_SIZE, TLS_TCB_ALIGN);
 
-  /* We must prevent gcc from being clever and move any of the
+  /* We must prevent gcc from being clever after inlining and moving any of the
      following code ahead of the __libc_setup_tls call.  This function
      will initialize the thread register which is subsequently
      used.  */
   __asm __volatile ("");
+}
 #endif
-
+void
+__pthread_initialize_minimal_internal (void)
+{
   /* Minimal initialization of the thread descriptor.  */
   struct pthread *pd = THREAD_SELF;
   __pthread_initialize_pids (pd);
-- 
2.8.2.202.g98588b6

^ permalink raw reply	[flat|nested] 44+ messages in thread

* [PATCH 04/14 v7] Mark all machinery needed in early static-link init as -fno-stack-protector.
  2016-06-07 11:06 --enable-stack-protector for glibc, v7 Nix
                   ` (12 preceding siblings ...)
  2016-06-07 11:27 ` [PATCH 01/14 v7] Configury support for --enable-stack-protector Nix
@ 2016-06-07 11:27 ` Nix
  2016-06-24 13:00   ` Florian Weimer
  2016-06-24 15:01 ` --enable-stack-protector for glibc, v7 Florian Weimer
  14 siblings, 1 reply; 44+ messages in thread
From: Nix @ 2016-06-07 11:27 UTC (permalink / raw)
  To: libc-alpha; +Cc: fweimer, Nick Alcock

From: Nick Alcock <nick.alcock@oracle.com>

The startup code in csu/, brk() and sbrk(), and the
__pthread_initialize_tcb_internal() function we just introduced are
needed very early in initialization of a statically-linked program,
before the stack guard is initialized.  Mark all of these as
-fno-stack-protector.

We also finally introduce @libc_cv_ssp@ and @no_stack_protector@, both
substituted by the configury changes made earlier, to detect the case
when -fno-stack-protector is supported by the compiler, and
unconditionally pass it in when this is the case, whether or not
--enable-stack-protector is passed to configure.  (This means that
it'll even work when the compiler's been hacked to pass
-fstack-protector by default, unless the hackage is so broken that
it does so in a way that is impossible to override.)

(At one point we marked __libc_fatal() as non-stack-protected too,
but this was pointless: all it did was call other routines which *are*
stack-protected.  The earliest __libc_fatal() call is in the
DL_SYSDEP_OSCHECK hook on some platforms, when statically linking:
this is fine, since it is after TLS and stack-canary initialization.
I have tested invocation of programs statically and dynamically
linked against this glibc on older kernels on x86 and ARM, and they
still "work", i.e. fail with the appropriate message.)

v2: No longer mark memcpy() as -fno-stack-protector.
v3: Use $(no-stack-protector).
v4: Use inhibit_stack_protector rather than de-protecting all of nptl-init.c.
v5: Don't stack-protect brk() and sbrk() in the shared library.
v7: Add comment in misc/Makefile.  Commit message tweak.

	* config.make.in (have-ssp): New.
	(no-stack-protector): New.
	* csu/Makefile (CFLAGS-.o): Use it.
	(CFLAGS-.og): Likewise.
	(CFLAGS-.op): Likewise.
	(CFLAGS-.os): Likewise.
	* misc/Makefile (CFLAGS-sbrk.o): Likewise.
	(CFLAGS-sbrk.op): Likewise.
	(CFLAGS-sbrk.og): Likewise.
	(CFLAGS-brk.o): Likewise.
	(CFLAGS-brk.op): Likewise.
	(CFLAGS-brk.og): Likewise.
	* nptl/nptl-init.c [!SHARED] (__pthread_initialize_tcb_internal):
	Likewise.
---
 config.make.in   | 2 ++
 csu/Makefile     | 5 +++++
 misc/Makefile    | 9 +++++++++
 nptl/nptl-init.c | 1 +
 4 files changed, 17 insertions(+)

diff --git a/config.make.in b/config.make.in
index 05ed6ec..9afd4ff 100644
--- a/config.make.in
+++ b/config.make.in
@@ -55,7 +55,9 @@ with-fp = @with_fp@
 enable-timezone-tools = @enable_timezone_tools@
 unwind-find-fde = @libc_cv_gcc_unwind_find_fde@
 have-fpie = @libc_cv_fpie@
+have-ssp = @libc_cv_ssp@
 stack-protector = @stack_protector@
+no-stack-protector = @no_stack_protector@
 have-selinux = @have_selinux@
 have-libaudit = @have_libaudit@
 have-libcap = @have_libcap@
diff --git a/csu/Makefile b/csu/Makefile
index 31e8bb9..22afe67 100644
--- a/csu/Makefile
+++ b/csu/Makefile
@@ -45,6 +45,11 @@ before-compile += $(objpfx)version-info.h
 tests := tst-empty tst-atomic tst-atomic-long
 tests-static := tst-empty
 
+CFLAGS-.o += $(no-stack-protector)
+CFLAGS-.og += $(no-stack-protector)
+CFLAGS-.op += $(no-stack-protector)
+CFLAGS-.os += $(no-stack-protector)
+
 ifeq (yes,$(build-shared))
 extra-objs += S$(start-installed-name) gmon-start.os
 ifneq ($(start-installed-name),$(static-start-installed-name))
diff --git a/misc/Makefile b/misc/Makefile
index d7bbc85..ac18fad 100644
--- a/misc/Makefile
+++ b/misc/Makefile
@@ -99,6 +99,15 @@ CFLAGS-getusershell.c = -fexceptions
 CFLAGS-err.c = -fexceptions
 CFLAGS-tst-tsearch.c = $(stack-align-test-flags)
 
+# Called during static library initialization, so turn stack-protection
+# off for non-shared builds.
+CFLAGS-sbrk.o = $(no-stack-protector)
+CFLAGS-sbrk.op = $(no-stack-protector)
+CFLAGS-sbrk.og = $(no-stack-protector)
+CFLAGS-brk.o = $(no-stack-protector)
+CFLAGS-brk.op = $(no-stack-protector)
+CFLAGS-brk.og = $(no-stack-protector)
+
 include ../Rules
 
 $(objpfx)libg.a: $(dep-dummy-lib); $(make-dummy-lib)
diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
index a4626be..2775d14 100644
--- a/nptl/nptl-init.c
+++ b/nptl/nptl-init.c
@@ -298,6 +298,7 @@ static bool __nptl_initial_report_events __attribute_used__;
 
 #ifndef SHARED
 void
+inhibit_stack_protector
 __pthread_initialize_tcb_internal (void)
 {
   /* Unlike in the dynamically linked case the dynamic linker has not
-- 
2.8.2.202.g98588b6

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH 07/14 v6] Prevent the rtld mapfile computation from dragging in __stack_chk_fail*.
  2016-06-07 11:26 ` [PATCH 07/14 v6] Prevent the rtld mapfile computation from dragging in __stack_chk_fail* Nix
@ 2016-06-07 11:39   ` Andreas Schwab
  2016-06-07 12:04     ` Nix
  2016-06-24 13:03   ` Florian Weimer
  1 sibling, 1 reply; 44+ messages in thread
From: Andreas Schwab @ 2016-06-07 11:39 UTC (permalink / raw)
  To: Nix; +Cc: libc-alpha, fweimer, Nick Alcock

Nix <nix@esperi.org.uk> writes:

>  $(objpfx)librtld.map: $(objpfx)dl-allobjs.os $(common-objpfx)libc_pic.a
>  	@-rm -f $@T
> -	$(reloc-link) -o $@.o '-Wl,-(' $^ -lgcc '-Wl,-)' -Wl,-Map,$@T
> +	$(reloc-link) -o $@.o $(dummy-stack-chk-fail) '-Wl,-(' $^ -lgcc '-Wl,-)' -Wl,-Map,$@T
>  	rm -f $@.o
>  	mv -f $@T $@

Please fold long lines.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH 07/14 v6] Prevent the rtld mapfile computation from dragging in __stack_chk_fail*.
  2016-06-07 11:39   ` Andreas Schwab
@ 2016-06-07 12:04     ` Nix
  2016-06-07 23:35       ` Paul Eggert
  0 siblings, 1 reply; 44+ messages in thread
From: Nix @ 2016-06-07 12:04 UTC (permalink / raw)
  To: Andreas Schwab; +Cc: libc-alpha, fweimer

On 7 Jun 2016, Andreas Schwab spake thusly:

> Nix <nix@esperi.org.uk> writes:
>
>>  $(objpfx)librtld.map: $(objpfx)dl-allobjs.os $(common-objpfx)libc_pic.a
>>  	@-rm -f $@T
>> -	$(reloc-link) -o $@.o '-Wl,-(' $^ -lgcc '-Wl,-)' -Wl,-Map,$@T
>> +	$(reloc-link) -o $@.o $(dummy-stack-chk-fail) '-Wl,-(' $^ -lgcc '-Wl,-)' -Wl,-Map,$@T
>>  	rm -f $@.o
>>  	mv -f $@T $@
>
> Please fold long lines.

Oops! Will be fixed in next posting. This and one comment in the same
patch (also fixed) are the only places where I overflow lines that are
fixable (configure.ac is more or less not fixable, AIUI).

-- 
NULL && (void)

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH 07/14 v6] Prevent the rtld mapfile computation from dragging in __stack_chk_fail*.
  2016-06-07 12:04     ` Nix
@ 2016-06-07 23:35       ` Paul Eggert
  0 siblings, 0 replies; 44+ messages in thread
From: Paul Eggert @ 2016-06-07 23:35 UTC (permalink / raw)
  To: Nix; +Cc: libc-alpha, fweimer

On 06/07/2016 05:03 AM, Nix wrote:
> This and one comment in the same
> patch (also fixed) are the only places where I overflow lines that are
> fixable (configure.ac is more or less not fixable, AIUI).

Why is configure.ac not fixable? E.g., in this patch:

+	      AC_HELP_STRING([--enable-stack-protector=@<:@yes|no|all|strong@:>@],
+			     [Use -fstack-protector[-all|-strong] to detect glibc buffer overflows]),


The commentary can contain folded lines without problem (the extra indenting is removed), and the first line can be moved down, e.g.,

   AC_HELP_STRING(
     [--enable-stack-protector=@<:@yes|no|all|strong@:>@],
     [Use -fstack-protector[-all|-strong] to detect glibc
      buffer overflows]),


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH 05/14 v6] Open-code the memcpy() at static TLS initialization time.
  2016-06-07 11:06 ` [PATCH 05/14 v6] Open-code the memcpy() at static TLS initialization time Nix
@ 2016-06-24 12:42   ` Florian Weimer
  2016-06-24 13:11     ` Nix
  0 siblings, 1 reply; 44+ messages in thread
From: Florian Weimer @ 2016-06-24 12:42 UTC (permalink / raw)
  To: Nix, libc-alpha; +Cc: Nick Alcock

On 06/07/2016 01:06 PM, Nix wrote:
> From: Nick Alcock <nick.alcock@oracle.com>
>
> This one is a bit nasty.  Now that we are initializing TLS earlier for
> the stack canary's sake, existing memcpy() implementations become
> problematic.  We can use the multiarch implementations, but they might
> not always be present, and even if they are present they might not always
> be in assembler, so might be compiled with stack-protection.  We cannot
> use posix/memcpy.c without marking both it and */wordcopy.c as non-stack-
> protected, which for memcpy() of all things seems like a seriously bad
> idea: if any function in glibc should be stack-protected, it's memcpy()
> (though stack-protecting the many optimized assembly versions is not done
> in this patch series).

Some of the pathnames in this explanation appear to be incorrect.

I looked at the memcpy.c and wordcopy.c implementations, and I do not 
see why these functions would need the stack protector.  They should not 
have addressable local variables, they should all be in registers.  As a 
result, the implementation should never derive a pointer from the 
current stack frame.

These functions might be passed pointers into the callers stack frame, 
but the memory looks like this (addresses increase from bottom to top, 
so the stack grows downwards on most architectures (except HPPA)):

     :                             :
     +-----------------------------+
     |  Caller stack frame         |
     |                             |
     |  (end of buf)               |
     |    :                        |
     |    :                        |
     |  char buf[64];              |
     |                             |
     +-----------------------------+
     | Return address              |
     | memcpy stack frame          |
     |                             |
     +-(current top of stack)------+
     :                             :
     +=============================+
     | XXX guard page XXXXXXXXXXXX |
     :                             :


As a result, a buffer overflow will run away from the return address 
associated with the memcpy activation frame.  To avoid that, you'd need 
a memcpy call with a destination address which points below the current 
top of stack before memcpy is called, that is an invalid, dangling 
pointer.  A random pointer with an excessive size is unlikely to work 
because the copying operation would eventually hit the guard page below 
the stack.  This is not a straight stack-based overflow anymore, and not 
something stack protector can help with.

(Attacks against runaway memcpy calls (with size arguments approaching 
SIZE_MAX) work against some memcpy implementations not because these 
implementations lack stack protector, but because they load variables 
from the stack in the inner loop, which may have been overwritten by 
attacker-controlled values.)

Anyway, if the above analysis is right, it should be safe to disable 
stack protector for functions such as memcpy (essentially doing manually 
what -fstack-protector-strong does automatically).  This would be my 
preferred approach here.

Thanks,
Florian

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH 04/14 v7] Mark all machinery needed in early static-link init as -fno-stack-protector.
  2016-06-07 11:27 ` [PATCH 04/14 v7] Mark all machinery needed in early static-link init as -fno-stack-protector Nix
@ 2016-06-24 13:00   ` Florian Weimer
  0 siblings, 0 replies; 44+ messages in thread
From: Florian Weimer @ 2016-06-24 13:00 UTC (permalink / raw)
  To: Nix; +Cc: libc-alpha, Nick Alcock

On 06/07/2016 01:06 PM, Nix wrote:
> The startup code in csu/, brk() and sbrk(), and the
> __pthread_initialize_tcb_internal() function we just introduced are
> needed very early in initialization of a statically-linked program,
> before the stack guard is initialized.  Mark all of these as
> -fno-stack-protector.

I think the problem is that the TCB is not initialized, which contains 
the stack guard on some architectures, as explained in other commits.

The change itself looks good to me.

Florian

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH 07/14 v6] Prevent the rtld mapfile computation from dragging in __stack_chk_fail*.
  2016-06-07 11:26 ` [PATCH 07/14 v6] Prevent the rtld mapfile computation from dragging in __stack_chk_fail* Nix
  2016-06-07 11:39   ` Andreas Schwab
@ 2016-06-24 13:03   ` Florian Weimer
  1 sibling, 0 replies; 44+ messages in thread
From: Florian Weimer @ 2016-06-24 13:03 UTC (permalink / raw)
  To: Nix; +Cc: libc-alpha, Nick Alcock

On 06/07/2016 01:06 PM, Nix wrote:
> 	* elf/Makefile (dummy-stack-chk-fail): New.
> 	($(objpfx)librtld.map): Use it.

This looks good to me (except for the long lines already pointed out).

Thanks,
Florian

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH 05/14 v6] Open-code the memcpy() at static TLS initialization time.
  2016-06-24 12:42   ` Florian Weimer
@ 2016-06-24 13:11     ` Nix
  2016-06-24 13:19       ` Florian Weimer
  0 siblings, 1 reply; 44+ messages in thread
From: Nix @ 2016-06-24 13:11 UTC (permalink / raw)
  To: Florian Weimer; +Cc: libc-alpha, Nick Alcock

On 24 Jun 2016, Florian Weimer said:

> On 06/07/2016 01:06 PM, Nix wrote:
>> From: Nick Alcock <nick.alcock@oracle.com>
>>
>> This one is a bit nasty.  Now that we are initializing TLS earlier for
>> the stack canary's sake, existing memcpy() implementations become
>> problematic.  We can use the multiarch implementations, but they might
>> not always be present, and even if they are present they might not always
>> be in assembler, so might be compiled with stack-protection.  We cannot
>> use posix/memcpy.c without marking both it and */wordcopy.c as non-stack-
>> protected, which for memcpy() of all things seems like a seriously bad
>> idea: if any function in glibc should be stack-protected, it's memcpy()
>> (though stack-protecting the many optimized assembly versions is not done
>> in this patch series).
>
> Some of the pathnames in this explanation appear to be incorrect.

Oh, it's string/memcpy.c, isn't it? Sorry.

> I looked at the memcpy.c and wordcopy.c implementations, and I do not
> see why these functions would need the stack protector. They should
> not have addressable local variables, they should all be in registers.

Now we're depending on the details of compiler register allocation for
security? What if the compiler changes?

> As a result, the implementation should never derive a pointer from the
> current stack frame.

Oh, they almost certainly don't, but my attitude here is that I'm less
smart than the attackers and I should just try to protect everything
possible (that doesn't require horrific pain like ld.so would).

> These functions might be passed pointers into the callers stack frame,

That was my worry. I've had exactly that sort of obviously-problematic
bug in the past, as have we all...

> but the memory looks like this (addresses increase from bottom to top,
> so the stack grows downwards on most architectures (except HPPA)):

You see, there are "except"s creeping in already :) every one is a
possible mistake...

>     :                             :
>     +-----------------------------+
>     |  Caller stack frame         |
>     |                             |
>     |  (end of buf)               |
>     |    :                        |
>     |    :                        |
>     |  char buf[64];              |
>     |                             |
>     +-----------------------------+
>     | Return address              |
>     | memcpy stack frame          |
>     |                             |
>     +-(current top of stack)------+
>     :                             :
>     +=============================+
>     | XXX guard page XXXXXXXXXXXX |
>     :                             :
>
>
> As a result, a buffer overflow will run away from the return address associated with the memcpy activation frame.  To avoid that,

Hm, good point. The caller would need stack-protection, but it's got it.

Do we ever see crashes inside memcpy() from return address overwrite?
You're right, I'm not sure I've ever seen one. (But then, I don't use
HP-PA.)

> Anyway, if the above analysis is right, it should be safe to disable
> stack protector for functions such as memcpy (essentially doing
> manually what -fstack-protector-strong does automatically). This would
> be my preferred approach here.

... which would mean we could drop this patch, too.

-- 
NULL && (void)

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH 06/14 v3] Compile the entire dynamic linker with -fno-stack-protector.
  2016-06-07 11:25 ` [PATCH 06/14 v3] Compile the entire dynamic linker with -fno-stack-protector Nix
@ 2016-06-24 13:13   ` Florian Weimer
  0 siblings, 0 replies; 44+ messages in thread
From: Florian Weimer @ 2016-06-24 13:13 UTC (permalink / raw)
  To: Nix; +Cc: libc-alpha, Nick Alcock

On 06/07/2016 01:06 PM, Nix wrote:
> 	* elf/Makefile (elide-stack-protector): New.
> 	(CFLAGS-.os): Use it, eliding $(all-rtld-routines).
> 	(CFLAGS-.oX): Likewise, eliding $(elide-routines.os).
> 	(rtld-CFLAGS): Likewise.

This looks okay to me.

Thanks,
Florian

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH 05/14 v6] Open-code the memcpy() at static TLS initialization time.
  2016-06-24 13:11     ` Nix
@ 2016-06-24 13:19       ` Florian Weimer
  2016-06-24 13:25         ` Nix
  0 siblings, 1 reply; 44+ messages in thread
From: Florian Weimer @ 2016-06-24 13:19 UTC (permalink / raw)
  To: Nix; +Cc: libc-alpha, Nick Alcock

On 06/24/2016 03:10 PM, Nix wrote:
> On 24 Jun 2016, Florian Weimer said:
>
>> On 06/07/2016 01:06 PM, Nix wrote:
>>> From: Nick Alcock <nick.alcock@oracle.com>
>>>
>>> This one is a bit nasty.  Now that we are initializing TLS earlier for
>>> the stack canary's sake, existing memcpy() implementations become
>>> problematic.  We can use the multiarch implementations, but they might
>>> not always be present, and even if they are present they might not always
>>> be in assembler, so might be compiled with stack-protection.  We cannot
>>> use posix/memcpy.c without marking both it and */wordcopy.c as non-stack-
>>> protected, which for memcpy() of all things seems like a seriously bad
>>> idea: if any function in glibc should be stack-protected, it's memcpy()
>>> (though stack-protecting the many optimized assembly versions is not done
>>> in this patch series).
>>
>> Some of the pathnames in this explanation appear to be incorrect.
>
> Oh, it's string/memcpy.c, isn't it? Sorry.

Yes, that seems better.

>> I looked at the memcpy.c and wordcopy.c implementations, and I do not
>> see why these functions would need the stack protector. They should
>> not have addressable local variables, they should all be in registers.
>
> Now we're depending on the details of compiler register allocation for
> security? What if the compiler changes?

The stack protector already has such a dependency on compiler internals. 
  I'm not too worried about this aspect.

>> but the memory looks like this (addresses increase from bottom to top,
>> so the stack grows downwards on most architectures (except HPPA)):
>
> You see, there are "except"s creeping in already :) every one is a
> possible mistake...

HPPA is just very special (and very, very dead).

>> Anyway, if the above analysis is right, it should be safe to disable
>> stack protector for functions such as memcpy (essentially doing
>> manually what -fstack-protector-strong does automatically). This would
>> be my preferred approach here.
>
> ... which would mean we could drop this patch, too.

Wouldn't you have to supply $(no-stack-protector), for the sake of the 
dynamic linker?  Or does the rtld recompilation take care of that?

Thanks,
Florian

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH 03/14 v7] Do not stack-protect ifunc resolvers.
  2016-06-07 11:06 ` [PATCH 03/14 v7] Do not stack-protect ifunc resolvers Nix
@ 2016-06-24 13:21   ` Florian Weimer
  2016-07-05 16:48     ` Nick Alcock
  0 siblings, 1 reply; 44+ messages in thread
From: Florian Weimer @ 2016-06-24 13:21 UTC (permalink / raw)
  To: Nix; +Cc: libc-alpha, Nick Alcock

On 06/07/2016 01:06 PM, Nix wrote:
> When dynamically linking, ifunc resolvers are called before TLS is
> initialized, so they cannot be safely stack-protected.

You should base this on top of Stefan Liebler's IFUNC resolver cleanups. 
  It should be a one-liner then.

Thanks,
Florian

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH 05/14 v6] Open-code the memcpy() at static TLS initialization time.
  2016-06-24 13:19       ` Florian Weimer
@ 2016-06-24 13:25         ` Nix
  2016-06-24 13:53           ` Florian Weimer
  0 siblings, 1 reply; 44+ messages in thread
From: Nix @ 2016-06-24 13:25 UTC (permalink / raw)
  To: Florian Weimer; +Cc: libc-alpha, Nick Alcock

On 24 Jun 2016, Florian Weimer said:

> On 06/24/2016 03:10 PM, Nix wrote:
>> On 24 Jun 2016, Florian Weimer said:
>>> I looked at the memcpy.c and wordcopy.c implementations, and I do not
>>> see why these functions would need the stack protector. They should
>>> not have addressable local variables, they should all be in registers.
>>
>> Now we're depending on the details of compiler register allocation for
>> security? What if the compiler changes?
>
> The stack protector already has such a dependency on compiler internals. I'm not too worried about this aspect.

The stack protector is *part* of the compiler, though. That's a bit
different, in my eyes.

>>> Anyway, if the above analysis is right, it should be safe to disable
>>> stack protector for functions such as memcpy (essentially doing
>>> manually what -fstack-protector-strong does automatically). This would
>>> be my preferred approach here.
>>
>> ... which would mean we could drop this patch, too.
>
> Wouldn't you have to supply $(no-stack-protector), for the sake of the dynamic linker?  Or does the rtld recompilation take care of
> that?

Yeah, that's handled by the elide-stack-protector stuff in elf/Makefile.
(If we had to de-stack-protect everything ld.so used by hand, the patch
really would be unmaintainable.)

-- 
NULL && (void)

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH 12/14] Drop explicit stack-protection of pieces of the system.
  2016-06-07 11:26 ` [PATCH 12/14] Drop explicit stack-protection of pieces of the system Nix
@ 2016-06-24 13:25   ` Florian Weimer
  0 siblings, 0 replies; 44+ messages in thread
From: Florian Weimer @ 2016-06-24 13:25 UTC (permalink / raw)
  To: Nix, libc-alpha; +Cc: Nick Alcock

On 06/07/2016 01:06 PM, Nix wrote:
> From: Nick Alcock <nick.alcock@oracle.com>
>
> This is probably a bad idea: maybe we want to stack-protect some parts
> of the system even when ! --enable-stack-protector.  I can easily adjust
> the patch to do that (though it'll mean introducing a new variable
> analogous to $(stack-protector) but not controlled by the configure
> flag.)
>
> But if we wanted to value consistency over security, and use the same
> stack-protection configure flag to control everything, this is how we'd
> do it!
>
> ("Always include at least one patch with something obviously wrong with
> it.")
>
> 	* login/Makefile (pt_chown-cflags): Remove.
> 	* nscd/Makefile (CFLAGS-nscd): Likewise.
> 	* resolv/Makefile (CFLAGS-libresolv): Likewise.

I would like to see this go in.  The existing logic is very misleading, 
particularly in resolv/Makefile, where nss_dns is *not* protected.

Thanks,
Florian

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH 13/14 v7] Do not stack-protect sigreturn stubs.
  2016-06-07 11:26 ` [PATCH 13/14 v7] Do not stack-protect sigreturn stubs Nix
@ 2016-06-24 13:52   ` Florian Weimer
  0 siblings, 0 replies; 44+ messages in thread
From: Florian Weimer @ 2016-06-24 13:52 UTC (permalink / raw)
  To: Nix, libc-alpha; +Cc: Nick Alcock

On 06/07/2016 01:06 PM, Nix wrote:
> 	* signal/Makefile (CFLAGS-sigreturn.c): Use
> 	$(no-stack-protector).

I still don't think C implementations of sigreturn are reliable (in the 
sense that they keep working despite future compiler changes).  I think 
it's just laziness not have a separate .S file.

But there does not seem to be consensus about this alternative approach, 
so this patch is okay.

Thanks,
Florian

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH 05/14 v6] Open-code the memcpy() at static TLS initialization time.
  2016-06-24 13:25         ` Nix
@ 2016-06-24 13:53           ` Florian Weimer
  0 siblings, 0 replies; 44+ messages in thread
From: Florian Weimer @ 2016-06-24 13:53 UTC (permalink / raw)
  To: Nix; +Cc: libc-alpha, Nick Alcock

On 06/24/2016 03:25 PM, Nix wrote:

> Yeah, that's handled by the elide-stack-protector stuff in elf/Makefile.
> (If we had to de-stack-protect everything ld.so used by hand, the patch
> really would be unmaintainable.)

Okay, then let's drop this.  Thanks.

Florian

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH 02/14 v6] Initialize the stack guard earlier when linking statically.
  2016-06-07 11:27 ` [PATCH 02/14 v6] Initialize the stack guard earlier when linking statically Nix
@ 2016-06-24 14:18   ` Florian Weimer
  0 siblings, 0 replies; 44+ messages in thread
From: Florian Weimer @ 2016-06-24 14:18 UTC (permalink / raw)
  To: Nix; +Cc: libc-alpha, Nick Alcock

On 06/07/2016 01:06 PM, Nix wrote:
> +  /* Perform IREL{,A} relocations.  */
> +  apply_irel ();
> +
> +  /* The stack guard goes into the TCB.  */
> +  __pthread_initialize_tcb_internal ();

I think this order of initialization is wrong.  IFUNC resolvers need the 
TCB.

What's the alternative here?  De-protect everything that is called from 
__pthread_initialize_tcb_internal?

Thanks,
Florian

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH 08/14 v6] Work even with compilers hacked to enable -fstack-protector by default.
  2016-06-07 11:06 ` [PATCH 08/14 v6] Work even with compilers hacked to enable -fstack-protector by default Nix
@ 2016-06-24 14:32   ` Florian Weimer
  0 siblings, 0 replies; 44+ messages in thread
From: Florian Weimer @ 2016-06-24 14:32 UTC (permalink / raw)
  To: Nix, libc-alpha; +Cc: Nick Alcock

On 06/07/2016 01:06 PM, Nix wrote:
> 	* configure.ac: Add check for unsupported stack-protection level.
> 	(libc_cv_predef_stack_protector): Remove.
> 	(no_ssp): New variable.
> 	(libc_cv_ld_gnu_indirect_function): Use it.
> 	(libc_cv_asm_set_directive): Likewise.
> 	(libc_cv_protected_data): Likewise.
> 	(libc_cv_z_combreloc): Likewise.
> 	(libc_cv_hashstyle): Likewise.
> 	(libc_cv_has_glob_dat): Likewise.
> 	(libc_cv_output_format): Likewise.
> 	(libc_cv_ehdr_start): Likewise.
> 	* aclocal.m4 (LIBC_TRY_LINK_STATIC): Likewise.
> 	(LIBC_LINKER_FEATURE): Likewise.
> 	(LIBC_COMPILER_BUILTIN_INLINED): Likewise.

Looks okay to me.

Thanks,
Florian

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH 10/14 v6] De-PLTize __stack_chk_fail internal calls within libc.so.
  2016-06-07 11:25 ` [PATCH 10/14 v6] De-PLTize __stack_chk_fail internal calls within libc.so Nix
@ 2016-06-24 14:37   ` Florian Weimer
  0 siblings, 0 replies; 44+ messages in thread
From: Florian Weimer @ 2016-06-24 14:37 UTC (permalink / raw)
  To: Nix, libc-alpha; +Cc: Adhemerval Zanella

On 06/07/2016 01:06 PM, Nix wrote:
> --- a/sysdeps/generic/symbol-hacks.h
> +++ b/sysdeps/generic/symbol-hacks.h
> @@ -5,3 +5,9 @@ asm ("memmove = __GI_memmove");
>  asm ("memset = __GI_memset");
>  asm ("memcpy = __GI_memcpy");
>  #endif
> +
> +/* -fstack-protector generates calls to __stack_chk_fail, which need
> +   similar adjustments to avoid going through the PLT.  */
> +#if !defined __ASSEMBLER__ && IS_IN (libc) && defined SHARED
> +asm ("__stack_chk_fail = __stack_chk_fail_local");
> +#endif

The condition can be merged with the previous block, I think.

Otherwise, looks reasonable.

Thanks,
Florian

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH 09/14 v6] Add stack_chk_fail_local to libc.so.
  2016-06-07 11:26 ` [PATCH 09/14 v6] Add stack_chk_fail_local to libc.so Nix
@ 2016-06-24 14:39   ` Florian Weimer
  0 siblings, 0 replies; 44+ messages in thread
From: Florian Weimer @ 2016-06-24 14:39 UTC (permalink / raw)
  To: Nix, libc-alpha; +Cc: Nick Alcock

On 06/07/2016 01:06 PM, Nix wrote:
> This is required by the next commit, which routes all
> __stack_chk_fail() calls in libc.so via this function to avoid
> the PLT.  It has be duplicated in libc.so and libc_nonshared.a
> because its entire reason for existence is to be hidden and avoid
> the PLT, so the copy in libc.so is not visible from elsewhere.

Why can't this use the regular libc_hidden_proto/libc_hidden_def 
mechanism?  The proto part is taken care of by the next patch, I get 
that, but why doesn't this follow the usual __GI_ pattern?

Thanks,
Florian

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH 11/14 v3] Link various tests with -fno-stack-protector.
  2016-06-07 11:26 ` [PATCH 11/14 v3] Link various tests with -fno-stack-protector Nix
@ 2016-06-24 14:40   ` Florian Weimer
  0 siblings, 0 replies; 44+ messages in thread
From: Florian Weimer @ 2016-06-24 14:40 UTC (permalink / raw)
  To: Nix, libc-alpha; +Cc: Nick Alcock

On 06/07/2016 01:06 PM, Nix wrote:
> 	* elf/Makefile (CFLAGS-filtmod1.c): Use $(no-stack-protector) for
> 	non-libc-linking testcase.
> 	(CFLAGS-filtmod2.c): Likewise.
> 	* stdlib/Makefile (CFLAGS-tst-putenvmod.c): Likewise.
> 	* sysdeps/x86_64/Makefile (CFLAGS-tst-quad1pie.c): Likewise.
> 	(CFLAGS-tst-quad2pie.c): Likewise.

Okay.  (I assume there is a reason why these tests are linked this way, 
and we want to preserve that.)

Thanks,
Florian

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH 14/14 v5] Enable -fstack-protector=* when requested by configure.
  2016-06-07 11:25 ` [PATCH 14/14 v5] Enable -fstack-protector=* when requested by configure Nix
@ 2016-06-24 14:47   ` Florian Weimer
  0 siblings, 0 replies; 44+ messages in thread
From: Florian Weimer @ 2016-06-24 14:47 UTC (permalink / raw)
  To: Nix, libc-alpha; +Cc: Nick Alcock

On 06/07/2016 01:06 PM, Nix wrote:
> 	* Makeconfig (+stack-protector): New variable.
> 	(+cflags): Use it.

Looks okay, thanks.

Florian

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH 01/14 v7] Configury support for --enable-stack-protector.
  2016-06-07 11:27 ` [PATCH 01/14 v7] Configury support for --enable-stack-protector Nix
@ 2016-06-24 14:50   ` Florian Weimer
  0 siblings, 0 replies; 44+ messages in thread
From: Florian Weimer @ 2016-06-24 14:50 UTC (permalink / raw)
  To: Nix; +Cc: libc-alpha, Nick Alcock

On 06/07/2016 01:06 PM, Nix wrote:

> +	      AC_HELP_STRING([--enable-stack-protector=@<:@yes|no|all|strong@:>@],
> +			     [Use -fstack-protector[-all|-strong] to detect glibc buffer overflows]),

I think this should say: “Compile glibc with 
-fstack-protector{,-all,-strong}”.  The expectation is that this detects 
not just glibc buffer overflows.  (I do not have strong opinions about 
the option formatting.)

> +@item --enable-stack-protector
> +@itemx --enable-stack-protector=strong
> +@itemx --enable-stack-protector=all
> +Compile the C library and all other parts of the glibc package
> +(including the threading and math libraries, NSS modules, and
> +transliteration modules) using the GCC @option{-fstack-protector},
> +@option{-fstack-protector-strong} or @option{-fstack-protector-all}
> +options to detect stack overruns.  Only the dynamic linker and a small
> +number of routines called directly from assembler are excluded from this
> +protection.

I would drop the final sentence.  It does not provide much information 
and is also not entirely correct (being called from assembler doesn't 
make much of a difference).

Thanks,
Florian

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: --enable-stack-protector for glibc, v7
  2016-06-07 11:06 --enable-stack-protector for glibc, v7 Nix
                   ` (13 preceding siblings ...)
  2016-06-07 11:27 ` [PATCH 04/14 v7] Mark all machinery needed in early static-link init as -fno-stack-protector Nix
@ 2016-06-24 15:01 ` Florian Weimer
  2016-06-27 15:28   ` Nix
  14 siblings, 1 reply; 44+ messages in thread
From: Florian Weimer @ 2016-06-24 15:01 UTC (permalink / raw)
  To: Nix; +Cc: libc-alpha

On 06/07/2016 01:06 PM, Nix wrote:
> This is version 7 of the stack-protected glibc patch, incorporating all review
> comments to date (unless I missed some).

I went through this patch series.

Taking stock, I think the only remaining problem is the order of IFUNC 
resolvers and TCB initializers in the static linking case.  The patch 
series does IFUNCs first, then TCBs.  I think we should keep the 
existing order, TCB first, then IFUNCs.  It seems the more conservative 
choice, but I do not have much experience with static linking.

(It might also help to avoid dropping stack protection from IFUNCs, but 
since IFUNCs can do so little, that should not matter.)

Thanks,
Florian

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: --enable-stack-protector for glibc, v7
  2016-06-24 15:01 ` --enable-stack-protector for glibc, v7 Florian Weimer
@ 2016-06-27 15:28   ` Nix
  2016-07-04 15:06     ` Florian Weimer
  0 siblings, 1 reply; 44+ messages in thread
From: Nix @ 2016-06-27 15:28 UTC (permalink / raw)
  To: Florian Weimer; +Cc: libc-alpha

On 24 Jun 2016, Florian Weimer stated:

> On 06/07/2016 01:06 PM, Nix wrote:
>> This is version 7 of the stack-protected glibc patch, incorporating all review
>> comments to date (unless I missed some).
>
> I went through this patch series.
>
> Taking stock, I think the only remaining problem is the order of IFUNC
> resolvers and TCB initializers in the static linking case. The patch
> series does IFUNCs first, then TCBs. I think we should keep the
> existing order, TCB first, then IFUNCs. It seems the more conservative
> choice, but I do not have much experience with static linking.

I didn't either before, I did this work. You get much more experienced
with it when it is your primary source of mysterious crashes for ages :)


As far as I can see, the order has not changed. We used to do:

-  /* Perform IREL{,A} relocations.  */
-  apply_irel ();
-
   /* Initialize the thread library at least a bit since the libgcc
      functions are using thread functions if these are available and
      we need to setup errno.  */
   __pthread_initialize_minimal ();

... and then set up the canary.  pthread_initialize_minimal() sets up
the TCB, so it's IREL, then TCB.

We now do

+  /* Perform IREL{,A} relocations.  */
+  apply_irel ();
+
+  /* The stack guard goes into the TCB.  */
+  __pthread_initialize_tcb_internal ();
+
+  /* Set up the stack checker's canary.  */
+  uintptr_t stack_chk_guard = _dl_setup_stack_chk_guard (_dl_random);
+# ifdef THREAD_SET_STACK_GUARD
+  THREAD_SET_STACK_GUARD (stack_chk_guard);
+# else
+  __stack_chk_guard = stack_chk_guard;
+# endif
+

So it's the same thing, only more explicit.

The real difference is that we now do the TCB init longer before the
rest of pthread initialization, and do all of that before the
sysdep-oscheck stuff, so that we don't need to figure out what the
sysdep-oscheck stuff might call and de-stack-protect it (under the
general rule that the less we stack-protect, the better).

> (It might also help to avoid dropping stack protection from IFUNCs,
> but since IFUNCs can do so little, that should not matter.)

I don't think we can avoid that.  In the dynamic-linking case, the
canary is initialized a long, long time after ifunc resolvers get
applied and ifuncs start getting called, and I don't want to start
messing with ld.so in this patch series :)

(Testing the other changes you suggested now.)

-- 
NULL && (void)

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: --enable-stack-protector for glibc, v7
  2016-06-27 15:28   ` Nix
@ 2016-07-04 15:06     ` Florian Weimer
  2016-07-04 15:15       ` Nix
  0 siblings, 1 reply; 44+ messages in thread
From: Florian Weimer @ 2016-07-04 15:06 UTC (permalink / raw)
  To: Nix; +Cc: libc-alpha

On 06/27/2016 05:27 PM, Nix wrote:
> On 24 Jun 2016, Florian Weimer stated:
>
>> On 06/07/2016 01:06 PM, Nix wrote:
>>> This is version 7 of the stack-protected glibc patch, incorporating all review
>>> comments to date (unless I missed some).
>>
>> I went through this patch series.
>>
>> Taking stock, I think the only remaining problem is the order of IFUNC
>> resolvers and TCB initializers in the static linking case. The patch
>> series does IFUNCs first, then TCBs. I think we should keep the
>> existing order, TCB first, then IFUNCs. It seems the more conservative
>> choice, but I do not have much experience with static linking.
>
> I didn't either before, I did this work. You get much more experienced
> with it when it is your primary source of mysterious crashes for ages :)
>
>
> As far as I can see, the order has not changed. We used to do:

> So it's the same thing, only more explicit.

Right, thanks for explaining this to me.

>> (It might also help to avoid dropping stack protection from IFUNCs,
>> but since IFUNCs can do so little, that should not matter.)
>
> I don't think we can avoid that.  In the dynamic-linking case, the
> canary is initialized a long, long time after ifunc resolvers get
> applied and ifuncs start getting called, and I don't want to start
> messing with ld.so in this patch series :)

But this affects not just IFUNC resolvers in glibc, but also external 
IFUNC resolvers in static libraries.  They might have been compiled with 
-fstack-protector-all, and require a TCB.

The issue here is not that the stack guard has not been initialized, 
it's that the TCB address has not been set.  And as a result of that, 
any TLS is just invalid.  This would also affect error reporting through 
errno.

If we can land Stefan Liebler's IFUNC cleanup, then disabling 
stack-protector for IFUNC resolvers will be a very simple patch.  So we 
could start with that approach, and revisit this topic later.  As you 
pointed out, we are not changing the IFUNC resolver calling order with 
regards to TCB setup, so lack of support for stack-protected IFUNC 
resolvers is not a regression.

Thanks,
Florian

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: --enable-stack-protector for glibc, v7
  2016-07-04 15:06     ` Florian Weimer
@ 2016-07-04 15:15       ` Nix
  2016-07-04 19:24         ` Florian Weimer
  0 siblings, 1 reply; 44+ messages in thread
From: Nix @ 2016-07-04 15:15 UTC (permalink / raw)
  To: Florian Weimer; +Cc: libc-alpha

On 4 Jul 2016, Florian Weimer said:

> On 06/27/2016 05:27 PM, Nix wrote:
>>> (It might also help to avoid dropping stack protection from IFUNCs,
>>> but since IFUNCs can do so little, that should not matter.)
>>
>> I don't think we can avoid that.  In the dynamic-linking case, the
>> canary is initialized a long, long time after ifunc resolvers get
>> applied and ifuncs start getting called, and I don't want to start
>> messing with ld.so in this patch series :)
>
> But this affects not just IFUNC resolvers in glibc, but also external
> IFUNC resolvers in static libraries. They might have been compiled
> with -fstack-protector-all, and require a TCB.

And they'll crash before this patch and will crash afterwards. There is
no change (as you note below). This is probably more important now that
things like function multiversioning are making heavier use of ifuncs
(though I guess that, being part of GCC itself, the function
multiversioning takes care not to stack-protect the resolvers even if
the file is compiled with -fstack-protector-all? If not, that would seem
to be a bug in GCC.)

> The issue here is not that the stack guard has not been initialized,
> it's that the TCB address has not been set. And as a result of that,
> any TLS is just invalid. This would also affect error reporting
> through errno.

Agreed (though any ifunc resolvers using errno are in for a hard, hard
time).

> If we can land Stefan Liebler's IFUNC cleanup, then disabling
> stack-protector for IFUNC resolvers will be a very simple patch. So we
> could start with that approach,

You are of course quite welcome to do that at merge time :) all the
ifunc stuff is in one patch here so is easy for you to drop and replace
with something better. I am not emotionally attached to the bitty
touch-every-resolver approach I'm using now, indeed I'd prefer something
better since the whole thrust of this patch series has been to avoid
having to do anything to every class of any function ('every function in
ld.so' for instance) because the result would be more or less
unmaintainable.

>                                 and revisit this topic later. As you
> pointed out, we are not changing the IFUNC resolver calling order with
> regards to TCB setup, so lack of support for stack-protected IFUNC
> resolvers is not a regression.

Agreed, but that does mean that we need to prevent the things from being
stack-protected!

(fwiw, all tests passed against my patch series with every review
request incorporated bar this one, including the dropping of the
open-coding-strcpy() patch in favour of no-stack-protecting memcpy() et
al: want a reposting? This *does* include the ifuncing up of stuff, for
now, because we do need to do this somehow or all the tests explode in
fire and tears. But those are self-contained and can be replaced with a
better approach easily enough.)

-- 
NULL && (void)

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: --enable-stack-protector for glibc, v7
  2016-07-04 15:15       ` Nix
@ 2016-07-04 19:24         ` Florian Weimer
  0 siblings, 0 replies; 44+ messages in thread
From: Florian Weimer @ 2016-07-04 19:24 UTC (permalink / raw)
  To: Nix; +Cc: libc-alpha

On 07/04/2016 05:15 PM, Nix wrote:
> (fwiw, all tests passed against my patch series with every review
> request incorporated bar this one, including the dropping of the
> open-coding-strcpy() patch in favour of no-stack-protecting memcpy() et
> al: want a reposting? This *does* include the ifuncing up of stuff, for
> now, because we do need to do this somehow or all the tests explode in
> fire and tears. But those are self-contained and can be replaced with a
> better approach easily enough.)

Let's wait until we can get Stefan's series in and you can rebase.

I suggest to split out the definition of inhibit_stack_protector from 
the IFUNC patch because the definition is used later on in the series, too.

Florian

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH 03/14 v7] Do not stack-protect ifunc resolvers.
  2016-06-24 13:21   ` Florian Weimer
@ 2016-07-05 16:48     ` Nick Alcock
  2016-07-06 11:31       ` Florian Weimer
  0 siblings, 1 reply; 44+ messages in thread
From: Nick Alcock @ 2016-07-05 16:48 UTC (permalink / raw)
  To: Florian Weimer; +Cc: libc-alpha

On 24 Jun 2016, Florian Weimer said:

> On 06/07/2016 01:06 PM, Nix wrote:
>> When dynamically linking, ifunc resolvers are called before TLS is
>> initialized, so they cannot be safely stack-protected.
>
> You should base this on top of Stefan Liebler's IFUNC resolver cleanups. It should be a one-liner then.

Hm. OK, I'll rebase the series atop that, once I can figure out which
patches to use :) (or would you rather I pull those patches into my
patch series and not rebase it? I'm happy to do either, or rebase atop
master: I'll have to retest in any case, so the usual arguments against
rebases don't apply, and the testing is almost entirely automated so I
don't mind having to rerun it.)

-- 
NULL && (void)

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH 03/14 v7] Do not stack-protect ifunc resolvers.
  2016-07-05 16:48     ` Nick Alcock
@ 2016-07-06 11:31       ` Florian Weimer
  2016-07-06 18:24         ` Nick Alcock
  0 siblings, 1 reply; 44+ messages in thread
From: Florian Weimer @ 2016-07-06 11:31 UTC (permalink / raw)
  To: Nick Alcock; +Cc: libc-alpha

On 07/05/2016 06:48 PM, Nick Alcock wrote:
> On 24 Jun 2016, Florian Weimer said:
>
>> On 06/07/2016 01:06 PM, Nix wrote:
>>> When dynamically linking, ifunc resolvers are called before TLS is
>>> initialized, so they cannot be safely stack-protected.
>>
>> You should base this on top of Stefan Liebler's IFUNC resolver cleanups. It should be a one-liner then.
>
> Hm. OK, I'll rebase the series atop that, once I can figure out which
> patches to use :) (or would you rather I pull those patches into my
> patch series and not rebase it? I'm happy to do either, or rebase atop
> master: I'll have to retest in any case, so the usual arguments against
> rebases don't apply, and the testing is almost entirely automated so I
> don't mind having to rerun it.)

My intention was to suggest to rebase your series on master after the 
IFUNC patches are in.

Unfortunately, this means that the stack-protector changes will not be 
part of glibc 2.24, but Adhemerval already rejected the stack-protector 
changes as such.

Florian

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [PATCH 03/14 v7] Do not stack-protect ifunc resolvers.
  2016-07-06 11:31       ` Florian Weimer
@ 2016-07-06 18:24         ` Nick Alcock
  0 siblings, 0 replies; 44+ messages in thread
From: Nick Alcock @ 2016-07-06 18:24 UTC (permalink / raw)
  To: Florian Weimer; +Cc: libc-alpha

On 6 Jul 2016, Florian Weimer spake thusly:

> My intention was to suggest to rebase your series on master after the IFUNC patches are in.

Aha! OK, that's fine. I'll do that.

> Unfortunately, this means that the stack-protector changes will not be
> part of glibc 2.24, but Adhemerval already rejected the
> stack-protector changes as such.

Well, as long as it's just for 2.24, and not a NAK for ever (which seems
likely given that Adhemerval *contributed* to these changes and seemed
to like the general idea of them), I'm happy enough with that -- they've
been stuck in limbo since *2.9*, after all. I'm happy to submit them for
2.25.

They are, ah, a bit risky for this late in the cycle: there are surely
obscure arches the series hasn't been tested on and won't be tested on
until it hits trunk.

-- 
NULL && (void)

^ permalink raw reply	[flat|nested] 44+ messages in thread

end of thread, other threads:[~2016-07-06 18:24 UTC | newest]

Thread overview: 44+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-07 11:06 --enable-stack-protector for glibc, v7 Nix
2016-06-07 11:06 ` [PATCH 03/14 v7] Do not stack-protect ifunc resolvers Nix
2016-06-24 13:21   ` Florian Weimer
2016-07-05 16:48     ` Nick Alcock
2016-07-06 11:31       ` Florian Weimer
2016-07-06 18:24         ` Nick Alcock
2016-06-07 11:06 ` [PATCH 08/14 v6] Work even with compilers hacked to enable -fstack-protector by default Nix
2016-06-24 14:32   ` Florian Weimer
2016-06-07 11:06 ` [PATCH 05/14 v6] Open-code the memcpy() at static TLS initialization time Nix
2016-06-24 12:42   ` Florian Weimer
2016-06-24 13:11     ` Nix
2016-06-24 13:19       ` Florian Weimer
2016-06-24 13:25         ` Nix
2016-06-24 13:53           ` Florian Weimer
2016-06-07 11:25 ` [PATCH 06/14 v3] Compile the entire dynamic linker with -fno-stack-protector Nix
2016-06-24 13:13   ` Florian Weimer
2016-06-07 11:25 ` [PATCH 14/14 v5] Enable -fstack-protector=* when requested by configure Nix
2016-06-24 14:47   ` Florian Weimer
2016-06-07 11:25 ` [PATCH 10/14 v6] De-PLTize __stack_chk_fail internal calls within libc.so Nix
2016-06-24 14:37   ` Florian Weimer
2016-06-07 11:26 ` [PATCH 07/14 v6] Prevent the rtld mapfile computation from dragging in __stack_chk_fail* Nix
2016-06-07 11:39   ` Andreas Schwab
2016-06-07 12:04     ` Nix
2016-06-07 23:35       ` Paul Eggert
2016-06-24 13:03   ` Florian Weimer
2016-06-07 11:26 ` [PATCH 11/14 v3] Link various tests with -fno-stack-protector Nix
2016-06-24 14:40   ` Florian Weimer
2016-06-07 11:26 ` [PATCH 09/14 v6] Add stack_chk_fail_local to libc.so Nix
2016-06-24 14:39   ` Florian Weimer
2016-06-07 11:26 ` [PATCH 12/14] Drop explicit stack-protection of pieces of the system Nix
2016-06-24 13:25   ` Florian Weimer
2016-06-07 11:26 ` [PATCH 13/14 v7] Do not stack-protect sigreturn stubs Nix
2016-06-24 13:52   ` Florian Weimer
2016-06-07 11:27 ` [PATCH 02/14 v6] Initialize the stack guard earlier when linking statically Nix
2016-06-24 14:18   ` Florian Weimer
2016-06-07 11:27 ` [PATCH 01/14 v7] Configury support for --enable-stack-protector Nix
2016-06-24 14:50   ` Florian Weimer
2016-06-07 11:27 ` [PATCH 04/14 v7] Mark all machinery needed in early static-link init as -fno-stack-protector Nix
2016-06-24 13:00   ` Florian Weimer
2016-06-24 15:01 ` --enable-stack-protector for glibc, v7 Florian Weimer
2016-06-27 15:28   ` Nix
2016-07-04 15:06     ` Florian Weimer
2016-07-04 15:15       ` Nix
2016-07-04 19:24         ` Florian Weimer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).