Message ID | 20190210232910.22652-1-slyfox@gentoo.org |
---|---|
State | Committed |
Headers |
Received: (qmail 46266 invoked by alias); 10 Feb 2019 23:29:36 -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 46257 invoked by uid 89); 10 Feb 2019 23:29:36 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 spammy=James, Hx-languages-length:1294, Myers, myers X-HELO: smtp.gentoo.org From: Sergei Trofimovich <slyfox@gentoo.org> To: libc-alpha@sourceware.org Cc: Andreas Schwab <schwab@suse.de>, Sergei Trofimovich <slyfox@gentoo.org>, James Le Cuirot <chewi@gentoo.org>, Andreas Schwab <schwab@linux-m68k.org> Subject: [PATCH] m68k: fix clobbering a5 in setjmp() [BZ #24202] Date: Sun, 10 Feb 2019 23:29:10 +0000 Message-Id: <20190210232910.22652-1-slyfox@gentoo.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit |
Commit Message
Sergei Trofimovich
Feb. 10, 2019, 11:29 p.m. UTC
setjmp() uses C code to store current registers into jmp_buf
environment. -fstack-protector-all places canary into setjmp()
prologue and clobbers 'a5' before it gets saved.
The change inhibits stack canary injection to avoid clobber.
[BZ #24202]
* sysdeps/m68k/setjmp.c (*setjmp): Use
inhibit_stack_protector.
CC: James Le Cuirot <chewi@gentoo.org>
CC: Andreas Schwab <schwab@linux-m68k.org>
Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org>
---
ChangeLog | 6 ++++++
sysdeps/m68k/setjmp.c | 1 +
2 files changed, 7 insertions(+)
Comments
On 10/02/2019 21:29, Sergei Trofimovich wrote: > setjmp() uses C code to store current registers into jmp_buf > environment. -fstack-protector-all places canary into setjmp() > prologue and clobbers 'a5' before it gets saved. > > The change inhibits stack canary injection to avoid clobber. > > [BZ #24202] > * sysdeps/m68k/setjmp.c (*setjmp): Use > inhibit_stack_protector. LGTM. I am not seeing the stack smash issue with example provided in BZ#24202 in my environment (gcc 6.2.1, Aranym2015Jan on 3.16.0-4-m68k), however the fix shows the expected printed value. > > CC: James Le Cuirot <chewi@gentoo.org> > CC: Andreas Schwab <schwab@linux-m68k.org> > Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org> > --- > ChangeLog | 6 ++++++ > sysdeps/m68k/setjmp.c | 1 + > 2 files changed, 7 insertions(+) > > diff --git a/ChangeLog b/ChangeLog > index c143073ca7..c1e8dd9c3a 100644 > --- a/ChangeLog > +++ b/ChangeLog > @@ -1,3 +1,9 @@ > +2019-02-10 Sergei Trofimovich <slyfox@gentoo.org> > + > + [BZ #24202] > + * sysdeps/m68k/setjmp.c (*setjmp): Use > + inhibit_stack_protector. > + > 2019-02-06 Joseph Myers <joseph@codesourcery.com> > > * elf/dl-load.h (_dl_postprocess_loadcmd): Use __always_inline > diff --git a/sysdeps/m68k/setjmp.c b/sysdeps/m68k/setjmp.c > index 39ab7178a0..62bd281119 100644 > --- a/sysdeps/m68k/setjmp.c > +++ b/sysdeps/m68k/setjmp.c > @@ -19,6 +19,7 @@ > > /* Save the current program position in ENV and return 0. */ > int > +inhibit_stack_protector > #if defined BSD_SETJMP > # undef setjmp > # define savemask 1 >
On 10 Feb 2019, Sergei Trofimovich uttered the following: > setjmp() uses C code to store current registers into jmp_buf > environment. -fstack-protector-all places canary into setjmp() > prologue and clobbers 'a5' before it gets saved. > > The change inhibits stack canary injection to avoid clobber. LGTM. I tried to identify everything that depended on clobbering no regs and inhibit stack-protection in all of them, but clearly failed to spot several setjmp implementations. Sorry!
* Sergei Trofimovich: > setjmp() uses C code to store current registers into jmp_buf > environment. -fstack-protector-all places canary into setjmp() > prologue and clobbers 'a5' before it gets saved. > > The change inhibits stack canary injection to avoid clobber. > > [BZ #24202] > * sysdeps/m68k/setjmp.c (*setjmp): Use > inhibit_stack_protector. The code is still invalid. The C compiler can still clobber any register it wants. So this does not actually fix the bug. But I think it's an incremental improvement. Thanks, Florian
On 24 Feb 2019, Florian Weimer uttered the following: >> setjmp() uses C code to store current registers into jmp_buf >> environment. -fstack-protector-all places canary into setjmp() >> prologue and clobbers 'a5' before it gets saved. >> >> The change inhibits stack canary injection to avoid clobber. >> >> [BZ #24202] >> * sysdeps/m68k/setjmp.c (*setjmp): Use >> inhibit_stack_protector. > > The code is still invalid. The C compiler can still clobber any > register it wants. So this does not actually fix the bug. But I think > it's an incremental improvement. I'd say it's in the same state as pthread_cond_wait() et al were in on i386 before 7a25d6a84df9fea56963569ceccaaf7c2a88f161: works with the compiler as it is now, by sheer good fortune. To me this feels like code that can't really be written in C safely, but it's so arch-dependent that having it depend on the compiler not emitting anything else in the prologue is probably not *too* likely to break. But we should probably add the reproducer for this to glibc's 'make check' to stop it regressing again.
On 2/11/19 4:59 AM, Sergei Trofimovich wrote: > setjmp() uses C code to store current registers into jmp_buf > environment. -fstack-protector-all places canary into setjmp() > prologue and clobbers 'a5' before it gets saved. > > The change inhibits stack canary injection to avoid clobber. > I've dropped the Signed-off-by and pushed this since it had approvals but wasn't committed, probably due to oversight. Thanks, Siddhesh
diff --git a/ChangeLog b/ChangeLog index c143073ca7..c1e8dd9c3a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +2019-02-10 Sergei Trofimovich <slyfox@gentoo.org> + + [BZ #24202] + * sysdeps/m68k/setjmp.c (*setjmp): Use + inhibit_stack_protector. + 2019-02-06 Joseph Myers <joseph@codesourcery.com> * elf/dl-load.h (_dl_postprocess_loadcmd): Use __always_inline diff --git a/sysdeps/m68k/setjmp.c b/sysdeps/m68k/setjmp.c index 39ab7178a0..62bd281119 100644 --- a/sysdeps/m68k/setjmp.c +++ b/sysdeps/m68k/setjmp.c @@ -19,6 +19,7 @@ /* Save the current program position in ENV and return 0. */ int +inhibit_stack_protector #if defined BSD_SETJMP # undef setjmp # define savemask 1