diff mbox

posix_spawn: use a larger min stack for -fstack-check [BZ #21253]

Message ID 20170316073012.22763-1-vapier@gentoo.org
State New, archived
Headers

Commit Message

Mike Frysinger March 16, 2017, 7:30 a.m. UTC 
  When glibc is built with -fstack-check, trying to use posix_spawn can
lead to segfaults due to gcc internally probing stack memory too far.
The new spawn API will allocate a minimum of 1 page, but the stack
checking logic might probe a couple of pages.  When it tries to walk
them, everything falls apart.

The gcc internal docs [1] state the default interval checking is one
page.  Which means we need two pages (the current one, and the next
probed).  No target currently defines it larger.

Further, it mentions that the default minimum stack size needed to
recover from an overflow is 4/8KiB for sjlj or 8/12KiB for others.
But some Linux targets (like mips and ppc) go up to 16KiB (and some
non-Linux targets go up to 24KiB).

Let's create each child with a minimum of 32KiB to support them all,
and give us future breathing room.

No test is added as existing ones crash.  Even a simple call is
enough to trigger the problem:
	char *argv[] = { "/bin/ls", NULL };
	posix_spawn(&pid, "/bin/ls", NULL, NULL, argv, NULL);

[1] https://gcc.gnu.org/onlinedocs/gcc-6.3.0/gccint/Stack-Checking.html
---
 sysdeps/unix/sysv/linux/spawni.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Florian Weimer March 16, 2017, 8:17 a.m. UTC | #1 
On 03/16/2017 08:30 AM, Mike Frysinger wrote:
> When glibc is built with -fstack-check, trying to use posix_spawn can
> lead to segfaults due to gcc internally probing stack memory too far.
> The new spawn API will allocate a minimum of 1 page, but the stack
> checking logic might probe a couple of pages.  When it tries to walk
> them, everything falls apart.
>
> The gcc internal docs [1] state the default interval checking is one
> page.  Which means we need two pages (the current one, and the next
> probed).  No target currently defines it larger.

GCC miscomputes the offsets in some cases, so I would not rely on this.

Would it be possible compile the functions involved without 
-fstack-check instead?

>    /* Add a slack area for child's stack.  */
>    size_t argv_size = (argc * sizeof (void *)) + 512;
> -  size_t stack_size = ALIGN_UP (argv_size, GLRO(dl_pagesize));
> +  /* We need at least a few pages in case the compiler's stack checking is
> +     enabled.  In some configs, it is known to use at least 24KiB.  */
> +  size_t stack_size = ALIGN_UP (argv_size, 32 * 1024);

64 KiB pages are common, so this reduces the stack size in many cases.

Thanks,
Florian
Adhemerval Zanella March 16, 2017, 11:29 a.m. UTC | #2 
On 16/03/2017 05:17, Florian Weimer wrote:
> On 03/16/2017 08:30 AM, Mike Frysinger wrote:
>> When glibc is built with -fstack-check, trying to use posix_spawn can
>> lead to segfaults due to gcc internally probing stack memory too far.
>> The new spawn API will allocate a minimum of 1 page, but the stack
>> checking logic might probe a couple of pages.  When it tries to walk
>> them, everything falls apart.
>>
>> The gcc internal docs [1] state the default interval checking is one
>> page.  Which means we need two pages (the current one, and the next
>> probed).  No target currently defines it larger.
> 
> GCC miscomputes the offsets in some cases, so I would not rely on this.
> 
> Would it be possible compile the functions involved without -fstack-check instead?

There is some old bug reports about this GCC option which states it
somewhat unreliable in some cases [1].  However what really worries me
is the bug report [2] stating that the probe check range can wrap
around and thus totally infective in some cases (although this case
indeed might be not common).

Anyway, I am with Florian, we should not rely on this.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=13182
[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66479

> 
>>    /* Add a slack area for child's stack.  */
>>    size_t argv_size = (argc * sizeof (void *)) + 512;
>> -  size_t stack_size = ALIGN_UP (argv_size, GLRO(dl_pagesize));
>> +  /* We need at least a few pages in case the compiler's stack checking is
>> +     enabled.  In some configs, it is known to use at least 24KiB.  */
>> +  size_t stack_size = ALIGN_UP (argv_size, 32 * 1024);
> 
> 64 KiB pages are common, so this reduces the stack size in many cases.
> 
> Thanks,
> Florian
>
Mike Frysinger March 16, 2017, 9:52 p.m. UTC | #3 
On 16 Mar 2017 09:17, Florian Weimer wrote:
> On 03/16/2017 08:30 AM, Mike Frysinger wrote:
> > When glibc is built with -fstack-check, trying to use posix_spawn can
> > lead to segfaults due to gcc internally probing stack memory too far.
> > The new spawn API will allocate a minimum of 1 page, but the stack
> > checking logic might probe a couple of pages.  When it tries to walk
> > them, everything falls apart.
> >
> > The gcc internal docs [1] state the default interval checking is one
> > page.  Which means we need two pages (the current one, and the next
> > probed).  No target currently defines it larger.
> 
> GCC miscomputes the offsets in some cases, so I would not rely on this.
> 
> Would it be possible compile the functions involved without 
> -fstack-check instead?

i mentioned in the bug that it's not feasible: compiling this one file
doesn't help as it calls other glibc funcs which will have checking
enabled.  so we'd have to manually track the full call stack here and
disable it on all the files which is a fairly fragile/burdensome process.

> >    /* Add a slack area for child's stack.  */
> >    size_t argv_size = (argc * sizeof (void *)) + 512;
> > -  size_t stack_size = ALIGN_UP (argv_size, GLRO(dl_pagesize));
> > +  /* We need at least a few pages in case the compiler's stack checking is
> > +     enabled.  In some configs, it is known to use at least 24KiB.  */
> > +  size_t stack_size = ALIGN_UP (argv_size, 32 * 1024);
> 
> 64 KiB pages are common, so this reduces the stack size in many cases.

common where ?  are you talking about large pages ?

going by PAGE_SHIFT in the kernel, looks to me like very few
targets allow using 64 KiB at all, let alone being common.

alpha: 8 KiB
arc: kconfig allows 4, 8 (default), or 16 KiB
arm: 4 KiB
arm64: kconfig allows 4, 16, or 64 KiB (default depends on # of VMA bits)
avr32: 4 KiB
cris: 8 KiB
frv: 16 KiB
hexagon: kconfig allows 4 (default), 16, 64, 256 KiB, or 1 MiB
ia64: kconfig allows 4, 8, 16 (default), or 64 KiB
m32r: 4 KiB
m68k: 4 (non-coldfire) or 8 KiB (coldfire)
meta: kconfig allows 4 (default), 8, or 16 KiB
microblaze: kconfig allows 4 (default), 16, or 64 KiB
mips: kconfig allows 4 (default for most, but gets tricky), 8, 16, 32, or 64 KiB
mn10300: 4 KiB
nios2: 4 KiB
openrisc: 8 KiB
parisc: kconfig allows 4 (default), 16, or 64 KiB
powerpc: kconfig allows 4 (default), 16, 64, or 256 KiB
s390: 4 KiB
score: 4 KiB
sh: 4 KiB (for no-mmu, you have other choices)
sparc32: 4 KiB
sparc64: 8 KiB
tile: kconfig allows 4, 16, or 64 KiB (default)
unicore32: 4 KiB
x86: 4 KiB
xtensa: 4 KiB

i picked 32 KiB as a trade off of it being more than big enough and
being a nice multiple of 4, 8, and 16, while not overtly allocating
too much otherwise unused.
-mike
Andrew Pinski March 16, 2017, 9:56 p.m. UTC | #4 
On Thu, Mar 16, 2017 at 2:52 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On 16 Mar 2017 09:17, Florian Weimer wrote:
>> On 03/16/2017 08:30 AM, Mike Frysinger wrote:
>> > When glibc is built with -fstack-check, trying to use posix_spawn can
>> > lead to segfaults due to gcc internally probing stack memory too far.
>> > The new spawn API will allocate a minimum of 1 page, but the stack
>> > checking logic might probe a couple of pages.  When it tries to walk
>> > them, everything falls apart.
>> >
>> > The gcc internal docs [1] state the default interval checking is one
>> > page.  Which means we need two pages (the current one, and the next
>> > probed).  No target currently defines it larger.
>>
>> GCC miscomputes the offsets in some cases, so I would not rely on this.
>>
>> Would it be possible compile the functions involved without
>> -fstack-check instead?
>
> i mentioned in the bug that it's not feasible: compiling this one file
> doesn't help as it calls other glibc funcs which will have checking
> enabled.  so we'd have to manually track the full call stack here and
> disable it on all the files which is a fairly fragile/burdensome process.
>
>> >    /* Add a slack area for child's stack.  */
>> >    size_t argv_size = (argc * sizeof (void *)) + 512;
>> > -  size_t stack_size = ALIGN_UP (argv_size, GLRO(dl_pagesize));
>> > +  /* We need at least a few pages in case the compiler's stack checking is
>> > +     enabled.  In some configs, it is known to use at least 24KiB.  */
>> > +  size_t stack_size = ALIGN_UP (argv_size, 32 * 1024);
>>
>> 64 KiB pages are common, so this reduces the stack size in many cases.
>
> common where ?  are you talking about large pages ?
>
> going by PAGE_SHIFT in the kernel, looks to me like very few
> targets allow using 64 KiB at all, let alone being common.

RHEL on ARM64 defaults to 64k.
For Cavium Octeon SDK, the mips kernel defaults to 64k page size.

Thanks,
Andrew

>
> alpha: 8 KiB
> arc: kconfig allows 4, 8 (default), or 16 KiB
> arm: 4 KiB
> arm64: kconfig allows 4, 16, or 64 KiB (default depends on # of VMA bits)
> avr32: 4 KiB
> cris: 8 KiB
> frv: 16 KiB
> hexagon: kconfig allows 4 (default), 16, 64, 256 KiB, or 1 MiB
> ia64: kconfig allows 4, 8, 16 (default), or 64 KiB
> m32r: 4 KiB
> m68k: 4 (non-coldfire) or 8 KiB (coldfire)
> meta: kconfig allows 4 (default), 8, or 16 KiB
> microblaze: kconfig allows 4 (default), 16, or 64 KiB
> mips: kconfig allows 4 (default for most, but gets tricky), 8, 16, 32, or 64 KiB
> mn10300: 4 KiB
> nios2: 4 KiB
> openrisc: 8 KiB
> parisc: kconfig allows 4 (default), 16, or 64 KiB
> powerpc: kconfig allows 4 (default), 16, 64, or 256 KiB
> s390: 4 KiB
> score: 4 KiB
> sh: 4 KiB (for no-mmu, you have other choices)
> sparc32: 4 KiB
> sparc64: 8 KiB
> tile: kconfig allows 4, 16, or 64 KiB (default)
> unicore32: 4 KiB
> x86: 4 KiB
> xtensa: 4 KiB
>
> i picked 32 KiB as a trade off of it being more than big enough and
> being a nice multiple of 4, 8, and 16, while not overtly allocating
> too much otherwise unused.
> -mike
Mike Frysinger March 16, 2017, 10:41 p.m. UTC | #5 
On 16 Mar 2017 14:56, Andrew Pinski wrote:
> On Thu, Mar 16, 2017 at 2:52 PM, Mike Frysinger wrote:
> > On 16 Mar 2017 09:17, Florian Weimer wrote:
> >> On 03/16/2017 08:30 AM, Mike Frysinger wrote:
> >> > When glibc is built with -fstack-check, trying to use posix_spawn can
> >> > lead to segfaults due to gcc internally probing stack memory too far.
> >> > The new spawn API will allocate a minimum of 1 page, but the stack
> >> > checking logic might probe a couple of pages.  When it tries to walk
> >> > them, everything falls apart.
> >> >
> >> > The gcc internal docs [1] state the default interval checking is one
> >> > page.  Which means we need two pages (the current one, and the next
> >> > probed).  No target currently defines it larger.
> >>
> >> GCC miscomputes the offsets in some cases, so I would not rely on this.
> >>
> >> Would it be possible compile the functions involved without
> >> -fstack-check instead?
> >
> > i mentioned in the bug that it's not feasible: compiling this one file
> > doesn't help as it calls other glibc funcs which will have checking
> > enabled.  so we'd have to manually track the full call stack here and
> > disable it on all the files which is a fairly fragile/burdensome process.
> >
> >> >    /* Add a slack area for child's stack.  */
> >> >    size_t argv_size = (argc * sizeof (void *)) + 512;
> >> > -  size_t stack_size = ALIGN_UP (argv_size, GLRO(dl_pagesize));
> >> > +  /* We need at least a few pages in case the compiler's stack checking is
> >> > +     enabled.  In some configs, it is known to use at least 24KiB.  */
> >> > +  size_t stack_size = ALIGN_UP (argv_size, 32 * 1024);
> >>
> >> 64 KiB pages are common, so this reduces the stack size in many cases.
> >
> > common where ?  are you talking about large pages ?
> >
> > going by PAGE_SHIFT in the kernel, looks to me like very few
> > targets allow using 64 KiB at all, let alone being common.
> 
> RHEL on ARM64 defaults to 64k.
> For Cavium Octeon SDK, the mips kernel defaults to 64k page size.

i think it's fair to say that those userbases do not constitute anywhere
close to a majority, or even a signficiantly large presence.

from what i can tell, gcc does not expose any CPP defines we can use to
see if stack checking is enabled.  so we don't have a way of turning the
extra allocations on conditionally.

wouldn't be easy to add a configure check either since -fstack-check can
be turned on via default specs.
-mike
Szabolcs Nagy March 17, 2017, 2:38 p.m. UTC | #6 
On 16/03/17 08:17, Florian Weimer wrote:
> On 03/16/2017 08:30 AM, Mike Frysinger wrote:
>> When glibc is built with -fstack-check, trying to use posix_spawn can
>> lead to segfaults due to gcc internally probing stack memory too far.
>> The new spawn API will allocate a minimum of 1 page, but the stack
>> checking logic might probe a couple of pages.  When it tries to walk
>> them, everything falls apart.
>>
>> The gcc internal docs [1] state the default interval checking is one
>> page.  Which means we need two pages (the current one, and the next
>> probed).  No target currently defines it larger.
> 
> GCC miscomputes the offsets in some cases, so I would not rely on this.
> 
> Would it be possible compile the functions involved without -fstack-check instead?
> 

+1 for compiling with -fno-stack-check

>>    /* Add a slack area for child's stack.  */
>>    size_t argv_size = (argc * sizeof (void *)) + 512;
>> -  size_t stack_size = ALIGN_UP (argv_size, GLRO(dl_pagesize));
>> +  /* We need at least a few pages in case the compiler's stack checking is
>> +     enabled.  In some configs, it is known to use at least 24KiB.  */
>> +  size_t stack_size = ALIGN_UP (argv_size, 32 * 1024);
> 
> 64 KiB pages are common, so this reduces the stack size in many cases.
> 

i think mmap will align up, so there is no actual reduction,
but i don't think the magic value is justified.
Andreas Schwab March 17, 2017, 5:07 p.m. UTC | #7 
On Mär 16 2017, Mike Frysinger <vapier@gentoo.org> wrote:

> common where ?

SLES on POWER uses 64K pages.

> i picked 32 KiB as a trade off

??? Just use the max of pagesize and 32k.

Andreas.
Florian Weimer March 20, 2017, 8:56 a.m. UTC | #8 
On 03/16/2017 10:52 PM, Mike Frysinger wrote:
> i picked 32 KiB as a trade off of it being more than big enough and
> being a nice multiple of 4, 8, and 16, while not overtly allocating
> too much otherwise unused.

Depending on what constant page size GCC uses, this will not fix the 
-fstack-check bug on all architectures.

I doubt there is a good solution to this, except fixing the GCC bugs in 
this area and allocating an additional page in glibc.

Thanks,
Florian
Mike Frysinger March 20, 2017, 9:04 a.m. UTC | #9 
On 20 Mar 2017 09:56, Florian Weimer wrote:
> On 03/16/2017 10:52 PM, Mike Frysinger wrote:
> > i picked 32 KiB as a trade off of it being more than big enough and
> > being a nice multiple of 4, 8, and 16, while not overtly allocating
> > too much otherwise unused.
> 
> Depending on what constant page size GCC uses, this will not fix the 
> -fstack-check bug on all architectures.

do you know of one today ?  i grepped through the latest gcc source to
come up with the numbers that i used, and i think 32 KiB should cover
them all.

> I doubt there is a good solution to this, except fixing the GCC bugs in 
> this area and allocating an additional page in glibc.

what gcc bugs do you mean ?
-mike
Florian Weimer March 20, 2017, 1:26 p.m. UTC | #10 
On 03/20/2017 10:04 AM, Mike Frysinger wrote:
> On 20 Mar 2017 09:56, Florian Weimer wrote:
>> On 03/16/2017 10:52 PM, Mike Frysinger wrote:
>>> i picked 32 KiB as a trade off of it being more than big enough and
>>> being a nice multiple of 4, 8, and 16, while not overtly allocating
>>> too much otherwise unused.
>>
>> Depending on what constant page size GCC uses, this will not fix the
>> -fstack-check bug on all architectures.
>
> do you know of one today ?  i grepped through the latest gcc source to
> come up with the numbers that i used, and i think 32 KiB should cover
> them all.

GCC stack probing assumes a 4 KiB stack size, which should be a 
conservative assumption almost everywhere.  But the first probe is off 
by a few bytes.  This seems to be a common issue across many target, 
e.g. on GCC 6 for aarch64:

void external(void *);

void
int1 (void)
{
   int a;
   external (&a);
}


         .text
         .align  2
         .p2align 3,,7
         .global int1
         .type   int1, %function
int1:
.LFB0:
         .cfi_startproc
         sub     x9, sp, #8192
         str     xzr, [x9, 4064]
         stp     x29, x30, [sp, -32]!
         .cfi_def_cfa_offset 32
         .cfi_offset 29, -32
         .cfi_offset 30, -24
         add     x29, sp, 0
         .cfi_def_cfa_register 29
         add     x0, x29, 28
         bl      external
         ldp     x29, x30, [sp], 32
         .cfi_restore 30
         .cfi_restore 29
         .cfi_def_cfa 31, 0
         ret
         .cfi_endproc
.LFE0:
         .size   int1, .-int1

Or ppc64le:

         .abiversion 2
         .section        ".text"
         .align 2
         .p2align 4,,15
         .globl int1
         .type   int1, @function
int1:
.LCF0:
0:      addis 2,12,.TOC.-.LCF0@ha
         addi 2,2,.TOC.-.LCF0@l
         .localentry     int1,.-int1
         mflr 0
         std 0,-16432(1)
         std 0,16(1)
         stdu 1,-48(1)
         addi 3,1,32
         bl external
         nop
         addi 1,1,48
         ld 0,16(1)
         mtlr 0
         blr
         .long 0
         .byte 0,0,0,1,128,0,0,0
         .size   int1,.-int1


Or x86_64:

         .text
         .p2align 4,,15
         .globl  int1
         .type   int1, @function
int1:
.LFB0:
         .cfi_startproc
         subq    $4152, %rsp
         orq     $0, (%rsp)
         addq    $4128, %rsp
         .cfi_def_cfa_offset 32
         leaq    12(%rsp), %rdi
         call    external
         addq    $24, %rsp
         .cfi_def_cfa_offset 8
         ret
         .cfi_endproc
.LFE0:
         .size   int1, .-int1
         .p2align 4,,15

I know x86-64 best, so I'm going to explain the issue there.  Suppose 
that at the time of the call instruction, %rsp (the stack pointer) ends 
with …3000.  Then on function entry, %rsp == …3008.  Decrementing %rsp 
by 4152 results in …1FD0.  The 8-byte stack probe touches the bytes from 
…1FD0 to …1FD7.  This skips over the guard page at …2000 (which extends 
to …2FFF).

The other problem is that GCC does not take the implied stack probe by 
the call instruction into account, or other forms of linear stack 
access.  This leads to grossly inefficient code with -fstack-check.

I actually think that compiling with -fstack-check is a good idea, but 
GCC really needs fixes first.  We do not know the root cause of these 
issues (note the 3 * 4096 probe offset on POWER), and just throwing 
random workarounds into glibc does not seem right.

Thanks,
Florian
Adhemerval Zanella March 20, 2017, 1:44 p.m. UTC | #11 
On 20/03/2017 10:26, Florian Weimer wrote:
> On 03/20/2017 10:04 AM, Mike Frysinger wrote:
>> On 20 Mar 2017 09:56, Florian Weimer wrote:
>>> On 03/16/2017 10:52 PM, Mike Frysinger wrote:
>>>> i picked 32 KiB as a trade off of it being more than big enough and
>>>> being a nice multiple of 4, 8, and 16, while not overtly allocating
>>>> too much otherwise unused.
>>>
>>> Depending on what constant page size GCC uses, this will not fix the
>>> -fstack-check bug on all architectures.
>>
>> do you know of one today ?  i grepped through the latest gcc source to
>> come up with the numbers that i used, and i think 32 KiB should cover
>> them all.
> 
> GCC stack probing assumes a 4 KiB stack size, which should be a conservative assumption almost everywhere.  But the first probe is off by a few bytes.  This seems to be a common issue across many target, e.g. on GCC 6 for aarch64:
> 
> void external(void *);
> 
> void
> int1 (void)
> {
>   int a;
>   external (&a);
> }
> 
> 
>         .text
>         .align  2
>         .p2align 3,,7
>         .global int1
>         .type   int1, %function
> int1:
> .LFB0:
>         .cfi_startproc
>         sub     x9, sp, #8192
>         str     xzr, [x9, 4064]
>         stp     x29, x30, [sp, -32]!
>         .cfi_def_cfa_offset 32
>         .cfi_offset 29, -32
>         .cfi_offset 30, -24
>         add     x29, sp, 0
>         .cfi_def_cfa_register 29
>         add     x0, x29, 28
>         bl      external
>         ldp     x29, x30, [sp], 32
>         .cfi_restore 30
>         .cfi_restore 29
>         .cfi_def_cfa 31, 0
>         ret
>         .cfi_endproc
> .LFE0:
>         .size   int1, .-int1
> 
> Or ppc64le:
> 
>         .abiversion 2
>         .section        ".text"
>         .align 2
>         .p2align 4,,15
>         .globl int1
>         .type   int1, @function
> int1:
> .LCF0:
> 0:      addis 2,12,.TOC.-.LCF0@ha
>         addi 2,2,.TOC.-.LCF0@l
>         .localentry     int1,.-int1
>         mflr 0
>         std 0,-16432(1)
>         std 0,16(1)
>         stdu 1,-48(1)
>         addi 3,1,32
>         bl external
>         nop
>         addi 1,1,48
>         ld 0,16(1)
>         mtlr 0
>         blr
>         .long 0
>         .byte 0,0,0,1,128,0,0,0
>         .size   int1,.-int1
> 
> 
> Or x86_64:
> 
>         .text
>         .p2align 4,,15
>         .globl  int1
>         .type   int1, @function
> int1:
> .LFB0:
>         .cfi_startproc
>         subq    $4152, %rsp
>         orq     $0, (%rsp)
>         addq    $4128, %rsp
>         .cfi_def_cfa_offset 32
>         leaq    12(%rsp), %rdi
>         call    external
>         addq    $24, %rsp
>         .cfi_def_cfa_offset 8
>         ret
>         .cfi_endproc
> .LFE0:
>         .size   int1, .-int1
>         .p2align 4,,15
> 
> I know x86-64 best, so I'm going to explain the issue there.  Suppose that at the time of the call instruction, %rsp (the stack pointer) ends with …3000.  Then on function entry, %rsp == …3008.  Decrementing %rsp by 4152 results in …1FD0.  The 8-byte stack probe touches the bytes from …1FD0 to …1FD7.  This skips over the guard page at …2000 (which extends to …2FFF).
> 
> The other problem is that GCC does not take the implied stack probe by the call instruction into account, or other forms of linear stack access.  This leads to grossly inefficient code with -fstack-check.
> 
> I actually think that compiling with -fstack-check is a good idea, but GCC really needs fixes first.  We do not know the root cause of these issues (note the 3 * 4096 probe offset on POWER), and just throwing random workarounds into glibc does not seem right.
> 
> Thanks,
> Florian

Should also -fstack-check check for probe value underflow/overflow? The cases should be 
rare (if valid), but at least gcc has one bug report about it [1].

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66479
Florian Weimer March 20, 2017, 1:51 p.m. UTC | #12 
On 03/20/2017 02:44 PM, Adhemerval Zanella wrote:

> Should also -fstack-check check for probe value underflow/overflow? The cases should be
> rare (if valid), but at least gcc has one bug report about it [1].
>
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66479

I think what we want is a well-controlled page fault.  How we get there 
does not really matter.

Florian
Mike Frysinger March 20, 2017, 2:35 p.m. UTC | #13 
On 20 Mar 2017 14:26, Florian Weimer wrote:
> On 03/20/2017 10:04 AM, Mike Frysinger wrote:
> > On 20 Mar 2017 09:56, Florian Weimer wrote:
> >> On 03/16/2017 10:52 PM, Mike Frysinger wrote:
> >>> i picked 32 KiB as a trade off of it being more than big enough and
> >>> being a nice multiple of 4, 8, and 16, while not overtly allocating
> >>> too much otherwise unused.
> >>
> >> Depending on what constant page size GCC uses, this will not fix the
> >> -fstack-check bug on all architectures.
> >
> > do you know of one today ?  i grepped through the latest gcc source to
> > come up with the numbers that i used, and i think 32 KiB should cover
> > them all.
> 
> GCC stack probing assumes a 4 KiB stack size, which should be a 
> conservative assumption almost everywhere.  But the first probe is off 
> by a few bytes.  This seems to be a common issue across many target, 
> e.g. on GCC 6 for aarch64:
> 
> void external(void *);
> 
> void
> int1 (void)
> {
>    int a;
>    external (&a);
> }
> 
> 
>          .text
>          .align  2
>          .p2align 3,,7
>          .global int1
>          .type   int1, %function
> int1:
> .LFB0:
>          .cfi_startproc
>          sub     x9, sp, #8192
>          str     xzr, [x9, 4064]
>          stp     x29, x30, [sp, -32]!
>          .cfi_def_cfa_offset 32
>          .cfi_offset 29, -32
>          .cfi_offset 30, -24
>          add     x29, sp, 0
>          .cfi_def_cfa_register 29
>          add     x0, x29, 28
>          bl      external
>          ldp     x29, x30, [sp], 32
>          .cfi_restore 30
>          .cfi_restore 29
>          .cfi_def_cfa 31, 0
>          ret
>          .cfi_endproc
> .LFE0:
>          .size   int1, .-int1
> 
> Or ppc64le:
> 
>          .abiversion 2
>          .section        ".text"
>          .align 2
>          .p2align 4,,15
>          .globl int1
>          .type   int1, @function
> int1:
> .LCF0:
> 0:      addis 2,12,.TOC.-.LCF0@ha
>          addi 2,2,.TOC.-.LCF0@l
>          .localentry     int1,.-int1
>          mflr 0
>          std 0,-16432(1)
>          std 0,16(1)
>          stdu 1,-48(1)
>          addi 3,1,32
>          bl external
>          nop
>          addi 1,1,48
>          ld 0,16(1)
>          mtlr 0
>          blr
>          .long 0
>          .byte 0,0,0,1,128,0,0,0
>          .size   int1,.-int1
> 
> 
> Or x86_64:
> 
>          .text
>          .p2align 4,,15
>          .globl  int1
>          .type   int1, @function
> int1:
> .LFB0:
>          .cfi_startproc
>          subq    $4152, %rsp
>          orq     $0, (%rsp)
>          addq    $4128, %rsp
>          .cfi_def_cfa_offset 32
>          leaq    12(%rsp), %rdi
>          call    external
>          addq    $24, %rsp
>          .cfi_def_cfa_offset 8
>          ret
>          .cfi_endproc
> .LFE0:
>          .size   int1, .-int1
>          .p2align 4,,15
> 
> I know x86-64 best, so I'm going to explain the issue there.  Suppose 
> that at the time of the call instruction, %rsp (the stack pointer) ends 
> with …3000.  Then on function entry, %rsp == …3008.  Decrementing %rsp 
> by 4152 results in …1FD0.  The 8-byte stack probe touches the bytes from 
> …1FD0 to …1FD7.  This skips over the guard page at …2000 (which extends 
> to …2FFF).
> 
> The other problem is that GCC does not take the implied stack probe by 
> the call instruction into account, or other forms of linear stack 
> access.  This leads to grossly inefficient code with -fstack-check.
> 
> I actually think that compiling with -fstack-check is a good idea, but 
> GCC really needs fixes first.  We do not know the root cause of these 
> issues (note the 3 * 4096 probe offset on POWER), and just throwing 
> random workarounds into glibc does not seem right.

i don't think this qualifies as random workarounds.  we know gcc is
going to be probing the stack, by design, past the edge.  atm, we only
give it 512 bytes of breathing room.  regardless of the efficiency or
specific distance, 512 is never going to be enough.  therefore, glibc
is broken.  now we have to figure out how much slack will gcc want to
see w/out triggering a false positive.  based on my research of the
gcc tree, 32 KiB gets us enough space.  yes, there might be some fudge
factor in there, but it isn't like adding only 4 KiB would ever be the
correct size.
-mike
diff mbox

Patch

diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
index bb3eecfde1a5..e206f069bc5a 100644
--- a/sysdeps/unix/sysv/linux/spawni.c
+++ b/sysdeps/unix/sysv/linux/spawni.c
@@ -329,7 +329,9 @@  __spawnix (pid_t * pid, const char *file,
 
   /* Add a slack area for child's stack.  */
   size_t argv_size = (argc * sizeof (void *)) + 512;
-  size_t stack_size = ALIGN_UP (argv_size, GLRO(dl_pagesize));
+  /* We need at least a few pages in case the compiler's stack checking is
+     enabled.  In some configs, it is known to use at least 24KiB.  */
+  size_t stack_size = ALIGN_UP (argv_size, 32 * 1024);
   void *stack = __mmap (NULL, stack_size, prot,
 			MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0);
   if (__glibc_unlikely (stack == MAP_FAILED))