Message ID | 20190210232910.22652-1-slyfox@gentoo.org |
---|---|
State | New |
Headers | show |
Series | m68k: fix clobbering a5 in setjmp() [BZ #24202] | expand |
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
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(+)