[PATCH 0/3] Clean up stack-protector-all build

[PATCH 0/3] Clean up stack-protector-all build

[Siddhesh Poyarekar]

This series of patches cleans up state for when glibc is configured with
--enable-stack-protector=all.  Now tests for this build configuration
runs clean with the exception of tst-reload2, which is an unrelated
issue:

https://sourceware.org/bugzilla/show_bug.cgi?id=27537

Siddhesh Poyarekar (3):
  Build get-cpuid-feature-leaf.c without stack-protector
  Disable stack protector for ifunc resolvers in tests
  Build libc-start with stack protector for SHARED

 Makeconfig                  |  4 ++++
 csu/Makefile                | 22 ++++++++++++----------
 elf/Makefile                |  4 ----
 elf/ifuncmain9.c            |  1 +
 sysdeps/x86/Makefile        |  2 ++
 sysdeps/x86/tst-ifunc-isa.h |  2 ++
 6 files changed, 21 insertions(+), 14 deletions(-)

-- 
2.29.2

[PATCH 1/3] Build get-cpuid-feature-leaf.c without stack-protector

[Siddhesh Poyarekar]

__x86_get_cpuid_feature_leaf is called during early startup, before
the stack check guard is initialized and is hence not safe to build
with -fstack-protector.
---
 sysdeps/x86/Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sysdeps/x86/Makefile b/sysdeps/x86/Makefile
index e1f9379fd8..ca3ef886c1 100644
--- a/sysdeps/x86/Makefile
+++ b/sysdeps/x86/Makefile
@@ -7,6 +7,8 @@ sysdep_routines += get-cpuid-feature-leaf
 sysdep-dl-routines += dl-get-cpu-features
 sysdep_headers += sys/platform/x86.h
 
+CFLAGS-get-cpuid-feature-leaf.c += $(no-stack-protector)
+
 tests += tst-get-cpu-features tst-get-cpu-features-static \
 	 tst-cpu-features-cpuinfo tst-cpu-features-cpuinfo-static \
 	 tst-cpu-features-supports tst-cpu-features-supports-static
-- 
2.29.2

Re: [PATCH 1/3] Build get-cpuid-feature-leaf.c without stack-protector

[Florian Weimer]

* Siddhesh Poyarekar via Libc-alpha:

> __x86_get_cpuid_feature_leaf is called during early startup, before
> the stack check guard is initialized and is hence not safe to build
> with -fstack-protector.

This should be restricted to the static case, I think.

Thanks,
Florian

[PATCH 2/3] Disable stack protector for ifunc resolvers in tests

[Siddhesh Poyarekar]

IFUNC resolvers get called too early for stack protector to be useful,
so fix the elf tests to disable stack protector for the resolver
functions.  This fixes test failures in the static tests on x86_64.
---
 elf/ifuncmain9.c            | 1 +
 sysdeps/x86/tst-ifunc-isa.h | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/elf/ifuncmain9.c b/elf/ifuncmain9.c
index 2c4e95a051..e775c5cfa9 100644
--- a/elf/ifuncmain9.c
+++ b/elf/ifuncmain9.c
@@ -43,6 +43,7 @@ implementation (void)
 }
 
 static __typeof__ (implementation) *
+inhibit_stack_protector
 resolver (void)
 {
   ++resolver_called;
diff --git a/sysdeps/x86/tst-ifunc-isa.h b/sysdeps/x86/tst-ifunc-isa.h
index 60aa1cea6a..6d906966a7 100644
--- a/sysdeps/x86/tst-ifunc-isa.h
+++ b/sysdeps/x86/tst-ifunc-isa.h
@@ -29,6 +29,7 @@ enum isa
 };
 
 enum isa
+__attribute__ ((__optimize__ ("-fno-stack-protector")))
 get_isa (void)
 {
   if (CPU_FEATURE_USABLE (AVX512F))
@@ -83,6 +84,7 @@ isa_none (void)
 int foo (void) __attribute__ ((ifunc ("foo_ifunc")));
 
 void *
+__attribute__ ((__optimize__ ("-fno-stack-protector")))
 foo_ifunc (void)
 {
   switch (get_isa ())
-- 
2.29.2

Re: [PATCH 2/3] Disable stack protector for ifunc resolvers in tests

[Florian Weimer]

* Siddhesh Poyarekar via Libc-alpha:

> IFUNC resolvers get called too early for stack protector to be useful,
> so fix the elf tests to disable stack protector for the resolver
> functions.  This fixes test failures in the static tests on x86_64.

With -fstack-protector-all, presumably.  Does this need a bug in
Bugzilla?

Thanks,
Florian

Re: [PATCH 2/3] Disable stack protector for ifunc resolvers in tests

[Siddhesh Poyarekar]

On 3/10/21 1:29 PM, Florian Weimer via Libc-alpha wrote:
> * Siddhesh Poyarekar via Libc-alpha:
> 
>> IFUNC resolvers get called too early for stack protector to be useful,
>> so fix the elf tests to disable stack protector for the resolver
>> functions.  This fixes test failures in the static tests on x86_64.
> 
> With -fstack-protector-all, presumably.  Does this need a bug in
> Bugzilla?

I started to file it and found there already was a report[1] and a patch 
that fixes it.  I'll remove the overlapping hunk from my patch and 
credit David with it and close that bug once this goes in.

Siddhesh

Re: [PATCH 2/3] Disable stack protector for ifunc resolvers in tests

[Florian Weimer]

* Siddhesh Poyarekar:

> On 3/10/21 1:29 PM, Florian Weimer via Libc-alpha wrote:
>> * Siddhesh Poyarekar via Libc-alpha:
>> 
>>> IFUNC resolvers get called too early for stack protector to be useful,
>>> so fix the elf tests to disable stack protector for the resolver
>>> functions.  This fixes test failures in the static tests on x86_64.
>> With -fstack-protector-all, presumably.  Does this need a bug in
>> Bugzilla?
>
> I started to file it and found there already was a report[1] and a
> patch that fixes it.  I'll remove the overlapping hunk from my patch
> and credit David with it and close that bug once this goes in.

Okay, please mention the bug number in the commit message then.
(I assume there's a bug for it, [1] is dangling.)

Thanks,
Florian

Re: [PATCH 2/3] Disable stack protector for ifunc resolvers in tests

[Siddhesh Poyarekar]

On 3/10/21 3:09 PM, Florian Weimer via Libc-alpha wrote:
> * Siddhesh Poyarekar:
> 
>> On 3/10/21 1:29 PM, Florian Weimer via Libc-alpha wrote:
>>> * Siddhesh Poyarekar via Libc-alpha:
>>>
>>>> IFUNC resolvers get called too early for stack protector to be useful,
>>>> so fix the elf tests to disable stack protector for the resolver
>>>> functions.  This fixes test failures in the static tests on x86_64.
>>> With -fstack-protector-all, presumably.  Does this need a bug in
>>> Bugzilla?
>>
>> I started to file it and found there already was a report[1] and a
>> patch that fixes it.  I'll remove the overlapping hunk from my patch
>> and credit David with it and close that bug once this goes in.
> 
> Okay, please mention the bug number in the commit message then.
> (I assume there's a bug for it, [1] is dangling.)

Hah, I was just seeing if you were paying attention ;)

Siddhesh

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=25680

[PATCH 3/3] Build libc-start with stack protector for SHARED

[Siddhesh Poyarekar]

This does not change the emitted code since __libc_start_main does not
return, but is important for formal flags compliance.

This also cleans up the cosmetic inconsistency in the stack protector
flags in csu, especially the incorrect value of STACK_PROTECTOR_LEVEL.
---
 Makeconfig   |  4 ++++
 csu/Makefile | 22 ++++++++++++----------
 elf/Makefile |  4 ----
 3 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/Makeconfig b/Makeconfig
index 0a4811b5e5..c99464fdfa 100644
--- a/Makeconfig
+++ b/Makeconfig
@@ -856,6 +856,10 @@ ifneq ($(stack-protector),)
 +stack-protector=$(stack-protector)
 endif
 
+define elide-stack-protector
+$(if $(filter $(@F),$(patsubst %,%$(1),$(2))), $(no-stack-protector))
+endef
+
 # 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.
diff --git a/csu/Makefile b/csu/Makefile
index e587434be8..3054329cea 100644
--- a/csu/Makefile
+++ b/csu/Makefile
@@ -45,18 +45,20 @@ install-lib = $(start-installed-name) g$(start-installed-name) $(csu-dummies)
 # code is compiled with special flags.
 tests =
 
-CFLAGS-.o += $(no-stack-protector)
-CFLAGS-.op += $(no-stack-protector)
-CFLAGS-.os += $(no-stack-protector)
-
-# Dummy object not actually used for anything.  It is linked into
-# crt1.o nevertheless, which in turn is statically linked into
+# static-reloc.os is a dummy object not actually used for anything.  It is
+# linked into crt1.o nevertheless, which in turn is statically linked into
 # applications, so that build flags matter.
 # See <https://sourceware.org/ml/libc-alpha/2018-07/msg00101.html>.
-# NB: Using $(stack-protector) in this way causes a wrong definition
-# STACK_PROTECTOR_LEVEL due to the preceding $(no-stack-protector),
-# but it does not matter for this source file.
-CFLAGS-static-reloc.os += $(stack-protector)
+#
+# libc-start.os is safe to be built with stack protector since
+# __libc_start_main is called after stack canary setup is done.
+ssp-safe.os = static-reloc libc-start
+
+CFLAGS-.o += $(call elide-stack-protector,.o,$(routines))
+CFLAGS-.op += $(call elide-stack-protector,.op,$(routines))
+CFLAGS-.oS += $(call elide-stack-protector,.oS,$(routines))
+CFLAGS-.os += $(call elide-stack-protector,.os,$(filter-out \
+						 $(ssp-safe.os),$(routines)))
 
 ifeq (yes,$(build-shared))
 extra-objs += S$(start-installed-name) gmon-start.os
diff --git a/elf/Makefile b/elf/Makefile
index b06bf6ca20..285d9f2f3c 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -83,10 +83,6 @@ endif
 # Also compile all routines in the static library that are elided from
 # the shared libc because they are in libc.a in 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-.os += $(call elide-stack-protector,.os,$(all-rtld-routines))
-- 
2.29.2

Re: [PATCH 3/3] Build libc-start with stack protector for SHARED

[Florian Weimer]

* Siddhesh Poyarekar via Libc-alpha:

> This does not change the emitted code since __libc_start_main does not
> return, but is important for formal flags compliance.
>
> This also cleans up the cosmetic inconsistency in the stack protector
> flags in csu, especially the incorrect value of STACK_PROTECTOR_LEVEL.
> ---
>  Makeconfig   |  4 ++++
>  csu/Makefile | 22 ++++++++++++----------
>  elf/Makefile |  4 ----
>  3 files changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/Makeconfig b/Makeconfig
> index 0a4811b5e5..c99464fdfa 100644
> --- a/Makeconfig
> +++ b/Makeconfig
> @@ -856,6 +856,10 @@ ifneq ($(stack-protector),)
>  +stack-protector=$(stack-protector)
>  endif
>  
> +define elide-stack-protector
> +$(if $(filter $(@F),$(patsubst %,%$(1),$(2))), $(no-stack-protector))
> +endef

Please add a comment to this definition, now that it is available
everywhere.  Okay with that change.

Thanks,
Florian