public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
* --enable-stack-protector for glibc, v9
@ 2016-11-28 12:33 Nix
  2016-11-28 12:33 ` [PATCH 04/12] Compile the entire dynamic linker with -fno-stack-protector Nix
                   ` (11 more replies)
  0 siblings, 12 replies; 40+ messages in thread
From: Nix @ 2016-11-28 12:33 UTC (permalink / raw)
  To: libc-alpha; +Cc: fweimer, Nix

Here, as promised, is version 9 of the stack-protected glibc patch,
incorporating all review comments to date (unless I missed some).  Sorry for the
delay: my ARM test box was acting unreliable and I spent way too long ruling out
obscure failure modes before I noticed that its swap device was throwing I/O
errors on all writes...

It's against glibc head as of Saturday 26th, bf469f0ce98.

Tested with these flag combinations on {i686,x86_64)-pc-linux-gnu (with GCC
6.2.1-20161118, binutils 2.27.0.20160920, and kernel headers for v4.7.4):

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

(The only skipped tests were the AVX math tests, since the test machine is not
AVX-capable.)

Tested with with these flag combinations on sparc{32,64}-pc-linux-gnu (with GCC
4.9.1-20140922 (a bit old, sorry), binutils 2.24, and kernel headers for
v4.1.12):

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

Tested with these flag combinations on armv7l-unknown-linux-gnueabihf (with GCC
4.8.5-2ubuntu1~14.04.1 (so -strong isn't available), binutils 2.24, and kernel
headers for v3.13.11):

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

No failures are observed that are not also observed on an unpatched glibc with
the same flag combinations (though there was one round of failures of the
localedata/wcs* tests with --enable-stack-protector on x86-32, and an
intermittent failure of the assertion in stdlib/tst-makecontext on sparc32,
these went away on retesting, so I regard them as likely spurious and unrelated
to the stack-protector patches.  The makecontext assertion also seems likely to
depend on the behaviour of the installed libgcc_s.so...)

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

Overview of changes in this posting:

 - Drop some more libc_cv_predef_stack_protector checks that landed since my
   last series

 - Adjust assignment of rtld-CFLAGS to allow for i386 adding -mno-sse etc to it

 - Rebase atop the latest glibc and the ifunc_resolver work

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

* [PATCH 07/12] Add stack_chk_fail_local to libc.so.
  2016-11-28 12:33 --enable-stack-protector for glibc, v9 Nix
                   ` (3 preceding siblings ...)
  2016-11-28 12:33 ` [PATCH 12/12] Enable -fstack-protector=* when requested by configure Nix
@ 2016-11-28 12:33 ` Nix
  2016-11-28 13:25 ` [PATCH 08/12] De-PLTize __stack_chk_fail internal calls within libc.so Nix
                   ` (6 subsequent siblings)
  11 siblings, 0 replies; 40+ messages in thread
From: Nix @ 2016-11-28 12:33 UTC (permalink / raw)
  To: libc-alpha; +Cc: fweimer, Nick Alcock

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

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

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

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

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

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

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

* [PATCH 12/12] Enable -fstack-protector=* when requested by configure.
  2016-11-28 12:33 --enable-stack-protector for glibc, v9 Nix
                   ` (2 preceding siblings ...)
  2016-11-28 12:33 ` [PATCH 03/12] Mark all machinery needed in early static-link init as -fno-stack-protector Nix
@ 2016-11-28 12:33 ` Nix
  2016-11-28 12:33 ` [PATCH 07/12] Add stack_chk_fail_local to libc.so Nix
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 40+ messages in thread
From: Nix @ 2016-11-28 12:33 UTC (permalink / raw)
  To: libc-alpha; +Cc: fweimer, Nick Alcock

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

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

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

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

diff --git a/Makeconfig b/Makeconfig
index a785860..2b84d93 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.10.1.208.gbec66bc

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

* [PATCH 03/12] Mark all machinery needed in early static-link init as -fno-stack-protector.
  2016-11-28 12:33 --enable-stack-protector for glibc, v9 Nix
  2016-11-28 12:33 ` [PATCH 04/12] Compile the entire dynamic linker with -fno-stack-protector Nix
  2016-11-28 12:33 ` [PATCH 06/12] Work even with compilers hacked to enable -fstack-protector by default Nix
@ 2016-11-28 12:33 ` Nix
  2016-12-15 14:01   ` Florian Weimer
  2016-11-28 12:33 ` [PATCH 12/12] Enable -fstack-protector=* when requested by configure Nix
                   ` (8 subsequent siblings)
  11 siblings, 1 reply; 40+ messages in thread
From: Nix @ 2016-11-28 12:33 UTC (permalink / raw)
  To: libc-alpha; +Cc: fweimer, Nick Alcock

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

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

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

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

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

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

diff --git a/config.make.in b/config.make.in
index 04a8b3e..bfc7d39 100644
--- a/config.make.in
+++ b/config.make.in
@@ -58,7 +58,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 3d2ebb8..c382f92 100644
--- a/misc/Makefile
+++ b/misc/Makefile
@@ -105,6 +105,15 @@ CFLAGS-getusershell.c = -fexceptions
 CFLAGS-err.c = -fexceptions
 CFLAGS-tst-tsearch.c = $(stack-align-test-flags)
 
+# Called during static library initialization, so turn stack-protection
+# off for non-shared builds.
+CFLAGS-sbrk.o = $(no-stack-protector)
+CFLAGS-sbrk.op = $(no-stack-protector)
+CFLAGS-sbrk.og = $(no-stack-protector)
+CFLAGS-brk.o = $(no-stack-protector)
+CFLAGS-brk.op = $(no-stack-protector)
+CFLAGS-brk.og = $(no-stack-protector)
+
 include ../Rules
 
 $(objpfx)libg.a: $(dep-dummy-lib); $(make-dummy-lib)
diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
index dea335d..3288a85 100644
--- a/nptl/nptl-init.c
+++ b/nptl/nptl-init.c
@@ -287,6 +287,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
diff --git a/string/Makefile b/string/Makefile
index 69d3f80..3e35dca 100644
--- a/string/Makefile
+++ b/string/Makefile
@@ -71,6 +71,10 @@ CFLAGS-stratcliff.c = -fno-builtin
 CFLAGS-test-ffs.c = -fno-builtin
 CFLAGS-tst-inlcall.c = -fno-builtin
 
+# Called during TLS initialization.
+CFLAGS-memcpy.c = $(no-stack-protector)
+CFLAGS-wordcopy.c = $(no-stack-protector)
+
 ifeq ($(run-built-tests),yes)
 $(objpfx)tst-svc-cmp.out: tst-svc.expect $(objpfx)tst-svc.out
 	cmp $^ > $@; \
-- 
2.10.1.208.gbec66bc

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

* [PATCH 04/12] Compile the entire dynamic linker with -fno-stack-protector.
  2016-11-28 12:33 --enable-stack-protector for glibc, v9 Nix
@ 2016-11-28 12:33 ` Nix
  2016-11-28 12:33 ` [PATCH 06/12] Work even with compilers hacked to enable -fstack-protector by default Nix
                   ` (10 subsequent siblings)
  11 siblings, 0 replies; 40+ messages in thread
From: Nix @ 2016-11-28 12:33 UTC (permalink / raw)
  To: libc-alpha; +Cc: fweimer, Nick Alcock

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

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

v3: Use $(no-stack-protector).
    Introduce $(elide-stack-protector) and use it to reduce redundancy.
    Bring all the elisions together textually.
v9: Adapt to no-sse adjustments in sysdeps/i386/Makefile.

	* elf/Makefile (elide-stack-protector): New.
	(CFLAGS-.os): Use it, eliding $(all-rtld-routines).
	(CFLAGS-.oX): Likewise, eliding $(elide-routines.os).
	(rtld-CFLAGS): Likewise.
	sysdeps/i386/Makefile (rtld-CFLAGS): Use +=, not =.
---
 elf/Makefile          | 13 +++++++++++++
 elf/rtld-Rules        |  2 ++
 sysdeps/i386/Makefile |  2 +-
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/elf/Makefile b/elf/Makefile
index 82c7e05..d14d48d 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..84d9387 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
diff --git a/sysdeps/i386/Makefile b/sysdeps/i386/Makefile
index e94f2cb..e30e133 100644
--- a/sysdeps/i386/Makefile
+++ b/sysdeps/i386/Makefile
@@ -88,7 +88,7 @@ endif
 # the first 3 mm/xmm/ymm/zmm registers are used to pass vector parameters
 # which must be preserved.
 # With SSE disabled, ensure -fpmath is not set to use sse either.
-rtld-CFLAGS = -mno-sse -mno-mmx -mfpmath=387
+rtld-CFLAGS += -mno-sse -mno-mmx -mfpmath=387
 ifeq ($(subdir),elf)
 CFLAGS-.os += $(if $(filter $(@F),$(patsubst %,%.os,$(all-rtld-routines))),\
 		   $(rtld-CFLAGS))
-- 
2.10.1.208.gbec66bc

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

* [PATCH 06/12] Work even with compilers hacked to enable -fstack-protector by default.
  2016-11-28 12:33 --enable-stack-protector for glibc, v9 Nix
  2016-11-28 12:33 ` [PATCH 04/12] Compile the entire dynamic linker with -fno-stack-protector Nix
@ 2016-11-28 12:33 ` Nix
  2016-11-28 12:33 ` [PATCH 03/12] Mark all machinery needed in early static-link init as -fno-stack-protector Nix
                   ` (9 subsequent siblings)
  11 siblings, 0 replies; 40+ messages in thread
From: Nix @ 2016-11-28 12:33 UTC (permalink / raw)
  To: libc-alpha; +Cc: fweimer, Nick Alcock

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

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

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

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

	* configure.ac: Add check for unsupported stack-protection level.
	(libc_cv_predef_stack_protector): Remove.
	(no_ssp): New variable.
	(libc_cv_ld_gnu_indirect_function): Use it.
	(libc_cv_asm_set_directive): Likewise.
	(libc_cv_protected_data): Likewise.
	(libc_cv_z_combreloc): Likewise.
	(libc_cv_hashstyle): Likewise.
	(libc_cv_has_glob_dat): Likewise.
	(libc_cv_output_format): Likewise.
	(libc_cv_ehdr_start): Likewise.
	* aclocal.m4 (LIBC_TRY_LINK_STATIC): Likewise.
	(LIBC_LINKER_FEATURE): Likewise.
	(LIBC_COMPILER_BUILTIN_INLINED): Likewise.
---
 aclocal.m4   |  6 ++---
 configure.ac | 72 ++++++++++++++++++------------------------------------------
 2 files changed, 24 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 da37319..2396c1f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -656,6 +656,18 @@ AC_SUBST(libc_cv_ssp)
 AC_SUBST(stack_protector)
 AC_SUBST(no_stack_protector)
 
+if test -n "$stack_protector"; then
+  dnl Don't run configure tests with stack-protection on, to avoid problems with
+  dnl bootstrapping.
+  no_ssp=-fno-stack-protector
+else
+  no_ssp=
+
+  if test "$enable_stack_protector" != no; then
+    AC_MSG_ERROR([--enable-stack-protector=$enable_stack_protector specified, but specified level of stack protection is not supported by the compiler.])
+  fi
+fi
+
 # For the multi-arch option we need support in the assembler & linker.
 AC_CACHE_CHECK([for assembler and linker STT_GNU_IFUNC support],
 	       libc_cv_ld_gnu_indirect_function, [dnl
@@ -675,7 +687,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
@@ -1185,7 +1197,7 @@ extern int glibc_conftest_frobozz;
 void _start() { glibc_conftest_frobozz = 1; }
 EOF
 if ${CC-cc} $CFLAGS $CPPFLAGS $LDFLAGS \
-	    -nostartfiles -nostdlib \
+	    -nostartfiles -nostdlib $no_ssp \
 	    -o conftest conftest.s conftest1.c 1>&AS_MESSAGE_LOG_FD 2>&AS_MESSAGE_LOG_FD; then
   libc_cv_asm_set_directive=yes
 else
@@ -1202,12 +1214,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
@@ -1329,7 +1341,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
@@ -1367,7 +1379,7 @@ AC_CACHE_CHECK(for --hash-style option,
 cat > conftest.c <<EOF
 int _start (void) { return 42; }
 EOF
-if AC_TRY_COMMAND([${CC-cc} $CFLAGS $CPPFLAGS $LDFLAGS
+if AC_TRY_COMMAND([${CC-cc} $CFLAGS $CPPFLAGS $LDFLAGS $no_ssp
 			    -fPIC -shared -o conftest.so conftest.c
 			    -Wl,--hash-style=both -nostdlib 1>&AS_MESSAGE_LOG_FD])
 then
@@ -1439,7 +1451,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.
@@ -1456,7 +1468,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
@@ -1675,48 +1687,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 or __stack_chk_fail_local. However,
-# compilers like that have not been encountered in practice.
-libc_undefs=`echo "$libc_undefs" | \
-  egrep '^(foobar|__stack_chk_fail|__stack_chk_fail_local)$'`
-case "$libc_undefs" in
-foobar) libc_cv_predef_stack_protector=no ;;
-'__stack_chk_fail
-foobar'|'__stack_chk_fail_local
-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).
@@ -1726,7 +1696,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.10.1.208.gbec66bc

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

* [PATCH 08/12] De-PLTize __stack_chk_fail internal calls within libc.so.
  2016-11-28 12:33 --enable-stack-protector for glibc, v9 Nix
                   ` (4 preceding siblings ...)
  2016-11-28 12:33 ` [PATCH 07/12] Add stack_chk_fail_local to libc.so Nix
@ 2016-11-28 13:25 ` Nix
  2016-12-15 13:56   ` Florian Weimer
  2016-11-28 13:25 ` [PATCH 09/12] Link various tests with -fno-stack-protector Nix
                   ` (5 subsequent siblings)
  11 siblings, 1 reply; 40+ messages in thread
From: Nix @ 2016-11-28 13:25 UTC (permalink / raw)
  To: libc-alpha; +Cc: fweimer, Adhemerval Zanella

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

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

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

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

diff --git a/sysdeps/generic/symbol-hacks.h b/sysdeps/generic/symbol-hacks.h
index ce576c9..36908b5 100644
--- a/sysdeps/generic/symbol-hacks.h
+++ b/sysdeps/generic/symbol-hacks.h
@@ -4,4 +4,8 @@
 asm ("memmove = __GI_memmove");
 asm ("memset = __GI_memset");
 asm ("memcpy = __GI_memcpy");
+
+/* -fstack-protector generates calls to __stack_chk_fail, which need
+   similar adjustments to avoid going through the PLT.  */
+asm ("__stack_chk_fail = __stack_chk_fail_local");
 #endif
-- 
2.10.1.208.gbec66bc

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

* [PATCH 09/12] Link various tests with -fno-stack-protector.
  2016-11-28 12:33 --enable-stack-protector for glibc, v9 Nix
                   ` (5 preceding siblings ...)
  2016-11-28 13:25 ` [PATCH 08/12] De-PLTize __stack_chk_fail internal calls within libc.so Nix
@ 2016-11-28 13:25 ` Nix
  2016-12-15 14:43   ` Florian Weimer
  2016-11-28 13:25 ` [PATCH 10/12] Drop explicit stack-protection of pieces of the system Nix
                   ` (4 subsequent siblings)
  11 siblings, 1 reply; 40+ messages in thread
From: Nix @ 2016-11-28 13:25 UTC (permalink / raw)
  To: libc-alpha; +Cc: fweimer, Nick Alcock

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

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

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

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

diff --git a/elf/Makefile b/elf/Makefile
index daf0ebd..7588ca0 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -775,6 +775,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 3cce9d9..6d7586e 100644
--- a/stdlib/Makefile
+++ b/stdlib/Makefile
@@ -187,6 +187,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 6d99284..fbe138f 100644
--- a/sysdeps/x86_64/Makefile
+++ b/sysdeps/x86_64/Makefile
@@ -46,6 +46,9 @@ tests-pie += $(quad-pie-test)
 test-extras += tst-quadmod1pie tst-quadmod2pie
 extra-test-objs += tst-quadmod1pie.o tst-quadmod2pie.o
 
+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.10.1.208.gbec66bc

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

* [PATCH 10/12] Drop explicit stack-protection of pieces of the system.
  2016-11-28 12:33 --enable-stack-protector for glibc, v9 Nix
                   ` (6 preceding siblings ...)
  2016-11-28 13:25 ` [PATCH 09/12] Link various tests with -fno-stack-protector Nix
@ 2016-11-28 13:25 ` Nix
  2016-11-28 13:26 ` [PATCH 11/12] Do not stack-protect sigreturn stubs Nix
                   ` (3 subsequent siblings)
  11 siblings, 0 replies; 40+ messages in thread
From: Nix @ 2016-11-28 13:25 UTC (permalink / raw)
  To: libc-alpha; +Cc: fweimer, Nick Alcock

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

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

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

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

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

diff --git a/login/Makefile b/login/Makefile
index 9ff36d6..1a6161c 100644
--- a/login/Makefile
+++ b/login/Makefile
@@ -58,7 +58,6 @@ CFLAGS-getpt.c = -fexceptions
 ifeq (yesyes,$(have-fpie)$(build-shared))
 pt_chown-cflags += $(pie-ccflag)
 endif
-pt_chown-cflags += $(stack-protector)
 ifeq (yes,$(have-libcap))
 libcap = -lcap
 endif
diff --git a/nscd/Makefile b/nscd/Makefile
index 50bad32..bfd72d5 100644
--- a/nscd/Makefile
+++ b/nscd/Makefile
@@ -84,7 +84,6 @@ CPPFLAGS-nscd += -D_FORTIFY_SOURCE=2
 ifeq (yesyes,$(have-fpie)$(build-shared))
 CFLAGS-nscd += $(pie-ccflag)
 endif
-CFLAGS-nscd += $(stack-protector)
 
 ifeq (yesyes,$(have-fpie)$(build-shared))
 LDFLAGS-nscd = -Wl,-z,now
diff --git a/resolv/Makefile b/resolv/Makefile
index be20368..06329e1 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 DNS NSS modules needs the resolver.
-- 
2.10.1.208.gbec66bc

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

* [PATCH 11/12] Do not stack-protect sigreturn stubs.
  2016-11-28 12:33 --enable-stack-protector for glibc, v9 Nix
                   ` (7 preceding siblings ...)
  2016-11-28 13:25 ` [PATCH 10/12] Drop explicit stack-protection of pieces of the system Nix
@ 2016-11-28 13:26 ` Nix
  2016-11-28 13:26 ` [PATCH 01/12] Initialize the stack guard earlier when linking statically Nix
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 40+ messages in thread
From: Nix @ 2016-11-28 13:26 UTC (permalink / raw)
  To: libc-alpha; +Cc: fweimer, Nick Alcock

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

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

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

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

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

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

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

* [PATCH 02/12] Do not stack-protect ifunc resolvers.
  2016-11-28 12:33 --enable-stack-protector for glibc, v9 Nix
                   ` (9 preceding siblings ...)
  2016-11-28 13:26 ` [PATCH 01/12] Initialize the stack guard earlier when linking statically Nix
@ 2016-11-28 13:26 ` Nix
  2016-12-15 12:18   ` Florian Weimer
  2016-11-28 13:26 ` [PATCH 05/12] Prevent the rtld mapfile computation from dragging in __stack_chk_fail* Nix
  11 siblings, 1 reply; 40+ messages in thread
From: Nix @ 2016-11-28 13:26 UTC (permalink / raw)
  To: libc-alpha; +Cc: fweimer, Nick Alcock

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

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

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

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

	* config.h.in (HAVE_CC_NO_STACK_PROTECTOR): New macro.
	* include/libc-symbols.h (inhibit_stack_protector): New macro.
	(__ifunc_resolver): Use it.
	* elf/ifuncdep2.c (foo1_ifunc): Add inhibit_stack_protector.
	(foo2_ifunc): Likewise.
	(foo3_ifunc): Likewise.
	* elf/ifuncmain6pie.c (foo_ifunc): Likewise.
	* elf/ifuncmain7.c (foo_ifunc): Likewise.
	* elf/ifuncmod1.c (foo_ifunc): Likewise.
	(foo_hidden_ifunc): Likewise.
	(foo_protected_ifunc): Likewise.
	* elf/ifuncmod5.c (foo_ifunc): Likewise.
	(foo_hidden_ifunc): Likewise.
	(foo_protected_ifunc): Likewise.
	* sysdeps/x86_64/ifuncmod8.c (foo_ifunc): Likewise.
	* sysdeps/generic/ifunc-sel.h (ifunc_sel): Likewise.
	(ifunc_one): Likewise.
	* sysdeps/nacl/nacl_interface_query.c [SHARED]
	(nacl_interface_query_ifunc): Likewise.
	* sysdeps/powerpc/ifunc-sel.h (ifunc_sel): Likewise.
	(ifunc_one): Likewise.
	* sysdeps/unix/make-syscalls.sh: Likewise.
	* sysdeps/unix/sysv/linux/powerpc/gettimeofday.c
	(gettimeofday_ifunc): Likewise.
	* sysdeps/unix/sysv/linux/x86/gettimeofday.c (gettimeofday_ifunc):
	Likewise.
	* sysdeps/unix/sysv/linux/x86_64/x32/getcpu.c (getcpu_ifunc):
	Likewise.
---
 config.h.in                                    |  4 ++++
 configure.ac                                   |  1 +
 elf/ifuncdep2.c                                |  3 +++
 elf/ifuncmain6pie.c                            |  1 +
 elf/ifuncmain7.c                               |  1 +
 elf/ifuncmod1.c                                |  3 +++
 elf/ifuncmod5.c                                |  3 +++
 include/libc-symbols.h                         | 12 +++++++++++-
 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/x86/gettimeofday.c     |  1 +
 sysdeps/unix/sysv/linux/x86_64/x32/getcpu.c    |  1 +
 sysdeps/x86_64/ifuncmod8.c                     |  1 +
 16 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/config.h.in b/config.h.in
index 1b58612..b42c4d8 100644
--- a/config.h.in
+++ b/config.h.in
@@ -48,6 +48,10 @@
 /* Define if compiler accepts -ftree-loop-distribute-patterns.  */
 #undef  HAVE_CC_INHIBIT_LOOP_TO_LIBCALL
 
+/* Define if compiler accepts -fno-stack-protector in an
+   __attribute__((__optimize__)).  */
+#undef	HAVE_CC_NO_STACK_PROTECTOR
+
 /* The level of stack protection in use for glibc as a whole.  */
 #undef	STACK_PROTECTOR_LEVEL
 
diff --git a/configure.ac b/configure.ac
index 859f90b..da37319 100644
--- a/configure.ac
+++ b/configure.ac
@@ -639,6 +639,7 @@ stack_protector=
 no_stack_protector=
 if test "$libc_cv_ssp" = yes; then
   no_stack_protector="-fno-stack-protector"
+  AC_DEFINE(HAVE_CC_NO_STACK_PROTECTOR)
 fi
 
 if test "$enable_stack_protector" = yes && test "$libc_cv_ssp" = yes; then
diff --git a/elf/ifuncdep2.c b/elf/ifuncdep2.c
index 6e66d31..d87d61d 100644
--- a/elf/ifuncdep2.c
+++ b/elf/ifuncdep2.c
@@ -32,6 +32,7 @@ void * foo1_ifunc (void) __asm__ ("foo1");
 __asm__(".type foo1, %gnu_indirect_function");
 
 void *
+inhibit_stack_protector
 foo1_ifunc (void)
 {
   return ifunc_sel (one, minus_one, zero);
@@ -41,6 +42,7 @@ void * foo2_ifunc (void) __asm__ ("foo2");
 __asm__(".type foo2, %gnu_indirect_function");
 
 void *
+inhibit_stack_protector
 foo2_ifunc (void)
 {
   return ifunc_sel (minus_one, one, zero);
@@ -50,6 +52,7 @@ void * foo3_ifunc (void) __asm__ ("foo3");
 __asm__(".type foo3, %gnu_indirect_function");
 
 void *
+inhibit_stack_protector
 foo3_ifunc (void)
 {
   return ifunc_sel (one, zero, minus_one);
diff --git a/elf/ifuncmain6pie.c b/elf/ifuncmain6pie.c
index 8478d4c..04faeb8 100644
--- a/elf/ifuncmain6pie.c
+++ b/elf/ifuncmain6pie.c
@@ -21,6 +21,7 @@ void * foo_ifunc (void) __asm__ ("foo");
 __asm__(".type foo, %gnu_indirect_function");
 
 void *
+inhibit_stack_protector
 foo_ifunc (void)
 {
   return ifunc_one (one);
diff --git a/elf/ifuncmain7.c b/elf/ifuncmain7.c
index 617a596..1e8f7ea 100644
--- a/elf/ifuncmain7.c
+++ b/elf/ifuncmain7.c
@@ -20,6 +20,7 @@ __asm__(".type foo, %gnu_indirect_function");
 
 static void *
 __attribute__ ((used))
+inhibit_stack_protector
 foo_ifunc (void)
 {
   return ifunc_one (one);
diff --git a/elf/ifuncmod1.c b/elf/ifuncmod1.c
index 0b61380..f0bf5fb 100644
--- a/elf/ifuncmod1.c
+++ b/elf/ifuncmod1.c
@@ -36,6 +36,7 @@ void * foo_ifunc (void) __asm__ ("foo");
 __asm__(".type foo, %gnu_indirect_function");
 
 void *
+inhibit_stack_protector
 foo_ifunc (void)
 {
   return ifunc_sel (one, minus_one, zero);
@@ -45,6 +46,7 @@ void * foo_hidden_ifunc (void) __asm__ ("foo_hidden");
 __asm__(".type foo_hidden, %gnu_indirect_function");
 
 void *
+inhibit_stack_protector
 foo_hidden_ifunc (void)
 {
   return ifunc_sel (minus_one, one, zero);
@@ -54,6 +56,7 @@ void * foo_protected_ifunc (void) __asm__ ("foo_protected");
 __asm__(".type foo_protected, %gnu_indirect_function");
 
 void *
+inhibit_stack_protector
 foo_protected_ifunc (void)
 {
   return ifunc_sel (one, zero, minus_one);
diff --git a/elf/ifuncmod5.c b/elf/ifuncmod5.c
index 0e65a63..5a95780 100644
--- a/elf/ifuncmod5.c
+++ b/elf/ifuncmod5.c
@@ -31,6 +31,7 @@ void * foo_ifunc (void) __asm__ ("foo");
 __asm__(".type foo, %gnu_indirect_function");
 
 void *
+inhibit_stack_protector
 foo_ifunc (void)
 {
   return ifunc_sel (one, minus_one, zero);
@@ -40,6 +41,7 @@ void * foo_hidden_ifunc (void) __asm__ ("foo_hidden");
 __asm__(".type foo_hidden, %gnu_indirect_function");
 
 void *
+inhibit_stack_protector
 foo_hidden_ifunc (void)
 {
   return ifunc_sel (minus_one, one, zero);
@@ -49,6 +51,7 @@ void * foo_protected_ifunc (void) __asm__ ("foo_protected");
 __asm__(".type foo_protected, %gnu_indirect_function");
 
 void *
+inhibit_stack_protector
 foo_protected_ifunc (void)
 {
   return ifunc_sel (one, zero, minus_one);
diff --git a/include/libc-symbols.h b/include/libc-symbols.h
index 1c91f2e..d2f3d3a 100644
--- a/include/libc-symbols.h
+++ b/include/libc-symbols.h
@@ -338,6 +338,16 @@ for linking")
 
 #define attribute_relro __attribute__ ((section (".data.rel.ro")))
 
+
+/* Used to disable stack protection in sensitive places, like ifunc
+   resolvers and early static TLS init.  */
+#ifdef HAVE_CC_NO_STACK_PROTECTOR
+# define inhibit_stack_protector \
+    __attribute__ ((__optimize__ ("-fno-stack-protector")))
+#else
+# define inhibit_stack_protector
+#endif
+
 /* The following macros are used for PLT bypassing within libc.so
    (and if needed other libraries similarly).
    First of all, you need to have the function prototyped somewhere,
@@ -739,7 +749,7 @@ for linking")
 
 /* Helper / base  macros for indirect function symbols.  */
 #define __ifunc_resolver(type_name, name, expr, arg, init, classifier)	\
-  classifier void *name##_ifunc (arg)					\
+  classifier inhibit_stack_protector void *name##_ifunc (arg)					\
   {									\
     init ();								\
     __typeof (type_name) *res = expr;					\
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 ac589bd..bdb00bf 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");
@@ -32,6 +33,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 16c00d7..46608ee 100644
--- a/sysdeps/unix/sysv/linux/powerpc/gettimeofday.c
+++ b/sysdeps/unix/sysv/linux/powerpc/gettimeofday.c
@@ -33,6 +33,7 @@
 #  undef __gettimeofday
 
 int
+inhibit_stack_protector
 __gettimeofday_vsyscall (struct timeval *tv, struct timezone *tz)
 {
   return INLINE_VSYSCALL (gettimeofday, 2, tv, tz);
diff --git a/sysdeps/unix/sysv/linux/x86/gettimeofday.c b/sysdeps/unix/sysv/linux/x86/gettimeofday.c
index c82452f..a419c4d 100644
--- a/sysdeps/unix/sysv/linux/x86/gettimeofday.c
+++ b/sysdeps/unix/sysv/linux/x86/gettimeofday.c
@@ -24,6 +24,7 @@
 # include <errno.h>
 
 static int
+inhibit_stack_protector
 __gettimeofday_syscall (struct timeval *tv, struct timezone *tz)
 {
   return INLINE_SYSCALL (gettimeofday, 2, tv, tz);
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.10.1.208.gbec66bc

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

* [PATCH 01/12] Initialize the stack guard earlier when linking statically.
  2016-11-28 12:33 --enable-stack-protector for glibc, v9 Nix
                   ` (8 preceding siblings ...)
  2016-11-28 13:26 ` [PATCH 11/12] Do not stack-protect sigreturn stubs Nix
@ 2016-11-28 13:26 ` Nix
  2016-11-28 13:26 ` [PATCH 02/12] Do not stack-protect ifunc resolvers Nix
  2016-11-28 13:26 ` [PATCH 05/12] Prevent the rtld mapfile computation from dragging in __stack_chk_fail* Nix
  11 siblings, 0 replies; 40+ messages in thread
From: Nix @ 2016-11-28 13:26 UTC (permalink / raw)
  To: libc-alpha; +Cc: fweimer, Nick Alcock

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

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

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

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

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

diff --git a/csu/libc-start.c b/csu/libc-start.c
index 99c040a..0bd4385 100644
--- a/csu/libc-start.c
+++ b/csu/libc-start.c
@@ -30,6 +30,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.  */
@@ -175,6 +176,20 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
         }
     }
 
+  /* Perform IREL{,A} relocations.  */
+  apply_irel ();
+
+  /* The stack guard goes into the TCB.  */
+  __pthread_initialize_tcb_internal ();
+
+  /* Set up the stack checker's canary.  */
+  uintptr_t stack_chk_guard = _dl_setup_stack_chk_guard (_dl_random);
+# ifdef THREAD_SET_STACK_GUARD
+  THREAD_SET_STACK_GUARD (stack_chk_guard);
+# else
+  __stack_chk_guard = stack_chk_guard;
+# endif
+
 # ifdef DL_SYSDEP_OSCHECK
   if (!__libc_multiple_libcs)
     {
@@ -184,22 +199,11 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
     }
 # endif
 
-  /* Perform IREL{,A} relocations.  */
-  apply_irel ();
-
   /* Initialize the thread library at least a bit since the libgcc
      functions are using thread functions if these are available and
      we need to setup errno.  */
   __pthread_initialize_minimal ();
 
-  /* Set up the stack checker's canary.  */
-  uintptr_t stack_chk_guard = _dl_setup_stack_chk_guard (_dl_random);
-# ifdef THREAD_SET_STACK_GUARD
-  THREAD_SET_STACK_GUARD (stack_chk_guard);
-# else
-  __stack_chk_guard = stack_chk_guard;
-# endif
-
   /* Set up the pointer guard value.  */
   uintptr_t pointer_chk_guard = _dl_setup_pointer_guard (_dl_random,
 							 stack_chk_guard);
diff --git a/csu/libc-tls.c b/csu/libc-tls.c
index 235ac79..b92c567 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 48fab50..dea335d 100644
--- a/nptl/nptl-init.c
+++ b/nptl/nptl-init.c
@@ -285,21 +285,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.10.1.208.gbec66bc

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

* [PATCH 05/12] Prevent the rtld mapfile computation from dragging in __stack_chk_fail*.
  2016-11-28 12:33 --enable-stack-protector for glibc, v9 Nix
                   ` (10 preceding siblings ...)
  2016-11-28 13:26 ` [PATCH 02/12] Do not stack-protect ifunc resolvers Nix
@ 2016-11-28 13:26 ` Nix
  11 siblings, 0 replies; 40+ messages in thread
From: Nix @ 2016-11-28 13:26 UTC (permalink / raw)
  To: libc-alpha; +Cc: fweimer, Nick Alcock

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

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

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

v2: New.
v6: Dummy out stack_chk_fail_local too.
v7: Fix word-wrapping.

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

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

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

* Re: [PATCH 02/12] Do not stack-protect ifunc resolvers.
  2016-11-28 13:26 ` [PATCH 02/12] Do not stack-protect ifunc resolvers Nix
@ 2016-12-15 12:18   ` Florian Weimer
  2016-12-15 12:34     ` Nick Alcock
  0 siblings, 1 reply; 40+ messages in thread
From: Florian Weimer @ 2016-12-15 12:18 UTC (permalink / raw)
  To: Nix, libc-alpha; +Cc: Nick Alcock

On 11/28/2016 01:32 PM, Nix wrote:
> diff --git a/config.h.in b/config.h.in
> index 1b58612..b42c4d8 100644
> --- a/config.h.in
> +++ b/config.h.in
> @@ -48,6 +48,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__)).  */

Space after __attribute__; at least that's the current style.

> +#undef	HAVE_CC_NO_STACK_PROTECTOR
> +
>  /* The level of stack protection in use for glibc as a whole.  */
>  #undef	STACK_PROTECTOR_LEVEL

I don't have STACK_PROTECTOR_LEVEL in my sources, so this patch does not 
apply.  Did you post the sequence in the right order?

Thanks,
Florian

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

* Re: [PATCH 02/12] Do not stack-protect ifunc resolvers.
  2016-12-15 12:18   ` Florian Weimer
@ 2016-12-15 12:34     ` Nick Alcock
  2016-12-15 12:58       ` Florian Weimer
  0 siblings, 1 reply; 40+ messages in thread
From: Nick Alcock @ 2016-12-15 12:34 UTC (permalink / raw)
  To: Florian Weimer; +Cc: libc-alpha

On 15 Dec 2016, Florian Weimer verbalised:

> On 11/28/2016 01:32 PM, Nix wrote:
>> diff --git a/config.h.in b/config.h.in
>> index 1b58612..b42c4d8 100644
>> --- a/config.h.in
>> +++ b/config.h.in
>> @@ -48,6 +48,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__)).  */
>
> Space after __attribute__; at least that's the current style.

OK. (But note that there is another instance of this problem elsewhere
in the same file, only a few lines above, in HAVE_SECTION_QUOTES: I
was copying it, for consistency.)

>> +#undef	HAVE_CC_NO_STACK_PROTECTOR
>> +
>>  /* The level of stack protection in use for glibc as a whole.  */
>>  #undef	STACK_PROTECTOR_LEVEL
>
> I don't have STACK_PROTECTOR_LEVEL in my sources, so this patch does not apply.  Did you post the sequence in the right order?

Ooof, patch 1 is missing due to a git commit ID foulup. This, of course,
renumbers all the later patches -- but here it is anyway, so you can
apply the series as a whole:

From c612d071f28b18418873379598ccef5b41bedb34 Mon Sep 17 00:00:00 2001
From: Nick Alcock <nick.alcock@oracle.com>
Date: Fri, 19 Feb 2016 17:25:05 +0000
Subject: [PATCH] Configury support for --enable-stack-protector.

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

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

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

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

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

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

diff --git a/INSTALL b/INSTALL
index b5acedc..2b0abf9 100644
--- a/INSTALL
+++ b/INSTALL
@@ -135,20 +135,31 @@ will be used, and CFLAGS sets optimization options for the compiler.
 '--enable-lock-elision=yes'
      Enable lock elision for pthread mutexes by default.
 
-'--enable-pt_chown'
-     The file 'pt_chown' is a helper binary for 'grantpt' (*note
-     Pseudo-Terminals: Allocation.) that is installed setuid root to fix
-     up pseudo-terminal ownership.  It is not built by default because
-     systems using the Linux kernel are commonly built with the 'devpts'
-     filesystem enabled and mounted at '/dev/pts', which manages
-     pseudo-terminal ownership automatically.  By using
-     '--enable-pt_chown', you may build 'pt_chown' and install it setuid
-     and owned by 'root'.  The use of 'pt_chown' introduces additional
-     security risks to the system and you should enable it only if you
-     understand and accept those risks.
-
-'--disable-werror'
-     By default, the GNU C Library is built with '-Werror'.  If you wish
+`--enable-stack-protector'
+`--enable-stack-protector=strong'
+`--enable-stack-protector=all'
+     Compile the C library and all other parts of the glibc package
+     (including the threading and math libraries, NSS modules, and
+     transliteration modules) using the GCC `-fstack-protector',
+     `-fstack-protector-strong' or `-fstack-protector-all' options to
+     detect stack overruns.  Only the dynamic linker and a small number
+     of routines called directly from assembler are excluded from this
+     protection.
+
+`--enable-pt_chown'
+     The file `pt_chown' is a helper binary for `grantpt' (*note
+     Pseudo-Terminals: Allocation.) that is installed setuid root to
+     fix up pseudo-terminal ownership.  It is not built by default
+     because systems using the Linux kernel are commonly built with the
+     `devpts' filesystem enabled and mounted at `/dev/pts', which
+     manages pseudo-terminal ownership automatically.  By using
+     `--enable-pt_chown', you may build `pt_chown' and install it
+     setuid and owned by `root'.  The use of `pt_chown' introduces
+     additional security risks to the system and you should enable it
+     only if you understand and accept those risks.
+
+`--disable-werror'
+     By default, the GNU C Library is built with `-Werror'.  If you wish
      to build without this option (for example, if building with a newer
      version of GCC than this version of the GNU C Library was tested
      with, so new warnings cause the build with '-Werror' to fail), you
diff --git a/config.h.in b/config.h.in
index 33757bd..1b58612 100644
--- a/config.h.in
+++ b/config.h.in
@@ -48,6 +48,9 @@
 /* Define if compiler accepts -ftree-loop-distribute-patterns.  */
 #undef  HAVE_CC_INHIBIT_LOOP_TO_LIBCALL
 
+/* The level of stack protection in use for glibc as a whole.  */
+#undef	STACK_PROTECTOR_LEVEL
+
 /* Define if the regparm attribute shall be used for local functions
    (gcc on ix86 only).  */
 #undef	USE_REGPARMS
diff --git a/configure.ac b/configure.ac
index de0a40f..859f90b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -235,6 +235,18 @@ if test "x$bindnow" = xyes; then
   AC_DEFINE(BIND_NOW)
 fi
 
+dnl Build glibc with -fstack-protector, -fstack-protector-all, or
+dnl -fstack-protector-strong.
+AC_ARG_ENABLE([stack-protector],
+	      AC_HELP_STRING([--enable-stack-protector=@<:@yes|no|all|strong@:>@],
+			     [Use -fstack-protector[-all|-strong] to detect glibc buffer overflows]),
+	      [enable_stack_protector=$enableval],
+	      [enable_stack_protector=no])
+case "$enable_stack_protector" in
+all|yes|no|strong) ;;
+*) AC_MSG_ERROR([Not a valid argument for --enable-stack-protector: \"$enable_stack_protector\"]);;
+esac
+
 dnl On some platforms we cannot use dynamic loading.  We must provide
 dnl static NSS modules.
 AC_ARG_ENABLE([static-nss],
@@ -605,6 +617,44 @@ fi
 test -n "$base_machine" || base_machine=$machine
 AC_SUBST(base_machine)
 
+AC_CACHE_CHECK(for -fstack-protector, libc_cv_ssp, [dnl
+LIBC_TRY_CC_OPTION([$CFLAGS $CPPFLAGS -Werror -fstack-protector],
+		   [libc_cv_ssp=yes],
+		   [libc_cv_ssp=no])
+])
+
+AC_CACHE_CHECK(for -fstack-protector-strong, libc_cv_ssp_strong, [dnl
+LIBC_TRY_CC_OPTION([$CFLAGS $CPPFLAGS -Werror -fstack-protector-strong],
+		   [libc_cv_ssp_strong=yes],
+		   [libc_cv_ssp_strong=no])
+])
+
+AC_CACHE_CHECK(for -fstack-protector-all, libc_cv_ssp_all, [dnl
+LIBC_TRY_CC_OPTION([$CFLAGS $CPPFLAGS -Werror -fstack-protector-all],
+		   [libc_cv_ssp_all=yes],
+		   [libc_cv_ssp_all=no])
+])
+
+stack_protector=
+no_stack_protector=
+if test "$libc_cv_ssp" = yes; then
+  no_stack_protector="-fno-stack-protector"
+fi
+
+if test "$enable_stack_protector" = yes && test "$libc_cv_ssp" = yes; then
+  stack_protector="-fstack-protector"
+  AC_DEFINE(STACK_PROTECTOR_LEVEL, 1)
+elif test "$enable_stack_protector" = all && test "$libc_cv_ssp_all" = yes; then
+  stack_protector="-fstack-protector-all"
+  AC_DEFINE(STACK_PROTECTOR_LEVEL, 2)
+elif test "$enable_stack_protector" = strong && test "$libc_cv_ssp_strong" = yes; then
+  stack_protector="-fstack-protector-strong"
+  AC_DEFINE(STACK_PROTECTOR_LEVEL, 3)
+fi
+AC_SUBST(libc_cv_ssp)
+AC_SUBST(stack_protector)
+AC_SUBST(no_stack_protector)
+
 # For the multi-arch option we need support in the assembler & linker.
 AC_CACHE_CHECK([for assembler and linker STT_GNU_IFUNC support],
 	       libc_cv_ld_gnu_indirect_function, [dnl
@@ -1433,26 +1483,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([for -mtls-dialect=gnu2], libc_cv_mtls_dialect_gnu2,
 [dnl
 cat > conftest.c <<EOF
diff --git a/manual/install.texi b/manual/install.texi
index de1c203..b5be87d 100644
--- a/manual/install.texi
+++ b/manual/install.texi
@@ -164,6 +164,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.10.1.208.gbec66bc

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

* Re: [PATCH 02/12] Do not stack-protect ifunc resolvers.
  2016-12-15 12:34     ` Nick Alcock
@ 2016-12-15 12:58       ` Florian Weimer
  0 siblings, 0 replies; 40+ messages in thread
From: Florian Weimer @ 2016-12-15 12:58 UTC (permalink / raw)
  To: Nick Alcock; +Cc: libc-alpha

On 12/15/2016 01:34 PM, Nick Alcock wrote:
> On 15 Dec 2016, Florian Weimer verbalised:
>
>> On 11/28/2016 01:32 PM, Nix wrote:
>>> diff --git a/config.h.in b/config.h.in
>>> index 1b58612..b42c4d8 100644
>>> --- a/config.h.in
>>> +++ b/config.h.in
>>> @@ -48,6 +48,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__)).  */
>>
>> Space after __attribute__; at least that's the current style.
>
> OK. (But note that there is another instance of this problem elsewhere
> in the same file, only a few lines above, in HAVE_SECTION_QUOTES: I
> was copying it, for consistency.)

Oh.  Yes, the style has changed over the years.  Previously, you needed 
to look at the type of the preceding identifier to determine if a space 
is inserted.  __attribute__ was treated like a macro and did not trigger 
insertion of a space.

>>> +#undef	HAVE_CC_NO_STACK_PROTECTOR
>>> +
>>>  /* The level of stack protection in use for glibc as a whole.  */
>>>  #undef	STACK_PROTECTOR_LEVEL
>>
>> I don't have STACK_PROTECTOR_LEVEL in my sources, so this patch does not apply.  Did you post the sequence in the right order?
>
> Ooof, patch 1 is missing due to a git commit ID foulup. This, of course,
> renumbers all the later patches -- but here it is anyway, so you can
> apply the series as a whole:

Okay, got it now.  Initial build and test run of the whole series was 
successful.  Back to reviewing the patches Â…

Thanks,
Florian

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

* Re: [PATCH 08/12] De-PLTize __stack_chk_fail internal calls within libc.so.
  2016-11-28 13:25 ` [PATCH 08/12] De-PLTize __stack_chk_fail internal calls within libc.so Nix
@ 2016-12-15 13:56   ` Florian Weimer
  2016-12-15 14:16     ` Nix
  0 siblings, 1 reply; 40+ messages in thread
From: Florian Weimer @ 2016-12-15 13:56 UTC (permalink / raw)
  To: Nix, libc-alpha; +Cc: Adhemerval Zanella

On 11/28/2016 01:32 PM, Nix wrote:
> From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
>
> We use the same assembler-macro trick we use to de-PLTize
> compiler-generated libcalls to memcpy and memset to redirect
> __stack_chk_fail to __stack_chk_fail_local.
>
> v5: New.
> v6: Only do it within the shared library: with __stack_chk_fail_local
>     in libc_pic.a now we don't need to worry about calls from inside
>     other routines in libc_nonshared.a any more.
> v8: Merge #ifdef blocks.
>
> 	* sysdeps/generic/symbol-hacks.h (__stack_chk_fail): Add internal
> 	alias.
> ---
>  sysdeps/generic/symbol-hacks.h | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/sysdeps/generic/symbol-hacks.h b/sysdeps/generic/symbol-hacks.h
> index ce576c9..36908b5 100644
> --- a/sysdeps/generic/symbol-hacks.h
> +++ b/sysdeps/generic/symbol-hacks.h
> @@ -4,4 +4,8 @@
>  asm ("memmove = __GI_memmove");
>  asm ("memset = __GI_memset");
>  asm ("memcpy = __GI_memcpy");
> +
> +/* -fstack-protector generates calls to __stack_chk_fail, which need
> +   similar adjustments to avoid going through the PLT.  */
> +asm ("__stack_chk_fail = __stack_chk_fail_local");
>  #endif

We should do this only if we compile glibc with stack protector support 
enabled, and disable this for the files which we compile without stack 
protector.  I hope this will fix an assembler error while compiling 
__stack_chk_fail.c on ia64:

/tmp/ccCNZVJs.s:51: Error: `__stack_chk_fail' was not defined within 
procedure
/tmp/ccCNZVJs.s:51: Warning: `__stack_chk_fail#' was not specified with 
previous .proc
/tmp/ccCNZVJs.s:51: Warning: `__stack_chk_fail' should be an operand to 
this .endp

The .s file looks like this:

       1         .file   "stack_chk_fail.c"
       2         .pred.safe_across_calls p1-p5,p16-p63
       3         .text
       4 .Ltext0:
       5 #APP
       6         memmove = __GI_memmove
       7         memset = __GI_memset
       8         memcpy = __GI_memcpy
       9         __stack_chk_fail = __stack_chk_fail_local
      10         .section        .rodata.str1.8,"aMS",@progbits,1
      11         .align 8
      12 .LC0:
      13         stringz "stack smashing detected"
      14 #NO_APP
      15         .text
      16         .align 16
      17         .align 64
      18         .global __stack_chk_fail#
      19         .type   __stack_chk_fail#, @function
      20         .proc __stack_chk_fail#
      21 __stack_chk_fail:
      22 [.LFB33:]
      23         .file 1 "stack_chk_fail.c"
      24         .loc 1 27 0
      25         .prologue 12, 32
      26         .mib
      27         .save ar.pfs, r33
      28         alloc r33 = ar.pfs, 0, 3, 1, 0
      29 [.LCFI0:]
      30         .save rp, r32
      31         mov r32 = b0
      32 [.LCFI1:]
      33         .loc 1 28 0
      34         nop 0
      35         .mlx
      36         nop 0
      37         movl r35 = @gprel(.LC0)
      38         .loc 1 27 0
      39         .body
      40         .loc 1 28 0
      41         ;;
      42         .mib
      43         nop 0
      44         add r35 = r1, r35
      45         br.call.sptk.many b0 = __GI___fortify_fail
      46 [.LVL0:]
      47         ;;
      48         break.f 0
      49         ;;
      50 .LFE33:
      51         .endp __stack_chk_fail#

Thanks,
Florian

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

* Re: [PATCH 03/12] Mark all machinery needed in early static-link init as -fno-stack-protector.
  2016-11-28 12:33 ` [PATCH 03/12] Mark all machinery needed in early static-link init as -fno-stack-protector Nix
@ 2016-12-15 14:01   ` Florian Weimer
  2016-12-15 14:21     ` Nick Alcock
  0 siblings, 1 reply; 40+ messages in thread
From: Florian Weimer @ 2016-12-15 14:01 UTC (permalink / raw)
  To: Nix, libc-alpha; +Cc: Nick Alcock

On 11/28/2016 01:32 PM, Nix wrote:
> 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)

This also applies to the tests in the csu subdirectory, which is 
probably not what we want.

We have two options here: List the .c source files explicitly (like you 
do in string/), or move the test to a different subdirectory (perhaps 
misc/ or stdlib/).  In the second case, there should be an empty “tests” 
variable assignment in csu/Makefile which documents why there can't be 
any tests in this subdirectory.

Thanks,
Florian

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

* Re: [PATCH 08/12] De-PLTize __stack_chk_fail internal calls within libc.so.
  2016-12-15 13:56   ` Florian Weimer
@ 2016-12-15 14:16     ` Nix
  2016-12-15 14:21       ` Florian Weimer
  0 siblings, 1 reply; 40+ messages in thread
From: Nix @ 2016-12-15 14:16 UTC (permalink / raw)
  To: Florian Weimer; +Cc: libc-alpha, Adhemerval Zanella

On 15 Dec 2016, Florian Weimer said:

> On 11/28/2016 01:32 PM, Nix wrote:
>> From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
[...]
>> diff --git a/sysdeps/generic/symbol-hacks.h b/sysdeps/generic/symbol-hacks.h
>> index ce576c9..36908b5 100644
>> --- a/sysdeps/generic/symbol-hacks.h
>> +++ b/sysdeps/generic/symbol-hacks.h
>> @@ -4,4 +4,8 @@
>>  asm ("memmove = __GI_memmove");
>>  asm ("memset = __GI_memset");
>>  asm ("memcpy = __GI_memcpy");
>> +
>> +/* -fstack-protector generates calls to __stack_chk_fail, which need
>> +   similar adjustments to avoid going through the PLT.  */
>> +asm ("__stack_chk_fail = __stack_chk_fail_local");
>>  #endif
>
> We should do this only if we compile glibc with stack protector support enabled, and disable this for the files which we compile
> without stack protector.  I hope this will fix an assembler error while compiling __stack_chk_fail.c on ia64:

I don't think we need to disable it for *all* such files -- but at the
very least it must be disabled for debug/stack_chk_fail.c, and if we're
doing that it's probably easier to do as you suggest (though it's
invasive enough I'll have to kick another test cycle off, sigh).

Possible fix, untested:

diff --git a/sysdeps/generic/symbol-hacks.h b/sysdeps/generic/symbol-hacks.h
index 36908b5..0679354 100644
--- a/sysdeps/generic/symbol-hacks.h
+++ b/sysdeps/generic/symbol-hacks.h
@@ -7,5 +7,7 @@ asm ("memcpy = __GI_memcpy");
 
 /* -fstack-protector generates calls to __stack_chk_fail, which need
    similar adjustments to avoid going through the PLT.  */
+#if defined __SSP__ || defined __SSP_ALL__ || defined __SSP_STRONG__
 asm ("__stack_chk_fail = __stack_chk_fail_local");
 #endif
+#endif

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

* Re: [PATCH 08/12] De-PLTize __stack_chk_fail internal calls within libc.so.
  2016-12-15 14:16     ` Nix
@ 2016-12-15 14:21       ` Florian Weimer
  2016-12-15 14:29         ` Nix
  0 siblings, 1 reply; 40+ messages in thread
From: Florian Weimer @ 2016-12-15 14:21 UTC (permalink / raw)
  To: Nix; +Cc: libc-alpha, Adhemerval Zanella

On 12/15/2016 03:15 PM, Nix wrote:

> Possible fix, untested:
>
> diff --git a/sysdeps/generic/symbol-hacks.h b/sysdeps/generic/symbol-hacks.h
> index 36908b5..0679354 100644
> --- a/sysdeps/generic/symbol-hacks.h
> +++ b/sysdeps/generic/symbol-hacks.h
> @@ -7,5 +7,7 @@ asm ("memcpy = __GI_memcpy");
>
>  /* -fstack-protector generates calls to __stack_chk_fail, which need
>     similar adjustments to avoid going through the PLT.  */
> +#if defined __SSP__ || defined __SSP_ALL__ || defined __SSP_STRONG__
>  asm ("__stack_chk_fail = __stack_chk_fail_local");
>  #endif
> +#endif

The condition looks rather brittle.  What if GCC grows an 
-fstack-protector-light switch and __SSP_LIGHT__ macro?

I wonder if it's better to add something to $(no-stack-protector) and 
use that in the conditional.

Thanks,
Florian

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

* Re: [PATCH 03/12] Mark all machinery needed in early static-link init as -fno-stack-protector.
  2016-12-15 14:01   ` Florian Weimer
@ 2016-12-15 14:21     ` Nick Alcock
  2016-12-15 14:31       ` Florian Weimer
  0 siblings, 1 reply; 40+ messages in thread
From: Nick Alcock @ 2016-12-15 14:21 UTC (permalink / raw)
  To: Florian Weimer; +Cc: libc-alpha

On 15 Dec 2016, Florian Weimer outgrape:

> On 11/28/2016 01:32 PM, Nix wrote:
>> 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)
>
> This also applies to the tests in the csu subdirectory, which is probably not what we want.

Definitely not!

> We have two options here: List the .c source files explicitly (like you do in string/), or move the test to a different subdirectory
> (perhaps misc/ or stdlib/).  In the second case, there should be an empty “tests” variable assignment in csu/Makefile which
> documents why there can't be any tests in this subdirectory.

Listing explicitly seems likely to be horrifically hard to maintain. I'm
not even sure how many .o's there *are* that this would apply to.

Is there really no way to say 'add these flags to all the tests' so we
can add a $(stack-protector) to that?

-- 
NULL && (void)

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

* Re: [PATCH 08/12] De-PLTize __stack_chk_fail internal calls within libc.so.
  2016-12-15 14:21       ` Florian Weimer
@ 2016-12-15 14:29         ` Nix
  2016-12-15 14:33           ` Florian Weimer
  0 siblings, 1 reply; 40+ messages in thread
From: Nix @ 2016-12-15 14:29 UTC (permalink / raw)
  To: Florian Weimer; +Cc: libc-alpha, Adhemerval Zanella

On 15 Dec 2016, Florian Weimer told this:

> On 12/15/2016 03:15 PM, Nix wrote:
>
>> Possible fix, untested:
>>
>> diff --git a/sysdeps/generic/symbol-hacks.h b/sysdeps/generic/symbol-hacks.h
>> index 36908b5..0679354 100644
>> --- a/sysdeps/generic/symbol-hacks.h
>> +++ b/sysdeps/generic/symbol-hacks.h
>> @@ -7,5 +7,7 @@ asm ("memcpy = __GI_memcpy");
>>
>>  /* -fstack-protector generates calls to __stack_chk_fail, which need
>>     similar adjustments to avoid going through the PLT.  */
>> +#if defined __SSP__ || defined __SSP_ALL__ || defined __SSP_STRONG__
>>  asm ("__stack_chk_fail = __stack_chk_fail_local");
>>  #endif
>> +#endif
>
> The condition looks rather brittle.  What if GCC grows an -fstack-protector-light switch and __SSP_LIGHT__ macro?

We'd need to change configure.ac before that would have an effect in any
case... but it does seem likely that changing this too would be
overlooked.

> I wonder if it's better to add something to $(no-stack-protector) and use that in the conditional.

That was my other option, but the total absence of anything in
configure.ac passing -D made me think twice.

Something like this? (even more untested than the last one, if
possible -- but adds a new possibility: we can now differentiate between
"glibc built without stack protector" and "glibc built with stack
protector but this file doesn't have it" without relying on GCC
predefined macros. The __WITH_ naming scheme is completely arbitrary
and I can change it to anything you prefer.)

diff --git a/configure.ac b/configure.ac
index 2396c1f..8bb8c2c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -638,18 +638,18 @@ LIBC_TRY_CC_OPTION([$CFLAGS $CPPFLAGS -Werror -fstack-protector-all],
 stack_protector=
 no_stack_protector=
 if test "$libc_cv_ssp" = yes; then
-  no_stack_protector="-fno-stack-protector"
+  no_stack_protector="-fno-stack-protector -D__WITH_STACK_PROTECTOR=0"
   AC_DEFINE(HAVE_CC_NO_STACK_PROTECTOR)
 fi
 
 if test "$enable_stack_protector" = yes && test "$libc_cv_ssp" = yes; then
-  stack_protector="-fstack-protector"
+  stack_protector="-fstack-protector -D__WITH_STACK_PROTECTOR=1"
   AC_DEFINE(STACK_PROTECTOR_LEVEL, 1)
 elif test "$enable_stack_protector" = all && test "$libc_cv_ssp_all" = yes; then
-  stack_protector="-fstack-protector-all"
+  stack_protector="-fstack-protector-all -D__WITH_STACK_PROTECTOR=2"
   AC_DEFINE(STACK_PROTECTOR_LEVEL, 2)
 elif test "$enable_stack_protector" = strong && test "$libc_cv_ssp_strong" = yes; then
-  stack_protector="-fstack-protector-strong"
+  stack_protector="-fstack-protector-strong -D__WITH_STACK_PROTECTOR=3"
   AC_DEFINE(STACK_PROTECTOR_LEVEL, 3)
 fi
 AC_SUBST(libc_cv_ssp)
diff --git a/sysdeps/generic/symbol-hacks.h b/sysdeps/generic/symbol-hacks.h
index 36908b5..12b4fe7 100644
--- a/sysdeps/generic/symbol-hacks.h
+++ b/sysdeps/generic/symbol-hacks.h
@@ -7,5 +7,7 @@ asm ("memcpy = __GI_memcpy");
 
 /* -fstack-protector generates calls to __stack_chk_fail, which need
    similar adjustments to avoid going through the PLT.  */
+#if defined __WITH_STACK_PROTECTOR && __WITH_STACK_PROTECTOR > 0
 asm ("__stack_chk_fail = __stack_chk_fail_local");
 #endif
+#endif

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

* Re: [PATCH 03/12] Mark all machinery needed in early static-link init as -fno-stack-protector.
  2016-12-15 14:21     ` Nick Alcock
@ 2016-12-15 14:31       ` Florian Weimer
  2016-12-15 14:44         ` Nick Alcock
  0 siblings, 1 reply; 40+ messages in thread
From: Florian Weimer @ 2016-12-15 14:31 UTC (permalink / raw)
  To: Nick Alcock; +Cc: libc-alpha

On 12/15/2016 03:21 PM, Nick Alcock wrote:
> On 15 Dec 2016, Florian Weimer outgrape:
>
>> On 11/28/2016 01:32 PM, Nix wrote:
>>> 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)
>>
>> This also applies to the tests in the csu subdirectory, which is probably not what we want.
>
> Definitely not!
>
>> We have two options here: List the .c source files explicitly (like you do in string/), or move the test to a different subdirectory
>> (perhaps misc/ or stdlib/).  In the second case, there should be an empty “tests” variable assignment in csu/Makefile which
>> documents why there can't be any tests in this subdirectory.
>
> Listing explicitly seems likely to be horrifically hard to maintain. I'm
> not even sure how many .o's there *are* that this would apply to.

I think you added some magic to rtld CFLAGS to get this working without 
eval.

> Is there really no way to say 'add these flags to all the tests' so we
> can add a $(stack-protector) to that?

Even if we have that, it's very subtle because it would depend on the 
relative order of the expansion points (last flag wins).

So maybe moving the tests is the best option here.

Florian

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

* Re: [PATCH 08/12] De-PLTize __stack_chk_fail internal calls within libc.so.
  2016-12-15 14:29         ` Nix
@ 2016-12-15 14:33           ` Florian Weimer
  2016-12-15 14:40             ` Nix
  0 siblings, 1 reply; 40+ messages in thread
From: Florian Weimer @ 2016-12-15 14:33 UTC (permalink / raw)
  To: Nix; +Cc: libc-alpha, Adhemerval Zanella

On 12/15/2016 03:29 PM, Nix wrote:
> On 15 Dec 2016, Florian Weimer told this:
>
>> On 12/15/2016 03:15 PM, Nix wrote:
>>
>>> Possible fix, untested:
>>>
>>> diff --git a/sysdeps/generic/symbol-hacks.h b/sysdeps/generic/symbol-hacks.h
>>> index 36908b5..0679354 100644
>>> --- a/sysdeps/generic/symbol-hacks.h
>>> +++ b/sysdeps/generic/symbol-hacks.h
>>> @@ -7,5 +7,7 @@ asm ("memcpy = __GI_memcpy");
>>>
>>>  /* -fstack-protector generates calls to __stack_chk_fail, which need
>>>     similar adjustments to avoid going through the PLT.  */
>>> +#if defined __SSP__ || defined __SSP_ALL__ || defined __SSP_STRONG__
>>>  asm ("__stack_chk_fail = __stack_chk_fail_local");
>>>  #endif
>>> +#endif
>>
>> The condition looks rather brittle.  What if GCC grows an -fstack-protector-light switch and __SSP_LIGHT__ macro?
>
> We'd need to change configure.ac before that would have an effect in any
> case... but it does seem likely that changing this too would be
> overlooked.

Right.

>> I wonder if it's better to add something to $(no-stack-protector) and use that in the conditional.
>
> That was my other option, but the total absence of anything in
> configure.ac passing -D made me think twice.
>
> Something like this? (even more untested than the last one, if
> possible -- but adds a new possibility: we can now differentiate between
> "glibc built without stack protector" and "glibc built with stack
> protector but this file doesn't have it" without relying on GCC
> predefined macros. The __WITH_ naming scheme is completely arbitrary
> and I can change it to anything you prefer.)

WITH_STACK_PROTECTOR (without the leading underscores) looks okay to me 
because it's only used at build time.  Or you could call it 
STACK_PROTECTOR_LEVEL, to match the other variable.

> diff --git a/configure.ac b/configure.ac
> index 2396c1f..8bb8c2c 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -638,18 +638,18 @@ LIBC_TRY_CC_OPTION([$CFLAGS $CPPFLAGS -Werror -fstack-protector-all],
>  stack_protector=
>  no_stack_protector=
>  if test "$libc_cv_ssp" = yes; then
> -  no_stack_protector="-fno-stack-protector"
> +  no_stack_protector="-fno-stack-protector -D__WITH_STACK_PROTECTOR=0"
>    AC_DEFINE(HAVE_CC_NO_STACK_PROTECTOR)
>  fi
>
>  if test "$enable_stack_protector" = yes && test "$libc_cv_ssp" = yes; then
> -  stack_protector="-fstack-protector"
> +  stack_protector="-fstack-protector -D__WITH_STACK_PROTECTOR=1"
>    AC_DEFINE(STACK_PROTECTOR_LEVEL, 1)
>  elif test "$enable_stack_protector" = all && test "$libc_cv_ssp_all" = yes; then
> -  stack_protector="-fstack-protector-all"
> +  stack_protector="-fstack-protector-all -D__WITH_STACK_PROTECTOR=2"
>    AC_DEFINE(STACK_PROTECTOR_LEVEL, 2)
>  elif test "$enable_stack_protector" = strong && test "$libc_cv_ssp_strong" = yes; then
> -  stack_protector="-fstack-protector-strong"
> +  stack_protector="-fstack-protector-strong -D__WITH_STACK_PROTECTOR=3"
>    AC_DEFINE(STACK_PROTECTOR_LEVEL, 3)
>  fi
>  AC_SUBST(libc_cv_ssp)
> diff --git a/sysdeps/generic/symbol-hacks.h b/sysdeps/generic/symbol-hacks.h
> index 36908b5..12b4fe7 100644
> --- a/sysdeps/generic/symbol-hacks.h
> +++ b/sysdeps/generic/symbol-hacks.h
> @@ -7,5 +7,7 @@ asm ("memcpy = __GI_memcpy");
>
>  /* -fstack-protector generates calls to __stack_chk_fail, which need
>     similar adjustments to avoid going through the PLT.  */
> +#if defined __WITH_STACK_PROTECTOR && __WITH_STACK_PROTECTOR > 0
>  asm ("__stack_chk_fail = __stack_chk_fail_local");
>  #endif
> +#endif

The new #if/#endif need to be indented.

Thanks,
Florian

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

* Re: [PATCH 08/12] De-PLTize __stack_chk_fail internal calls within libc.so.
  2016-12-15 14:33           ` Florian Weimer
@ 2016-12-15 14:40             ` Nix
  2016-12-15 17:24               ` Nix
  0 siblings, 1 reply; 40+ messages in thread
From: Nix @ 2016-12-15 14:40 UTC (permalink / raw)
  To: Florian Weimer; +Cc: libc-alpha, Adhemerval Zanella

On 15 Dec 2016, Florian Weimer uttered the following:

> On 12/15/2016 03:29 PM, Nix wrote:
>> On 15 Dec 2016, Florian Weimer told this:
>>> I wonder if it's better to add something to $(no-stack-protector) and use that in the conditional.
>>
>> That was my other option, but the total absence of anything in
>> configure.ac passing -D made me think twice.
>>
>> Something like this? (even more untested than the last one, if
>> possible -- but adds a new possibility: we can now differentiate between
>> "glibc built without stack protector" and "glibc built with stack
>> protector but this file doesn't have it" without relying on GCC
>> predefined macros. The __WITH_ naming scheme is completely arbitrary
>> and I can change it to anything you prefer.)
>
> WITH_STACK_PROTECTOR (without the leading underscores) looks okay to
> me because it's only used at build time. Or you could call it
> STACK_PROTECTOR_LEVEL, to match the other variable.

That's definitely the best choice, because it means we don't need
to change configure.ac as much, and we don't have a confusing mix
of nonzero STACK_PROTECTOR_LEVEL and zero WITH_STACK_PROTECTOR (or
whatever) in some files.  It needs a tiny adjustment of config.h.in
to stop it being overridden though.

Something like this:

diff --git a/config.h.in b/config.h.in
index 610fa49..82f95a6 100644
--- a/config.h.in
+++ b/config.h.in
@@ -52,8 +52,11 @@
    __attribute__ ((__optimize__)).  */
 #undef	HAVE_CC_NO_STACK_PROTECTOR
 
-/* The level of stack protection in use for glibc as a whole.  */
+/* The level of stack protection in use for glibc as a whole.
+   May be overridden on a file-by-file basis.  */
+#ifndef STACK_PROTECTOR_LEVEL
 #undef	STACK_PROTECTOR_LEVEL
+#endif
 
 /* Define if the regparm attribute shall be used for local functions
    (gcc on ix86 only).  */
diff --git a/configure.ac b/configure.ac
index 2396c1f..a420428 100644
--- a/configure.ac
+++ b/configure.ac
@@ -638,7 +638,7 @@ LIBC_TRY_CC_OPTION([$CFLAGS $CPPFLAGS -Werror -fstack-protector-all],
 stack_protector=
 no_stack_protector=
 if test "$libc_cv_ssp" = yes; then
-  no_stack_protector="-fno-stack-protector"
+  no_stack_protector="-fno-stack-protector -DSTACK_PROTECTOR_LEVEL=0"
   AC_DEFINE(HAVE_CC_NO_STACK_PROTECTOR)
 fi
 
diff --git a/sysdeps/generic/symbol-hacks.h b/sysdeps/generic/symbol-hacks.h
index 36908b5..15ff56a 100644
--- a/sysdeps/generic/symbol-hacks.h
+++ b/sysdeps/generic/symbol-hacks.h
@@ -7,5 +7,7 @@ asm ("memcpy = __GI_memcpy");
 
 /* -fstack-protector generates calls to __stack_chk_fail, which need
    similar adjustments to avoid going through the PLT.  */
+# if defined STACK_PROTECTOR_LEVEL && STACK_PROTECTOR_LEVEL > 0
 asm ("__stack_chk_fail = __stack_chk_fail_local");
+# endif
 #endif

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

* Re: [PATCH 09/12] Link various tests with -fno-stack-protector.
  2016-11-28 13:25 ` [PATCH 09/12] Link various tests with -fno-stack-protector Nix
@ 2016-12-15 14:43   ` Florian Weimer
  2016-12-15 14:54     ` Nix
  0 siblings, 1 reply; 40+ messages in thread
From: Florian Weimer @ 2016-12-15 14:43 UTC (permalink / raw)
  To: Nix, libc-alpha; +Cc: Nick Alcock

On 11/28/2016 01:32 PM, Nix wrote:

> diff --git a/elf/Makefile b/elf/Makefile
> index daf0ebd..7588ca0 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -775,6 +775,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)

Is this really necessary for filtmod2.c?

> diff --git a/stdlib/Makefile b/stdlib/Makefile
> index 3cce9d9..6d7586e 100644
> --- a/stdlib/Makefile
> +++ b/stdlib/Makefile
> @@ -187,6 +187,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)

I think this papers over an actual failure.  Based on my build logs, 
tst-putenvmod.so is properly linked against libc (including 
libc_nonshared.a).

> diff --git a/sysdeps/x86_64/Makefile b/sysdeps/x86_64/Makefile
> index 6d99284..fbe138f 100644
> --- a/sysdeps/x86_64/Makefile
> +++ b/sysdeps/x86_64/Makefile
> @@ -46,6 +46,9 @@ tests-pie += $(quad-pie-test)
>  test-extras += tst-quadmod1pie tst-quadmod2pie
>  extra-test-objs += tst-quadmod1pie.o tst-quadmod2pie.o
>
> +CFLAGS-tst-quad1pie.c = $(no-stack-protector)
> +CFLAGS-tst-quad2pie.c = $(no-stack-protector)

This looks like a genuine test failure as well.  I'll try to reproduce 
it, after I have tracked down the more catastrophic MIPS build failure Â…

Thanks,
Florian

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

* Re: [PATCH 03/12] Mark all machinery needed in early static-link init as -fno-stack-protector.
  2016-12-15 14:31       ` Florian Weimer
@ 2016-12-15 14:44         ` Nick Alcock
  0 siblings, 0 replies; 40+ messages in thread
From: Nick Alcock @ 2016-12-15 14:44 UTC (permalink / raw)
  To: Florian Weimer; +Cc: libc-alpha

On 15 Dec 2016, Florian Weimer outgrape:

> On 12/15/2016 03:21 PM, Nick Alcock wrote:
>> On 15 Dec 2016, Florian Weimer outgrape:
>>
>>> On 11/28/2016 01:32 PM, Nix wrote:
>>>> 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)
>>>
>>> This also applies to the tests in the csu subdirectory, which is probably not what we want.
>>
>> Definitely not!
>>
>>> We have two options here: List the .c source files explicitly (like you do in string/), or move the test to a different subdirectory
>>> (perhaps misc/ or stdlib/).  In the second case, there should be an empty “tests” variable assignment in csu/Makefile which
>>> documents why there can't be any tests in this subdirectory.
>>
>> Listing explicitly seems likely to be horrifically hard to maintain. I'm
>> not even sure how many .o's there *are* that this would apply to.
>
> I think you added some magic to rtld CFLAGS to get this working without eval.

My argument just collapsed because I was looking at elf/, not csu/. csu/
is comparatively tiny and much easier to fix things up for. :)

> So maybe moving the tests is the best option here.

Yeah. None of these tests actually look like they belong in csu/, except
maybe tst-empty.  misc/ seems like a better place for all of them.

I'll whip up a moving patch (it'll be a bit longer because I think I
should actually do at least one quick test run before sending it).

-- 
NULL && (void)

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

* Re: [PATCH 09/12] Link various tests with -fno-stack-protector.
  2016-12-15 14:43   ` Florian Weimer
@ 2016-12-15 14:54     ` Nix
  2016-12-15 15:54       ` Florian Weimer
  2016-12-15 15:55       ` Nix
  0 siblings, 2 replies; 40+ messages in thread
From: Nix @ 2016-12-15 14:54 UTC (permalink / raw)
  To: Florian Weimer; +Cc: libc-alpha

On 15 Dec 2016, Florian Weimer uttered the following:

> On 11/28/2016 01:32 PM, Nix wrote:
>
>> diff --git a/elf/Makefile b/elf/Makefile
>> index daf0ebd..7588ca0 100644
>> --- a/elf/Makefile
>> +++ b/elf/Makefile
>> @@ -775,6 +775,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)
>
> Is this really necessary for filtmod2.c?

I assumed it was wisest to link it with the same options as the library
it was the filter for. It might be harmless, though -- I'll drop it and
find out!

>> diff --git a/stdlib/Makefile b/stdlib/Makefile
>> index 3cce9d9..6d7586e 100644
>> --- a/stdlib/Makefile
>> +++ b/stdlib/Makefile
>> @@ -187,6 +187,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)
>
> I think this papers over an actual failure. Based on my build logs,
> tst-putenvmod.so is properly linked against libc (including
> libc_nonshared.a).

Hmm! I'll take that out and have a look. Maybe __attribute
((constructor)) is causing problems, though I'd assume this would work
fine or other stack-protector users would be howling.

Much of this may be residual fallout from the days before we had the
stack-protector symbol export from libc/internal refs inside libc
working right.

>> diff --git a/sysdeps/x86_64/Makefile b/sysdeps/x86_64/Makefile
>> index 6d99284..fbe138f 100644
>> --- a/sysdeps/x86_64/Makefile
>> +++ b/sysdeps/x86_64/Makefile
>> @@ -46,6 +46,9 @@ tests-pie += $(quad-pie-test)
>>  test-extras += tst-quadmod1pie tst-quadmod2pie
>>  extra-test-objs += tst-quadmod1pie.o tst-quadmod2pie.o
>>
>> +CFLAGS-tst-quad1pie.c = $(no-stack-protector)
>> +CFLAGS-tst-quad2pie.c = $(no-stack-protector)
>
> This looks like a genuine test failure as well.  I'll try to reproduce it, after I have tracked down the more catastrophic MIPS
> build failure …

Yeah: they link against libc as well. Maybe I was misled by an
old-binutils problem back in the early days of this patch.

I'll try pulling this out as well...

(the MIPS one is easily explained by my not having a MIPS available and
having done zero testing on it. God only knows what system-dependent
horrors we need to work around. I haven't even *used* a MIPS bigger than
my wifi router since the early '90s... :) )

-- 
NULL && (void)

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

* Re: [PATCH 09/12] Link various tests with -fno-stack-protector.
  2016-12-15 14:54     ` Nix
@ 2016-12-15 15:54       ` Florian Weimer
  2016-12-15 15:56         ` Nix
  2016-12-15 15:55       ` Nix
  1 sibling, 1 reply; 40+ messages in thread
From: Florian Weimer @ 2016-12-15 15:54 UTC (permalink / raw)
  To: Nix; +Cc: libc-alpha

On 12/15/2016 03:54 PM, Nix wrote:
> On 15 Dec 2016, Florian Weimer uttered the following:
>
>> On 11/28/2016 01:32 PM, Nix wrote:
>>
>>> diff --git a/elf/Makefile b/elf/Makefile
>>> index daf0ebd..7588ca0 100644
>>> --- a/elf/Makefile
>>> +++ b/elf/Makefile
>>> @@ -775,6 +775,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)
>>
>> Is this really necessary for filtmod2.c?
>
> I assumed it was wisest to link it with the same options as the library
> it was the filter for. It might be harmless, though -- I'll drop it and
> find out!
>
>>> diff --git a/stdlib/Makefile b/stdlib/Makefile
>>> index 3cce9d9..6d7586e 100644
>>> --- a/stdlib/Makefile
>>> +++ b/stdlib/Makefile
>>> @@ -187,6 +187,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)
>>
>> I think this papers over an actual failure. Based on my build logs,
>> tst-putenvmod.so is properly linked against libc (including
>> libc_nonshared.a).
>
> Hmm! I'll take that out and have a look. Maybe __attribute
> ((constructor)) is causing problems, though I'd assume this would work
> fine or other stack-protector users would be howling.
>
> Much of this may be residual fallout from the days before we had the
> stack-protector symbol export from libc/internal refs inside libc
> working right.
>
>>> diff --git a/sysdeps/x86_64/Makefile b/sysdeps/x86_64/Makefile
>>> index 6d99284..fbe138f 100644
>>> --- a/sysdeps/x86_64/Makefile
>>> +++ b/sysdeps/x86_64/Makefile
>>> @@ -46,6 +46,9 @@ tests-pie += $(quad-pie-test)
>>>  test-extras += tst-quadmod1pie tst-quadmod2pie
>>>  extra-test-objs += tst-quadmod1pie.o tst-quadmod2pie.o
>>>
>>> +CFLAGS-tst-quad1pie.c = $(no-stack-protector)
>>> +CFLAGS-tst-quad2pie.c = $(no-stack-protector)
>>
>> This looks like a genuine test failure as well.  I'll try to reproduce it, after I have tracked down the more catastrophic MIPS
>> build failure …
>
> Yeah: they link against libc as well. Maybe I was misled by an
> old-binutils problem back in the early days of this patch.
>
> I'll try pulling this out as well...
>
> (the MIPS one is easily explained by my not having a MIPS available and
> having done zero testing on it. God only knows what system-dependent
> horrors we need to work around. I haven't even *used* a MIPS bigger than
> my wifi router since the early '90s... :) )

Joseph added build-many-glibcs.py recently, which really helps 
diagnosing build-time failures and ABI issues.

The MIPS failure appears to be similar to the ia64 failure because the 
R_MIPS_CALL16 relocation in __stack_chk_fail_local triggers it:

000f4590 <__stack_chk_fail_local>:
    f4590:       27bdfff0        addiu   sp,sp,-16
    f4594:       ffbc0000        sd      gp,0(sp)
    f4598:       3c1c0000        lui     gp,0x0
                         f4598: R_MIPS_GPREL16   __stack_chk_fail_local
                         f4598: R_MIPS_SUB       *ABS*
                         f4598: R_MIPS_HI16      *ABS*
    f459c:       0399e021        addu    gp,gp,t9
    f45a0:       279c0000        addiu   gp,gp,0
                         f45a0: R_MIPS_GPREL16   __stack_chk_fail_local
                         f45a0: R_MIPS_SUB       *ABS*
                         f45a0: R_MIPS_LO16      *ABS*
    f45a4:       8f990000        lw      t9,0(gp)
                         f45a4: R_MIPS_CALL16    .text+0xf4590
    f45a8:       ffbf0008        sd      ra,8(sp)
    f45ac:       0320f809        jalr    t9
                         f45ac: R_MIPS_JALR      __stack_chk_fail
    f45b0:       00000000        nop

So I'll construct a new series with your STACK_PROTECTOR_LEVEL patch and 
see where things go with that.

Thanks,
Florian

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

* Re: [PATCH 09/12] Link various tests with -fno-stack-protector.
  2016-12-15 14:54     ` Nix
  2016-12-15 15:54       ` Florian Weimer
@ 2016-12-15 15:55       ` Nix
  1 sibling, 0 replies; 40+ messages in thread
From: Nix @ 2016-12-15 15:55 UTC (permalink / raw)
  To: Florian Weimer; +Cc: libc-alpha

On 15 Dec 2016, nix@esperi.org.uk spake thusly:

> On 15 Dec 2016, Florian Weimer uttered the following:
>
>> On 11/28/2016 01:32 PM, Nix wrote:
>>
>>> diff --git a/elf/Makefile b/elf/Makefile
>>> index daf0ebd..7588ca0 100644
>>> --- a/elf/Makefile
>>> +++ b/elf/Makefile
>>> @@ -775,6 +775,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)
>>
>> Is this really necessary for filtmod2.c?
>
> I assumed it was wisest to link it with the same options as the library
> it was the filter for. It might be harmless, though -- I'll drop it and
> find out!

It's harmless! Removed.

>>> diff --git a/stdlib/Makefile b/stdlib/Makefile
>>> index 3cce9d9..6d7586e 100644
>>> --- a/stdlib/Makefile
>>> +++ b/stdlib/Makefile
>>> @@ -187,6 +187,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)
>>
>> I think this papers over an actual failure. Based on my build logs,
>> tst-putenvmod.so is properly linked against libc (including
>> libc_nonshared.a).
>
> Hmm! I'll take that out and have a look. Maybe __attribute
> ((constructor)) is causing problems, though I'd assume this would work
> fine or other stack-protector users would be howling.
>
> Much of this may be residual fallout from the days before we had the
> stack-protector symbol export from libc/internal refs inside libc
> working right.

Likewise harmless: removed. (At least, it works on x86_64. I'll do a
full test cycle before sending the next series, so it gets tested on
x86-32, sparc64/32, and ARM as well. It might be necessary on one of
those. :) )

>>> diff --git a/sysdeps/x86_64/Makefile b/sysdeps/x86_64/Makefile
>>> index 6d99284..fbe138f 100644
>>> --- a/sysdeps/x86_64/Makefile
>>> +++ b/sysdeps/x86_64/Makefile
>>> @@ -46,6 +46,9 @@ tests-pie += $(quad-pie-test)
>>>  test-extras += tst-quadmod1pie tst-quadmod2pie
>>>  extra-test-objs += tst-quadmod1pie.o tst-quadmod2pie.o
>>>
>>> +CFLAGS-tst-quad1pie.c = $(no-stack-protector)
>>> +CFLAGS-tst-quad2pie.c = $(no-stack-protector)
>>
>> This looks like a genuine test failure as well.  I'll try to reproduce it, after I have tracked down the more catastrophic MIPS
>> build failure …
>
> Yeah: they link against libc as well. Maybe I was misled by an
> old-binutils problem back in the early days of this patch.
>
> I'll try pulling this out as well...

Nope nope nope:

gcc ../sysdeps/x86_64/tst-quad1pie.c -c -std=gnu11 -fgnu89-inline  -O2 -Wall -Werror -Wundef -Wwrite-strings -fmerge-all-constants -frounding-math -fstack-protector-all -g -Wstrict-prototypes -Wold-style-definition            -I../include -I/home/oranix/oracle/src/x86_64-spindle/elf  -I/home/oranix/oracle/src/x86_64-spindle  -I../sysdeps/unix/sysv/linux/x86_64/64  -I../sysdeps/unix/sysv/linux/x86_64  -I../sysdeps/unix/sysv/linux/x86  -I../sysdeps/unix/sysv/linux/wordsize-64  -I../sysdeps/x86_64/nptl  -I../sysdeps/unix/sysv/linux/include -I../sysdeps/unix/sysv/linux  -I../sysdeps/nptl  -I../sysdeps/pthread  -I../sysdeps/gnu  -I../sysdeps/unix/inet  -I../sysdeps/unix/sysv  -I../sysdeps/unix/x86_64 -I../sysdeps/unix  -I../sysdeps/posix  -I../sysdeps/x86_64/64  -I../sysdeps/x86_64/fpu/multiarch  -I../sysdeps/x86_64/fpu  -I../sysdeps/x86/fpu/include -I../sysdeps/x86/fpu  -I../sysdeps/x86_64/multiarch  -I../sysdeps/x86_64  -I../sysdeps/x86  -I../sysdeps/ieee754/ldbl-96/include -I../sysdeps/ieee754/ldbl-96  -I../sysdeps/ieee754/dbl-64/wordsize-64  -I../sysdeps/ieee754/dbl-64  -I../sysdeps/ieee754/flt-32  -I../sysdeps/wordsize-64  -I../sysdeps/ieee754  -I../sysdeps/generic  -I.. -I../libio -I.   -D_LIBC_REENTRANT -include /home/oranix/oracle/src/x86_64-spindle/libc-modules.h -DMODULE_NAME=nonlib -include ../include/libc-symbols.h       -o /home/oranix/oracle/src/x86_64-spindle/elf/tst-quad1pie.o -MD -MP -MF /home/oranix/oracle/src/x86_64-spindle/elf/tst-quad1pie.o.dt -MT /home/oranix/oracle/src/x86_64-spindle/elf/tst-quad1pie.o
gcc ../sysdeps/x86_64/tst-quad2pie.c -c -std=gnu11 -fgnu89-inline  -O2 -Wall -Werror -Wundef -Wwrite-strings -fmerge-all-constants -frounding-math -fstack-protector-all -g -Wstrict-prototypes -Wold-style-definition            -I../include -I/home/oranix/oracle/src/x86_64-spindle/elf  -I/home/oranix/oracle/src/x86_64-spindle  -I../sysdeps/unix/sysv/linux/x86_64/64  -I../sysdeps/unix/sysv/linux/x86_64  -I../sysdeps/unix/sysv/linux/x86  -I../sysdeps/unix/sysv/linux/wordsize-64  -I../sysdeps/x86_64/nptl  -I../sysdeps/unix/sysv/linux/include -I../sysdeps/unix/sysv/linux  -I../sysdeps/nptl  -I../sysdeps/pthread  -I../sysdeps/gnu  -I../sysdeps/unix/inet  -I../sysdeps/unix/sysv  -I../sysdeps/unix/x86_64  -I../sysdeps/unix  -I../sysdeps/posix  -I../sysdeps/x86_64/64  -I../sysdeps/x86_64/fpu/multiarch  -I../sysdeps/x86_64/fpu  -I../sysdeps/x86/fpu/include -I../sysdeps/x86/fpu  -I../sysdeps/x86_64/multiarch  -I../sysdeps/x86_64  -I../sysdeps/x86  -I../sysdeps/ieee754/ldbl-96/include -I../sysdeps/ieee754/ldbl-96  -I../sysdeps/ieee754/dbl-64/wordsize-64  -I../sysdeps/ieee754/dbl-64  -I../sysdeps/ieee754/flt-32  -I../sysdeps/wordsize-64  -I../sysdeps/ieee754  -I../sysdeps/generic  -I.. -I../libio -I.   -D_LIBC_REENTRANT -include /home/oranix/oracle/src/x86_64-spindle/libc-modules.h -DMODULE_NAME=nonlib -include ../include/libc-symbols.h       -o /home/oranix/oracle/src/x86_64-spindle/elf/tst-quad2pie.o -MD -MP -MF /home/oranix/oracle/src/x86_64-spindle/elf/tst-quad2pie.o.dt -MT /home/oranix/oracle/src/x86_64-spindle/elf/tst-quad2pie.o
gcc -pie -Wl,-O1 -nostdlib -nostartfiles -o /home/oranix/oracle/src/x86_64-spindle/elf/tst-quad1pie    -Wl,-z,combreloc -Wl,-z,relro -Wl,--hash-style=both /home/oranix/oracle/src/x86_64-spindle/csu/Scrt1.o /home/oranix/oracle/src/x86_64-spindle/csu/crti.o `gcc  --print-file-name=crtbeginS.o` /home/oranix/oracle/src/x86_64-spindle/elf/tst-quad1pie.o /home/oranix/oracle/src/x86_64-spindle/elf/tst-quadmod1pie.o  -Wl,-dynamic-linker=/lib64/ld-linux-x86-64.so.2 -Wl,-rpath-link=/home/oranix/oracle/src/x86_64-spindle:/home/oranix/oracle/src/x86_64-spindle/math:/home/oranix/oracle/src/x86_64-spindle/elf:/home/oranix/oracle/src/x86_64-spindle/dlfcn:/home/oranix/oracle/src/x86_64-spindle/nss:/home/oranix/oracle/src/x86_64-spindle/nis:/home/oranix/oracle/src/x86_64-spindle/rt:/home/oranix/oracle/src/x86_64-spindle/resolv:/home/oranix/oracle/src/x86_64-spindle/crypt:/home/oranix/oracle/src/x86_64-spindle/mathvec:/home/oranix/oracle/src/x86_64-spindle/nptl /home/oranix/oracle/src/x86_64-spindle/libc.so.6 /home/oranix/oracle/src/x86_64-spindle/libc_nonshared.a -Wl,--as-needed /home/oranix/oracle/src/x86_64-spindle/elf/ld.so -Wl,--no-as-needed -lgcc -Wl,--as-needed -lgcc_s  -Wl,--no-as-needed `gcc  --print-file-name=crtendS.o` /home/oranix/oracle/src/x86_64-spindle/csu/crtn.o
/usr/bin/ld: /home/oranix/oracle/src/x86_64-spindle/elf/tst-quad1pie.o: relocation R_X86_64_PC32 against symbol `__stack_chk_fail@@GLIBC_2.4' can not be used when making a shared object; recompile with -fPIC
/usr/bin/ld: final link failed: Bad value
gcc -pie -Wl,-O1 -nostdlib -nostartfiles -o /home/oranix/oracle/src/x86_64-spindle/elf/tst-quad2pie    -Wl,-z,combreloc -Wl,-z,relro -Wl,--hash-style=both /home/oranix/oracle/src/x86_64-spindle/csu/Scrt1.o /home/oranix/oracle/src/x86_64-spindle/csu/crti.o `gcc  --print-file-name=crtbeginS.o` /home/oranix/oracle/src/x86_64-spindle/elf/tst-quad2pie.o /home/oranix/oracle/src/x86_64-spindle/elf/tst-quadmod2pie.o  -Wl,-dynamic-linker=/lib64/ld-linux-x86-64.so.2 -Wl,-rpath-link=/home/oranix/oracle/src/x86_64-spindle:/home/oranix/oracle/src/x86_64-spindle/math:/home/oranix/oracle/src/x86_64-spindle/elf:/home/oranix/oracle/src/x86_64-spindle/dlfcn:/home/oranix/oracle/src/x86_64-spindle/nss:/home/oranix/oracle/src/x86_64-spindle/nis:/home/oranix/oracle/src/x86_64-spindle/rt:/home/oranix/oracle/src/x86_64-spindle/resolv:/home/oranix/oracle/src/x86_64-spindle/crypt:/home/oranix/oracle/src/x86_64-spindle/mathvec:/home/oranix/oracle/src/x86_64-spindle/nptl /home/oranix/oracle/src/x86_64-spindle/libc.so.6 /home/oranix/oracle/src/x86_64-spindle/libc_nonshared.a -Wl,--as-needed /home/oranix/oracle/src/x86_64-spindle/elf/ld.so -Wl,--no-as-needed -lgcc -Wl,--as-needed -lgcc_s  -Wl,--no-as-needed `gcc  --print-file-name=crtendS.o` /home/oranix/oracle/src/x86_64-spindle/csu/crtn.o
/usr/bin/ld: /home/oranix/oracle/src/x86_64-spindle/elf/tst-quad2pie.o: relocation R_X86_64_PC32 against symbol `__stack_chk_fail@@GLIBC_2.4' can not be used when making a shared object; recompile with -fPIC
/usr/bin/ld: final link failed: Bad value

I note that the quad 'pie' tests are not actually being build with -fpie
or anything like it, so it's no surprise that GCC is generating non-PIE
relocations. That's probably the underlying bug here. This seems to fix
it, and mirrors what is being done for elf/vismain.c. (Will roll it in
if you agree, or post it separately, 'cos it's really an unrelated bug.)

Fix to all the above, as delta against previous patch:

diff --git a/elf/Makefile b/elf/Makefile
index 7588ca0..2c87a94 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -775,9 +775,8 @@ $(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.
+# This does 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 6d7586e..cae2e8d 100644
--- a/stdlib/Makefile
+++ b/stdlib/Makefile
@@ -187,8 +187,6 @@ 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
 
diff --git a/sysdeps/x86_64/Makefile b/sysdeps/x86_64/Makefile
index fbe138f..5f25893 100644
--- a/sysdeps/x86_64/Makefile
+++ b/sysdeps/x86_64/Makefile
@@ -46,12 +46,12 @@ tests-pie += $(quad-pie-test)
 test-extras += tst-quadmod1pie tst-quadmod2pie
 extra-test-objs += tst-quadmod1pie.o tst-quadmod2pie.o
 
-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
 
+CFLAGS-tst-quad1pie.c = $(PIE-ccflag)
+CFLAGS-tst-quad2pie.c = $(PIE-ccflag)
+
 tests += tst-audit3 tst-audit4 tst-audit5 tst-audit6 tst-audit7 tst-audit10
 test-extras += tst-audit4-aux tst-audit10-aux
 extra-test-objs += tst-audit4-aux.o tst-audit10-aux.o

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

* Re: [PATCH 09/12] Link various tests with -fno-stack-protector.
  2016-12-15 15:54       ` Florian Weimer
@ 2016-12-15 15:56         ` Nix
  0 siblings, 0 replies; 40+ messages in thread
From: Nix @ 2016-12-15 15:56 UTC (permalink / raw)
  To: Florian Weimer; +Cc: libc-alpha

On 15 Dec 2016, Florian Weimer uttered the following:

> So I'll construct a new series with your STACK_PROTECTOR_LEVEL patch and see where things go with that.

I'll throw everything you proposed together into a new series and throw
my full test cycle at it too.

-- 
NULL && (void)

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

* Re: [PATCH 08/12] De-PLTize __stack_chk_fail internal calls within libc.so.
  2016-12-15 14:40             ` Nix
@ 2016-12-15 17:24               ` Nix
  2016-12-15 18:22                 ` Florian Weimer
  0 siblings, 1 reply; 40+ messages in thread
From: Nix @ 2016-12-15 17:24 UTC (permalink / raw)
  To: Florian Weimer; +Cc: libc-alpha, Adhemerval Zanella

On 15 Dec 2016, nix@esperi.org.uk verbalised:

> diff --git a/sysdeps/generic/symbol-hacks.h b/sysdeps/generic/symbol-hacks.h
> index 36908b5..15ff56a 100644
> --- a/sysdeps/generic/symbol-hacks.h
> +++ b/sysdeps/generic/symbol-hacks.h
> @@ -7,5 +7,7 @@ asm ("memcpy = __GI_memcpy");
>  
>  /* -fstack-protector generates calls to __stack_chk_fail, which need
>     similar adjustments to avoid going through the PLT.  */
> +# if defined STACK_PROTECTOR_LEVEL && STACK_PROTECTOR_LEVEL > 0
>  asm ("__stack_chk_fail = __stack_chk_fail_local");
> +# endif
>  #endif

This causes (minor) problems on SPARC:

Extra PLT reference: libc.so: __stack_chk_fail

Whether we can disregard this, I don't know, but it does feel wrong.

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

* Re: [PATCH 08/12] De-PLTize __stack_chk_fail internal calls within libc.so.
  2016-12-15 17:24               ` Nix
@ 2016-12-15 18:22                 ` Florian Weimer
  2016-12-15 20:00                   ` Nix
  0 siblings, 1 reply; 40+ messages in thread
From: Florian Weimer @ 2016-12-15 18:22 UTC (permalink / raw)
  To: Nix; +Cc: libc-alpha, Adhemerval Zanella

On 12/15/2016 06:24 PM, Nix wrote:
> On 15 Dec 2016, nix@esperi.org.uk verbalised:
>
>> diff --git a/sysdeps/generic/symbol-hacks.h b/sysdeps/generic/symbol-hacks.h
>> index 36908b5..15ff56a 100644
>> --- a/sysdeps/generic/symbol-hacks.h
>> +++ b/sysdeps/generic/symbol-hacks.h
>> @@ -7,5 +7,7 @@ asm ("memcpy = __GI_memcpy");
>>
>>  /* -fstack-protector generates calls to __stack_chk_fail, which need
>>     similar adjustments to avoid going through the PLT.  */
>> +# if defined STACK_PROTECTOR_LEVEL && STACK_PROTECTOR_LEVEL > 0
>>  asm ("__stack_chk_fail = __stack_chk_fail_local");
>> +# endif
>>  #endif
>
> This causes (minor) problems on SPARC:
>
> Extra PLT reference: libc.so: __stack_chk_fail
>
> Whether we can disregard this, I don't know, but it does feel wrong.

Well, avoiding this is the point of __stack_chk_fail_local, isn't it? 
So we surely can't ignore it.

I think what is going on is that once a symbol has a hidden anywhere in 
a static link, all references to it are turned hidden.  Previously, this 
hidden reference occurred in the file which *defined* 
__stack_chk_fail_local, but is now gone from there.  (At the assembler 
level, the .hidden directive is markedly different from the GCC 
visibility attribute.)

Could you try this?

# if defined STACK_PROTECTOR_LEVEL && STACK_PROTECTOR_LEVEL > 0
asm (".hidden __stack_chk_fail_local");
asm ("__stack_chk_fail = __stack_chk_fail_local");
# endif

Thanks,
Florian

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

* Re: [PATCH 08/12] De-PLTize __stack_chk_fail internal calls within libc.so.
  2016-12-15 18:22                 ` Florian Weimer
@ 2016-12-15 20:00                   ` Nix
  2016-12-15 20:22                     ` Florian Weimer
  0 siblings, 1 reply; 40+ messages in thread
From: Nix @ 2016-12-15 20:00 UTC (permalink / raw)
  To: Florian Weimer; +Cc: libc-alpha, Adhemerval Zanella

On 15 Dec 2016, Florian Weimer verbalised:

> On 12/15/2016 06:24 PM, Nix wrote:
>> This causes (minor) problems on SPARC:
>>
>> Extra PLT reference: libc.so: __stack_chk_fail
>>
>> Whether we can disregard this, I don't know, but it does feel wrong.
>
> Well, avoiding this is the point of __stack_chk_fail_local, isn't it?
> So we surely can't ignore it.

Agreed. (I just wasn't sure I could remember what this was added for...)

> I think what is going on is that once a symbol has a hidden anywhere
> in a static link, all references to it are turned hidden. Previously,
> this hidden reference occurred in the file which *defined*
> __stack_chk_fail_local, but is now gone from there. (At the assembler

Oof. And with no such reference, it ends up visible again...

> Could you try this?
>
> # if defined STACK_PROTECTOR_LEVEL && STACK_PROTECTOR_LEVEL > 0
> asm (".hidden __stack_chk_fail_local");
> asm ("__stack_chk_fail = __stack_chk_fail_local");
> # endif

No change :( the only reference to __stack_chk_fail is still inside
stack_chk_fail_local:

Symbols from libc_pic.a[libc-stack_chk_fail_local.os]:

Name                        Value            Class  Type     Size             Line Section

__GI_memcpy                ||GLOBAL|NOTYPE  ||    |UNDEF
__GI_memmove               ||GLOBAL|NOTYPE  ||    |UNDEF
__GI_memset                ||GLOBAL|NOTYPE  ||    |UNDEF
__stack_chk_fail           ||GLOBAL|NOTYPE  ||    |UNDEF
__stack_chk_fail_local     |0000000000000000|GLOBAL|FUNC    |0000000000000010|    |.text
libc-stack_chk_fail_local.c|0000000000000000|LOCAL |FILE    |0000000000000000|    |ABS

(And, of course, this code is not affected by your suggestion, because
it's compiled with -fno-stack-protector -DSTACK_PROTECTOR_LEVEL=0.)

-- 
NULL && (void)

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

* Re: [PATCH 08/12] De-PLTize __stack_chk_fail internal calls within libc.so.
  2016-12-15 20:00                   ` Nix
@ 2016-12-15 20:22                     ` Florian Weimer
  2016-12-15 20:44                       ` Florian Weimer
  2016-12-16 11:39                       ` Florian Weimer
  0 siblings, 2 replies; 40+ messages in thread
From: Florian Weimer @ 2016-12-15 20:22 UTC (permalink / raw)
  To: Nix; +Cc: libc-alpha, Adhemerval Zanella

On 12/15/2016 09:00 PM, Nix wrote:

>> Could you try this?
>>
>> # if defined STACK_PROTECTOR_LEVEL && STACK_PROTECTOR_LEVEL > 0
>> asm (".hidden __stack_chk_fail_local");
>> asm ("__stack_chk_fail = __stack_chk_fail_local");
>> # endif
>
> No change :( the only reference to __stack_chk_fail is still inside
> stack_chk_fail_local:
>
> Symbols from libc_pic.a[libc-stack_chk_fail_local.os]:
>
> Name                        Value            Class  Type     Size             Line Section
>
> __GI_memcpy                ||GLOBAL|NOTYPE  ||    |UNDEF
> __GI_memmove               ||GLOBAL|NOTYPE  ||    |UNDEF
> __GI_memset                ||GLOBAL|NOTYPE  ||    |UNDEF
> __stack_chk_fail           ||GLOBAL|NOTYPE  ||    |UNDEF
> __stack_chk_fail_local     |0000000000000000|GLOBAL|FUNC    |0000000000000010|    |.text
> libc-stack_chk_fail_local.c|0000000000000000|LOCAL |FILE    |0000000000000000|    |ABS
>
> (And, of course, this code is not affected by your suggestion, because
> it's compiled with -fno-stack-protector -DSTACK_PROTECTOR_LEVEL=0.)

I think this attempt at PLT avoidance within libc.so itself is subtly 
wrong.  We need to mirror more closely what 
libc_hidden_proto/libc_hidden_def does, and perhaps disentangle this 
from the __stack_chk_fail_local definition used in other DSOs.

I think this means removing any definition of a C function definition 
called __stack_chk_fail_local from libc.so, and instead use a strong 
alias from __stack_chk_fail to __stack_chk_fail_local to define the 
symbol.  The alias will not incorporate a PLT reference.  If you look at 
include/libc-symbols.h, strong_alias and hidden_def are quite similar.

It's too late for me to try this today. :-/

Florian

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

* Re: [PATCH 08/12] De-PLTize __stack_chk_fail internal calls within libc.so.
  2016-12-15 20:22                     ` Florian Weimer
@ 2016-12-15 20:44                       ` Florian Weimer
  2016-12-15 20:49                         ` Nix
  2016-12-16 11:39                       ` Florian Weimer
  1 sibling, 1 reply; 40+ messages in thread
From: Florian Weimer @ 2016-12-15 20:44 UTC (permalink / raw)
  To: Nix; +Cc: libc-alpha, Adhemerval Zanella

* Florian Weimer:

> On 12/15/2016 09:00 PM, Nix wrote:
>
>>> Could you try this?
>>>
>>> # if defined STACK_PROTECTOR_LEVEL && STACK_PROTECTOR_LEVEL > 0
>>> asm (".hidden __stack_chk_fail_local");
>>> asm ("__stack_chk_fail = __stack_chk_fail_local");
>>> # endif
>>
>> No change :( the only reference to __stack_chk_fail is still inside
>> stack_chk_fail_local:
>>
>> Symbols from libc_pic.a[libc-stack_chk_fail_local.os]:
>>
>> Name Value Class Type Size Line Section
>>
>> __GI_memcpy                ||GLOBAL|NOTYPE  ||    |UNDEF
>> __GI_memmove               ||GLOBAL|NOTYPE  ||    |UNDEF
>> __GI_memset                ||GLOBAL|NOTYPE  ||    |UNDEF
>> __stack_chk_fail           ||GLOBAL|NOTYPE  ||    |UNDEF
>> __stack_chk_fail_local |0000000000000000|GLOBAL|FUNC
>> |0000000000000010| |.text
>> libc-stack_chk_fail_local.c|0000000000000000|LOCAL |FILE
>> |0000000000000000| |ABS
>>
>> (And, of course, this code is not affected by your suggestion, because
>> it's compiled with -fno-stack-protector -DSTACK_PROTECTOR_LEVEL=0.)
>
> I think this attempt at PLT avoidance within libc.so itself is subtly 
> wrong.  We need to mirror more closely what 
> libc_hidden_proto/libc_hidden_def does, and perhaps disentangle this 
> from the __stack_chk_fail_local definition used in other DSOs.
>
> I think this means removing any definition of a C function definition 
> called __stack_chk_fail_local from libc.so, and instead use a strong 
> alias from __stack_chk_fail to __stack_chk_fail_local to define the 
> symbol.  The alias will not incorporate a PLT reference.  If you look at 
> include/libc-symbols.h, strong_alias and hidden_def are quite similar.

It may also be a good idea to switch to a different symbol for
__stack_chk_fail_local because this collides with the name GCC uses on
some architectures for a similar purpose.  Or is this the intent here?

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

* Re: [PATCH 08/12] De-PLTize __stack_chk_fail internal calls within libc.so.
  2016-12-15 20:44                       ` Florian Weimer
@ 2016-12-15 20:49                         ` Nix
  2016-12-15 20:58                           ` Florian Weimer
  0 siblings, 1 reply; 40+ messages in thread
From: Nix @ 2016-12-15 20:49 UTC (permalink / raw)
  To: Florian Weimer; +Cc: libc-alpha, Adhemerval Zanella

On 15 Dec 2016, Florian Weimer spake thusly:

> * Florian Weimer:
>
>> I think this means removing any definition of a C function definition 
>> called __stack_chk_fail_local from libc.so, and instead use a strong 
>> alias from __stack_chk_fail to __stack_chk_fail_local to define the 
>> symbol.  The alias will not incorporate a PLT reference.  If you look at 
>> include/libc-symbols.h, strong_alias and hidden_def are quite similar.
>
> It may also be a good idea to switch to a different symbol for
> __stack_chk_fail_local because this collides with the name GCC uses on
> some architectures for a similar purpose.  Or is this the intent here?

That's the point. On targets where __stack_chk_fail_local is called
(e.g. x86-32), we're not generating the calls to this thing: GCC is.
We cannot pick a different name.

-- 
NULL && (void)

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

* Re: [PATCH 08/12] De-PLTize __stack_chk_fail internal calls within libc.so.
  2016-12-15 20:49                         ` Nix
@ 2016-12-15 20:58                           ` Florian Weimer
  0 siblings, 0 replies; 40+ messages in thread
From: Florian Weimer @ 2016-12-15 20:58 UTC (permalink / raw)
  To: Nix; +Cc: libc-alpha, Adhemerval Zanella

> On 15 Dec 2016, Florian Weimer spake thusly:
>
>> * Florian Weimer:
>>
>>> I think this means removing any definition of a C function definition 
>>> called __stack_chk_fail_local from libc.so, and instead use a strong 
>>> alias from __stack_chk_fail to __stack_chk_fail_local to define the 
>>> symbol.  The alias will not incorporate a PLT reference.  If you look at 
>>> include/libc-symbols.h, strong_alias and hidden_def are quite similar.
>>
>> It may also be a good idea to switch to a different symbol for
>> __stack_chk_fail_local because this collides with the name GCC uses on
>> some architectures for a similar purpose.  Or is this the intent here?
>
> That's the point. On targets where __stack_chk_fail_local is called
> (e.g. x86-32), we're not generating the calls to this thing: GCC is.
> We cannot pick a different name.

Ahh, and GCC does not synthesize a local definition, so there is no
actual collision.

Then the strong_alias approach should work for libc.so.
libc_nonshared.a still needs a hidden function definition, of course.

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

* Re: [PATCH 08/12] De-PLTize __stack_chk_fail internal calls within libc.so.
  2016-12-15 20:22                     ` Florian Weimer
  2016-12-15 20:44                       ` Florian Weimer
@ 2016-12-16 11:39                       ` Florian Weimer
  2016-12-16 11:44                         ` Nix
  1 sibling, 1 reply; 40+ messages in thread
From: Florian Weimer @ 2016-12-16 11:39 UTC (permalink / raw)
  To: Nix; +Cc: libc-alpha, Adhemerval Zanella

[-- Attachment #1: Type: text/plain, Size: 1912 bytes --]

On 12/15/2016 09:22 PM, Florian Weimer wrote:
> On 12/15/2016 09:00 PM, Nix wrote:
>
>>> Could you try this?
>>>
>>> # if defined STACK_PROTECTOR_LEVEL && STACK_PROTECTOR_LEVEL > 0
>>> asm (".hidden __stack_chk_fail_local");
>>> asm ("__stack_chk_fail = __stack_chk_fail_local");
>>> # endif
>>
>> No change :( the only reference to __stack_chk_fail is still inside
>> stack_chk_fail_local:
>>
>> Symbols from libc_pic.a[libc-stack_chk_fail_local.os]:
>>
>> Name                        Value            Class  Type
>> Size             Line Section
>>
>> __GI_memcpy                ||GLOBAL|NOTYPE  ||    |UNDEF
>> __GI_memmove               ||GLOBAL|NOTYPE  ||    |UNDEF
>> __GI_memset                ||GLOBAL|NOTYPE  ||    |UNDEF
>> __stack_chk_fail           ||GLOBAL|NOTYPE  ||    |UNDEF
>> __stack_chk_fail_local     |0000000000000000|GLOBAL|FUNC
>> |0000000000000010|    |.text
>> libc-stack_chk_fail_local.c|0000000000000000|LOCAL |FILE
>> |0000000000000000|    |ABS
>>
>> (And, of course, this code is not affected by your suggestion, because
>> it's compiled with -fno-stack-protector -DSTACK_PROTECTOR_LEVEL=0.)
>
> I think this attempt at PLT avoidance within libc.so itself is subtly
> wrong.  We need to mirror more closely what
> libc_hidden_proto/libc_hidden_def does, and perhaps disentangle this
> from the __stack_chk_fail_local definition used in other DSOs.
>
> I think this means removing any definition of a C function definition
> called __stack_chk_fail_local from libc.so, and instead use a strong
> alias from __stack_chk_fail to __stack_chk_fail_local to define the
> symbol.  The alias will not incorporate a PLT reference.  If you look at
> include/libc-symbols.h, strong_alias and hidden_def are quite similar.

With this patch on top of the series you posted (without any other 
changes), I get both MIPS and ia64 to build, and I don't see a PLT 
failure on SPARC.

Thanks,
Florian

[-- Attachment #2: ssp.patch --]
[-- Type: text/x-patch, Size: 2088 bytes --]

diff --git a/debug/Makefile b/debug/Makefile
index 27da081..cfcf392 100644
--- a/debug/Makefile
+++ b/debug/Makefile
@@ -48,14 +48,10 @@ routines  = backtrace backtracesyms backtracesymsfd noophooks \
 	    vdprintf_chk obprintf_chk \
 	    longjmp_chk ____longjmp_chk \
 	    fdelt_chk poll_chk ppoll_chk \
-	    stack_chk_fail fortify_fail libc-stack_chk_fail_local \
+	    stack_chk_fail fortify_fail \
 	    $(static-only-routines)
 
-# stack_chk_fail_local must be non-PIC, thus static-only, but we also
-# want an identical thunk hidden in libc.so to avoid going via the PLT.
-
 static-only-routines := warning-nop stack_chk_fail_local
-shared-only-routines += libc-stack_chk_fail_local
 
 # Building the stack-protector failure routines with stack protection
 # makes no sense.
diff --git a/debug/libc-stack_chk_fail_local.c b/debug/libc-stack_chk_fail_local.c
deleted file mode 100644
index 73da970..0000000
--- a/debug/libc-stack_chk_fail_local.c
+++ /dev/null
@@ -1,3 +0,0 @@
-/* This goes into the shared libc.  */
-
-#include <stack_chk_fail_local.c>
diff --git a/debug/stack_chk_fail.c b/debug/stack_chk_fail.c
index 4d0796f..a545239 100644
--- a/debug/stack_chk_fail.c
+++ b/debug/stack_chk_fail.c
@@ -27,3 +27,11 @@ __stack_chk_fail (void)
 {
   __fortify_fail ("stack smashing detected");
 }
+
+#ifdef SHARED
+/* The compiler generates hidden references to __stack_chk_fail for
+   PLT avoidance.  Outside of libc.so, a definition provided by
+   libc_nonshared.a, but within libc.so, a local definition is
+   needed.  */
+strong_alias (__stack_chk_fail, __stack_chk_fail_local)
+#endif
diff --git a/sysdeps/generic/symbol-hacks.h b/sysdeps/generic/symbol-hacks.h
index 36908b5..ce576c9 100644
--- a/sysdeps/generic/symbol-hacks.h
+++ b/sysdeps/generic/symbol-hacks.h
@@ -4,8 +4,4 @@
 asm ("memmove = __GI_memmove");
 asm ("memset = __GI_memset");
 asm ("memcpy = __GI_memcpy");
-
-/* -fstack-protector generates calls to __stack_chk_fail, which need
-   similar adjustments to avoid going through the PLT.  */
-asm ("__stack_chk_fail = __stack_chk_fail_local");
 #endif

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

* Re: [PATCH 08/12] De-PLTize __stack_chk_fail internal calls within libc.so.
  2016-12-16 11:39                       ` Florian Weimer
@ 2016-12-16 11:44                         ` Nix
  0 siblings, 0 replies; 40+ messages in thread
From: Nix @ 2016-12-16 11:44 UTC (permalink / raw)
  To: Florian Weimer; +Cc: libc-alpha, Adhemerval Zanella

On 16 Dec 2016, Florian Weimer outgrape:

> On 12/15/2016 09:22 PM, Florian Weimer wrote:
>> I think this attempt at PLT avoidance within libc.so itself is subtly
>> wrong.  We need to mirror more closely what
>> libc_hidden_proto/libc_hidden_def does, and perhaps disentangle this
>> from the __stack_chk_fail_local definition used in other DSOs.
>>
>> I think this means removing any definition of a C function definition
>> called __stack_chk_fail_local from libc.so, and instead use a strong
>> alias from __stack_chk_fail to __stack_chk_fail_local to define the
>> symbol.  The alias will not incorporate a PLT reference.  If you look at
>> include/libc-symbols.h, strong_alias and hidden_def are quite similar.
>
> With this patch on top of the series you posted (without any other changes), I get both MIPS and ia64 to build, and I don't see a
> PLT failure on SPARC.

I'll give this a spin, then post a new series.

It'll be a few hours, as usual for a full test cycle :(

-- 
NULL && (void)

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

end of thread, other threads:[~2016-12-16 11:44 UTC | newest]

Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-28 12:33 --enable-stack-protector for glibc, v9 Nix
2016-11-28 12:33 ` [PATCH 04/12] Compile the entire dynamic linker with -fno-stack-protector Nix
2016-11-28 12:33 ` [PATCH 06/12] Work even with compilers hacked to enable -fstack-protector by default Nix
2016-11-28 12:33 ` [PATCH 03/12] Mark all machinery needed in early static-link init as -fno-stack-protector Nix
2016-12-15 14:01   ` Florian Weimer
2016-12-15 14:21     ` Nick Alcock
2016-12-15 14:31       ` Florian Weimer
2016-12-15 14:44         ` Nick Alcock
2016-11-28 12:33 ` [PATCH 12/12] Enable -fstack-protector=* when requested by configure Nix
2016-11-28 12:33 ` [PATCH 07/12] Add stack_chk_fail_local to libc.so Nix
2016-11-28 13:25 ` [PATCH 08/12] De-PLTize __stack_chk_fail internal calls within libc.so Nix
2016-12-15 13:56   ` Florian Weimer
2016-12-15 14:16     ` Nix
2016-12-15 14:21       ` Florian Weimer
2016-12-15 14:29         ` Nix
2016-12-15 14:33           ` Florian Weimer
2016-12-15 14:40             ` Nix
2016-12-15 17:24               ` Nix
2016-12-15 18:22                 ` Florian Weimer
2016-12-15 20:00                   ` Nix
2016-12-15 20:22                     ` Florian Weimer
2016-12-15 20:44                       ` Florian Weimer
2016-12-15 20:49                         ` Nix
2016-12-15 20:58                           ` Florian Weimer
2016-12-16 11:39                       ` Florian Weimer
2016-12-16 11:44                         ` Nix
2016-11-28 13:25 ` [PATCH 09/12] Link various tests with -fno-stack-protector Nix
2016-12-15 14:43   ` Florian Weimer
2016-12-15 14:54     ` Nix
2016-12-15 15:54       ` Florian Weimer
2016-12-15 15:56         ` Nix
2016-12-15 15:55       ` Nix
2016-11-28 13:25 ` [PATCH 10/12] Drop explicit stack-protection of pieces of the system Nix
2016-11-28 13:26 ` [PATCH 11/12] Do not stack-protect sigreturn stubs Nix
2016-11-28 13:26 ` [PATCH 01/12] Initialize the stack guard earlier when linking statically Nix
2016-11-28 13:26 ` [PATCH 02/12] Do not stack-protect ifunc resolvers Nix
2016-12-15 12:18   ` Florian Weimer
2016-12-15 12:34     ` Nick Alcock
2016-12-15 12:58       ` Florian Weimer
2016-11-28 13:26 ` [PATCH 05/12] Prevent the rtld mapfile computation from dragging in __stack_chk_fail* Nix

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