[PATCH] Fix __minimal_malloc segfaults in __mmap due to stack-protector

[PATCH] Fix __minimal_malloc segfaults in __mmap due to stack-protector

[Stefan Liebler]

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(+)

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
 
-- 
2.33.1

Re: [PATCH] Fix __minimal_malloc segfaults in __mmap due to stack-protector

[Siddhesh Poyarekar]

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

Re: [PATCH] Fix __minimal_malloc segfaults in __mmap due to stack-protector

[H.J. Lu]

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


-- 
H.J.

Re: [PATCH] Fix __minimal_malloc segfaults in __mmap due to stack-protector

[Stefan Liebler]

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

Re: [PATCH] Fix __minimal_malloc segfaults in __mmap due to stack-protector

[Adhemerval Zanella]

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?

Re: [PATCH] Fix __minimal_malloc segfaults in __mmap due to stack-protector

[Stefan Liebler]

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?

Re: [PATCH] Fix __minimal_malloc segfaults in __mmap due to stack-protector

[Adhemerval Zanella]

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.