| Message ID | 20211216114711.3090519-1-stli@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix __minimal_malloc segfaults in __mmap due to stack-protector | expand |
On 12/16/21 17:17, Stefan Liebler via Libc-alpha wrote: > Starting with commit b05fae4d8e34604a72ee36d2d3164391b76fcf0b > "elf: Use the minimal malloc on tunables_strdup", > I get lots of segfaults in static tests on s390x when also using, e.g.: > export GLIBC_TUNABLES="glibc.elision.enable=1" > > tunables_strdup callls __minimal_malloc which tries to call __mmap > due to insufficient space left. __mmap itself first setups a new > stack frame and segfaults when copying the stack-protector canary > from thread-pointer. The latter one is not yet setup. > > Thus this patch also turns off stack-protection for mmap. LGTM. Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org> > --- > misc/Makefile | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/misc/Makefile b/misc/Makefile > index 3b66cb9f6a..db40312ba9 100644 > --- a/misc/Makefile > +++ b/misc/Makefile > @@ -149,6 +149,10 @@ CFLAGS-sbrk.o = $(no-stack-protector) > CFLAGS-sbrk.op = $(no-stack-protector) > CFLAGS-brk.o = $(no-stack-protector) > CFLAGS-brk.op = $(no-stack-protector) > +CFLAGS-mmap.o = $(no-stack-protector) > +CFLAGS-mmap.op = $(no-stack-protector) > +CFLAGS-mmap64.o = $(no-stack-protector) > +CFLAGS-mmap64.op = $(no-stack-protector) > > include ../Rules > >
On Thu, Dec 16, 2021 at 3:54 AM Siddhesh Poyarekar <siddhesh@gotplt.org> wrote: > > On 12/16/21 17:17, Stefan Liebler via Libc-alpha wrote: > > Starting with commit b05fae4d8e34604a72ee36d2d3164391b76fcf0b > > "elf: Use the minimal malloc on tunables_strdup", > > I get lots of segfaults in static tests on s390x when also using, e.g.: > > export GLIBC_TUNABLES="glibc.elision.enable=1" > > > > tunables_strdup callls __minimal_malloc which tries to call __mmap > > due to insufficient space left. __mmap itself first setups a new > > stack frame and segfaults when copying the stack-protector canary > > from thread-pointer. The latter one is not yet setup. > > > > Thus this patch also turns off stack-protection for mmap. > > LGTM. > > Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org> This also fixed: FAIL: elf/tst-env-setuid on x86-64 with GCC 12: (gdb) set env MALLOC_CHECK_=2 (gdb) r --direct Starting program: /export/build/gnu/tools-build/glibc-test/build-x86_64-linux/elf/tst-env-setuid --direct Program received signal SIGSEGV, Segmentation fault. 0x00007ffff7f67053 in do_tunable_update_val (cur=cur@entry=0xffffefefe7f0, valp=valp@entry=0x7fffffffdd38, minp=minp@entry=0x0, maxp=maxp@entry=0x0) at dl-tunables.c:102 102 if (cur->type.type_code == TUNABLE_TYPE_STRING) (gdb) bt #0 0x00007ffff7f67053 in do_tunable_update_val (cur=cur@entry=0xffffefefe7f0, valp=valp@entry=0x7fffffffdd38, minp=minp@entry=0x0, maxp=maxp@entry=0x0) at dl-tunables.c:102 #1 0x00007ffff7f6733c in tunable_initialize (strval=0x7fffffffefa7 "2", cur=<optimized out>) at dl-tunables.c:151 #2 __tunables_init (envp=0x7fffffffe058) at dl-tunables.c:349 #3 0x00007ffff7f15f67 in __libc_start_main_impl (main=0x7ffff7f128f0 <main>, argc=2, argv=0x7fffffffdea8, init=<optimized out>, fini=<optimized out>, rtld_fini=0x0, stack_end=0x7fffffffde98) at ../csu/libc-start.c:291 #4 0x00007ffff7f129c5 in _start () at ../sysdeps/x86_64/start.S:115 (gdb) > > --- > > misc/Makefile | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/misc/Makefile b/misc/Makefile > > index 3b66cb9f6a..db40312ba9 100644 > > --- a/misc/Makefile > > +++ b/misc/Makefile > > @@ -149,6 +149,10 @@ CFLAGS-sbrk.o = $(no-stack-protector) > > CFLAGS-sbrk.op = $(no-stack-protector) > > CFLAGS-brk.o = $(no-stack-protector) > > CFLAGS-brk.op = $(no-stack-protector) > > +CFLAGS-mmap.o = $(no-stack-protector) > > +CFLAGS-mmap.op = $(no-stack-protector) > > +CFLAGS-mmap64.o = $(no-stack-protector) > > +CFLAGS-mmap64.op = $(no-stack-protector) > > > > include ../Rules > > > > >
On 16/12/2021 12:54, Siddhesh Poyarekar wrote: > On 12/16/21 17:17, Stefan Liebler via Libc-alpha wrote: >> Starting with commit b05fae4d8e34604a72ee36d2d3164391b76fcf0b >> "elf: Use the minimal malloc on tunables_strdup", >> I get lots of segfaults in static tests on s390x when also using, e.g.: >> export GLIBC_TUNABLES="glibc.elision.enable=1" >> >> tunables_strdup callls __minimal_malloc which tries to call __mmap >> due to insufficient space left. __mmap itself first setups a new >> stack frame and segfaults when copying the stack-protector canary >> from thread-pointer. The latter one is not yet setup. >> >> Thus this patch also turns off stack-protection for mmap. > > LGTM. > > Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org> > >> --- >> misc/Makefile | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/misc/Makefile b/misc/Makefile >> index 3b66cb9f6a..db40312ba9 100644 >> --- a/misc/Makefile >> +++ b/misc/Makefile >> @@ -149,6 +149,10 @@ CFLAGS-sbrk.o = $(no-stack-protector) >> CFLAGS-sbrk.op = $(no-stack-protector) >> CFLAGS-brk.o = $(no-stack-protector) >> CFLAGS-brk.op = $(no-stack-protector) >> +CFLAGS-mmap.o = $(no-stack-protector) >> +CFLAGS-mmap.op = $(no-stack-protector) >> +CFLAGS-mmap64.o = $(no-stack-protector) >> +CFLAGS-mmap64.op = $(no-stack-protector) >> include ../Rules >> > Committed. Thanks
On 16/12/2021 11:20, Stefan Liebler via Libc-alpha wrote: > On 16/12/2021 12:54, Siddhesh Poyarekar wrote: >> On 12/16/21 17:17, Stefan Liebler via Libc-alpha wrote: >>> Starting with commit b05fae4d8e34604a72ee36d2d3164391b76fcf0b >>> "elf: Use the minimal malloc on tunables_strdup", >>> I get lots of segfaults in static tests on s390x when also using, e.g.: >>> export GLIBC_TUNABLES="glibc.elision.enable=1" >>> >>> tunables_strdup callls __minimal_malloc which tries to call __mmap >>> due to insufficient space left. __mmap itself first setups a new >>> stack frame and segfaults when copying the stack-protector canary >>> from thread-pointer. The latter one is not yet setup. >>> >>> Thus this patch also turns off stack-protection for mmap. >> >> LGTM. >> >> Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org> >> >>> --- >>> misc/Makefile | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/misc/Makefile b/misc/Makefile >>> index 3b66cb9f6a..db40312ba9 100644 >>> --- a/misc/Makefile >>> +++ b/misc/Makefile >>> @@ -149,6 +149,10 @@ CFLAGS-sbrk.o = $(no-stack-protector) >>> CFLAGS-sbrk.op = $(no-stack-protector) >>> CFLAGS-brk.o = $(no-stack-protector) >>> CFLAGS-brk.op = $(no-stack-protector) >>> +CFLAGS-mmap.o = $(no-stack-protector) >>> +CFLAGS-mmap.op = $(no-stack-protector) >>> +CFLAGS-mmap64.o = $(no-stack-protector) >>> +CFLAGS-mmap64.op = $(no-stack-protector) >>> include ../Rules >>> >> > Committed. > Thanks Thanks for catching it, I think I have not seeing it before because I got lucky the data segments has some slack space and not required called malloc. Btw, do we still need to use no-stack-protector?
On 16/12/2021 15:44, Adhemerval Zanella wrote: > > > On 16/12/2021 11:20, Stefan Liebler via Libc-alpha wrote: >> On 16/12/2021 12:54, Siddhesh Poyarekar wrote: >>> On 12/16/21 17:17, Stefan Liebler via Libc-alpha wrote: >>>> Starting with commit b05fae4d8e34604a72ee36d2d3164391b76fcf0b >>>> "elf: Use the minimal malloc on tunables_strdup", >>>> I get lots of segfaults in static tests on s390x when also using, e.g.: >>>> export GLIBC_TUNABLES="glibc.elision.enable=1" >>>> >>>> tunables_strdup callls __minimal_malloc which tries to call __mmap >>>> due to insufficient space left. __mmap itself first setups a new >>>> stack frame and segfaults when copying the stack-protector canary >>>> from thread-pointer. The latter one is not yet setup. >>>> >>>> Thus this patch also turns off stack-protection for mmap. >>> >>> LGTM. >>> >>> Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org> >>> >>>> --- >>>> misc/Makefile | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/misc/Makefile b/misc/Makefile >>>> index 3b66cb9f6a..db40312ba9 100644 >>>> --- a/misc/Makefile >>>> +++ b/misc/Makefile >>>> @@ -149,6 +149,10 @@ CFLAGS-sbrk.o = $(no-stack-protector) >>>> CFLAGS-sbrk.op = $(no-stack-protector) >>>> CFLAGS-brk.o = $(no-stack-protector) >>>> CFLAGS-brk.op = $(no-stack-protector) >>>> +CFLAGS-mmap.o = $(no-stack-protector) >>>> +CFLAGS-mmap.op = $(no-stack-protector) >>>> +CFLAGS-mmap64.o = $(no-stack-protector) >>>> +CFLAGS-mmap64.op = $(no-stack-protector) >>>> include ../Rules >>>> >>> >> Committed. >> Thanks > > > Thanks for catching it, I think I have not seeing it before because I got lucky > the data segments has some slack space and not required called malloc. > > Btw, do we still need to use no-stack-protector? > At least for the new mmap files, this is needed. Do you mean for [s]brk?
On 16/12/2021 12:28, Stefan Liebler wrote: > On 16/12/2021 15:44, Adhemerval Zanella wrote: >> >> >> On 16/12/2021 11:20, Stefan Liebler via Libc-alpha wrote: >>> On 16/12/2021 12:54, Siddhesh Poyarekar wrote: >>>> On 12/16/21 17:17, Stefan Liebler via Libc-alpha wrote: >>>>> Starting with commit b05fae4d8e34604a72ee36d2d3164391b76fcf0b >>>>> "elf: Use the minimal malloc on tunables_strdup", >>>>> I get lots of segfaults in static tests on s390x when also using, e.g.: >>>>> export GLIBC_TUNABLES="glibc.elision.enable=1" >>>>> >>>>> tunables_strdup callls __minimal_malloc which tries to call __mmap >>>>> due to insufficient space left. __mmap itself first setups a new >>>>> stack frame and segfaults when copying the stack-protector canary >>>>> from thread-pointer. The latter one is not yet setup. >>>>> >>>>> Thus this patch also turns off stack-protection for mmap. >>>> >>>> LGTM. >>>> >>>> Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org> >>>> >>>>> --- >>>>> misc/Makefile | 4 ++++ >>>>> 1 file changed, 4 insertions(+) >>>>> >>>>> diff --git a/misc/Makefile b/misc/Makefile >>>>> index 3b66cb9f6a..db40312ba9 100644 >>>>> --- a/misc/Makefile >>>>> +++ b/misc/Makefile >>>>> @@ -149,6 +149,10 @@ CFLAGS-sbrk.o = $(no-stack-protector) >>>>> CFLAGS-sbrk.op = $(no-stack-protector) >>>>> CFLAGS-brk.o = $(no-stack-protector) >>>>> CFLAGS-brk.op = $(no-stack-protector) >>>>> +CFLAGS-mmap.o = $(no-stack-protector) >>>>> +CFLAGS-mmap.op = $(no-stack-protector) >>>>> +CFLAGS-mmap64.o = $(no-stack-protector) >>>>> +CFLAGS-mmap64.op = $(no-stack-protector) >>>>> include ../Rules >>>>> >>>> >>> Committed. >>> Thanks >> >> >> Thanks for catching it, I think I have not seeing it before because I got lucky >> the data segments has some slack space and not required called malloc. >> >> Btw, do we still need to use no-stack-protector? >> > At least for the new mmap files, this is needed. > Do you mean for [s]brk? Yes, I forgot to add it.
diff --git a/misc/Makefile b/misc/Makefile index 3b66cb9f6a..db40312ba9 100644 --- a/misc/Makefile +++ b/misc/Makefile @@ -149,6 +149,10 @@ CFLAGS-sbrk.o = $(no-stack-protector) CFLAGS-sbrk.op = $(no-stack-protector) CFLAGS-brk.o = $(no-stack-protector) CFLAGS-brk.op = $(no-stack-protector) +CFLAGS-mmap.o = $(no-stack-protector) +CFLAGS-mmap.op = $(no-stack-protector) +CFLAGS-mmap64.o = $(no-stack-protector) +CFLAGS-mmap64.op = $(no-stack-protector) include ../Rules