diff --git a/src/runtime/mgc.go b/src/runtime/mgc.go index b8f99d1ed4..97a0ce8b98 100644 --- a/src/runtime/mgc.go +++ b/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. diff --git a/src/runtime/proc.go b/src/runtime/proc.go index 8027ac5bee..db4febace9 100644 --- a/src/runtime/proc.go +++ b/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 diff --git a/src/runtime/runtime2.go b/src/runtime/runtime2.go index 578458afa0..95dc834717 100644 --- a/src/runtime/runtime2.go +++ b/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. // diff --git a/src/runtime/secret.go b/src/runtime/secret.go index 8aad63b54f..8942df7051 100644 --- a/src/runtime/secret.go +++ b/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. // diff --git a/src/runtime/secret_nosecret.go b/src/runtime/secret_nosecret.go index 0692d6bf70..581a092948 100644 --- a/src/runtime/secret_nosecret.go +++ b/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() {} diff --git a/src/runtime/signal_unix.go b/src/runtime/signal_unix.go index f352cb3c02..5269e2aa7f 100644 --- a/src/runtime/signal_unix.go +++ b/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)