* [PATCH] Unconditionally define __stack_chk_guard [BZ #26817]
@ 2021-01-11 3:12 Fangrui Song
2021-01-11 9:39 ` Florian Weimer
0 siblings, 1 reply; 6+ messages in thread
From: Fangrui Song @ 2021-01-11 3:12 UTC (permalink / raw)
To: libc-alpha; +Cc: Fangrui Song
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(-)
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
--
2.30.0.284.gd98b1dd5eaa7-goog
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Unconditionally define __stack_chk_guard [BZ #26817]
2021-01-11 3:12 [PATCH] Unconditionally define __stack_chk_guard [BZ #26817] Fangrui Song
@ 2021-01-11 9:39 ` Florian Weimer
2021-01-11 17:40 ` Fāng-ruì Sòng
0 siblings, 1 reply; 6+ messages in thread
From: Florian Weimer @ 2021-01-11 9:39 UTC (permalink / raw)
To: Fangrui Song via Libc-alpha; +Cc: Fangrui Song
* 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
--
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Unconditionally define __stack_chk_guard [BZ #26817]
2021-01-11 9:39 ` Florian Weimer
@ 2021-01-11 17:40 ` Fāng-ruì Sòng
2021-01-11 17:49 ` Florian Weimer
0 siblings, 1 reply; 6+ messages in thread
From: Fāng-ruì Sòng @ 2021-01-11 17:40 UTC (permalink / raw)
To: Florian Weimer; +Cc: Fangrui Song via Libc-alpha
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Unconditionally define __stack_chk_guard [BZ #26817]
2021-01-11 17:40 ` Fāng-ruì Sòng
@ 2021-01-11 17:49 ` Florian Weimer
2021-01-12 7:58 ` Fāng-ruì Sòng
0 siblings, 1 reply; 6+ messages in thread
From: Florian Weimer @ 2021-01-11 17:49 UTC (permalink / raw)
To: Fāng-ruì Sòng; +Cc: Fangrui Song via Libc-alpha
* 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
--
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Unconditionally define __stack_chk_guard [BZ #26817]
2021-01-11 17:49 ` Florian Weimer
@ 2021-01-12 7:58 ` Fāng-ruì Sòng
2021-01-14 10:09 ` Florian Weimer
0 siblings, 1 reply; 6+ messages in thread
From: Fāng-ruì Sòng @ 2021-01-12 7:58 UTC (permalink / raw)
To: Florian Weimer; +Cc: Fangrui Song via Libc-alpha
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Unconditionally define __stack_chk_guard [BZ #26817]
2021-01-12 7:58 ` Fāng-ruì Sòng
@ 2021-01-14 10:09 ` Florian Weimer
0 siblings, 0 replies; 6+ messages in thread
From: Florian Weimer @ 2021-01-14 10:09 UTC (permalink / raw)
To: Fāng-ruì Sòng; +Cc: Fangrui Song via Libc-alpha
* 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
--
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-14 10:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-11 3:12 [PATCH] Unconditionally define __stack_chk_guard [BZ #26817] Fangrui Song
2021-01-11 9:39 ` Florian Weimer
2021-01-11 17:40 ` Fāng-ruì Sòng
2021-01-11 17:49 ` Florian Weimer
2021-01-12 7:58 ` Fāng-ruì Sòng
2021-01-14 10:09 ` Florian Weimer
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).