diff mbox series

Unconditionally define __stack_chk_guard [BZ #26817]

Message ID 20210111031210.4020603-1-maskray@google.com
State New
Headers show
Series Unconditionally define __stack_chk_guard [BZ #26817] | expand

Commit Message

Fāng-ruì Sòng Jan. 11, 2021, 3:12 a.m. UTC
This makes -mstack-protector-guard=global work even if
-mstack-protector-guard=tls is the default.
---
 csu/libc-start.c | 8 ++------
 elf/rtld.c       | 8 ++------
 2 files changed, 4 insertions(+), 12 deletions(-)

Comments

Florian Weimer Jan. 11, 2021, 9:39 a.m. UTC | #1
* Fangrui Song via Libc-alpha:

> This makes -mstack-protector-guard=global work even if
> -mstack-protector-guard=tls is the default.

It's unclear how you intend this to work.  Is it just for building
glibc?  Or also for user code?

In the latter case, we either need to provide a way to initialize a
hidden __stack_chk_guard symbol (probably with new relocations), or
export __stack_chk_guard from glibc, under a GLIBC_2.33 symbol version
for architectures that were missing it before.

Thanks,
Florian
Fāng-ruì Sòng Jan. 11, 2021, 5:40 p.m. UTC | #2
On Mon, Jan 11, 2021 at 1:39 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Fangrui Song via Libc-alpha:
>
> > This makes -mstack-protector-guard=global work even if
> > -mstack-protector-guard=tls is the default.
>
> It's unclear how you intend this to work.  Is it just for building
> glibc?  Or also for user code?

It is for user applications.

gcc -fstack-protector -mstack-protector-guard=global a.c
or
clang -fstack-protector -mstack-protector-guard=global a.c (clang 12)

Currently there is an undefined symbol error. This patch fixes the problem.

> In the latter case, we either need to provide a way to initialize a
> hidden __stack_chk_guard symbol (probably with new relocations), or
> export __stack_chk_guard from glibc, under a GLIBC_2.33 symbol version
> for architectures that were missing it before.

In elf/Versions, __stack_chk_guard is already exported as
__stack_chk_guard@@GLIBC_2.4


> Thanks,
> Florian
> --
> Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
> Commercial register: Amtsgericht Muenchen, HRB 153243,
> Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill
>
Florian Weimer Jan. 11, 2021, 5:49 p.m. UTC | #3
* Fāng-ruì Sòng:

> On Mon, Jan 11, 2021 at 1:39 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * Fangrui Song via Libc-alpha:
>>
>> > This makes -mstack-protector-guard=global work even if
>> > -mstack-protector-guard=tls is the default.
>>
>> It's unclear how you intend this to work.  Is it just for building
>> glibc?  Or also for user code?
>
> It is for user applications.
>
> gcc -fstack-protector -mstack-protector-guard=global a.c
> or
> clang -fstack-protector -mstack-protector-guard=global a.c (clang 12)
>
> Currently there is an undefined symbol error. This patch fixes the problem.

And we really want to use a global data symbol for this?  Nothing else
works at the moment, but it seems rather wasteful.

>> In the latter case, we either need to provide a way to initialize a
>> hidden __stack_chk_guard symbol (probably with new relocations), or
>> export __stack_chk_guard from glibc, under a GLIBC_2.33 symbol version
>> for architectures that were missing it before.
>
> In elf/Versions, __stack_chk_guard is already exported as
> __stack_chk_guard@@GLIBC_2.4

GLIBC_2.4 is done and dusted, we can't add symbols to it.  New symbols
must have symbol version GLIBC_2.33 or later, while preserving the old
symbol versions for the ports that already export it.

If you run ”make check”, you'll see abilist comparison failures for most
ports.  You need to fix those as well.

Thanks,
Florian
Fāng-ruì Sòng Jan. 12, 2021, 7:58 a.m. UTC | #4
On Mon, Jan 11, 2021 at 9:50 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Fāng-ruì Sòng:
>
> > On Mon, Jan 11, 2021 at 1:39 AM Florian Weimer <fweimer@redhat.com> wrote:
> >>
> >> * Fangrui Song via Libc-alpha:
> >>
> >> > This makes -mstack-protector-guard=global work even if
> >> > -mstack-protector-guard=tls is the default.
> >>
> >> It's unclear how you intend this to work.  Is it just for building
> >> glibc?  Or also for user code?
> >
> > It is for user applications.
> >
> > gcc -fstack-protector -mstack-protector-guard=global a.c
> > or
> > clang -fstack-protector -mstack-protector-guard=global a.c (clang 12)
> >
> > Currently there is an undefined symbol error. This patch fixes the problem.
>
> And we really want to use a global data symbol for this?  Nothing else
> works at the moment, but it seems rather wasteful.

https://sourceware.org/bugzilla/show_bug.cgi?id=22850
For a new thread, nptl/allocatestack.c allocates a memory region, places the
static TLS block at the end, and uses the remaining space as the thread stack.
The stack pointer may be just a few hundred bytes below the canary address
(%fs:0x28 on x86-64) and a large out-of-bounds write can potentially
override it.

This patch will make -mstack-protector-guard=global work, with very
little overhead (a word) in ld.so.

> >> In the latter case, we either need to provide a way to initialize a
> >> hidden __stack_chk_guard symbol (probably with new relocations), or
> >> export __stack_chk_guard from glibc, under a GLIBC_2.33 symbol version
> >> for architectures that were missing it before.
> >
> > In elf/Versions, __stack_chk_guard is already exported as
> > __stack_chk_guard@@GLIBC_2.4
>
> GLIBC_2.4 is done and dusted, we can't add symbols to it.  New symbols
> must have symbol version GLIBC_2.33 or later, while preserving the old
> symbol versions for the ports that already export it.
>
> If you run ”make check”, you'll see abilist comparison failures for most
> ports.  You need to fix those as well.

OK. Added %include "tls.h" to get THREAD_SET_STACK_GUARD,

https://github.com/MaskRay/glibc/tree/stack_chk_guard
I can post it here if it looks good.

> Thanks,
> Florian
> --
> Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
> Commercial register: Amtsgericht Muenchen, HRB 153243,
> Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill
>
Florian Weimer Jan. 14, 2021, 10:09 a.m. UTC | #5
* Fāng-ruì Sòng:

> On Mon, Jan 11, 2021 at 9:50 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * Fāng-ruì Sòng:
>>
>> > On Mon, Jan 11, 2021 at 1:39 AM Florian Weimer <fweimer@redhat.com> wrote:
>> >>
>> >> * Fangrui Song via Libc-alpha:
>> >>
>> >> > This makes -mstack-protector-guard=global work even if
>> >> > -mstack-protector-guard=tls is the default.
>> >>
>> >> It's unclear how you intend this to work.  Is it just for building
>> >> glibc?  Or also for user code?
>> >
>> > It is for user applications.
>> >
>> > gcc -fstack-protector -mstack-protector-guard=global a.c
>> > or
>> > clang -fstack-protector -mstack-protector-guard=global a.c (clang 12)
>> >
>> > Currently there is an undefined symbol error. This patch fixes the problem.
>>
>> And we really want to use a global data symbol for this?  Nothing else
>> works at the moment, but it seems rather wasteful.
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=22850
> For a new thread, nptl/allocatestack.c allocates a memory region, places the
> static TLS block at the end, and uses the remaining space as the thread stack.
> The stack pointer may be just a few hundred bytes below the canary address
> (%fs:0x28 on x86-64) and a large out-of-bounds write can potentially
> override it.

Is this the only reason?

We need to change the TCB allocation anyway.  Unlike LinuxThreads, there
is no technical requirement to have the TCB at the top of the stack.

> This patch will make -mstack-protector-guard=global work, with very
> little overhead (a word) in ld.so.

On the other hand, it may discourage people from enabling the stack
protector because the overhead encountered by instrumented functions
increases.

Perhaps we should add new relocation types and use a hidden, per-DSO
symbol.  The canary value does not have to be the same for all
functions.  I believe OpenBSD does this already.

>> >> In the latter case, we either need to provide a way to initialize a
>> >> hidden __stack_chk_guard symbol (probably with new relocations), or
>> >> export __stack_chk_guard from glibc, under a GLIBC_2.33 symbol version
>> >> for architectures that were missing it before.
>> >
>> > In elf/Versions, __stack_chk_guard is already exported as
>> > __stack_chk_guard@@GLIBC_2.4
>>
>> GLIBC_2.4 is done and dusted, we can't add symbols to it.  New symbols
>> must have symbol version GLIBC_2.33 or later, while preserving the old
>> symbol versions for the ports that already export it.
>>
>> If you run ”make check”, you'll see abilist comparison failures for most
>> ports.  You need to fix those as well.
>
> OK. Added %include "tls.h" to get THREAD_SET_STACK_GUARD,
>
> https://github.com/MaskRay/glibc/tree/stack_chk_guard
> I can post it here if it looks good.

It builds and produces the expected ABI, but the %ifdef kludges are
quite ugly.

Thanks,
Florian
diff mbox series

Patch

diff --git a/csu/libc-start.c b/csu/libc-start.c
index db859c3bed..e46d402664 100644
--- a/csu/libc-start.c
+++ b/csu/libc-start.c
@@ -33,11 +33,8 @@  extern void __libc_init_first (int argc, char **argv, char **envp);
 #include <tls.h>
 #ifndef SHARED
 # include <dl-osinfo.h>
-# ifndef THREAD_SET_STACK_GUARD
-/* Only exported for architectures that don't store the stack guard canary
-   in thread local area.  */
+/* Also export to architectures which prefer -mstack-protector-guard=tls.  */
 uintptr_t __stack_chk_guard attribute_relro;
-# endif
 # ifndef  THREAD_SET_POINTER_GUARD
 /* Only exported for architectures that don't store the pointer guard
    value in thread local area.  */
@@ -206,9 +203,8 @@  LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
   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
+# endif
   __stack_chk_guard = stack_chk_guard;
-# endif
 
 # ifdef DL_SYSDEP_OSCHECK
   {
diff --git a/elf/rtld.c b/elf/rtld.c
index 8d9add90e3..cd70428c46 100644
--- a/elf/rtld.c
+++ b/elf/rtld.c
@@ -154,11 +154,8 @@  unsigned int _dl_skip_args attribute_relro attribute_hidden;
 #endif
 rtld_hidden_data_def (_dl_argv)
 
-#ifndef THREAD_SET_STACK_GUARD
-/* Only exported for architectures that don't store the stack guard canary
-   in thread local area.  */
+/* Also export to architectures which prefer -mstack-protector-guard=tls.  */
 uintptr_t __stack_chk_guard attribute_relro;
-#endif
 
 /* Only exported for architectures that don't store the pointer guard
    value in thread local area.  */
@@ -865,9 +862,8 @@  security_init (void)
   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
+#endif
   __stack_chk_guard = stack_chk_guard;
-#endif
 
   /* Set up the pointer guard as well, if necessary.  */
   uintptr_t pointer_chk_guard