Message ID | 87lgvhjlx1.fsf@esperi.org.uk |
---|---|
State | New |
Headers | show |
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.
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
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.)
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: > 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?
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.
> 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 --git a/config.h.in b/config.h.in index 610fa49..82f95a6 100644 --- a/config.h.in +++ b/config.h.in @@ -52,8 +52,11 @@ __attribute__ ((__optimize__)). */ #undef HAVE_CC_NO_STACK_PROTECTOR -/* The level of stack protection in use for glibc as a whole. */ +/* The level of stack protection in use for glibc as a whole. + May be overridden on a file-by-file basis. */ +#ifndef STACK_PROTECTOR_LEVEL #undef STACK_PROTECTOR_LEVEL +#endif /* Define if the regparm attribute shall be used for local functions (gcc on ix86 only). */ diff --git a/configure.ac b/configure.ac index 2396c1f..a420428 100644 --- a/configure.ac +++ b/configure.ac @@ -638,7 +638,7 @@ 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 -DSTACK_PROTECTOR_LEVEL=0" AC_DEFINE(HAVE_CC_NO_STACK_PROTECTOR) fi 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