Build libc-start with stack protector for SHARED

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 | 30 ++++++++++++++++--------------
 elf/Makefile |  4 ----
 3 files changed, 20 insertions(+), 18 deletions(-)

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

Would it be cleaner to split the shared and static __libc_start_main
implementations into different files?  There isn't that much overlap
between the two.

Thanks,
Florian

On 2/3/21 3:02 PM, Florian Weimer via Libc-alpha wrote:
> Would it be cleaner to split the shared and static __libc_start_main
> implementations into different files?  There isn't that much overlap
> between the two.

The makefile twiddling might become a bit more tedious because it won't 
be just a case of .o vs .os.  However it's tempting because it'll 
simplify the macro soup in there quite significantly.  I'll give it a 
shot, but can I do it independently of this change?

Siddhesh

* Siddhesh Poyarekar:

> On 2/3/21 3:02 PM, Florian Weimer via Libc-alpha wrote:
>> Would it be cleaner to split the shared and static __libc_start_main
>> implementations into different files?  There isn't that much overlap
>> between the two.
>
> The makefile twiddling might become a bit more tedious because it
> won't be just a case of .o vs .os.  However it's tempting because
> it'll simplify the macro soup in there quite significantly.  I'll give
> it a shot, but can I do it independently of this change?

Sure, I was just wondering aloud, sorry.

Thanks,
Florian

On 2/3/21 7:18 PM, Florian Weimer via Libc-alpha wrote:
> * Siddhesh Poyarekar:
> 
>> On 2/3/21 3:02 PM, Florian Weimer via Libc-alpha wrote:
>>> Would it be cleaner to split the shared and static __libc_start_main
>>> implementations into different files?  There isn't that much overlap
>>> between the two.
>>
>> The makefile twiddling might become a bit more tedious because it
>> won't be just a case of .o vs .os.  However it's tempting because
>> it'll simplify the macro soup in there quite significantly.  I'll give
>> it a shot, but can I do it independently of this change?
> 
> Sure, I was just wondering aloud, sorry.

That's OK :) I assume you're fine with this patch then?

Siddhesh

* Siddhesh Poyarekar:

> On 2/3/21 7:18 PM, Florian Weimer via Libc-alpha wrote:
>> * Siddhesh Poyarekar:
>> 
>>> On 2/3/21 3:02 PM, Florian Weimer via Libc-alpha wrote:
>>>> Would it be cleaner to split the shared and static __libc_start_main
>>>> implementations into different files?  There isn't that much overlap
>>>> between the two.
>>>
>>> The makefile twiddling might become a bit more tedious because it
>>> won't be just a case of .o vs .os.  However it's tempting because
>>> it'll simplify the macro soup in there quite significantly.  I'll give
>>> it a shot, but can I do it independently of this change?
>> Sure, I was just wondering aloud, sorry.
>
> That's OK :) I assume you're fine with this patch then?

Sorry, I haven't really looked at it.

Florian

On 3 Feb 2021, Siddhesh Poyarekar via Libc-alpha told this:

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

Is this true when -fstack-protector-all is actiev as well? That's the
troublesome case that led to my introducing most of these
$(no-stack-protector)s (and in general I only introduced them where I
saw real problems).

There *are* (obviously) changes in the function prologue too when
-fstack-protector is in use, and if there's anywhere those changes might
really matter it's somewhere like libc-start. Are those changes safe?
They weren't safe in the past...

> This also cleans up the cosmetic inconsistency in the stack protector
> flags in csu, especially the incorrect value of STACK_PROTECTOR_LEVEL.

Nice cleanup!

On 2/8/21 7:12 PM, Nix wrote:
> Is this true when -fstack-protector-all is actiev as well? That's the
> troublesome case that led to my introducing most of these
> $(no-stack-protector)s (and in general I only introduced them where I
> saw real problems).
> 
> There *are* (obviously) changes in the function prologue too when
> -fstack-protector is in use, and if there's anywhere those changes might
> really matter it's somewhere like libc-start. Are those changes safe?
> They weren't safe in the past...

The static libc-start is in fact unsafe with fstack-protector, which is 
why I enabled stack-protector only for libc-start.os and not 
libc-start.o.  In the dynamic case, the TLS initialization and stack 
canary setup ought to have already happened and using stack and if we 
see failures (which I didn't but maybe I missed the configurations 
you've tested?) then it's probably an indication of something else 
that's wrong.

Also, I checked codegen for libc-start.os with libc-start.os with 
stack-protector-strong as well as -all and found no differences compared 
to not having stack-protector enabled, which seems logical from the 
compiler standpoint since the function technically does not return.

Siddhesh

On 8 Feb 2021, Siddhesh Poyarekar outgrape:

> On 2/8/21 7:12 PM, Nix wrote:
>> Is this true when -fstack-protector-all is actiev as well? That's the
>> troublesome case that led to my introducing most of these
>> $(no-stack-protector)s (and in general I only introduced them where I
>> saw real problems).
>>
>> There *are* (obviously) changes in the function prologue too when
>> -fstack-protector is in use, and if there's anywhere those changes might
>> really matter it's somewhere like libc-start. Are those changes safe?
>> They weren't safe in the past...
>
> The static libc-start is in fact unsafe with fstack-protector, which
> is why I enabled stack-protector only for libc-start.os and not
> libc-start.o. In the dynamic case, the TLS initialization and stack
> canary setup ought to have already happened and using stack and if we
> see failures (which I didn't but maybe I missed the configurations
> you've tested?) then it's probably an indication of something else
> that's wrong.

I gave your patch a test locally (with my usual stack-protector-all test
platform, my 32-bit Geode firewall compilation container) and everythng
is working perfectly well and all the tests (that passed before) still
pass; so I'm worrying over nothing, it turns out, and this patch is
fine.

I suspect I was originally being too paranoid in extending no-stack-
protector from the static to the dynamic case for libc-start. So one
more function can get stack-protected, yeah!

> Also, I checked codegen for libc-start.os with libc-start.os with
> stack-protector-strong as well as -all and found no differences
> compared to not having stack-protector enabled, which seems logical
> from the compiler standpoint since the function technically does not
> return.

That's curious. I checked it and found a *lot* of differences, though
mostly in debuginfo/CFI and harmless register allocation changes. (Maybe
this is platform-specific? This was with an x86-32 build... in any case,
things clearly still work!)

On 2/8/21 9:28 PM, Nix wrote:
> I gave your patch a test locally (with my usual stack-protector-all test
> platform, my 32-bit Geode firewall compilation container) and everythng
> is working perfectly well and all the tests (that passed before) still
> pass; so I'm worrying over nothing, it turns out, and this patch is
> fine.
> 
> I suspect I was originally being too paranoid in extending no-stack-
> protector from the static to the dynamic case for libc-start. So one
> more function can get stack-protected, yeah!

Perfect, thanks for testing!

>> Also, I checked codegen for libc-start.os with libc-start.os with
>> stack-protector-strong as well as -all and found no differences
>> compared to not having stack-protector enabled, which seems logical
>> from the compiler standpoint since the function technically does not
>> return.
> 
> That's curious. I checked it and found a *lot* of differences, though
> mostly in debuginfo/CFI and harmless register allocation changes. (Maybe
> this is platform-specific? This was with an x86-32 build... in any case,
> things clearly still work!)

Interesting, I would love to know your configuration, including what 
compiler+binutils you're using.  I'm thinking of massaging the 
stack-protector bits a little more and your configuration could help me 
with better testing coverage.

Thanks,
Siddhesh

On 8 Feb 2021, Siddhesh Poyarekar told this:

> On 2/8/21 9:28 PM, Nix wrote:
>>> Also, I checked codegen for libc-start.os with libc-start.os with
>>> stack-protector-strong as well as -all and found no differences
>>> compared to not having stack-protector enabled, which seems logical
>>> from the compiler standpoint since the function technically does not
>>> return.
>>
>> That's curious. I checked it and found a *lot* of differences, though
>> mostly in debuginfo/CFI and harmless register allocation changes. (Maybe
>> this is platform-specific? This was with an x86-32 build... in any case,
>> things clearly still work!)
>
> Interesting, I would love to know your configuration, including what
> compiler+binutils you're using. I'm thinking of massaging the
> stack-protector bits a little more and your configuration could help
> me with better testing coverage.

Hah. I was hoping you wouldn't ask :P My config for this is... weird:
scripted autobuilders mean you can build up fairly complex
configuration, and I've definitely done that with the toolchain here!

The machine the stack-protector-all patches were written for is a Geode
LX firewall (an aging Soekris net5501), with builds run on a much faster
x86_64 box with a biarch GCC 10: the installation root for all this
stuff is rsynced to the real machine nightly, and manually rsynced for
things like glibc builds where I want to check them out by hand right
away. The compiler is hacked similarly to various distros' hardened
configurations, adding a 32-bit -march=geode multilib that jams in
different default specs, to wit extra C/C++ compiler flags of
"-fstack-clash-protection -fstack-protector-all", preprocessor flags of
-D_FORTIFY_SOURCE=2, and link flags of "-z relro -z now". This works
fine for almost all programs, but glibc wants more control over the
CFLAGS, so I reverse this hackery for glibc builds via -specs, where the
specs are dumped directly from an unmodified compiler, leaving only the
-m32 and the -march=geode:

export CPPFLAGS="-m32 -march=geode -specs=$SHAI_BUILD_CONFIG_PATH/scripts/specs.fold"
export CFLAGS="-O2 -m32 -march=geode -pipe -specs=$SHAI_BUILD_CONFIG_PATH/scripts/specs.fold -fomit-frame-pointer"
export ASFLAGS="-O2 -m32 -march=geode -pipe -specs=$SHAI_BUILD_CONFIG_PATH/scripts/specs.fold"
export ALL_LDFLAGS="-m32 -march=geode -specs=$SHAI_BUILD_CONFIG_PATH/scripts/specs.fold"

(glibc itself has a few light makefile modifications to make sure the
ALL_LDFLAGS, unlike the LDFLAGS, are passed to literally everything at
link time, so that the -specs are never accidentally omitted and the
stack protector state is exactly what glibc asks it to be).

(well, the -m32 is not actually there -- on all my machines,
/usr/bin/gcc and the other compiler drivers are replaced with a tiny
wrapper executable, statically linked to musl, which erases -m32 and
-m64 from the arguments and adds them back before invoking the real
driver, depending on whether the active personality is linux32 or
linux64. This means I can be sure that I never get a part-32/part-64
build, since personalities are inherited by subprocesses.)

But, honestly, most of the local modifications above, other than the
compiler driver wrapper, are doing something and then going to great
lengths to undo it again: a perfectly good test with an unmodified GCC
10.2 and binutils 2.35.x (I'm using a branch current as of 20201023) is
just to compile with "-O2 -m32 -march=geode" in the CFLAGS. My usual
configure line is perhaps a bit longer than is sane:

--build=i586-pc-linux-gnu --prefix=/usr --enable-shared --enable-bind-now \
  --enable-maintainer-mode --enable-kernel=5.7.0 --enable-check-abi=warn \
  --disable-werror --enable-hardcoded-path-in-tests \
  --with-nonshared-cflags=-Wp,-D_FORTIFY_SOURCE=2 \
  --enable-stackguard-randomization --enable-stack-protector=all \
  --enable-tunables=no --build=i586-pc-linux-gnu TIMEOUTFACTOR=5

(these have been accumulated over many years: several of them have since
become the default, and I'm honestly not sure why I put some of them in
any more. In particular the --with-nonshared-cflags is a bit of a
mystery, but probably just trying to get everything fortified!)

All the non-Geodes use a slightly saner config: a CONFIG_SITE that sets
the slibdir appropriately depending on the bitness of the build, and
compiler flags of

export CFLAGS="-O2 -march=native -pipe -fomit-frame-pointer"
export ASFLAGS="-O2 -march=native -pipe"

... and this configure line for the x86_64 hosts (both 32-bit and
64-bit), which have actual native compilers so none of this multilib
nonsense is necessary:

--prefix=/usr --enable-shared --enable-bind-now --enable-maintainer-mode \
  --enable-kernel=5.7.0 --enable-check-abi=warn --disable-werror \
  --enable-hardcoded-path-in-tests \
  --with-nonshared-cflags=-Wp,-D_FORTIFY_SOURCE=2 --enable-profile

On 2/3/21 11:12 AM, 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.

Any further comments on this patchset?  Is it OK to commit this?

Siddhesh

On 03/02/2021 02:42, 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.

I am seeing the following regressions with --enable-stack-protector=all
on x86_64 and i686 (I haven't seen is on powerpc64le-linux-gnu):

FAIL: elf/ifuncmain9picstatic
FAIL: elf/ifuncmain9static
FAIL: elf/tst-ifunc-isa-1-static
FAIL: elf/tst-ifunc-isa-2-static

$ gdb ./elf/ifuncmain9picstatic
[...]
(gdb) r
[...]
Program received signal SIGSEGV, Segmentation fault.
0x0000000000401944 in resolver () at ifuncmain9.c:47
47      {
(gdb) bt
#0  0x0000000000401944 in resolver () at ifuncmain9.c:47
#1  0x0000000000402e88 in elf_ifunc_invoke (addr=<optimized out>) at ../sysdeps/x86_64/dl-irel.h:44
#2  elf_irela (reloc=0x4004f0) at ../sysdeps/x86_64/dl-irel.h:44
#3  apply_irel () at ../csu/libc-start.c:86
#4  __libc_start_main (main=0x401690 <main>, argc=1, argv=0x7fffffffd808, init=0x403cd0 <__libc_csu_init>, fini=0x403d60 <__libc_csu_fini>, rtld_fini=0x0, stack_end=0x7fffffffd7f8) at ../csu/libc-start.c:206
#5  0x00000000004017ba in _start () at ../sysdeps/x86_64/start.S:120

(gdb) disas
Dump of assembler code for function resolver:
   0x0000000000401940 <+0>:     sub    $0x18,%rsp
=> 0x0000000000401944 <+4>:     mov    %fs:0x28,%rax
(gdb) i r fs
fs             0x0                 0

It seems we need to setup the thread pointer earlier.

> ---
>  Makeconfig   |  4 ++++
>  csu/Makefile | 30 ++++++++++++++++--------------
>  elf/Makefile |  4 ----
>  3 files changed, 20 insertions(+), 18 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.

Ok, it is from elf/Makefile.

> diff --git a/csu/Makefile b/csu/Makefile
> index c9385df2e9..b2e46ae1bb 100644
> --- a/csu/Makefile
> +++ b/csu/Makefile
> @@ -46,25 +46,27 @@ 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
>  

Ok, it seems to be safe for powerpc and aarch64 that reimplement it as
well.

> -# This file is not actually part of the startup code in the nonshared
> -# case and statically linked into applications.  See
> +# elf-init.oS is not actually part of the startup code in the nonshared case
> +# and statically linked into applications.  See
>  # <https://sourceware.org/bugzilla/show_bug.cgi?id=23323>,
>  # <https://sourceware.org/ml/libc-alpha/2018-06/msg00717.html>.
> -# Also see the note above regarding STACK_PROTECTOR_LEVEL.
> -CFLAGS-elf-init.oS += $(stack-protector)
> +ssp-safe.oS = elf-init
> +
> +CFLAGS-.o += $(call elide-stack-protector,.o,$(routines))
> +CFLAGS-.op += $(call elide-stack-protector,.op,$(routines))
> +CFLAGS-.os += $(call elide-stack-protector,.os,$(filter-out \
> +						 $(ssp-safe.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

Based on the regression, I think this is not fully correct.

> diff --git a/elf/Makefile b/elf/Makefile
> index 16c89b6d07..1c3045376e 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))
>

On 2/12/21 12:55 AM, Adhemerval Zanella wrote:
> 
> 
> On 03/02/2021 02:42, 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.
> 
> I am seeing the following regressions with --enable-stack-protector=all
> on x86_64 and i686 (I haven't seen is on powerpc64le-linux-gnu):
> 
> FAIL: elf/ifuncmain9picstatic
> FAIL: elf/ifuncmain9static
> FAIL: elf/tst-ifunc-isa-1-static
> FAIL: elf/tst-ifunc-isa-2-static
> 
> $ gdb ./elf/ifuncmain9picstatic
> [...]
> (gdb) r
> [...]
> Program received signal SIGSEGV, Segmentation fault.
> 0x0000000000401944 in resolver () at ifuncmain9.c:47
> 47      {
> (gdb) bt
> #0  0x0000000000401944 in resolver () at ifuncmain9.c:47
> #1  0x0000000000402e88 in elf_ifunc_invoke (addr=<optimized out>) at ../sysdeps/x86_64/dl-irel.h:44
> #2  elf_irela (reloc=0x4004f0) at ../sysdeps/x86_64/dl-irel.h:44
> #3  apply_irel () at ../csu/libc-start.c:86
> #4  __libc_start_main (main=0x401690 <main>, argc=1, argv=0x7fffffffd808, init=0x403cd0 <__libc_csu_init>, fini=0x403d60 <__libc_csu_fini>, rtld_fini=0x0, stack_end=0x7fffffffd7f8) at ../csu/libc-start.c:206
> #5  0x00000000004017ba in _start () at ../sysdeps/x86_64/start.S:120
> 
> (gdb) disas
> Dump of assembler code for function resolver:
>     0x0000000000401940 <+0>:     sub    $0x18,%rsp
> => 0x0000000000401944 <+4>:     mov    %fs:0x28,%rax
> (gdb) i r fs
> fs             0x0                 0
> 
> It seems we need to setup the thread pointer earlier.
> 

I wasn't able to reproduce this until I repeated the build from scratch. 
  Sorry, I'll take a look at this again.  Thanks for checking.

Siddhesh

On 2/12/21 6:26 AM, Siddhesh Poyarekar via Libc-alpha wrote:
> On 2/12/21 12:55 AM, Adhemerval Zanella wrote:
>>
>>
>> On 03/02/2021 02:42, 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.
>>
>> I am seeing the following regressions with --enable-stack-protector=all
>> on x86_64 and i686 (I haven't seen is on powerpc64le-linux-gnu):
>>
>> FAIL: elf/ifuncmain9picstatic
>> FAIL: elf/ifuncmain9static
>> FAIL: elf/tst-ifunc-isa-1-static
>> FAIL: elf/tst-ifunc-isa-2-static
>>
>> $ gdb ./elf/ifuncmain9picstatic
>> [...]
>> (gdb) r
>> [...]
>> Program received signal SIGSEGV, Segmentation fault.
>> 0x0000000000401944 in resolver () at ifuncmain9.c:47
>> 47      {
>> (gdb) bt
>> #0  0x0000000000401944 in resolver () at ifuncmain9.c:47
>> #1  0x0000000000402e88 in elf_ifunc_invoke (addr=<optimized out>) at 
>> ../sysdeps/x86_64/dl-irel.h:44
>> #2  elf_irela (reloc=0x4004f0) at ../sysdeps/x86_64/dl-irel.h:44
>> #3  apply_irel () at ../csu/libc-start.c:86
>> #4  __libc_start_main (main=0x401690 <main>, argc=1, 
>> argv=0x7fffffffd808, init=0x403cd0 <__libc_csu_init>, fini=0x403d60 
>> <__libc_csu_fini>, rtld_fini=0x0, stack_end=0x7fffffffd7f8) at 
>> ../csu/libc-start.c:206
>> #5  0x00000000004017ba in _start () at ../sysdeps/x86_64/start.S:120
>>
>> (gdb) disas
>> Dump of assembler code for function resolver:
>>     0x0000000000401940 <+0>:     sub    $0x18,%rsp
>> => 0x0000000000401944 <+4>:     mov    %fs:0x28,%rax
>> (gdb) i r fs
>> fs             0x0                 0
>>
>> It seems we need to setup the thread pointer earlier.
>>
> 
> I wasn't able to reproduce this until I repeated the build from scratch. 
>   Sorry, I'll take a look at this again.  Thanks for checking.

... and now I see these failures all the way back to 2.33 tag and 2 out 
of the 4 failures all the way back to 2.32.  memusagestat (of all 
things!) prevented my build from completing on Fedora 33 because it 
needs 2.32 symbols that it couldn't find during the build.

I'll work on detangling this and figuring out the root cause soon.

Siddhesh