diff mbox

[08/12] De-PLTize __stack_chk_fail internal calls within libc.so.

Message ID 20161128123228.30856-9-nix@esperi.org.uk
State New
Headers show

Commit Message

Nix Nov. 28, 2016, 12:32 p.m. UTC 
From: Adhemerval Zanella <adhemerval.zanella@linaro.org>


We use the same assembler-macro trick we use to de-PLTize
compiler-generated libcalls to memcpy and memset to redirect
__stack_chk_fail to __stack_chk_fail_local.

v5: New.
v6: Only do it within the shared library: with __stack_chk_fail_local
    in libc_pic.a now we don't need to worry about calls from inside
    other routines in libc_nonshared.a any more.
v8: Merge #ifdef blocks.

	* sysdeps/generic/symbol-hacks.h (__stack_chk_fail): Add internal
	alias.
---
 sysdeps/generic/symbol-hacks.h | 4 ++++
 1 file changed, 4 insertions(+)

-- 
2.10.1.208.gbec66bc

Comments

Florian Weimer Dec. 15, 2016, 1:56 p.m. UTC | #1 
On 11/28/2016 01:32 PM, Nix wrote:
> From: Adhemerval Zanella <adhemerval.zanella@linaro.org>

>

> We use the same assembler-macro trick we use to de-PLTize

> compiler-generated libcalls to memcpy and memset to redirect

> __stack_chk_fail to __stack_chk_fail_local.

>

> v5: New.

> v6: Only do it within the shared library: with __stack_chk_fail_local

>     in libc_pic.a now we don't need to worry about calls from inside

>     other routines in libc_nonshared.a any more.

> v8: Merge #ifdef blocks.

>

> 	* sysdeps/generic/symbol-hacks.h (__stack_chk_fail): Add internal

> 	alias.

> ---

>  sysdeps/generic/symbol-hacks.h | 4 ++++

>  1 file changed, 4 insertions(+)

>

> diff --git a/sysdeps/generic/symbol-hacks.h b/sysdeps/generic/symbol-hacks.h

> index ce576c9..36908b5 100644

> --- a/sysdeps/generic/symbol-hacks.h

> +++ b/sysdeps/generic/symbol-hacks.h

> @@ -4,4 +4,8 @@

>  asm ("memmove = __GI_memmove");

>  asm ("memset = __GI_memset");

>  asm ("memcpy = __GI_memcpy");

> +

> +/* -fstack-protector generates calls to __stack_chk_fail, which need

> +   similar adjustments to avoid going through the PLT.  */

> +asm ("__stack_chk_fail = __stack_chk_fail_local");

>  #endif


We should do this only if we compile glibc with stack protector support 
enabled, and disable this for the files which we compile without stack 
protector.  I hope this will fix an assembler error while compiling 
__stack_chk_fail.c on ia64:

/tmp/ccCNZVJs.s:51: Error: `__stack_chk_fail' was not defined within 
procedure
/tmp/ccCNZVJs.s:51: Warning: `__stack_chk_fail#' was not specified with 
previous .proc
/tmp/ccCNZVJs.s:51: Warning: `__stack_chk_fail' should be an operand to 
this .endp

The .s file looks like this:

       1         .file   "stack_chk_fail.c"
       2         .pred.safe_across_calls p1-p5,p16-p63
       3         .text
       4 .Ltext0:
       5 #APP
       6         memmove = __GI_memmove
       7         memset = __GI_memset
       8         memcpy = __GI_memcpy
       9         __stack_chk_fail = __stack_chk_fail_local
      10         .section        .rodata.str1.8,"aMS",@progbits,1
      11         .align 8
      12 .LC0:
      13         stringz "stack smashing detected"
      14 #NO_APP
      15         .text
      16         .align 16
      17         .align 64
      18         .global __stack_chk_fail#
      19         .type   __stack_chk_fail#, @function
      20         .proc __stack_chk_fail#
      21 __stack_chk_fail:
      22 [.LFB33:]
      23         .file 1 "stack_chk_fail.c"
      24         .loc 1 27 0
      25         .prologue 12, 32
      26         .mib
      27         .save ar.pfs, r33
      28         alloc r33 = ar.pfs, 0, 3, 1, 0
      29 [.LCFI0:]
      30         .save rp, r32
      31         mov r32 = b0
      32 [.LCFI1:]
      33         .loc 1 28 0
      34         nop 0
      35         .mlx
      36         nop 0
      37         movl r35 = @gprel(.LC0)
      38         .loc 1 27 0
      39         .body
      40         .loc 1 28 0
      41         ;;
      42         .mib
      43         nop 0
      44         add r35 = r1, r35
      45         br.call.sptk.many b0 = __GI___fortify_fail
      46 [.LVL0:]
      47         ;;
      48         break.f 0
      49         ;;
      50 .LFE33:
      51         .endp __stack_chk_fail#

Thanks,
Florian
Florian Weimer Dec. 15, 2016, 2:21 p.m. UTC | #2 
On 12/15/2016 03:15 PM, Nix wrote:

> Possible fix, untested:

>

> diff --git a/sysdeps/generic/symbol-hacks.h b/sysdeps/generic/symbol-hacks.h

> index 36908b5..0679354 100644

> --- a/sysdeps/generic/symbol-hacks.h

> +++ b/sysdeps/generic/symbol-hacks.h

> @@ -7,5 +7,7 @@ asm ("memcpy = __GI_memcpy");

>

>  /* -fstack-protector generates calls to __stack_chk_fail, which need

>     similar adjustments to avoid going through the PLT.  */

> +#if defined __SSP__ || defined __SSP_ALL__ || defined __SSP_STRONG__

>  asm ("__stack_chk_fail = __stack_chk_fail_local");

>  #endif

> +#endif


The condition looks rather brittle.  What if GCC grows an 
-fstack-protector-light switch and __SSP_LIGHT__ macro?

I wonder if it's better to add something to $(no-stack-protector) and 
use that in the conditional.

Thanks,
Florian
Florian Weimer Dec. 15, 2016, 2:33 p.m. UTC | #3 
On 12/15/2016 03:29 PM, Nix wrote:
> On 15 Dec 2016, Florian Weimer told this:

>

>> On 12/15/2016 03:15 PM, Nix wrote:

>>

>>> Possible fix, untested:

>>>

>>> diff --git a/sysdeps/generic/symbol-hacks.h b/sysdeps/generic/symbol-hacks.h

>>> index 36908b5..0679354 100644

>>> --- a/sysdeps/generic/symbol-hacks.h

>>> +++ b/sysdeps/generic/symbol-hacks.h

>>> @@ -7,5 +7,7 @@ asm ("memcpy = __GI_memcpy");

>>>

>>>  /* -fstack-protector generates calls to __stack_chk_fail, which need

>>>     similar adjustments to avoid going through the PLT.  */

>>> +#if defined __SSP__ || defined __SSP_ALL__ || defined __SSP_STRONG__

>>>  asm ("__stack_chk_fail = __stack_chk_fail_local");

>>>  #endif

>>> +#endif

>>

>> The condition looks rather brittle.  What if GCC grows an -fstack-protector-light switch and __SSP_LIGHT__ macro?

>

> We'd need to change configure.ac before that would have an effect in any

> case... but it does seem likely that changing this too would be

> overlooked.


Right.

>> I wonder if it's better to add something to $(no-stack-protector) and use that in the conditional.

>

> That was my other option, but the total absence of anything in

> configure.ac passing -D made me think twice.

>

> Something like this? (even more untested than the last one, if

> possible -- but adds a new possibility: we can now differentiate between

> "glibc built without stack protector" and "glibc built with stack

> protector but this file doesn't have it" without relying on GCC

> predefined macros. The __WITH_ naming scheme is completely arbitrary

> and I can change it to anything you prefer.)


WITH_STACK_PROTECTOR (without the leading underscores) looks okay to me 
because it's only used at build time.  Or you could call it 
STACK_PROTECTOR_LEVEL, to match the other variable.

> diff --git a/configure.ac b/configure.ac

> index 2396c1f..8bb8c2c 100644

> --- a/configure.ac

> +++ b/configure.ac

> @@ -638,18 +638,18 @@ LIBC_TRY_CC_OPTION([$CFLAGS $CPPFLAGS -Werror -fstack-protector-all],

>  stack_protector=

>  no_stack_protector=

>  if test "$libc_cv_ssp" = yes; then

> -  no_stack_protector="-fno-stack-protector"

> +  no_stack_protector="-fno-stack-protector -D__WITH_STACK_PROTECTOR=0"

>    AC_DEFINE(HAVE_CC_NO_STACK_PROTECTOR)

>  fi

>

>  if test "$enable_stack_protector" = yes && test "$libc_cv_ssp" = yes; then

> -  stack_protector="-fstack-protector"

> +  stack_protector="-fstack-protector -D__WITH_STACK_PROTECTOR=1"

>    AC_DEFINE(STACK_PROTECTOR_LEVEL, 1)

>  elif test "$enable_stack_protector" = all && test "$libc_cv_ssp_all" = yes; then

> -  stack_protector="-fstack-protector-all"

> +  stack_protector="-fstack-protector-all -D__WITH_STACK_PROTECTOR=2"

>    AC_DEFINE(STACK_PROTECTOR_LEVEL, 2)

>  elif test "$enable_stack_protector" = strong && test "$libc_cv_ssp_strong" = yes; then

> -  stack_protector="-fstack-protector-strong"

> +  stack_protector="-fstack-protector-strong -D__WITH_STACK_PROTECTOR=3"

>    AC_DEFINE(STACK_PROTECTOR_LEVEL, 3)

>  fi

>  AC_SUBST(libc_cv_ssp)

> diff --git a/sysdeps/generic/symbol-hacks.h b/sysdeps/generic/symbol-hacks.h

> index 36908b5..12b4fe7 100644

> --- a/sysdeps/generic/symbol-hacks.h

> +++ b/sysdeps/generic/symbol-hacks.h

> @@ -7,5 +7,7 @@ asm ("memcpy = __GI_memcpy");

>

>  /* -fstack-protector generates calls to __stack_chk_fail, which need

>     similar adjustments to avoid going through the PLT.  */

> +#if defined __WITH_STACK_PROTECTOR && __WITH_STACK_PROTECTOR > 0

>  asm ("__stack_chk_fail = __stack_chk_fail_local");

>  #endif

> +#endif


The new #if/#endif need to be indented.

Thanks,
Florian
Nix Dec. 15, 2016, 5:24 p.m. UTC | #4 
On 15 Dec 2016, nix@esperi.org.uk verbalised:

> diff --git a/sysdeps/generic/symbol-hacks.h b/sysdeps/generic/symbol-hacks.h

> index 36908b5..15ff56a 100644

> --- a/sysdeps/generic/symbol-hacks.h

> +++ b/sysdeps/generic/symbol-hacks.h

> @@ -7,5 +7,7 @@ asm ("memcpy = __GI_memcpy");

>  

>  /* -fstack-protector generates calls to __stack_chk_fail, which need

>     similar adjustments to avoid going through the PLT.  */

> +# if defined STACK_PROTECTOR_LEVEL && STACK_PROTECTOR_LEVEL > 0

>  asm ("__stack_chk_fail = __stack_chk_fail_local");

> +# endif

>  #endif


This causes (minor) problems on SPARC:

Extra PLT reference: libc.so: __stack_chk_fail

Whether we can disregard this, I don't know, but it does feel wrong.
Florian Weimer Dec. 15, 2016, 6:22 p.m. UTC | #5 
On 12/15/2016 06:24 PM, Nix wrote:
> On 15 Dec 2016, nix@esperi.org.uk verbalised:

>

>> diff --git a/sysdeps/generic/symbol-hacks.h b/sysdeps/generic/symbol-hacks.h

>> index 36908b5..15ff56a 100644

>> --- a/sysdeps/generic/symbol-hacks.h

>> +++ b/sysdeps/generic/symbol-hacks.h

>> @@ -7,5 +7,7 @@ asm ("memcpy = __GI_memcpy");

>>

>>  /* -fstack-protector generates calls to __stack_chk_fail, which need

>>     similar adjustments to avoid going through the PLT.  */

>> +# if defined STACK_PROTECTOR_LEVEL && STACK_PROTECTOR_LEVEL > 0

>>  asm ("__stack_chk_fail = __stack_chk_fail_local");

>> +# endif

>>  #endif

>

> This causes (minor) problems on SPARC:

>

> Extra PLT reference: libc.so: __stack_chk_fail

>

> Whether we can disregard this, I don't know, but it does feel wrong.


Well, avoiding this is the point of __stack_chk_fail_local, isn't it? 
So we surely can't ignore it.

I think what is going on is that once a symbol has a hidden anywhere in 
a static link, all references to it are turned hidden.  Previously, this 
hidden reference occurred in the file which *defined* 
__stack_chk_fail_local, but is now gone from there.  (At the assembler 
level, the .hidden directive is markedly different from the GCC 
visibility attribute.)

Could you try this?

# if defined STACK_PROTECTOR_LEVEL && STACK_PROTECTOR_LEVEL > 0
asm (".hidden __stack_chk_fail_local");
asm ("__stack_chk_fail = __stack_chk_fail_local");
# endif

Thanks,
Florian
Nix Dec. 15, 2016, 8 p.m. UTC | #6 
On 15 Dec 2016, Florian Weimer verbalised:

> On 12/15/2016 06:24 PM, Nix wrote:

>> This causes (minor) problems on SPARC:

>>

>> Extra PLT reference: libc.so: __stack_chk_fail

>>

>> Whether we can disregard this, I don't know, but it does feel wrong.

>

> Well, avoiding this is the point of __stack_chk_fail_local, isn't it?

> So we surely can't ignore it.


Agreed. (I just wasn't sure I could remember what this was added for...)

> I think what is going on is that once a symbol has a hidden anywhere

> in a static link, all references to it are turned hidden. Previously,

> this hidden reference occurred in the file which *defined*

> __stack_chk_fail_local, but is now gone from there. (At the assembler


Oof. And with no such reference, it ends up visible again...

> Could you try this?

>

> # if defined STACK_PROTECTOR_LEVEL && STACK_PROTECTOR_LEVEL > 0

> asm (".hidden __stack_chk_fail_local");

> asm ("__stack_chk_fail = __stack_chk_fail_local");

> # endif


No change :( the only reference to __stack_chk_fail is still inside
stack_chk_fail_local:

Symbols from libc_pic.a[libc-stack_chk_fail_local.os]:

Name                        Value            Class  Type     Size             Line Section

__GI_memcpy                ||GLOBAL|NOTYPE  ||    |UNDEF
__GI_memmove               ||GLOBAL|NOTYPE  ||    |UNDEF
__GI_memset                ||GLOBAL|NOTYPE  ||    |UNDEF
__stack_chk_fail           ||GLOBAL|NOTYPE  ||    |UNDEF
__stack_chk_fail_local     |0000000000000000|GLOBAL|FUNC    |0000000000000010|    |.text
libc-stack_chk_fail_local.c|0000000000000000|LOCAL |FILE    |0000000000000000|    |ABS

(And, of course, this code is not affected by your suggestion, because
it's compiled with -fno-stack-protector -DSTACK_PROTECTOR_LEVEL=0.)

-- 
NULL && (void)
Florian Weimer Dec. 15, 2016, 8:22 p.m. UTC | #7 
On 12/15/2016 09:00 PM, Nix wrote:

>> Could you try this?

>>

>> # if defined STACK_PROTECTOR_LEVEL && STACK_PROTECTOR_LEVEL > 0

>> asm (".hidden __stack_chk_fail_local");

>> asm ("__stack_chk_fail = __stack_chk_fail_local");

>> # endif

>

> No change :( the only reference to __stack_chk_fail is still inside

> stack_chk_fail_local:

>

> Symbols from libc_pic.a[libc-stack_chk_fail_local.os]:

>

> Name                        Value            Class  Type     Size             Line Section

>

> __GI_memcpy                ||GLOBAL|NOTYPE  ||    |UNDEF

> __GI_memmove               ||GLOBAL|NOTYPE  ||    |UNDEF

> __GI_memset                ||GLOBAL|NOTYPE  ||    |UNDEF

> __stack_chk_fail           ||GLOBAL|NOTYPE  ||    |UNDEF

> __stack_chk_fail_local     |0000000000000000|GLOBAL|FUNC    |0000000000000010|    |.text

> libc-stack_chk_fail_local.c|0000000000000000|LOCAL |FILE    |0000000000000000|    |ABS

>

> (And, of course, this code is not affected by your suggestion, because

> it's compiled with -fno-stack-protector -DSTACK_PROTECTOR_LEVEL=0.)


I think this attempt at PLT avoidance within libc.so itself is subtly 
wrong.  We need to mirror more closely what 
libc_hidden_proto/libc_hidden_def does, and perhaps disentangle this 
from the __stack_chk_fail_local definition used in other DSOs.

I think this means removing any definition of a C function definition 
called __stack_chk_fail_local from libc.so, and instead use a strong 
alias from __stack_chk_fail to __stack_chk_fail_local to define the 
symbol.  The alias will not incorporate a PLT reference.  If you look at 
include/libc-symbols.h, strong_alias and hidden_def are quite similar.

It's too late for me to try this today. :-/

Florian
Florian Weimer Dec. 15, 2016, 8:44 p.m. UTC | #8 
* Florian Weimer:

> On 12/15/2016 09:00 PM, Nix wrote:

>

>>> Could you try this?

>>>

>>> # if defined STACK_PROTECTOR_LEVEL && STACK_PROTECTOR_LEVEL > 0

>>> asm (".hidden __stack_chk_fail_local");

>>> asm ("__stack_chk_fail = __stack_chk_fail_local");

>>> # endif

>>

>> No change :( the only reference to __stack_chk_fail is still inside

>> stack_chk_fail_local:

>>

>> Symbols from libc_pic.a[libc-stack_chk_fail_local.os]:

>>

>> Name Value Class Type Size Line Section

>>

>> __GI_memcpy                ||GLOBAL|NOTYPE  ||    |UNDEF

>> __GI_memmove               ||GLOBAL|NOTYPE  ||    |UNDEF

>> __GI_memset                ||GLOBAL|NOTYPE  ||    |UNDEF

>> __stack_chk_fail           ||GLOBAL|NOTYPE  ||    |UNDEF

>> __stack_chk_fail_local |0000000000000000|GLOBAL|FUNC

>> |0000000000000010| |.text

>> libc-stack_chk_fail_local.c|0000000000000000|LOCAL |FILE

>> |0000000000000000| |ABS

>>

>> (And, of course, this code is not affected by your suggestion, because

>> it's compiled with -fno-stack-protector -DSTACK_PROTECTOR_LEVEL=0.)

>

> I think this attempt at PLT avoidance within libc.so itself is subtly 

> wrong.  We need to mirror more closely what 

> libc_hidden_proto/libc_hidden_def does, and perhaps disentangle this 

> from the __stack_chk_fail_local definition used in other DSOs.

>

> I think this means removing any definition of a C function definition 

> called __stack_chk_fail_local from libc.so, and instead use a strong 

> alias from __stack_chk_fail to __stack_chk_fail_local to define the 

> symbol.  The alias will not incorporate a PLT reference.  If you look at 

> include/libc-symbols.h, strong_alias and hidden_def are quite similar.


It may also be a good idea to switch to a different symbol for
__stack_chk_fail_local because this collides with the name GCC uses on
some architectures for a similar purpose.  Or is this the intent here?
Nix Dec. 15, 2016, 8:49 p.m. UTC | #9 
On 15 Dec 2016, Florian Weimer spake thusly:

> * Florian Weimer:

>

>> I think this means removing any definition of a C function definition 

>> called __stack_chk_fail_local from libc.so, and instead use a strong 

>> alias from __stack_chk_fail to __stack_chk_fail_local to define the 

>> symbol.  The alias will not incorporate a PLT reference.  If you look at 

>> include/libc-symbols.h, strong_alias and hidden_def are quite similar.

>

> It may also be a good idea to switch to a different symbol for

> __stack_chk_fail_local because this collides with the name GCC uses on

> some architectures for a similar purpose.  Or is this the intent here?


That's the point. On targets where __stack_chk_fail_local is called
(e.g. x86-32), we're not generating the calls to this thing: GCC is.
We cannot pick a different name.

-- 
NULL && (void)
Florian Weimer Dec. 15, 2016, 8:58 p.m. UTC | #10 
> On 15 Dec 2016, Florian Weimer spake thusly:

>

>> * Florian Weimer:

>>

>>> I think this means removing any definition of a C function definition 

>>> called __stack_chk_fail_local from libc.so, and instead use a strong 

>>> alias from __stack_chk_fail to __stack_chk_fail_local to define the 

>>> symbol.  The alias will not incorporate a PLT reference.  If you look at 

>>> include/libc-symbols.h, strong_alias and hidden_def are quite similar.

>>

>> It may also be a good idea to switch to a different symbol for

>> __stack_chk_fail_local because this collides with the name GCC uses on

>> some architectures for a similar purpose.  Or is this the intent here?

>

> That's the point. On targets where __stack_chk_fail_local is called

> (e.g. x86-32), we're not generating the calls to this thing: GCC is.

> We cannot pick a different name.


Ahh, and GCC does not synthesize a local definition, so there is no
actual collision.

Then the strong_alias approach should work for libc.so.
libc_nonshared.a still needs a hidden function definition, of course.
diff mbox

Patch

diff --git a/sysdeps/generic/symbol-hacks.h b/sysdeps/generic/symbol-hacks.h
index ce576c9..36908b5 100644
--- a/sysdeps/generic/symbol-hacks.h
+++ b/sysdeps/generic/symbol-hacks.h
@@ -4,4 +4,8 @@ 
 asm ("memmove = __GI_memmove");
 asm ("memset = __GI_memset");
 asm ("memcpy = __GI_memcpy");
+
+/* -fstack-protector generates calls to __stack_chk_fail, which need
+   similar adjustments to avoid going through the PLT.  */
+asm ("__stack_chk_fail = __stack_chk_fail_local");
 #endif