--enable-stack-protector for glibc, v4, now with arm

--enable-stack-protector for glibc, v4, now with arm

[Nix]

This is version 4 of the stack-protected glibc patch, incorporating all review
comments to date (unless I missed some), and adding a degree of arm support
(i.e. "I know nothing about the platform but the tests passed").

Still no changelog citing bug 7065: I'm not confident enough that things won't
change, but I'd be more confident if someone could look at patch 15 in this
series and tell me why on earth it's even necessary (which it definitely is).

It's not rebased and is still against glibc head as of a week ago,
a5df3210a641c17.

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
--without-stack-protector
--enable-stack-protector=no

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

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

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

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

No failures are observed that are not also observed on an unpatched glibc with
the same flag combinations, other than localplt (due to the new __stack_chk_fail
PLT, which I'd suggest is desirable: it seems like something people might
reasonably want to interpose, and interposing it will have no performance
implications).

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.


Overview of changes in this posting:

 - use inhibit_loop_to_libcall to prevent GCC folding our open-coded memcpy()
   in static TLS setup back into a (prohibited) libcall again

 - introduce new inhibit_stack_protector that turns off -fstack-protector for a
   single function, and use it to stack-protect only the necessary functions
   within nptl: most of nptl-init.c, pthread_mutex_cond_lock.c and
   pthread_mutex_unlock.c are now protected.

 - De-stack-protect all ifunc resolvers: they're called before the stack canary
   is initialized in the dynamic-linking case, so must never be stack-protected.
   (How it worked before on x86 and sparc I have no idea, but on arm, it fails.)

 - stack-protected glibc now passes its tests on armv7l-unknown-linux-gnueabihf
   (an odroid c1, because I already had one sitting upstairs being a home
   cinema).

Remaining mysteries:

 - In patch 15, I have no understanding of why __pthread_mutex_unlock_usercnt()
   and __pthread_mutex_cond_lock_adjust() are special: yes, they're called
   directly from assembler, but that shouldn't be a problem: -fstack-protector
   doesn't change the function-call ABI!  Nonetheless, it *is* a problem, and
   this is definitely necessary.  I just don't know what *else* might be
   necessary.  It is almost certain that this sort of thing will be the primary
   problem when making other arches happy with this change (it was the only real
   thing I had to do with sparc, in patch 14, -fno-sparc-protecting the
   sigreturn handler, and just as with the pthread functions I don't really know
   why -- but it is clearly necessary.)

   It *is* clear why we have to -fno-stack-protect setjmp/sigjmp.c: the thing is
   *sibcalled* from assembler, and the assembler has pre-built a stack frame,
   complete with lack of canary, so setjmp/sigjmp.c had better not introduce
   one.  (Even in the absence of local variables, -fstack-protector-all will add
   a canary to the lone function in that file.)

[PATCH 02/16] Initialize the stack guard earlier when linking statically.

[Nix]

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 do not move apply_irel() up, because applying irels can require
state set up by the DL_SYSDEP_OSCHECK hook on some platforms.  This
means, as a consequence, that __pthread_initialize_tcb_internal()
had better not call anything that requires ifuncs.  (This is dealt
with in a later patch.)

v2: describe why we don't move apply_irel() up, and the consequences.
---
 csu/libc-start.c | 20 ++++++++++++--------
 csu/libc-tls.c   |  8 ++++++++
 nptl/nptl-init.c | 11 +++++++----
 3 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/csu/libc-start.c b/csu/libc-start.c
index f4aa01a..140b079 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,17 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
         }
     }
 
+  /* 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)
     {
@@ -195,14 +207,6 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
      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.7.0.198.g6dd47b6

[PATCH 05/16] Open-code the memcpy() at static TLS initialization time.

[Nix]

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.

(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.
---
 csu/libc-tls.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/csu/libc-tls.c b/csu/libc-tls.c
index 3d67a64..ef5d76e 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,18 @@ __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.  */
-  memcpy (_dl_static_dtv[2].pointer.val, initimage, filesz);
+
+  /* sbrk gives us zero'd memory, so we don't need to clear the remainder.
+
+     Copy by hand, because memcpy() is stack-protected and is often multiarch
+     too.  */
+
+  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;
 
   /* Install the pointer to the dtv.  */
 
-- 
2.7.0.198.g6dd47b6

[PATCH 12/16] Work even with compilers hacked to enable -fstack-protector by default.

[Nix]

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.
---
 aclocal.m4   |  6 ++---
 configure.ac | 74 +++++++++++++++++++-----------------------------------------
 2 files changed, 26 insertions(+), 54 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 a920f89..55bfe89 100644
--- a/configure.ac
+++ b/configure.ac
@@ -664,6 +664,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 x"$enable_stack_protector" != xno; 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
@@ -683,7 +695,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
@@ -1152,8 +1164,8 @@ extern int glibc_conftest_frobozz;
 void _start() { glibc_conftest_frobozz = 1; }
 EOF
 if ${CC-cc} $CFLAGS $CPPFLAGS $LDFLAGS \
-	    -nostartfiles -nostdlib \
-	    -o conftest conftest.s conftest1.c 1>&AS_MESSAGE_LOG_FD 2>&AS_MESSAGE_LOG_FD; then
+	    -nostartfiles -nostdlib $no_ssp \
+	    -o conftest conftest.s conftest1.c $no_ssp 1>&AS_MESSAGE_LOG_FD 2>&AS_MESSAGE_LOG_FD; then
   libc_cv_asm_set_directive=yes
 else
   libc_cv_asm_set_directive=no
@@ -1169,12 +1181,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
@@ -1296,7 +1308,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
@@ -1334,9 +1346,9 @@ 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])
+			    -Wl,--hash-style=both -nostdlib $no_ssp 1>&AS_MESSAGE_LOG_FD])
 then
   libc_cv_hashstyle=yes
 else
@@ -1406,7 +1418,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.
@@ -1423,7 +1435,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
@@ -1622,46 +1634,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).
@@ -1671,7 +1643,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.7.0.198.g6dd47b6

[PATCH 10/16] Link various tests with -fno-stack-protector.

[Nix]

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            | 4 ++++
 stdlib/Makefile         | 3 +++
 sysdeps/x86_64/Makefile | 3 +++
 3 files changed, 10 insertions(+)

diff --git a/elf/Makefile b/elf/Makefile
index 8f0973d..7a1c939 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -751,6 +751,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.7.0.198.g6dd47b6

[PATCH 09/16] Link libc.so with libc_nonshared.a to pull in __stack_chk_fail.

[Nix]

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

This prevents a spurious undefined reference from libc (which would be
an ABI break).
---
 Makerules | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Makerules b/Makerules
index 53eabfa..d369d78 100644
--- a/Makerules
+++ b/Makerules
@@ -674,6 +674,7 @@ $(common-objpfx)linkobj/libc.so: link-libc-deps = # empty
 # libraries.
 $(common-objpfx)libc.so: $(elf-objpfx)soinit.os \
 			 $(common-objpfx)libc_pic.os$(libc_pic_clean) \
+			 $(common-objpfx)libc_nonshared.a \
 			 $(elf-objpfx)sofini.os \
 			 $(elf-objpfx)interp.os \
 			 $(elf-objpfx)ld.so \
@@ -683,6 +684,7 @@ $(common-objpfx)libc.so: $(elf-objpfx)soinit.os \
 
 $(common-objpfx)linkobj/libc.so: $(elf-objpfx)soinit.os \
 			 $(common-objpfx)linkobj/libc_pic.a \
+			 $(common-objpfx)libc_nonshared.a \
 			 $(elf-objpfx)sofini.os \
 			 $(elf-objpfx)interp.os \
 			 $(elf-objpfx)ld.so \
-- 
2.7.0.198.g6dd47b6

[PATCH 14/16] Drop explicit stack-protection of pieces of the system.

[Nix]

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  | 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.7.0.198.g6dd47b6

[PATCH 04/16] Mark all machinery needed in early static-link init as -fno-stack-protector.

[Nix]

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.
---
 config.make.in   | 2 ++
 csu/Makefile     | 5 +++++
 misc/Makefile    | 4 ++++
 nptl/nptl-init.c | 1 +
 4 files changed, 12 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..78005c8 100644
--- a/misc/Makefile
+++ b/misc/Makefile
@@ -99,6 +99,10 @@ CFLAGS-getusershell.c = -fexceptions
 CFLAGS-err.c = -fexceptions
 CFLAGS-tst-tsearch.c = $(stack-align-test-flags)
 
+# Called during static library initialization.
+CFLAGS-sbrk.c = $(no-stack-protector)
+CFLAGS-brk.c = $(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.7.0.198.g6dd47b6

[PATCH 11/16] Enable -fstack-protector=* when requested by configure.

[Nix]

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

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

v3: Wrap long lines.
---
 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.7.0.198.g6dd47b6

Re: [PATCH 11/16] Enable -fstack-protector=* when requested by configure.

[Andreas Schwab]

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

> +# We might want to compile with some stack-protection flag.
> +ifneq ($(stack-protector),)
> ++stack-protector=$(stack-protector)
> +endif

Why do you need that indirection?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH 11/16] Enable -fstack-protector=* when requested by configure.

[Nix]

On 28 Feb 2016, Andreas Schwab outgrape:

> Nix <nix@esperi.org.uk> writes:
>
>> +# We might want to compile with some stack-protection flag.
>> +ifneq ($(stack-protector),)
>> ++stack-protector=$(stack-protector)
>> +endif
>
> Why do you need that indirection?

For consistency: almost everything else added to +cflags has a
similarly-prepended name (even when it's always set to a literal
constant: e.g +merge-constants and +math-flags).

I'm happy to drop it if people think it's redundant: it's not like
$(+stack-protector) is used anywhere else.

-- 
NULL && (void)

[PATCH 16/16] sparc: do not stack-protect the sigreturn handler.

[Nix]

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

This is called from the kernel and must not have a canary.

v2: New.
v3: Use $(no-stack-protector).
v4: Use inhibit_stack_protector.
---
 sysdeps/unix/sysv/linux/sparc/sparc32/sigaction.c | 8 ++++++--
 sysdeps/unix/sysv/linux/sparc/sparc64/sigaction.c | 4 +++-
 2 files changed, 9 insertions(+), 3 deletions(-)

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.7.0.198.g6dd47b6

[PATCH 01/16] Configury support for --enable-stack-protector.

[Nix]

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.
---
 INSTALL             | 39 ++++++++++++++++++++-----------
 configure.ac        | 67 +++++++++++++++++++++++++++++++++++++----------------
 manual/install.texi | 12 ++++++++++
 3 files changed, 84 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/configure.ac b/configure.ac
index 3c766b7..7b81049 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@:>@],
+                           [Detect stack overflows in glibc functions, either with local buffers (yes), or with those plus arrays (strong), or all functions (all)]),
+            [enable_stack_protector=$enableval],
+            [enable_stack_protector=no])
+case x"$enable_stack_protector" in
+    xall|xyes|xno|xstrong) ;;
+    *) AC_MSG_ERROR([Not a valid argument for --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,41 @@ 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 x$libc_cv_ssp = xyes; then
+    no_stack_protector=-fno-stack-protector
+fi
+
+if test x$enable_stack_protector = xyes && test $libc_cv_ssp = yes; then
+  stack_protector=-fstack-protector
+elif test x$enable_stack_protector = xall && test $libc_cv_ssp_all = yes; then
+  stack_protector=-fstack-protector-all
+elif test x$enable_stack_protector = xstrong && test $libc_cv_ssp_strong = yes; then
+  stack_protector=-fstack-protector-strong
+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 +1436,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.7.0.198.g6dd47b6

[PATCH 06/16] Allow overriding of CFLAGS as well as CPPFLAGS for rtld.

[Nix]

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

We need this to pass -fno-stack-protector to all the pieces of rtld in
non-elf/ directories.
---
 elf/rtld-Rules         | 2 +-
 scripts/sysd-rules.awk | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/elf/rtld-Rules b/elf/rtld-Rules
index 94ca39b..c1bb506 100644
--- a/elf/rtld-Rules
+++ b/elf/rtld-Rules
@@ -90,7 +90,7 @@ else
 
 rtld-compile-command.S = $(compile-command.S) $(rtld-CPPFLAGS)
 rtld-compile-command.s = $(compile-command.s) $(rtld-CPPFLAGS)
-rtld-compile-command.c = $(compile-command.c) $(rtld-CPPFLAGS)
+rtld-compile-command.c = $(compile-command.c) $(rtld-CPPFLAGS) $(rtld-CFLAGS)
 
 # These are the basic compilation rules corresponding to the Makerules ones.
 # The sysd-rules generated makefile already defines pattern rules for rtld-%
diff --git a/scripts/sysd-rules.awk b/scripts/sysd-rules.awk
index cebc9d3..69af400 100644
--- a/scripts/sysd-rules.awk
+++ b/scripts/sysd-rules.awk
@@ -54,7 +54,7 @@ BEGIN {
           command_suffix = "";
         } else {
           prefix = gensub(/%/, "", 1, target_pattern);
-          command_suffix = " $(" prefix  "CPPFLAGS)";
+          command_suffix = " $(" prefix  "CPPFLAGS)" " $(" prefix  "CFLAGS)";
         }
         target = "$(objpfx)" target_pattern o ":";
         if (asm_rules) {
-- 
2.7.0.198.g6dd47b6

[PATCH 08/16] Prevent the rtld mapfile computation from dragging in __stack_chk_fail().

[Nix]

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() and all the libc
and libio code it uses.

To stop this happening, use --defsym in the test librtld.map-production
link to force the linker to predefine __stack_chk_fail() (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.
---
 elf/Makefile | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/elf/Makefile b/elf/Makefile
index 0037cca..8f0973d 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -349,9 +349,19 @@ $(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 a dummy __stack_chk_fail
+# symbol defined, to prevent the real thing 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'
+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.7.0.198.g6dd47b6

[PATCH 07/16] Compile the entire dynamic linker with -fno-stack-protector.

[Nix]

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   | 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 c1bb506..aff4e3e 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.7.0.198.g6dd47b6

[PATCH 13/16] Prohibit stack-protection if the compiler is not capable.

[Nix]

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

We need support for __attribute__ ((__optimize__ ("-fno-stack-protector")))
if -fno-stack-protector is to work reliably.

v4: New.
---
 configure.ac | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/configure.ac b/configure.ac
index 55bfe89..97ac691 100644
--- a/configure.ac
+++ b/configure.ac
@@ -665,6 +665,11 @@ AC_SUBST(stack_protector)
 AC_SUBST(no_stack_protector)
 
 if test -n "$stack_protector"; then
+  dnl We cannot work without the ability to disable -fstack-protector for
+  dnl specific functions.
+  if test $libc_cv_cc_no_stack_protector = no; then
+      AC_MSG_ERROR([--enable-stack-protector not supported without __attribute__ ((__optimize__ ("-fno-stack-protector"))).])
+  fi
   dnl Don't run configure tests with stack-protection on, to avoid problems with
   dnl bootstrapping.
   no_ssp=-fno-stack-protector
-- 
2.7.0.198.g6dd47b6

[PATCH 03/16] Do not stack-protect ifunc resolvers.

[Nix]

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
depending on the availability of __attribute__ ((__optimize__
("-fno-stack-protector"))), and turning 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.
---
 config.h.in                                    |  4 ++++
 configure.ac                                   | 15 +++++++++++++++
 elf/ifuncdep2.c                                |  3 +++
 elf/ifuncmain6pie.c                            |  1 +
 elf/ifuncmain7.c                               |  1 +
 elf/ifuncmod1.c                                |  3 +++
 elf/ifuncmod5.c                                |  3 +++
 include/libc-symbols.h                         | 19 +++++++++++++++++--
 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, 66 insertions(+), 3 deletions(-)

diff --git a/config.h.in b/config.h.in
index ec9c8bc..7bbc148 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
+
 /* 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 7b81049..a920f89 100644
--- a/configure.ac
+++ b/configure.ac
@@ -632,10 +632,25 @@ LIBC_TRY_CC_OPTION([$CFLAGS $CPPFLAGS -Werror -fstack-protector-all],
 		   [libc_cv_ssp_all=no])
 ])
 
+AC_CACHE_CHECK(if $CC accepts -fno-stack-protector with \
+__attribute__ ((__optimize__)), libc_cv_cc_no_stack_protector, [dnl
+cat > conftest.c <<EOF
+void
+__attribute__ ((__optimize__ ("-fno-stack-protector")))
+foo (void) {}
+EOF
+libc_cv_cc_no_stack_protector=no
+if AC_TRY_COMMAND([${CC-cc} $CFLAGS $CPPFLAGS -c conftest.c])
+then
+  libc_cv_cc_no_stack_protector=yes
+fi
+rm -f conftest*])
+
 stack_protector=
 no_stack_protector=
 if test x$libc_cv_ssp = xyes; then
     no_stack_protector=-fno-stack-protector
+    AC_DEFINE(HAVE_CC_NO_STACK_PROTECTOR)
 fi
 
 if test x$enable_stack_protector = xyes && 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..a865511 100644
--- a/include/libc-symbols.h
+++ b/include/libc-symbols.h
@@ -314,6 +314,17 @@ for linking")
 
 #define attribute_relro __attribute__ ((section (".data.rel.ro")))
 
+
+/* Add the compiler optimization to inhibit stack-protection, so that
+   we can 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 +727,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 +741,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.7.0.198.g6dd47b6

[PATCH 15/16] Avoid stack-protecting certain functions called from assembly.

[Nix]

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

This is the problematic part.  Without -fno-stack-protector on
__pthread_mutex_cond_lock_adjust() and __pthread_mutex_unlock_usercnt(),
nptl/tst-cond24 and nptl/tst-cond25 receive a NULL mutex at unlock time
and segfault.  However... I don't understand why.  It is the callee's
responsibility both to add the stack canary and to initialize it, just
like any other local variable.  It has to be, or the ABI for stack-
protected code would be incompatible with that for non-protected code.
But the fact remains that
sysdeps/unix/sysv/linux/i386/pthread_cond_timedwait.S both explicitly
mentions the stack frame layout and calls this function, and this call
goes wrong if we stack-protect it.

So this is somewhere where I need someone to tell me what's special about
sysdeps/unix/sysv/linux/i386/pthread_cond_timedwait.S (and in particular
special about priority-inheritance mutexes: everything else works),
before I can be confident that this is even remotely the right thing to
do.

We also de-stack-protect setjmp/sigjmp.c: it receives a sibcall from
sysdeps/x86_64/setjmp.S and lands in rtld, but is *not* rebuilt by
the machinery that rebuilds almost everything else that lands in
rtld with an appropriate MODULE_NAME.

Similar fixups may be required for things called directly from
assembly on other architectures.

v2: de-stack-protect setjmp/sigjmp.c.
v3: Use $(no-stack-protector).
v4: Use inhibit_stack_protector.
---
 nptl/pthread_mutex_lock.c   | 2 +-
 nptl/pthread_mutex_unlock.c | 2 +-
 setjmp/Makefile             | 4 ++++
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/nptl/pthread_mutex_lock.c b/nptl/pthread_mutex_lock.c
index bdfa529..87a1935 100644
--- a/nptl/pthread_mutex_lock.c
+++ b/nptl/pthread_mutex_lock.c
@@ -520,7 +520,7 @@ hidden_def (__pthread_mutex_lock)
 
 #ifdef NO_INCR
 void
-internal_function
+internal_function inhibit_stack_protector
 __pthread_mutex_cond_lock_adjust (pthread_mutex_t *mutex)
 {
   assert ((mutex->__data.__kind & PTHREAD_MUTEX_PRIO_INHERIT_NP) != 0);
diff --git a/nptl/pthread_mutex_unlock.c b/nptl/pthread_mutex_unlock.c
index 334ce38..5f23af1 100644
--- a/nptl/pthread_mutex_unlock.c
+++ b/nptl/pthread_mutex_unlock.c
@@ -33,7 +33,7 @@ __pthread_mutex_unlock_full (pthread_mutex_t *mutex, int decr)
      __attribute_noinline__;
 
 int
-internal_function attribute_hidden
+internal_function inhibit_stack_protector attribute_hidden
 __pthread_mutex_unlock_usercnt (pthread_mutex_t *mutex, int decr)
 {
   int type = PTHREAD_MUTEX_TYPE_ELISION (mutex);
diff --git a/setjmp/Makefile b/setjmp/Makefile
index 5b677cc..b617a84 100644
--- a/setjmp/Makefile
+++ b/setjmp/Makefile
@@ -35,3 +35,7 @@ tests-static	:= tst-setjmp-static
 include ../Rules
 
 $(objpfx)tst-setjmp-fp: $(libm)
+
+# This is sibcalled directly from arch-specific assembly, included in rtld,
+# but never rebuilt, so it must never be built with stack protection.
+CFLAGS-sigjmp.c += $(no-stack-protector)
-- 
2.7.0.198.g6dd47b6

Re: [PATCH 15/16] Avoid stack-protecting certain functions called from assembly.

[Szabolcs Nagy]

On 28/02/16 16:41, Nix wrote:
> From: Nick Alcock <nick.alcock@oracle.com>
> 
> This is the problematic part.  Without -fno-stack-protector on
> __pthread_mutex_cond_lock_adjust() and __pthread_mutex_unlock_usercnt(),
> nptl/tst-cond24 and nptl/tst-cond25 receive a NULL mutex at unlock time
> and segfault.  However... I don't understand why.  It is the callee's
> responsibility both to add the stack canary and to initialize it, just
> like any other local variable.  It has to be, or the ABI for stack-
> protected code would be incompatible with that for non-protected code.
> But the fact remains that
> sysdeps/unix/sysv/linux/i386/pthread_cond_timedwait.S both explicitly
> mentions the stack frame layout and calls this function, and this call
> goes wrong if we stack-protect it.
> 
> So this is somewhere where I need someone to tell me what's special about
> sysdeps/unix/sysv/linux/i386/pthread_cond_timedwait.S (and in particular
> special about priority-inheritance mutexes: everything else works),
> before I can be confident that this is even remotely the right thing to
> do.
> 

i think this should be investigated before adding any
workarounds there.

> We also de-stack-protect setjmp/sigjmp.c: it receives a sibcall from
> sysdeps/x86_64/setjmp.S and lands in rtld, but is *not* rebuilt by
> the machinery that rebuilds almost everything else that lands in
> rtld with an appropriate MODULE_NAME.
> 
> Similar fixups may be required for things called directly from
> assembly on other architectures.
> 

i don't understand the details here, but i don't think calling
something from asm should be a problem, only early function calls
(before thread pointer setup) should be problematic (on targets
where the canary is in the tcb).

> v2: de-stack-protect setjmp/sigjmp.c.
> v3: Use $(no-stack-protector).
> v4: Use inhibit_stack_protector.

Re: [PATCH 15/16] Avoid stack-protecting certain functions called from assembly.

[Nix]

On 2 Mar 2016, Szabolcs Nagy said:

> On 28/02/16 16:41, Nix wrote:
>> From: Nick Alcock <nick.alcock@oracle.com>
>> 
>> This is the problematic part.  Without -fno-stack-protector on
>> __pthread_mutex_cond_lock_adjust() and __pthread_mutex_unlock_usercnt(),
>> nptl/tst-cond24 and nptl/tst-cond25 receive a NULL mutex at unlock time
>> and segfault.  However... I don't understand why.  It is the callee's
>> responsibility both to add the stack canary and to initialize it, just
>> like any other local variable.  It has to be, or the ABI for stack-
>> protected code would be incompatible with that for non-protected code.
>> But the fact remains that
>> sysdeps/unix/sysv/linux/i386/pthread_cond_timedwait.S both explicitly
>> mentions the stack frame layout and calls this function, and this call
>> goes wrong if we stack-protect it.
>> 
>> So this is somewhere where I need someone to tell me what's special about
>> sysdeps/unix/sysv/linux/i386/pthread_cond_timedwait.S (and in particular
>> special about priority-inheritance mutexes: everything else works),
>> before I can be confident that this is even remotely the right thing to
>> do.
>
> i think this should be investigated before adding any
> workarounds there.

I'm reasonably certain it has to do with pthread_cond_timedwait.S
somehow setting up a stack frame and locals below it in a form which
__pthread_mutex_{cond_lock_adjust,unlock_usercnt} then rely upon. I just
don't understand how they could be doing that, since they seem to be
calling the normal entry-point as usual. It's almost as if the call
skips the usual stack-frame setup for the local variables in that
function (including the canary), only I don't see how that could even be
possible. But de-protecting them clearly works...

>> We also de-stack-protect setjmp/sigjmp.c: it receives a sibcall from
>> sysdeps/x86_64/setjmp.S and lands in rtld, but is *not* rebuilt by
>> the machinery that rebuilds almost everything else that lands in
>> rtld with an appropriate MODULE_NAME.
>> 
>> Similar fixups may be required for things called directly from
>> assembly on other architectures.
>
> i don't understand the details here, but i don't think calling
> something from asm should be a problem, only early function calls
> (before thread pointer setup) should be problematic (on targets
> where the canary is in the tcb).

Well yes, that's what I'd assume too! But that's clearly not so :(
This is the one great mystery, and I'm fairly glad to see that it's
mysterious to other people too so it's not just that I'm missing the
incredibly obvious somehow. Anyone know?

-- 
NULL && (void)

Re: [PATCH 15/16] Avoid stack-protecting certain functions called from assembly.

[Andreas Schwab]

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

> I'm reasonably certain it has to do with pthread_cond_timedwait.S
> somehow setting up a stack frame and locals below it in a form which
> __pthread_mutex_{cond_lock_adjust,unlock_usercnt} then rely upon. I just
> don't understand how they could be doing that, since they seem to be
> calling the normal entry-point as usual. It's almost as if the call
> skips the usual stack-frame setup for the local variables in that
> function (including the canary), only I don't see how that could even be
> possible. But de-protecting them clearly works...

Have you tried stepping through the function to find the place where it
breaks?

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."

Re: [PATCH 15/16] Avoid stack-protecting certain functions called from assembly.

[Nix]

On 3 Mar 2016, Andreas Schwab stated:

> Nix <nix@esperi.org.uk> writes:
>
>> I'm reasonably certain it has to do with pthread_cond_timedwait.S
>> somehow setting up a stack frame and locals below it in a form which
>> __pthread_mutex_{cond_lock_adjust,unlock_usercnt} then rely upon. I just
>> don't understand how they could be doing that, since they seem to be
>> calling the normal entry-point as usual. It's almost as if the call
>> skips the usual stack-frame setup for the local variables in that
>> function (including the canary), only I don't see how that could even be
>> possible. But de-protecting them clearly works...
>
> Have you tried stepping through the function to find the place where it
> breaks?

Yeah, but it was years ago, when x86 asm was even more opaque to me than
it is now. It looked like it was intentionally nulling the mutex
parameter out and then using that null later on -- but if my
vaguely-supported supposition in the message I just sent to Adhemerval
regarding the way the assembly implementation is sorta-sibcalling
functions overlaid on its own stack frame is right, I'm not surprised at
all that it was failing like that: the canary would have thrown off all
the arguments after it, in addition to overwriting something it wasn't
meant to on the stack frame (which seems to have had a hole left in it,
uncommented, for the 'type' local variable), following which the
assignment to the 'type' local variable in
__pthread_mutex_unlock_usercnt() would have blown up the next variable
in the caller's overlaid stack frame... oops.

This is all speculation, really, I don't read x86 asm well enough to be
sure what's going on in there. (If I did, I'd know what I was doing in
this change and this whole conversation would not arise!)

-- 
NULL && (void)

Re: --enable-stack-protector for glibc, v4, now with arm

[Nix]

On 28 Feb 2016, nix@esperi.org.uk spake thusly:

> This is version 4 of the stack-protected glibc patch, incorporating all review
> comments to date (unless I missed some), and adding a degree of arm support
> (i.e. "I know nothing about the platform but the tests passed").

So... other than the changelog, is there anything else I need to do to
get review of the trickier parts of this? I've exhausted all the
platforms I have access to and fixed all regressions on such platforms,
but am happy to test/fix on more if people give me access to them.

-- 
NULL && (void)

Re: --enable-stack-protector for glibc, v4, now with arm

[Adhemerval Zanella]

On 02-03-2016 15:28, Nix wrote:
> On 28 Feb 2016, nix@esperi.org.uk spake thusly:
> 
>> This is version 4 of the stack-protected glibc patch, incorporating all review
>> comments to date (unless I missed some), and adding a degree of arm support
>> (i.e. "I know nothing about the platform but the tests passed").
> 
> So... other than the changelog, is there anything else I need to do to
> get review of the trickier parts of this? I've exhausted all the
> platforms I have access to and fixed all regressions on such platforms,
> but am happy to test/fix on more if people give me access to them.
> 

I did not review the patch themselves in detail, but I gave them a try on
aarch64 and ppc64le using the default --enable-stack-protector. I saw 
only one regression, which I also saw on i686 and x86_64 and (and makes
me question either why you did not see it or if you just overlook it):

FAIL: elf/check-localplt

$ cat elf/check-localplt.out 
Extra PLT reference: libc.so: __stack_chk_fail

Since there is no information indicating __stack_chk_fail is local,
the linker creates internal PLT calls.  We can solve it by the same
strategy we used on memcpy/memmove compiler call generation by using
asm symbol hack directive:

diff --git a/sysdeps/generic/symbol-hacks.h b/sysdeps/generic/symbol-hacks.h
index ce576c9..12829cc 100644
--- a/sysdeps/generic/symbol-hacks.h
+++ b/sysdeps/generic/symbol-hacks.h
@@ -5,3 +5,7 @@ asm ("memmove = __GI_memmove");
 asm ("memset = __GI_memset");
 asm ("memcpy = __GI_memcpy");
 #endif
+
+#if !defined __ASSEMBLER__ && IS_IN (libc)
+asm ("__stack_chk_fail = __stack_chk_fail_local");
+#endif

This seems to fix x86_64 and powerpc64le (also if we decide to add this
patch please add a comments explaining the issue).

And I also agree with Szabolcs Nagy about patch 15/16, we need to understand
better what is happening on the mentioned assembly routines before adding
hacks. Have you tried remove the assembly implementations for i386 and
let is use the default C implementation to check if it is something
related to the asm routines themselves?

Re: --enable-stack-protector for glibc, v4, now with arm

[Nix]

On 2 Mar 2016, Adhemerval Zanella stated:

> On 02-03-2016 15:28, Nix wrote:
>> On 28 Feb 2016, nix@esperi.org.uk spake thusly:
>> 
>>> This is version 4 of the stack-protected glibc patch, incorporating all review
>>> comments to date (unless I missed some), and adding a degree of arm support
>>> (i.e. "I know nothing about the platform but the tests passed").
>> 
>> So... other than the changelog, is there anything else I need to do to
>> get review of the trickier parts of this? I've exhausted all the
>> platforms I have access to and fixed all regressions on such platforms,
>> but am happy to test/fix on more if people give me access to them.
>
> I did not review the patch themselves in detail, but I gave them a try on
> aarch64 and ppc64le using the default --enable-stack-protector. I saw 
> only one regression,

OK, if it worked that well not only on 64-bit ARM but also on a platform
I never went near, I think we can say all the major holes are covered!
(I honestly expected a bit of per-arch protection to be needed on most
arches, as was needed on SPARC and x86_64 both -- but none was needed on
32-bit ARM either, so maybe I was being too paranoid.)

>                      which I also saw on i686 and x86_64 and (and makes
> me question either why you did not see it or if you just overlook it):
>
> FAIL: elf/check-localplt
>
> $ cat elf/check-localplt.out 
> Extra PLT reference: libc.so: __stack_chk_fail
>
> Since there is no information indicating __stack_chk_fail is local,
> the linker creates internal PLT calls.

I left that in specifically because I wasn't sure which way to go (we
see it on all platforms, including SPARC and ARM). It is quite possibly
desirable to de-PLTify this (because these calls are frequently made),
but it's *also* quite possibly desirable not to do that, because, like
malloc(), __stack_chk_fail() might reasonably be something that people
might want to intercept, to change what happens on stack overrun.

I'm not sure which is preferable. I guess if the fortification hooks
aren't overrideable, neither should these be.  BUt then, we can't really
use __fortify_fail() as a model, because glibc *implements*
fortification, so it contains all the calls to __fortify_fail() itself:
the same is not true of stack-protection, where random user programs can
and will also call __stack_chk_fail().

>                                        We can solve it by the same
> strategy we used on memcpy/memmove compiler call generation by using
> asm symbol hack directive:

Yeah, that would work. __stack_chk_fail would still be there, and
callable by everyone else, and overrideable for users outside of glibc
(because __stack_chk_fail() still *has* a PLT). Of course, if any users
do override it, they'll see rather inconsistent behaviour, because libc
will still use the old one, but libresolv, for example, will use the
overridden one! Is that inconsistency something that we should just
ignore?

> This seems to fix x86_64 and powerpc64le (also if we decide to add this
> patch please add a comments explaining the issue).

Definitely!

> And I also agree with Szabolcs Nagy about patch 15/16, we need to understand
> better what is happening on the mentioned assembly routines before adding
> hacks. Have you tried remove the assembly implementations for i386 and
> let is use the default C implementation to check if it is something
> related to the asm routines themselves?

It must be: other platforms don't have those implementations, and also
don't need the hacks (they didn't exist before I started working on this
patch series, and x86_64 was fine: only x86_32 saw test failures).

In the case of __sigjmp_save, the cause is clear: it's sibcalled to from
assembler and clearly must be (One of my worries is that other arches
may have assembler that does sibcalls in similar fashion without
mentioning it in comments or anything so my greps failed to find it...
but I hope that if any such exists, it'll pop up as a bug so we can fix
it, or someone here will know and mention it.)

Hm.

Looking at the prototypes of the functions in question, we see (dropping
my additions):

int
internal_function attribute_hidden
__pthread_mutex_unlock_usercnt (pthread_mutex_t *mutex, int decr)

void
internal_function
__pthread_mutex_cond_lock_adjust (pthread_mutex_t *mutex)

and looking at the assembler in
sysdeps/unix/sysv/linux/i386/pthread_cond_timedwait.S, it is obvious
enough even to someone with my extremely minimal x86 asm experience that
this thing is not exactly feeling constrained by the normal way one
calls functions: the fast unlock path is calling
__pthread_mutex_unlock_usercnt() without having at any point adjusted
%esp downwards or anything, it does that afterwards on a slow path --
essentially the call is overlaid on top of the stack frame of
pthread_cond_timedwait() stack frame, like a weird handrolled sibcall
which returns conentionally or something. I'm not surprised that adding
a canary to something called like *that* makes things explode: I suspect
that adding any extra local variables to
__pthread_mutex_unlock_usercnt() that weren't optimized out would do the
same thing. (If so, there should probably be a warning comment in that
function!)

__pthread_mutex_cond_lock_adjust() can also be called in a similar
fashion (jumping via labels 1, 2, 5): __pthread_mutex_cond_lock() is
also called the same way but the function returns immediately
afterwards, and it also gets called more conventionally, so I guess no
harm is done there.)

Now if anyone who's actually done more than a trivial amount of x86
assembler in the last fifteen years could confirm that, I'd be very
happy... since it's quite possible I'm misreading it all. :)

Re: --enable-stack-protector for glibc, v4, now with arm

[Adhemerval Zanella]

On 03-03-2016 14:34, Nix wrote:
> On 2 Mar 2016, Adhemerval Zanella stated:
> 
>> On 02-03-2016 15:28, Nix wrote:
>>> On 28 Feb 2016, nix@esperi.org.uk spake thusly:
>>>
>>>> This is version 4 of the stack-protected glibc patch, incorporating all review
>>>> comments to date (unless I missed some), and adding a degree of arm support
>>>> (i.e. "I know nothing about the platform but the tests passed").
>>>
>>> So... other than the changelog, is there anything else I need to do to
>>> get review of the trickier parts of this? I've exhausted all the
>>> platforms I have access to and fixed all regressions on such platforms,
>>> but am happy to test/fix on more if people give me access to them.
>>
>> I did not review the patch themselves in detail, but I gave them a try on
>> aarch64 and ppc64le using the default --enable-stack-protector. I saw 
>> only one regression,
> 
> OK, if it worked that well not only on 64-bit ARM but also on a platform
> I never went near, I think we can say all the major holes are covered!
> (I honestly expected a bit of per-arch protection to be needed on most
> arches, as was needed on SPARC and x86_64 both -- but none was needed on
> 32-bit ARM either, so maybe I was being too paranoid.)

I thought too, although based on the patchset the level of arch-specific
hackery is still limited on some specific implementations.

I see also there are some arch specific implementations that issues
stack allocation (for instance strcspn for powerpc64) which won't have
instrumented code.  However I do not see how to accomplish unless it
it is being explicit added on such implementations.  I do no see this
as an impeding for patch, but I think it might require some notes in
documentation.

> 
>>                      which I also saw on i686 and x86_64 and (and makes
>> me question either why you did not see it or if you just overlook it):
>>
>> FAIL: elf/check-localplt
>>
>> $ cat elf/check-localplt.out 
>> Extra PLT reference: libc.so: __stack_chk_fail
>>
>> Since there is no information indicating __stack_chk_fail is local,
>> the linker creates internal PLT calls.
> 
> I left that in specifically because I wasn't sure which way to go (we
> see it on all platforms, including SPARC and ARM). It is quite possibly
> desirable to de-PLTify this (because these calls are frequently made),
> but it's *also* quite possibly desirable not to do that, because, like
> malloc(), __stack_chk_fail() might reasonably be something that people
> might want to intercept, to change what happens on stack overrun.
> 
> I'm not sure which is preferable. I guess if the fortification hooks
> aren't overrideable, neither should these be.  BUt then, we can't really
> use __fortify_fail() as a model, because glibc *implements*
> fortification, so it contains all the calls to __fortify_fail() itself:
> the same is not true of stack-protection, where random user programs can
> and will also call __stack_chk_fail().

I am not sure either, although imho I would prefer to not allow user to
override *internal* GLIBC stack overrun.  So I am inclined in de-PLTify
internal libc.so usage of __stack_chk_fail().

> 
>>                                        We can solve it by the same
>> strategy we used on memcpy/memmove compiler call generation by using
>> asm symbol hack directive:
> 
> Yeah, that would work. __stack_chk_fail would still be there, and
> callable by everyone else, and overrideable for users outside of glibc
> (because __stack_chk_fail() still *has* a PLT). Of course, if any users
> do override it, they'll see rather inconsistent behaviour, because libc
> will still use the old one, but libresolv, for example, will use the
> overridden one! Is that inconsistency something that we should just
> ignore?

One option is add a private symbol in libc.so, say __stack_chk_fail_glibc,
and add the alias for other libraries to this symbol.  This won't prevent
an user to really override with non-default options (like implementing
the __stack_chk_fail_glibc as well).

Another option will be to link each library with stack_chk_fail.o and
pulling a local copy of it and thus preventing overriding. 

> 
>> This seems to fix x86_64 and powerpc64le (also if we decide to add this
>> patch please add a comments explaining the issue).
> 
> Definitely!
> 
>> And I also agree with Szabolcs Nagy about patch 15/16, we need to understand
>> better what is happening on the mentioned assembly routines before adding
>> hacks. Have you tried remove the assembly implementations for i386 and
>> let is use the default C implementation to check if it is something
>> related to the asm routines themselves?
> 
> It must be: other platforms don't have those implementations, and also
> don't need the hacks (they didn't exist before I started working on this
> patch series, and x86_64 was fine: only x86_32 saw test failures).
> 
> In the case of __sigjmp_save, the cause is clear: it's sibcalled to from
> assembler and clearly must be (One of my worries is that other arches
> may have assembler that does sibcalls in similar fashion without
> mentioning it in comments or anything so my greps failed to find it...
> but I hope that if any such exists, it'll pop up as a bug so we can fix
> it, or someone here will know and mention it.)
> 
> Hm.
> 
> Looking at the prototypes of the functions in question, we see (dropping
> my additions):
> 
> int
> internal_function attribute_hidden
> __pthread_mutex_unlock_usercnt (pthread_mutex_t *mutex, int decr)
> 
> void
> internal_function
> __pthread_mutex_cond_lock_adjust (pthread_mutex_t *mutex)
> 
> and looking at the assembler in
> sysdeps/unix/sysv/linux/i386/pthread_cond_timedwait.S, it is obvious
> enough even to someone with my extremely minimal x86 asm experience that
> this thing is not exactly feeling constrained by the normal way one
> calls functions: the fast unlock path is calling
> __pthread_mutex_unlock_usercnt() without having at any point adjusted
> %esp downwards or anything, it does that afterwards on a slow path --
> essentially the call is overlaid on top of the stack frame of
> pthread_cond_timedwait() stack frame, like a weird handrolled sibcall
> which returns conentionally or something. I'm not surprised that adding
> a canary to something called like *that* makes things explode: I suspect
> that adding any extra local variables to
> __pthread_mutex_unlock_usercnt() that weren't optimized out would do the
> same thing. (If so, there should probably be a warning comment in that
> function!)
> 
> __pthread_mutex_cond_lock_adjust() can also be called in a similar
> fashion (jumping via labels 1, 2, 5): __pthread_mutex_cond_lock() is
> also called the same way but the function returns immediately
> afterwards, and it also gets called more conventionally, so I guess no
> harm is done there.)
> 
> Now if anyone who's actually done more than a trivial amount of x86
> assembler in the last fifteen years could confirm that, I'd be very
> happy... since it's quite possible I'm misreading it all. :)
> 

IMHO I would just prefer to get rid of this specific assembly 
implementation.  In fact there are some patches that do intend to do
it (new cond var from Tovarlds and my cancellation refactor) and your
work is another reason to avoid large reimplementation in assembly
with a very strong performance reason to do so.

Re: --enable-stack-protector for glibc, v4, now with arm

[Nix]

On 3 Mar 2016, Adhemerval Zanella outgrape:

> On 03-03-2016 14:34, Nix wrote:
>> OK, if it worked that well not only on 64-bit ARM but also on a platform
>> I never went near, I think we can say all the major holes are covered!
>> (I honestly expected a bit of per-arch protection to be needed on most
>> arches, as was needed on SPARC and x86_64 both -- but none was needed on
>> 32-bit ARM either, so maybe I was being too paranoid.)
>
> I thought too, although based on the patchset the level of arch-specific
> hackery is still limited on some specific implementations.
>
> I see also there are some arch specific implementations that issues
> stack allocation (for instance strcspn for powerpc64) which won't have
> instrumented code.  However I do not see how to accomplish unless it
> it is being explicit added on such implementations.  I do no see this
> as an impeding for patch, but I think it might require some notes in
> documentation.

Yeah, there are plenty of those. All the accelerated assembler
implementations of everything will not get stack-protection, at least
not yet, which includes a great many string functions. But this is a
start, and the remaining pieces can probably be filled in piecemeal by
someone who knows the assembler in question :)

>> I'm not sure which is preferable. I guess if the fortification hooks
>> aren't overrideable, neither should these be.  BUt then, we can't really
>> use __fortify_fail() as a model, because glibc *implements*
>> fortification, so it contains all the calls to __fortify_fail() itself:
>> the same is not true of stack-protection, where random user programs can
>> and will also call __stack_chk_fail().
>
> I am not sure either, although imho I would prefer to not allow user to
> override *internal* GLIBC stack overrun.  So I am inclined in de-PLTify
> internal libc.so usage of __stack_chk_fail().

OK, I'll make that change. (My worry is that people using things like X
servers, where if it dies the whole system might end up more or less
locking up, might want to replace __stack_chk_fail() in order to
possibly switch stacks and return the hardware to a sane state. This is
less relevant now that most X servers don't pound on the hardware
directly, though...)

>> Yeah, that would work. __stack_chk_fail would still be there, and
>> callable by everyone else, and overrideable for users outside of glibc
>> (because __stack_chk_fail() still *has* a PLT). Of course, if any users
>> do override it, they'll see rather inconsistent behaviour, because libc
>> will still use the old one, but libresolv, for example, will use the
>> overridden one! Is that inconsistency something that we should just
>> ignore?
>
> One option is add a private symbol in libc.so, say __stack_chk_fail_glibc,
> and add the alias for other libraries to this symbol.  This won't prevent
> an user to really override with non-default options (like implementing
> the __stack_chk_fail_glibc as well).

Yeah.

> Another option will be to link each library with stack_chk_fail.o and
> pulling a local copy of it and thus preventing overriding. 

I was trying to avoid that sort of duplication. It seemed to be a recipe
for trouble later on -- but it *does* have that one advantage.

I am neutral on this, pick one :)

>> and looking at the assembler in
>> sysdeps/unix/sysv/linux/i386/pthread_cond_timedwait.S, it is obvious
>> enough even to someone with my extremely minimal x86 asm experience that
>> this thing is not exactly feeling constrained by the normal way one
>> calls functions: the fast unlock path is calling
>> __pthread_mutex_unlock_usercnt() without having at any point adjusted
>> %esp downwards or anything, it does that afterwards on a slow path --
>> essentially the call is overlaid on top of the stack frame of
>> pthread_cond_timedwait() stack frame, like a weird handrolled sibcall
>> which returns conentionally or something. I'm not surprised that adding
>> a canary to something called like *that* makes things explode: I suspect
>> that adding any extra local variables to
>> __pthread_mutex_unlock_usercnt() that weren't optimized out would do the
>> same thing. (If so, there should probably be a warning comment in that
>> function!)
>> 
>> __pthread_mutex_cond_lock_adjust() can also be called in a similar
>> fashion (jumping via labels 1, 2, 5): __pthread_mutex_cond_lock() is
>> also called the same way but the function returns immediately
>> afterwards, and it also gets called more conventionally, so I guess no
>> harm is done there.)
>> 
>> Now if anyone who's actually done more than a trivial amount of x86
>> assembler in the last fifteen years could confirm that, I'd be very
>> happy... since it's quite possible I'm misreading it all. :)
>
> IMHO I would just prefer to get rid of this specific assembly 
> implementation.  In fact there are some patches that do intend to do
> it (new cond var from Tovarlds and my cancellation refactor) and your
> work is another reason to avoid large reimplementation in assembly
> with a very strong performance reason to do so.

I'd guess the strong performance reason was probably to make the
uncontended path blazingly fast (look at it, it's hardly any
instructions at all). Isn't that the whole point of futexes?

Anyway, I'm not ditching the implementation in this patch series!

-- 
NULL && (void)

Re: --enable-stack-protector for glibc, v4, now with arm

[Nix]

On 3 Mar 2016, nix@esperi.org.uk said:

> I left that in specifically because I wasn't sure which way to go (we
> see it on all platforms, including SPARC and ARM). It is quite possibly
> desirable to de-PLTify this (because these calls are frequently made),
> but it's *also* quite possibly desirable not to do that, because, like
> malloc(), __stack_chk_fail() might reasonably be something that people
> might want to intercept, to change what happens on stack overrun.

"It's plausible" is of course code for "I have no idea and am guessing".

I just had a look on codesearch.debian.net. Overriders (ignoring other
libcs and early runtimes in things like openbios and grub which do
not link against a libc anyway):

physfs, on MacOS X only, because libssp is missing;
wine, overriding it with nothing just to placate the linker;
dmtcp, just prints an error and aborts, would be happy with ours;
       run in bizarre circumstances out of an mmap()ed memory region and
       also includes stubs for a lot of libgcc
condor, provides __stack_chk_fail itself iff glibc is old, so clearly
        *expects* glibc to provide it

*Nobody* overrides __stack_chk_fail with anything significant enough
that they'd be unhappy if we provided an implementation, so I'm going to
go with what you suggested and just de-PLTify it for local within-glibc
calls, leaving it unchanged for everything else.


I also note this code in the binutils ld testsuite:

    set flags "--defsym __stack_chk_fail=0"

which gives me confidence that my independent evolution of the same
horrendous hack in the librtld mapfile computation is not all that bad :)

-- 
NULL && (void)

Re: --enable-stack-protector for glibc, v4, now with arm

[Nix]

On 3 Mar 2016, nix@esperi.org.uk spake thusly:
> Looking at the prototypes of the functions in question, we see (dropping
> my additions):
>
> int
> internal_function attribute_hidden
> __pthread_mutex_unlock_usercnt (pthread_mutex_t *mutex, int decr)
>
> void
> internal_function
> __pthread_mutex_cond_lock_adjust (pthread_mutex_t *mutex)
>
> and looking at the assembler in
> sysdeps/unix/sysv/linux/i386/pthread_cond_timedwait.S, it is obvious
> enough even to someone with my extremely minimal x86 asm experience that
> this thing is not exactly feeling constrained by the normal way one
> calls functions

Well, no. That's because internal_function expands to -mregparm on x86,
which I quite forgot -- and the cause depends on this.

Look at line 313 of pthread_cond_wait.S. Here's the block around it:

        /* We need to go back to futex_wait.  If we're using requeue_pi, then
           release the mutex we had acquired and go back.  */
22:     movl    16(%esp), %edx
        test    %edx, %edx
        jz      8b

        /* Adjust the mutex values first and then unlock it.  The unlock
           should always succeed or else the kernel did not lock the mutex
           correctly.  */
        movl    dep_mutex(%ebx), %eax
        call    __pthread_mutex_cond_lock_adjust
        xorl    %edx, %edx
        call    __pthread_mutex_unlock_usercnt
        jmp     8b

Now that second call to __pthread_mutex_unlock_usercnt() is interesting,
because *no* effort is made to set up the first parameter, the mutex, in
%eax: it's assumed to persist from __pthread_mutex_cond_lock_adjust().
And indeed if you look at the generated x86 assembler for the
non-stack-protected version of __pthread_mutex_cond_lock_adjust(),
you'll see that %eax is consulted but is never modified at any point.
However, when stack-protecting, %eax is used in the epilogue to test the
stack canary, and is smashed in the process, wrecking the call to
__pthread_mutex_unlock_usercnt(). (The second argument, passed in %edx,
is zeroed right there, so no such dependency exists for it.)

pthread_cond_timedwait.S has identical code in label 15.

How should I put it, this is dodgy. This is really dodgy. It's depending
on the compiler's register allocation for regparm functions never
changing to clobber %eax, which, well, we've already seen how reliable
*that* is. It's caller-saved and the caller is not saving it!

Adding a single instruction to both files to reload %eax fixes it, at
minimal cost (this is already a fallback path), and lets us stack-
protect the entire C-side mutex unlocking code path, eliminating the
mystery. I'll do that in the next patch series.

I have also audited all the x86 code in glibc for signs of the same
malaise, and found one instance, in
sysdeps/mach/hurd/i386/____longjmp_chk.S: I fixed that by decorating
_hurd_self_sigstate with inhibit_stack_protector, both because I can't
test it easily and because it seemed far simpler than the converse (that
function is normally inlined in any case). That'll also be in the
next series.

(Testing now, on the usual combination of x86-32/64, sparc-32/64 and
ARM.)