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

[PATCH v2 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

Changes from v1:

- Added David Hughes' patch
- Restrict disabling stack protector for get-cpuid-feature-leaf.c to the
  static object and consolidate the tst-ifunc-isa fix with it.
- Add a comment describing elide-stack-protector

David Hughes (1):
  Add inhibit_stack_protector to ifuncmain9 [BZ #25680]

Siddhesh Poyarekar (2):
  Build get-cpuid-feature-leaf.c without stack-protector [BZ #27555]
  Build libc-start with stack protector for SHARED

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

-- 
2.29.2

[PATCH 1/3] Add inhibit_stack_protector to ifuncmain9 [BZ #25680]

[Siddhesh Poyarekar]

From: David Hughes <davidhughes205@gmail.com>

Enabling --enable-stack-protector=all causes the following tests to fail:

    FAIL: elf/ifuncmain9picstatic
    FAIL: elf/ifuncmain9static

Nick Alcock (who committed the stack protector code) marked the IFUNC
resolvers with inhibit_stack_protector when he done the original work and
suggested doing so again @ BZ #25680. This patch adds
inhibit_stack_protector to ifuncmain9.

After patch is applied, --enable-stack-protector=all does not fail the
above tests.
---
 elf/ifuncmain9.c | 1 +
 1 file changed, 1 insertion(+)

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;
-- 
2.29.2

[PATCH 2/3] Build get-cpuid-feature-leaf.c without stack-protector [BZ #27555]

[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 stack-protector.

Additionally, IFUNC resolvers for static tst-ifunc-isa tests get
called too early for stack protector to be useful, so fix them to
disable stack protector for the resolver functions.

This fixes all failures seen with --enable-stack-protector=all
configuration.
---
 sysdeps/x86/Makefile        | 2 ++
 sysdeps/x86/tst-ifunc-isa.h | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/sysdeps/x86/Makefile b/sysdeps/x86/Makefile
index e1f9379fd8..8ccdef2534 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.o += $(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
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

[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   |  8 ++++++++
 csu/Makefile | 22 ++++++++++++----------
 elf/Makefile |  4 ----
 3 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/Makeconfig b/Makeconfig
index 0a4811b5e5..01f8638c2e 100644
--- a/Makeconfig
+++ b/Makeconfig
@@ -856,6 +856,14 @@ ifneq ($(stack-protector),)
 +stack-protector=$(stack-protector)
 endif
 
+# Some routines are unsafe to build with stack-protection since they're called
+# before the stack check guard is set up.  Provide a way to disable stack
+# protector.  The first argument is the extension (.o, .os, .oS) and the second
+# is a list of routines that this path should be applied to.
+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 1/3] Add inhibit_stack_protector to ifuncmain9 [BZ #25680]

[Adhemerval Zanella]

On 10/03/2021 07:13, Siddhesh Poyarekar via Libc-alpha wrote:
> From: David Hughes <davidhughes205@gmail.com>
> 
> Enabling --enable-stack-protector=all causes the following tests to fail:
> 
>     FAIL: elf/ifuncmain9picstatic
>     FAIL: elf/ifuncmain9static
> 
> Nick Alcock (who committed the stack protector code) marked the IFUNC
> resolvers with inhibit_stack_protector when he done the original work and
> suggested doing so again @ BZ #25680. This patch adds
> inhibit_stack_protector to ifuncmain9.
> 
> After patch is applied, --enable-stack-protector=all does not fail the
> above tests.

The BZ#25680 report makes me wonder if would be better to just disable
--enable-stack-protector=all on architecture with IFUNC for now.
This fix the issue on glibc testsuite, but it might still trigger
by users if this same trick is not used (as noted by Sergei).

Florian stated on comment #2 that “all” is very unlikely to add 
additional protection and this basically adds *another* undocumented 
ifunc restriction. And it seems likely that distributions like Gentoo 
will just use 'strong' instead of 'all'.

So, what would be the implications of limiting stack protection to
'strong' instead of 'all' for ABIs with ifunc? Is this issue only
redistricted to some ABIs (the reported indicates that only x86_64
is affected)?

> ---
>  elf/ifuncmain9.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> 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;
> 

Re: [PATCH 1/3] Add inhibit_stack_protector to ifuncmain9 [BZ #25680]

[Siddhesh Poyarekar]

On 3/10/21 6:20 PM, Adhemerval Zanella via Libc-alpha wrote:
> 
> 
> On 10/03/2021 07:13, Siddhesh Poyarekar via Libc-alpha wrote:
>> From: David Hughes <davidhughes205@gmail.com>
>>
>> Enabling --enable-stack-protector=all causes the following tests to fail:
>>
>>      FAIL: elf/ifuncmain9picstatic
>>      FAIL: elf/ifuncmain9static
>>
>> Nick Alcock (who committed the stack protector code) marked the IFUNC
>> resolvers with inhibit_stack_protector when he done the original work and
>> suggested doing so again @ BZ #25680. This patch adds
>> inhibit_stack_protector to ifuncmain9.
>>
>> After patch is applied, --enable-stack-protector=all does not fail the
>> above tests.
> 
> The BZ#25680 report makes me wonder if would be better to just disable
> --enable-stack-protector=all on architecture with IFUNC for now.
> This fix the issue on glibc testsuite, but it might still trigger
> by users if this same trick is not used (as noted by Sergei).

The general problem is that it is at best pointless to add stack 
protector prologue/epilogue to ifunc resolver functions in static 
binaries.  On x86_64 it is visible because it results in a crash due to 
TLS segment register %fs not being set up but elsewhere, the stack chk 
guard is zero, which makes the dereference safe, but useless.  On 
ppc64le, the TLS setup happens first, thus avoiding the crash and 
behaving like the other non-TLS stack_chk_guard architectures.

One way to make it not crash on x86_64 could be to delay ifunc 
resolution (which would then mean making sure that TLS doesn't use 
ifunc'd functions, e.g. memcpy) and bring it on par with the rest of the 
architectures.

I'm also considering proposing that gcc skips over ifunc resolvers for 
stack protection, at least in the static case.  The dynamic case happens 
to work somehow, I am yet to conclude that it's by design.

> Florian stated on comment #2 that “all” is very unlikely to add
> additional protection and this basically adds *another* undocumented
> ifunc restriction. And it seems likely that distributions like Gentoo
> will just use 'strong' instead of 'all'.

AFAICT Gentoo decided to use 'strong' instead of 'all' because of 
Florian's comment.

> So, what would be the implications of limiting stack protection to
> 'strong' instead of 'all' for ABIs with ifunc? Is this issue only
> redistricted to some ABIs (the reported indicates that only x86_64
> is affected)?

It's not the glibc build that's affected though, at least not to an 
extent that we ought to care.  What's broken is our support with 
binaries that were built with stack-protector-all *and* ifuncs.  This 
patch implies that we don't support it; all we need to do is document 
this somewhere and perhaps fix gcc to never emit the prologue/epilogue 
for ifunc resolvers.

Siddhesh

Re: [PATCH 1/3] Add inhibit_stack_protector to ifuncmain9 [BZ #25680]

[Adhemerval Zanella]

On 12/03/2021 05:52, Siddhesh Poyarekar wrote:
> On 3/10/21 6:20 PM, Adhemerval Zanella via Libc-alpha wrote:
>>
>>
>> On 10/03/2021 07:13, Siddhesh Poyarekar via Libc-alpha wrote:
>>> From: David Hughes <davidhughes205@gmail.com>
>>>
>>> Enabling --enable-stack-protector=all causes the following tests to fail:
>>>
>>>      FAIL: elf/ifuncmain9picstatic
>>>      FAIL: elf/ifuncmain9static
>>>
>>> Nick Alcock (who committed the stack protector code) marked the IFUNC
>>> resolvers with inhibit_stack_protector when he done the original work and
>>> suggested doing so again @ BZ #25680. This patch adds
>>> inhibit_stack_protector to ifuncmain9.
>>>
>>> After patch is applied, --enable-stack-protector=all does not fail the
>>> above tests.
>>
>> The BZ#25680 report makes me wonder if would be better to just disable
>> --enable-stack-protector=all on architecture with IFUNC for now.
>> This fix the issue on glibc testsuite, but it might still trigger
>> by users if this same trick is not used (as noted by Sergei).
> 
> The general problem is that it is at best pointless to add stack protector prologue/epilogue to ifunc resolver functions in static binaries.  On x86_64 it is visible because it results in a crash due to TLS segment register %fs not being set up but elsewhere, the stack chk guard is zero, which makes the dereference safe, but useless.  On ppc64le, the TLS setup happens first, thus avoiding the crash and behaving like the other non-TLS stack_chk_guard architectures.
> 
> One way to make it not crash on x86_64 could be to delay ifunc resolution (which would then mean making sure that TLS doesn't use ifunc'd functions, e.g. memcpy) and bring it on par with the rest of the architectures.

Is it something preventing us to make x86 follows other architectures in this
regard? 

> 
> I'm also considering proposing that gcc skips over ifunc resolvers for stack protection, at least in the static case.  The dynamic case happens to work somehow, I am yet to conclude that it's by design.

Not sure if adding exceptions for specific cases is the best approach,
the users would expect that such mitigations to work regardless of
compilation/linking model (although some do require extra linker/runtime
support).  My take is if we could fix on glibc we should aim for it.

> 
>> Florian stated on comment #2 that “all” is very unlikely to add
>> additional protection and this basically adds *another* undocumented
>> ifunc restriction. And it seems likely that distributions like Gentoo
>> will just use 'strong' instead of 'all'.
> 
> AFAICT Gentoo decided to use 'strong' instead of 'all' because of Florian's comment.
> 
>> So, what would be the implications of limiting stack protection to
>> 'strong' instead of 'all' for ABIs with ifunc? Is this issue only
>> redistricted to some ABIs (the reported indicates that only x86_64
>> is affected)?
> 
> It's not the glibc build that's affected though, at least not to an extent that we ought to care.  What's broken is our support with binaries that were built with stack-protector-all *and* ifuncs.  This patch implies that we don't support it; all we need to do is document this somewhere and perhaps fix gcc to never emit the prologue/epilogue for ifunc resolvers.

Yeah, I understood that and my concern is if users do start to use
ifunc more profusely they will need to keep adding the 
inhibit_stack_protector on resolvers to work around this issue.

Fixing the ifunc resolution order also might simplify the code
and avoid adding the inhibit_stack_protector on multiples files.

Re: [PATCH 1/3] Add inhibit_stack_protector to ifuncmain9 [BZ #25680]

[H.J. Lu]

On Fri, Mar 12, 2021 at 10:29 AM Adhemerval Zanella via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
>
>
> On 12/03/2021 05:52, Siddhesh Poyarekar wrote:
> > On 3/10/21 6:20 PM, Adhemerval Zanella via Libc-alpha wrote:
> >>
> >>
> >> On 10/03/2021 07:13, Siddhesh Poyarekar via Libc-alpha wrote:
> >>> From: David Hughes <davidhughes205@gmail.com>
> >>>
> >>> Enabling --enable-stack-protector=all causes the following tests to fail:
> >>>
> >>>      FAIL: elf/ifuncmain9picstatic
> >>>      FAIL: elf/ifuncmain9static
> >>>
> >>> Nick Alcock (who committed the stack protector code) marked the IFUNC
> >>> resolvers with inhibit_stack_protector when he done the original work and
> >>> suggested doing so again @ BZ #25680. This patch adds
> >>> inhibit_stack_protector to ifuncmain9.
> >>>
> >>> After patch is applied, --enable-stack-protector=all does not fail the
> >>> above tests.
> >>
> >> The BZ#25680 report makes me wonder if would be better to just disable
> >> --enable-stack-protector=all on architecture with IFUNC for now.
> >> This fix the issue on glibc testsuite, but it might still trigger
> >> by users if this same trick is not used (as noted by Sergei).
> >
> > The general problem is that it is at best pointless to add stack protector prologue/epilogue to ifunc resolver functions in static binaries.  On x86_64 it is visible because it results in a crash due to TLS segment register %fs not being set up but elsewhere, the stack chk guard is zero, which makes the dereference safe, but useless.  On ppc64le, the TLS setup happens first, thus avoiding the crash and behaving like the other non-TLS stack_chk_guard architectures.
> >
> > One way to make it not crash on x86_64 could be to delay ifunc resolution (which would then mean making sure that TLS doesn't use ifunc'd functions, e.g. memcpy) and bring it on par with the rest of the architectures.
>
> Is it something preventing us to make x86 follows other architectures in this
> regard?
>

On x86-64, IFUNC was disabled in ld.so before.   Now IFUNC is selectly
enabled in ld.so.
If you want to disable one specific IFUNC in ld.so on x86-64, please
open a glibc bug and
CC me.

-- 
H.J.

Re: [PATCH 1/3] Add inhibit_stack_protector to ifuncmain9 [BZ #25680]

[Siddhesh Poyarekar]

On 3/13/21 1:25 AM, H.J. Lu via Libc-alpha wrote:
> On x86-64, IFUNC was disabled in ld.so before.   Now IFUNC is selectly
> enabled in ld.so.
> If you want to disable one specific IFUNC in ld.so on x86-64, please
> open a glibc bug and
> CC me.

Not for ld.so, this is for static binary early startup, e.g. in 
__libc_init_tls.

Siddhesh

Re: [PATCH 1/3] Add inhibit_stack_protector to ifuncmain9 [BZ #25680]

[Siddhesh Poyarekar]

On 3/12/21 11:04 PM, Adhemerval Zanella via Libc-alpha wrote:
> Is it something preventing us to make x86 follows other architectures in this
> regard?

AFAICT, just more work to get it done :)  There's also the general 
hairiness of the whole sequence and historical issues with ifunc 
interplay that doesn't add much confidence to any fix that may come in 
this place.  But that shouldn't prevent us from trying I guess.

> Not sure if adding exceptions for specific cases is the best approach,
> the users would expect that such mitigations to work regardless of
> compilation/linking model (although some do require extra linker/runtime
> support).  My take is if we could fix on glibc we should aim for it.

I'm kinda on the fence, but will likely hop on to your side if I figure 
out a fix.

>> It's not the glibc build that's affected though, at least not to an extent that we ought to care.  What's broken is our support with binaries that were built with stack-protector-all *and* ifuncs.  This patch implies that we don't support it; all we need to do is document this somewhere and perhaps fix gcc to never emit the prologue/epilogue for ifunc resolvers.
> 
> Yeah, I understood that and my concern is if users do start to use
> ifunc more profusely they will need to keep adding the
> inhibit_stack_protector on resolvers to work around this issue.
> 
> Fixing the ifunc resolution order also might simplify the code
> and avoid adding the inhibit_stack_protector on multiples files.

This is probably more work than I had bargained for, so I'm going to put 
it back in the interest of looking at the more urgent problems (e.g. the 
tunables breakage).  I'll pick it up again later.  In the meantime, 
would you object to me pushing 2/3 and 3/3?  2/3 disables stack 
protector on an early startup routine (I'll remove the 
inhibit_stack_protector in the test so that we can fix it alongside 1/3) 
and 3/3 is the SHARED cleanup that I had posted when you noticed these 
test failures.  The cleanup doesn't cause these failures and seems safe.

Siddhesh

Re: [PATCH 1/3] Add inhibit_stack_protector to ifuncmain9 [BZ #25680]

[H.J. Lu]

On Fri, Mar 12, 2021 at 10:01 PM Siddhesh Poyarekar
<siddhesh@sourceware.org> wrote:
>
> On 3/13/21 1:25 AM, H.J. Lu via Libc-alpha wrote:
> > On x86-64, IFUNC was disabled in ld.so before.   Now IFUNC is selectly
> > enabled in ld.so.
> > If you want to disable one specific IFUNC in ld.so on x86-64, please
> > open a glibc bug and
> > CC me.
>
> Not for ld.so, this is for static binary early startup, e.g. in
> __libc_init_tls.

We have startup.h to handle special requirements during startup in static
libc.

-- 
H.J.

Re: [PATCH 1/3] Add inhibit_stack_protector to ifuncmain9 [BZ #25680]

[Adhemerval Zanella]

On 13/03/2021 03:07, Siddhesh Poyarekar wrote:
> On 3/12/21 11:04 PM, Adhemerval Zanella via Libc-alpha wrote:
>> Is it something preventing us to make x86 follows other architectures in this
>> regard?
> 
> AFAICT, just more work to get it done :)  There's also the general hairiness of the whole sequence and historical issues with ifunc interplay that doesn't add much confidence to any fix that may come in this place.  But that shouldn't prevent us from trying I guess.
> 
>> Not sure if adding exceptions for specific cases is the best approach,
>> the users would expect that such mitigations to work regardless of
>> compilation/linking model (although some do require extra linker/runtime
>> support).  My take is if we could fix on glibc we should aim for it.
> 
> I'm kinda on the fence, but will likely hop on to your side if I figure out a fix.
> 
>>> It's not the glibc build that's affected though, at least not to an extent that we ought to care.  What's broken is our support with binaries that were built with stack-protector-all *and* ifuncs.  This patch implies that we don't support it; all we need to do is document this somewhere and perhaps fix gcc to never emit the prologue/epilogue for ifunc resolvers.
>>
>> Yeah, I understood that and my concern is if users do start to use
>> ifunc more profusely they will need to keep adding the
>> inhibit_stack_protector on resolvers to work around this issue.
>>
>> Fixing the ifunc resolution order also might simplify the code
>> and avoid adding the inhibit_stack_protector on multiples files.
> 
> This is probably more work than I had bargained for, so I'm going to put it back in the interest of looking at the more urgent problems (e.g. the tunables breakage).  I'll pick it up again later.  In the meantime, would you object to me pushing 2/3 and 3/3?  2/3 disables stack protector on an early startup routine (I'll remove the inhibit_stack_protector in the test so that we can fix it alongside 1/3) and 3/3 is the SHARED cleanup that I had posted when you noticed these test failures.  The cleanup doesn't cause these failures and seems safe.

Ok, they patches itself do look and I don't really want to block them.
My idea of raising this issues is that besides glibc testsuite we should
also focus on the consumers and it maybe it would be better to partially
disable a functionally it is inherent broken in some corner cases. 

But if the idea is indeed to get back on this, I am ok with current
approach.

Re: [PATCH 1/3] Add inhibit_stack_protector to ifuncmain9 [BZ #25680]

[Adhemerval Zanella]

On 10/03/2021 07:13, Siddhesh Poyarekar via Libc-alpha wrote:
> From: David Hughes <davidhughes205@gmail.com>
> 
> Enabling --enable-stack-protector=all causes the following tests to fail:
> 
>     FAIL: elf/ifuncmain9picstatic
>     FAIL: elf/ifuncmain9static
> 
> Nick Alcock (who committed the stack protector code) marked the IFUNC
> resolvers with inhibit_stack_protector when he done the original work and
> suggested doing so again @ BZ #25680. This patch adds
> inhibit_stack_protector to ifuncmain9.
> 
> After patch is applied, --enable-stack-protector=all does not fail the
> above tests.

LGTM, based on previous discussions.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  elf/ifuncmain9.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> 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;
> 

Re: [PATCH 2/3] Build get-cpuid-feature-leaf.c without stack-protector [BZ #27555]

[Adhemerval Zanella]

On 10/03/2021 07:13, Siddhesh Poyarekar via Libc-alpha wrote:
> __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 stack-protector.
> 
> Additionally, IFUNC resolvers for static tst-ifunc-isa tests get
> called too early for stack protector to be useful, so fix them to
> disable stack protector for the resolver functions.
> 
> This fixes all failures seen with --enable-stack-protector=all
> configuration.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  sysdeps/x86/Makefile        | 2 ++
>  sysdeps/x86/tst-ifunc-isa.h | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/sysdeps/x86/Makefile b/sysdeps/x86/Makefile
> index e1f9379fd8..8ccdef2534 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.o += $(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
> 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 ())
> 

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

[Adhemerval Zanella]

On 10/03/2021 07:14, Siddhesh Poyarekar via Libc-alpha wrote:
> 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.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  Makeconfig   |  8 ++++++++
>  csu/Makefile | 22 ++++++++++++----------
>  elf/Makefile |  4 ----
>  3 files changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/Makeconfig b/Makeconfig
> index 0a4811b5e5..01f8638c2e 100644
> --- a/Makeconfig
> +++ b/Makeconfig
> @@ -856,6 +856,14 @@ ifneq ($(stack-protector),)
>  +stack-protector=$(stack-protector)
>  endif
>  
> +# Some routines are unsafe to build with stack-protection since they're called
> +# before the stack check guard is set up.  Provide a way to disable stack
> +# protector.  The first argument is the extension (.o, .os, .oS) and the second
> +# is a list of routines that this path should be applied to.
> +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.


Ok.

> 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)

Ok.

> +#
> +# 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

Ok.

> 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))
> 

Ok.

Re: [PATCH 1/3] Add inhibit_stack_protector to ifuncmain9 [BZ #25680]

[Siddhesh Poyarekar]

On 3/15/21 7:28 PM, Adhemerval Zanella wrote:
> Ok, they patches itself do look and I don't really want to block them.
> My idea of raising this issues is that besides glibc testsuite we should
> also focus on the consumers and it maybe it would be better to partially
> disable a functionally it is inherent broken in some corner cases.
> 
> But if the idea is indeed to get back on this, I am ok with current
> approach.


I've filed #27582 so that I don't forget.

Thanks,
Siddhesh