Message ID | 87lgvhjlx1.fsf@esperi.org.uk |
---|---|
State | Superseded |
Headers |
Received: (qmail 4453 invoked by alias); 15 Dec 2016 14:40:26 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 3769 invoked by uid 89); 15 Dec 2016 14:40:26 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.6 required=5.0 tests=AWL, BAYES_00, KAM_LAZY_DOMAIN_SECURITY, RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=(unknown) X-HELO: mail.esperi.org.uk From: Nix <nix@esperi.org.uk> To: Florian Weimer <fweimer@redhat.com> Cc: libc-alpha@sourceware.org, Adhemerval Zanella <adhemerval.zanella@linaro.org> Subject: Re: [PATCH 08/12] De-PLTize __stack_chk_fail internal calls within libc.so. References: <20161128123228.30856-1-nix@esperi.org.uk> <20161128123228.30856-9-nix@esperi.org.uk> <fbc21429-e5cd-1771-6165-ae5202511223@redhat.com> <87y3zhjn1s.fsf@esperi.org.uk> <78b1f109-91e5-4150-4c00-15a86aacb2f7@redhat.com> <87poktjmfn.fsf@esperi.org.uk> <b897224d-0522-8361-0712-082004c325f5@redhat.com> Date: Thu, 15 Dec 2016 14:40:10 +0000 In-Reply-To: <b897224d-0522-8361-0712-082004c325f5@redhat.com> (Florian Weimer's message of "Thu, 15 Dec 2016 15:33:16 +0100") Message-ID: <87lgvhjlx1.fsf@esperi.org.uk> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.0.94 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-DCC--Metrics: spindle 1480; Body=3 Fuz1=3 Fuz2=3 |
Commit Message
Nix
Dec. 15, 2016, 2:40 p.m. UTC
On 15 Dec 2016, Florian Weimer uttered the following: > On 12/15/2016 03:29 PM, Nix wrote: >> On 15 Dec 2016, Florian Weimer told this: >>> 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. That's definitely the best choice, because it means we don't need to change configure.ac as much, and we don't have a confusing mix of nonzero STACK_PROTECTOR_LEVEL and zero WITH_STACK_PROTECTOR (or whatever) in some files. It needs a tiny adjustment of config.h.in to stop it being overridden though. Something like this:
Comments
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