Fix sigaltstack semantics.

Walking off the bottom of the sigaltstack, for example with recursive faults,
results in forced signal delivery, not resetting the stack or pushing signal
stack to whatever happens to lie below the signal stack.

PiperOrigin-RevId: 199856085
Change-Id: I0004d2523f0df35d18714de2685b3eaa147837e0

5 files changed, 70 insertions, 35 deletions

diff --git a/pkg/sentry/arch/arch.go b/pkg/sentry/arch/arch.go
index 021789e4b..0189e958d 100644
--- a/pkg/sentry/arch/arch.go
+++ b/pkg/sentry/arch/arch.go
@@ -158,7 +158,7 @@ type Context interface {
 	// rt is true if SignalRestore is being entered from rt_sigreturn and
 	// false if SignalRestore is being entered from sigreturn.
 	// SignalRestore returns the thread's new signal mask.
-	SignalRestore(st *Stack, rt bool) (linux.SignalSet, error)
+	SignalRestore(st *Stack, rt bool) (linux.SignalSet, SignalStack, error)
 
 	// CPUIDEmulate emulates a CPUID instruction according to current register state.
 	CPUIDEmulate(l log.Logger)
diff --git a/pkg/sentry/arch/signal_amd64.go b/pkg/sentry/arch/signal_amd64.go
index 4040b530f..c1d743f38 100644
--- a/pkg/sentry/arch/signal_amd64.go
+++ b/pkg/sentry/arch/signal_amd64.go
@@ -377,6 +377,14 @@ func (c *context64) SignalSetup(st *Stack, act *SignalAct, info *SignalInfo, alt
 	sp = frameBottom + usermem.Addr(frameSize)
 	st.Bottom = sp
 
+	// Prior to proceeding, figure out if the frame will exhaust the range
+	// for the signal stack. This is not allowed, and should immediately
+	// force signal delivery (reverting to the default handler).
+	if act.IsOnStack() && alt.IsEnabled() && !alt.Contains(frameBottom) {
+		return syscall.EFAULT
+	}
+
+	// Adjust the code.
 	info.FixSignalCodeForUser()
 
 	// Set up the stack frame.
@@ -422,15 +430,15 @@ func (c *context64) SignalSetup(st *Stack, act *SignalAct, info *SignalInfo, alt
 
 // SignalRestore implements Context.SignalRestore. (Compare to Linux's
 // arch/x86/kernel/signal.c:sys_rt_sigreturn().)
-func (c *context64) SignalRestore(st *Stack, rt bool) (linux.SignalSet, error) {
+func (c *context64) SignalRestore(st *Stack, rt bool) (linux.SignalSet, SignalStack, error) {
 	// Copy out the stack frame.
 	var uc UContext64
 	if _, err := st.Pop(&uc); err != nil {
-		return 0, err
+		return 0, SignalStack{}, err
 	}
 	var info SignalInfo
 	if _, err := st.Pop(&info); err != nil {
-		return 0, err
+		return 0, SignalStack{}, err
 	}
 
 	// Restore registers.
@@ -472,5 +480,5 @@ func (c *context64) SignalRestore(st *Stack, rt bool) (linux.SignalSet, error) {
 		log.Infof("sigreturn unable to restore application fpstate")
 	}
 
-	return uc.Sigset, nil
+	return uc.Sigset, uc.Stack, nil
 }
diff --git a/pkg/sentry/arch/signal_stack.go b/pkg/sentry/arch/signal_stack.go
index 7c6531d79..ba43dd1d4 100644
--- a/pkg/sentry/arch/signal_stack.go
+++ b/pkg/sentry/arch/signal_stack.go
@@ -39,12 +39,19 @@ func (s SignalStack) Top() usermem.Addr {
 	return usermem.Addr(s.Addr + s.Size)
 }
 
-// SetOnStack marks this signal stack as in use. (This is only called on copies
-// sent to user applications, so there's no corresponding ClearOnStack.)
+// SetOnStack marks this signal stack as in use.
+//
+// Note that there is no corresponding ClearOnStack, and that this should only
+// be called on copies that are serialized to userspace.
 func (s *SignalStack) SetOnStack() {
 	s.Flags |= SignalStackFlagOnStack
 }
 
+// Contains checks if the stack pointer is within this stack.
+func (s *SignalStack) Contains(sp usermem.Addr) bool {
+	return usermem.Addr(s.Addr) < sp && sp <= usermem.Addr(s.Addr+s.Size)
+}
+
 // NativeSignalStack is a type that is equivalent to stack_t in the guest
 // architecture.
 type NativeSignalStack interface {
diff --git a/pkg/sentry/kernel/task_signals.go b/pkg/sentry/kernel/task_signals.go
index e4ef7fd67..91f6c0874 100644
--- a/pkg/sentry/kernel/task_signals.go
+++ b/pkg/sentry/kernel/task_signals.go
@@ -212,7 +212,9 @@ func (t *Task) deliverSignal(info *arch.SignalInfo, act arch.SignalAct) taskRunS
 		// Try to deliver the signal to the user-configured handler.
 		t.Debugf("Signal %d: delivering to handler", info.Signo)
 		if err := t.deliverSignalToHandler(info, act); err != nil {
-			t.Warningf("Failed to deliver signal %+v to user handler: %v", info, err)
+			// This is not a warning, it can occur during normal operation.
+			t.Debugf("Failed to deliver signal %+v to user handler: %v", info, err)
+
 			// Send a forced SIGSEGV. If the signal that couldn't be delivered
 			// was a SIGSEGV, force the handler to SIG_DFL.
 			t.forceSignal(linux.SIGSEGV, linux.Signal(info.Signo) == linux.SIGSEGV /* unconditional */)
@@ -241,7 +243,7 @@ func (t *Task) deliverSignalToHandler(info *arch.SignalInfo, act arch.SignalAct)
 	alt := t.signalStack
 	if act.IsOnStack() && alt.IsEnabled() {
 		alt.SetOnStack()
-		if !t.OnSignalStack(alt) {
+		if !alt.Contains(sp) {
 			sp = usermem.Addr(alt.Top())
 		}
 	}
@@ -275,18 +277,20 @@ var ctrlResume = &SyscallControl{ignoreReturn: true}
 // rt is true).
 func (t *Task) SignalReturn(rt bool) (*SyscallControl, error) {
 	st := t.Stack()
-	sigset, err := t.Arch().SignalRestore(st, rt)
+	sigset, alt, err := t.Arch().SignalRestore(st, rt)
 	if err != nil {
 		return nil, err
 	}
 
+	// Attempt to record the given signal stack. Note that we silently
+	// ignore failures here, as does Linux. Only an EFAULT may be
+	// generated, but SignalRestore has already deserialized the entire
+	// frame successfully.
+	t.SetSignalStack(alt)
+
 	// Restore our signal mask. SIGKILL and SIGSTOP should not be blocked.
 	t.SetSignalMask(sigset &^ UnblockableSignals)
 
-	// TODO: sys_rt_sigreturn also calls restore_altstack from
-	// uc.stack, allowing the signal handler to implicitly mutate the signal
-	// stack.
-
 	return ctrlResume, nil
 }
 
@@ -624,23 +628,41 @@ func (t *Task) SetSavedSignalMask(mask linux.SignalSet) {
 
 // SignalStack returns the task-private signal stack.
 func (t *Task) SignalStack() arch.SignalStack {
-	return t.signalStack
+	alt := t.signalStack
+	if t.onSignalStack(alt) {
+		alt.Flags |= arch.SignalStackFlagOnStack
+	}
+	return alt
 }
 
-// OnSignalStack returns true if, when the task resumes running, it will run on
-// the task-private signal stack.
-func (t *Task) OnSignalStack(s arch.SignalStack) bool {
+// onSignalStack returns true if the task is executing on the given signal stack.
+func (t *Task) onSignalStack(alt arch.SignalStack) bool {
 	sp := usermem.Addr(t.Arch().Stack())
-	return usermem.Addr(s.Addr) <= sp && sp < usermem.Addr(s.Addr+s.Size)
+	return alt.Contains(sp)
 }
 
-// SetSignalStack sets the task-private signal stack and clears the
-// SignalStackFlagDisable, since we have a signal stack.
-func (t *Task) SetSignalStack(alt arch.SignalStack) error {
-	// Mask out irrelevant parts: only disable matters.
-	alt.Flags &= arch.SignalStackFlagDisable
-	t.signalStack = alt
-	return nil
+// SetSignalStack sets the task-private signal stack.
+//
+// This value may not be changed if the task is currently executing on the
+// signal stack, i.e. if t.onSignalStack returns true. In this case, this
+// function will return false. Otherwise, true is returned.
+func (t *Task) SetSignalStack(alt arch.SignalStack) bool {
+	// Check that we're not executing on the stack.
+	if t.onSignalStack(t.signalStack) {
+		return false
+	}
+
+	if alt.Flags&arch.SignalStackFlagDisable != 0 {
+		// Don't record anything beyond the flags.
+		t.signalStack = arch.SignalStack{
+			Flags: arch.SignalStackFlagDisable,
+		}
+	} else {
+		// Mask out irrelevant parts: only disable matters.
+		alt.Flags &= arch.SignalStackFlagDisable
+		t.signalStack = alt
+	}
+	return true
 }
 
 // SetSignalAct atomically sets the thread group's signal action for signal sig
diff --git a/pkg/sentry/syscalls/linux/sys_signal.go b/pkg/sentry/syscalls/linux/sys_signal.go
index 93b3f531a..66ecb1299 100644
--- a/pkg/sentry/syscalls/linux/sys_signal.go
+++ b/pkg/sentry/syscalls/linux/sys_signal.go
@@ -315,25 +315,23 @@ func Sigaltstack(t *kernel.Task, args arch.SyscallArguments) (uintptr, *kernel.S
 	setaddr := args[0].Pointer()
 	oldaddr := args[1].Pointer()
 
+	alt := t.SignalStack()
 	if oldaddr != 0 {
-		alt := t.SignalStack()
-		if t.OnSignalStack(alt) {
-			alt.Flags |= arch.SignalStackFlagOnStack
-		}
 		if err := t.CopyOutSignalStack(oldaddr, &alt); err != nil {
 			return 0, nil, err
 		}
 	}
 	if setaddr != 0 {
-		if t.OnSignalStack(t.SignalStack()) {
-			return 0, nil, syserror.EPERM
-		}
 		alt, err := t.CopyInSignalStack(setaddr)
 		if err != nil {
 			return 0, nil, err
 		}
-		if err := t.SetSignalStack(alt); err != nil {
-			return 0, nil, err
+		// The signal stack cannot be changed if the task is currently
+		// on the stack. This is enforced at the lowest level because
+		// these semantics apply to changing the signal stack via a
+		// ucontext during a signal handler.
+		if !t.SetSignalStack(alt) {
+			return 0, nil, syserror.EPERM
 		}
 	}