Fix __minimal_malloc segfaults in __mmap due to stack-protector
Checks
| Context |
Check |
Description |
| dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
| dj/TryBot-32bit |
success
|
Build for i686
|
Commit Message
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.
---
misc/Makefile | 4 ++++
1 file changed, 4 insertions(+)
Comments
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.
@@ -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