runtime: do signal stack clearing when parking Ms

Clearing signal stacks during STW meant an added latency that was linear
with respect to the number of Ms, parked or running. Since that number
be quite high, we need an alternate scheme.

Instead of clearing in the GC cycle, clear the signal stack whenever an
M is parked and has the potential to stay parked until the end of the
next GC cycle. This implements the wanted behavior for runtime/secret at
the cost of a little bit of extra latency that will be dwarfed by the
time it takes to perform a syscall.

Change-Id: Ieb9618f46736c34486e17a3d6185661a98756d0d
Reviewed-on: https://go-review.googlesource.com/c/go/+/724282
Auto-Submit: Daniel Morsing <daniel.morsing@gmail.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>

src/runtime/mgc.go

@@ -839,30 +839,12 @@ func gcStart(trigger gcTrigger) {
 	work.cpuStats.accumulateGCPauseTime(stw.stoppingCPUTime, 1)
 
 	if goexperiment.RuntimeSecret {
-		// The world is stopped. Every M is either parked
-		// or in a syscall, or running some non-go code which can't run in secret mode.
-		// To get to a parked or a syscall state
-		// they have to transition through a point where we erase any
-		// confidential information in the registers. Making them
-		// handle a signal now would clobber the signal stack
-		// with non-confidential information.
-		//
-		// TODO(dmo): this is linear with respect to the number of Ms.
-		// Investigate just how long this takes and whether we can somehow
-		// loop over just the Ms that have secret info on their signal stack,
-		// or cooperatively have the Ms send signals to themselves just
-		// after they erase their registers, but before they enter a syscall
-		for mp := allm; mp != nil; mp = mp.alllink {
-			// even through the world is stopped, the kernel can still
-			// invoke our signal handlers. No confidential information can be spilled
-			// (because it's been erased by this time), but we can avoid
-			// sending additional signals by atomically inspecting this variable
-			if atomic.Xchg(&mp.signalSecret, 0) != 0 {
-				noopSignal(mp)
-			}
-			// TODO: syncronize with the signal handler to ensure that the signal
-			// was actually delivered.
-		}
+		// The world is stopped, which means every M is either idle, blocked
+		// in a syscall or this M that we are running on now.
+		// The blocked Ms had any secret spill on their signal stacks erased
+		// when they entered their respective states. Now we have to handle
+		// this one.
+		eraseSecretsSignalStk()
 	}
 
 	// Finish sweep before we start concurrent scan.

src/runtime/proc.go

@@ -1968,6 +1968,11 @@ func mstartm0() {
 //go:nosplit
 func mPark() {
 	gp := getg()
+	// This M might stay parked through an entire GC cycle.
+	// Erase any leftovers on the signal stack.
+	if goexperiment.RuntimeSecret {
+		eraseSecretsSignalStk()
+	}
 	notesleep(&gp.m.park)
 	noteclear(&gp.m.park)
 }
@@ -4635,6 +4640,30 @@ func reentersyscall(pc, sp, bp uintptr) {
 	// but can have inconsistent g->sched, do not let GC observe it.
 	gp.m.locks++
 
+	// This M may have a signal stack that is dirtied with secret information
+	// (see package "runtime/secret"). Since it's about to go into a syscall for
+	// an arbitrary amount of time and the G that put the secret info there
+	// might have returned from secret.Do, we have to zero it out now, lest we
+	// break the guarantee that secrets are purged by the next GC after a return
+	// to secret.Do.
+	//
+	// It might be tempting to think that we only need to zero out this if we're
+	// not running in secret mode anymore, but that leaves an ABA problem. The G
+	// that put the secrets onto our signal stack may not be the one that is
+	// currently executing.
+	//
+	// Logically, we should erase this when we lose our P, not when we enter the
+	// syscall. This would avoid a zeroing in the case where the call returns
+	// almost immediately. Since we use this path for cgo calls as well, these
+	// fast "syscalls" are quite common. However, since we only erase the signal
+	// stack if we were delivered a signal in secret mode and considering the
+	// cross-thread synchronization cost for the P, it hardly seems worth it.
+	//
+	// TODO(dmo): can we encode the goid into mp.signalSecret and avoid the ABA problem?
+	if goexperiment.RuntimeSecret {
+		eraseSecretsSignalStk()
+	}
+
 	// Entersyscall must not call any function that might split/grow the stack.
 	// (See details in comment above.)
 	// Catch calls that might, by replacing the stack guard with something that

src/runtime/runtime2.go

@@ -620,15 +620,18 @@ type m struct {
 
 	// Fields whose offsets are not known to debuggers.
 
-	procid       uint64            // for debuggers, but offset not hard-coded
-	gsignal      *g                // signal-handling g
-	goSigStack   gsignalStack      // Go-allocated signal handling stack
-	sigmask      sigset            // storage for saved signal mask
-	tls          [tlsSlots]uintptr // thread-local storage (for x86 extern register)
-	mstartfn     func()
-	curg         *g       // current running goroutine
-	caughtsig    guintptr // goroutine running during fatal signal
-	signalSecret uint32   // whether we have secret information in our signal stack
+	procid     uint64            // for debuggers, but offset not hard-coded
+	gsignal    *g                // signal-handling g
+	goSigStack gsignalStack      // Go-allocated signal handling stack
+	sigmask    sigset            // storage for saved signal mask
+	tls        [tlsSlots]uintptr // thread-local storage (for x86 extern register)
+	mstartfn   func()
+	curg       *g       // current running goroutine
+	caughtsig  guintptr // goroutine running during fatal signal
+
+	// Indicates whether we've received a signal while
+	// running in secret mode.
+	signalSecret bool
 
 	// p is the currently attached P for executing Go code, nil if not executing user Go code.
 	//

src/runtime/secret.go

@@ -71,15 +71,6 @@ func addSecret(p unsafe.Pointer, size uintptr) {
 	addspecial(p, &s.special, false)
 }
 
-// send a no-op signal to an M for the purposes of
-// clobbering the signal stack
-//
-// Use sigpreempt. If we don't have a preemption queued, this just
-// turns into a no-op
-func noopSignal(mp *m) {
-	signalM(mp, sigPreempt)
-}
-
 // secret_getStack returns the memory range of the
 // current goroutine's stack.
 // For testing only.
@@ -93,6 +84,33 @@ func secret_getStack() (uintptr, uintptr) {
 	return gp.stack.lo, gp.stack.hi
 }
 
+// erase any secrets that may have been spilled onto the signal stack during
+// signal handling. Must be called on g0 or inside STW to make sure we don't
+// get rescheduled onto a different M.
+//
+//go:nosplit
+func eraseSecretsSignalStk() {
+	mp := getg().m
+	if mp.signalSecret {
+		mp.signalSecret = false
+		// signal handlers get invoked atomically
+		// so it's fine for us to zero out the stack while a signal
+		// might get delivered. Worst case is we are currently running
+		// in secret mode and the signal spills fresh secret info onto
+		// the stack, but since we haven't returned from the secret.Do
+		// yet, we make no guarantees about that information.
+		//
+		// It might be tempting to only erase the part of the signal
+		// stack that has the context, but when running with forwarded
+		// signals, they might pull arbitrary data out of the context and
+		// store it elsewhere on the stack. We can't stop them from storing
+		// the data in arbitrary places, but we can erase the stack where
+		// they are likely to put it in cases of a register spill.
+		size := mp.gsignal.stack.hi - mp.gsignal.stack.lo
+		memclrNoHeapPointers(unsafe.Pointer(mp.gsignal.stack.lo), size)
+	}
+}
+
 // return a slice of all Ms signal stacks
 // For testing only.
 //

src/runtime/secret_nosecret.go

@@ -27,4 +27,4 @@ func addSecret(p unsafe.Pointer, size uintptr) {}
 //go:linkname secret_getStack runtime/secret.getStack
 func secret_getStack() (uintptr, uintptr) { return 0, 0 }
 
-func noopSignal(mp *m) {}
+func eraseSecretsSignalStk() {}

src/runtime/signal_unix.go

@@ -491,7 +491,7 @@ func sigtrampgo(sig uint32, info *siginfo, ctx unsafe.Pointer) {
 	sighandler(sig, info, ctx, gp)
 
 	if goexperiment.RuntimeSecret && gp.secret > 0 {
-		atomic.Store(&gp.m.signalSecret, 1)
+		gp.m.signalSecret = true
 	}
 
 	setg(gp)