From 169e2efc5a2116755beca91e65802780282ab4c1 Mon Sep 17 00:00:00 2001
From: Michael Pratt <mpratt@google.com>
Date: Fri, 7 Sep 2018 10:27:19 -0700
Subject: Continue handling signals after disabling forwarding

Before destroying the Kernel, we disable signal forwarding,
relinquishing control to the Go runtime. External signals that arrive
after disabling forwarding but before the sandbox exits thus may use
runtime.raise (i.e., tkill(2)) and violate the syscall filters.

Adjust forwardSignals to handle signals received after disabling
forwarding the same way they are handled before starting forwarding.
i.e., by implementing the standard Go runtime behavior using tgkill(2)
instead of tkill(2).

This also makes the stop callback block until forwarding actually stops.
This isn't required to avoid tkill(2) but is a saner interface.

PiperOrigin-RevId: 211995946
Change-Id: I3585841644409260eec23435cf65681ad41f5f03
---
 pkg/sentry/sighandling/sighandling.go | 41 +++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 16 deletions(-)

diff --git a/pkg/sentry/sighandling/sighandling.go b/pkg/sentry/sighandling/sighandling.go
index 5bac3a4e1..b08588c11 100644
--- a/pkg/sentry/sighandling/sighandling.go
+++ b/pkg/sentry/sighandling/sighandling.go
@@ -30,9 +30,11 @@ import (
 // numSignals is the number of normal (non-realtime) signals on Linux.
 const numSignals = 32
 
-// forwardSignals listens for incoming signals and delivers them to k. It starts
-// when the start channel is closed and stops when the stop channel is closed.
-func forwardSignals(k *kernel.Kernel, sigchans []chan os.Signal, start, stop chan struct{}) {
+// forwardSignals listens for incoming signals and delivers them to k.
+//
+// It starts when the start channel is closed, stops when the stop channel
+// is closed, and closes done once it will no longer deliver signals to k.
+func forwardSignals(k *kernel.Kernel, sigchans []chan os.Signal, start, stop, done chan struct{}) {
 	// Build a select case.
 	sc := []reflect.SelectCase{{Dir: reflect.SelectRecv, Chan: reflect.ValueOf(start)}}
 	for _, sigchan := range sigchans {
@@ -47,13 +49,19 @@ func forwardSignals(k *kernel.Kernel, sigchans []chan os.Signal, start, stop cha
 		// Was it the start / stop channel?
 		if index == 0 {
 			if !ok {
-				if started {
-					// stop channel
-					break
-				} else {
-					// start channel
+				if !started {
+					// start channel; start forwarding and
+					// swap this case for the stop channel
+					// to select stop requests.
 					started = true
 					sc[0] = reflect.SelectCase{Dir: reflect.SelectRecv, Chan: reflect.ValueOf(stop)}
+				} else {
+					// stop channel; stop forwarding and
+					// clear this case so it is never
+					// selected again.
+					started = false
+					close(done)
+					sc[0].Chan = reflect.Value{}
 				}
 			}
 			continue
@@ -69,7 +77,8 @@ func forwardSignals(k *kernel.Kernel, sigchans []chan os.Signal, start, stop cha
 		signal := linux.Signal(index)
 
 		if !started {
-			// Kernel is not ready to receive signals.
+			// Kernel cannot receive signals, either because it is
+			// not ready yet or is shutting down.
 			//
 			// Kill ourselves if this signal would have killed the
 			// process before PrepareForwarding was called. i.e., all
@@ -92,20 +101,19 @@ func forwardSignals(k *kernel.Kernel, sigchans []chan os.Signal, start, stop cha
 
 		k.SendExternalSignal(&arch.SignalInfo{Signo: int32(signal)}, "sentry")
 	}
-
-	// Close all individual channels.
-	for _, sigchan := range sigchans {
-		signal.Stop(sigchan)
-		close(sigchan)
-	}
 }
 
 // PrepareForwarding ensures that synchronous signals are forwarded to k and
 // returns a callback that starts signal delivery, which itself returns a
 // callback that stops signal forwarding.
+//
+// Note that this function permanently takes over signal handling. After the
+// stop callback, signals revert to the default Go runtime behavior, which
+// cannot be overridden with external calls to signal.Notify.
 func PrepareForwarding(k *kernel.Kernel, skipSignal syscall.Signal) func() func() {
 	start := make(chan struct{})
 	stop := make(chan struct{})
+	done := make(chan struct{})
 
 	// Register individual channels. One channel per standard signal is
 	// required as os.Notify() is non-blocking and may drop signals. To avoid
@@ -126,12 +134,13 @@ func PrepareForwarding(k *kernel.Kernel, skipSignal syscall.Signal) func() func(
 		signal.Notify(sigchan, syscall.Signal(sig))
 	}
 	// Start up our listener.
-	go forwardSignals(k, sigchans, start, stop) // S/R-SAFE: synchronized by Kernel.extMu
+	go forwardSignals(k, sigchans, start, stop, done) // S/R-SAFE: synchronized by Kernel.extMu.
 
 	return func() func() {
 		close(start)
 		return func() {
 			close(stop)
+			<-done
 		}
 	}
 }
-- 
cgit v1.2.3