Bug 16629 - [aarch64] setcontext clobbers alternate signal stack
Summary: [aarch64] setcontext clobbers alternate signal stack
Status: RESOLVED FIXED
Alias: None
Product: glibc
Classification: Unclassified
Component: libc (show other bugs)
Version: 2.19
: P2 normal
Target Milestone: 2.20
Assignee: Will Newton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-24 03:09 UTC by Michael Hudson-Doyle
Modified: 2014-06-13 06:48 UTC (History)
5 users (show)

See Also:
Host: aarch64
Target:
Build:
Last reconfirmed:
fweimer: security-


Attachments
adapted from example in http://pubs.opengroup.org/onlinepubs/009695399/functions/makecontext.html (374 bytes, text/x-csrc)
2014-02-24 03:09 UTC, Michael Hudson-Doyle
Details
Updated proposed patch (2.50 KB, patch)
2014-03-24 11:15 UTC, Will Newton
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Hudson-Doyle 2014-02-24 03:09:25 UTC
Created attachment 7435 [details]
adapted from example in http://pubs.opengroup.org/onlinepubs/009695399/functions/makecontext.html

I'm attaching a simple demo program for makecontext/swapcontext that I found somewhere with the addition of code to print the alternate signal stack before and after calling swapcontext.  On aarch64 it prints this:

start f2
start f1
finish f2
finish f1
{ss_sp: (nil), ss_flags: 2, ss_size: 0}
{ss_sp: 0x7ffbe931f8, ss_flags: 0, ss_size: 8192}

It turns out that because setcontext is implemented in terms of the rt_sigreturn syscall it ends up copying the uc_stack data from the passed context into the (kernel) task's sigaltstack parameters.  Hilarity ensues.  Specifically it means that programs linked against the gccgo runtime sometimes handle signals with SP pointing at memory that another thread is using for its stack, with predictably bad results.
Comment 1 Ryan S. Arnold 2014-03-21 15:51:36 UTC
Will has proposed a patch in the following mailing list thread:

https://sourceware.org/ml/libc-alpha/2014-03/msg00295.html
Comment 2 Michael Hudson-Doyle 2014-03-22 11:36:57 UTC
Hi, I'm on leave and don't really have time to dig into it, but applying the linked patch in isolation breaks the go runtime rather badly:

(t-mwhudson)mwhudson@am1:~/juju-core-1.17.2$ go
fatal error: runtime: mcall function returned

This error message is from http://gcc.gnu.org/viewcvs/gcc/trunk/libgo/runtime/proc.c?revision=208390&view=markup#l308 and implies that the call to setcontext returned normally (which it should not do, of course).

I may have screwed up applying the patch of course.
Comment 3 Will Newton 2014-03-24 11:14:05 UTC
Hi Michael,

Looks like you found the deliberate mistake in my patch! The updated attached patch fixes that issue and I would be grateful if you could test it when you have time.

Thanks,
Comment 4 Will Newton 2014-03-24 11:15:16 UTC
Created attachment 7489 [details]
Updated proposed patch
Comment 5 Michael Hudson-Doyle 2014-03-31 01:28:53 UTC
This patch does appear to fix the crash with the go runtime.  Thanks!
Comment 6 cvs-commit@gcc.gnu.org 2014-04-17 10:41:41 UTC
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU C Library master sources".

The branch, master has been updated
       via  7c6776620db8e48fca492dbcac88d1c0f239dcde (commit)
       via  e04a4e9d2e639a7770e1c0d24ecbcf92abf6bba8 (commit)
       via  37d350073888887637aa67dddf988d9c4b226032 (commit)
      from  423a7160af7fcffc61aac5e2e36d0b6b5b083214 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=7c6776620db8e48fca492dbcac88d1c0f239dcde

commit 7c6776620db8e48fca492dbcac88d1c0f239dcde
Author: Will Newton <will.newton@linaro.org>
Date:   Thu Mar 13 09:45:29 2014 +0000

    manual/setjmp.texi: Clarify setcontext and signal handlers text
    
    Calling setcontext from a signal handler can be done safely so
    it is sufficient to note that it is not recommended.
    
    Also mention in setcontext documentation that the behaviour of
    setcontext when restoring a context created by a call to a signal
    handler is unspecified.
    
    2014-04-17  Will Newton  <will.newton@linaro.org>
    
    	* manual/setjmp.texi (System V contexts): Add note that
    	calling setcontext on a context created by a call to a
    	signal handler is undefined.  Update text to note that
    	setcontext from a signal handler is possible but not
    	recommended.

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=e04a4e9d2e639a7770e1c0d24ecbcf92abf6bba8

commit e04a4e9d2e639a7770e1c0d24ecbcf92abf6bba8
Author: Will Newton <will.newton@linaro.org>
Date:   Tue Feb 25 14:29:32 2014 +0000

    stdlib/tst-setcontext.c: Check for clobbering of signal stack
    
    On aarch64 calling swapcontext clobbers the state of the signal
    stack (BZ #16629). Check that the address and size of the signal
    stack before and after the call to swapcontext remains the same.
    
    ChangeLog:
    
    2014-04-17  Will Newton  <will.newton@linaro.org>
    
    	[BZ #16629]
    	* stdlib/tst-setcontext.c: Include signal.h.
    	(main): Check that the signal stack before and
    	after swapcontext is the same.

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=37d350073888887637aa67dddf988d9c4b226032

commit 37d350073888887637aa67dddf988d9c4b226032
Author: Will Newton <will.newton@linaro.org>
Date:   Wed Mar 12 16:14:51 2014 +0000

    aarch64: Re-implement setcontext without rt_sigreturn syscall
    
    The current implementation of setcontext uses rt_sigreturn to restore
    the contents of registers. This contrasts with the way most other
    architectures implement setcontext:
    
      powerpc64, mips, tile:
    
      Call rt_sigreturn if context was created by a call to a signal handler,
      otherwise restore in user code.
    
      powerpc32:
    
      Call swapcontext system call and don't call sigreturn or rt_sigreturn.
    
      x86_64, sparc, hppa, sh, ia64, m68k, s390, arm:
    
      Only support restoring "synchronous" contexts, that is contexts
      created by getcontext, and restoring in user code and don't call
      sigreturn or rt_sigreturn.
    
      alpha:
    
      Call sigreturn (but not rt_sigreturn) in all cases to do the restore.
    
    The text of the setcontext manpage suggests that the requirement to be
    able to restore a signal handler created context has been dropped from
    SUSv2:
    
      If  the context was obtained by a call to a signal handler, then old
      standard text says that "program execution continues with the program
      instruction following the instruction interrupted by the signal".
      However, this sentence was removed in SUSv2, and the present verdict
      is "the result is unspecified".
    
    Implementing setcontext by calling rt_sigreturn unconditionally causes
    problems when used with sigaltstack as in BZ #16629. On this basis it
    seems that aarch64 is broken and that new ports should only support
    restoring contexts created with getcontext and do not need to call
    rt_sigreturn at all.
    
    This patch re-implements the aarch64 setcontext function to restore
    the context in user code in a similar manner to x86_64 and other ports.
    
    ChangeLog:
    
    2014-04-17  Will Newton  <will.newton@linaro.org>
    
    	[BZ #16629]
    	* sysdeps/unix/sysv/linux/aarch64/setcontext.S (__setcontext):
    	Re-implement to restore registers in user code and avoid
    	rt_sigreturn system call.

-----------------------------------------------------------------------

Summary of changes:
 ChangeLog                                    |   17 +++
 NEWS                                         |   10 +-
 manual/setjmp.texi                           |   19 ++--
 stdlib/tst-setcontext.c                      |   21 ++++
 sysdeps/unix/sysv/linux/aarch64/setcontext.S |  150 ++++++++++++++++----------
 5 files changed, 149 insertions(+), 68 deletions(-)
Comment 7 Will Newton 2014-04-17 10:45:57 UTC
Fixed in 37d350073888887637aa67dddf988d9c4b226032.