From 16dba7fa76771007a2969b44a93807e255d5964a Mon Sep 17 00:00:00 2001 From: Andrei Vagin Date: Sun, 25 Feb 2024 22:44:13 -0800 Subject: [PATCH] systrap: handle stub thread crashes If a stub thread crashes (exits), it means something wrong and the whole subprocess has to be killed. The sentry has to handle subprocess deaths and kills guest processes associated with crashed suprocesses. Reported-by: syzbot+8a8330333cd20c039a1b@syzkaller.appspotmail.com PiperOrigin-RevId: 610296930 --- pkg/sentry/platform/systrap/filters.go | 6 ++++ pkg/sentry/platform/systrap/shared_context.go | 8 ++++- pkg/sentry/platform/systrap/subprocess.go | 26 ++++++++++++--- .../platform/systrap/subprocess_amd64.go | 1 - .../platform/systrap/subprocess_arm64.go | 1 - .../platform/systrap/subprocess_linux.go | 20 ++++------- .../platform/systrap/subprocess_unsafe.go | 33 +++++++++++++++++++ pkg/sentry/platform/systrap/syscall_thread.go | 3 ++ pkg/sentry/platform/systrap/systrap_unsafe.go | 3 +- 9 files changed, 79 insertions(+), 22 deletions(-) diff --git a/pkg/sentry/platform/systrap/filters.go b/pkg/sentry/platform/systrap/filters.go index a9f61322e..7dc2cd5a1 100644 --- a/pkg/sentry/platform/systrap/filters.go +++ b/pkg/sentry/platform/systrap/filters.go @@ -94,6 +94,12 @@ func (systrapSeccomp) SyscallFilters(vars precompiledseccomp.Values) seccomp.Sys }, unix.SYS_TGKILL: seccomp.MatchAll{}, unix.SYS_WAIT4: seccomp.MatchAll{}, + unix.SYS_WAITID: seccomp.PerArg{ + seccomp.EqualTo(unix.P_PID), + seccomp.AnyValue{}, + seccomp.AnyValue{}, + seccomp.EqualTo(unix.WEXITED | unix.WNOHANG | unix.WNOWAIT), + }, unix.SYS_SETPRIORITY: seccomp.PerArg{ seccomp.EqualTo(unix.PRIO_PROCESS), seccomp.AnyValue{}, diff --git a/pkg/sentry/platform/systrap/shared_context.go b/pkg/sentry/platform/systrap/shared_context.go index 8ba1e79ee..232d7b64b 100644 --- a/pkg/sentry/platform/systrap/shared_context.go +++ b/pkg/sentry/platform/systrap/shared_context.go @@ -224,7 +224,9 @@ const ( stuckContextTimeout = 30 * time.Second ) -func (sc *sharedContext) sleepOnState(state sysmsg.ContextState) { +var errDeadSubprocess = fmt.Errorf("subprocess died") + +func (sc *sharedContext) sleepOnState(state sysmsg.ContextState) error { timeout := unix.Timespec{ Sec: 0, Nsec: contextPreemptTimeoutNsec, @@ -239,6 +241,9 @@ func (sc *sharedContext) sleepOnState(state sysmsg.ContextState) { if errno != unix.ETIMEDOUT { panic(fmt.Sprintf("error waiting for state: %v", errno)) } + if !sc.subprocess.alive() { + return errDeadSubprocess + } if time.Now().After(deadline) { log.Warningf("Systrap task goroutine has been waiting on ThreadContext.State futex too long. ThreadContext: %v", sc) } @@ -255,6 +260,7 @@ func (sc *sharedContext) sleepOnState(state sysmsg.ContextState) { timeout.Sec = contextCheckupTimeoutSec timeout.Nsec = 0 } + return nil } type fastPathDispatcher struct { diff --git a/pkg/sentry/platform/systrap/subprocess.go b/pkg/sentry/platform/systrap/subprocess.go index 102b6361f..965eca2e2 100644 --- a/pkg/sentry/platform/systrap/subprocess.go +++ b/pkg/sentry/platform/systrap/subprocess.go @@ -23,6 +23,7 @@ import ( "golang.org/x/sys/unix" "gvisor.dev/gvisor/pkg/abi/linux" + "gvisor.dev/gvisor/pkg/atomicbitops" "gvisor.dev/gvisor/pkg/hostarch" "gvisor.dev/gvisor/pkg/log" "gvisor.dev/gvisor/pkg/pool" @@ -169,6 +170,9 @@ type subprocess struct { // contextQueue is a queue of all contexts that are ready to switch back to // user mode. contextQueue *contextQueue + + // dead indicates whether the subprocess is alive or not. + dead atomicbitops.Bool } func (s *subprocess) initSyscallThread(ptraceThread *thread) error { @@ -452,13 +456,18 @@ func (s *subprocess) unmap() { // globalPool. This has the added benefit of reducing creation time for new // subprocesses. func (s *subprocess) Release() { + if !s.alive() { + return + } s.unmap() s.DecRef(s.release) } // release returns the subprocess to the global pool. func (s *subprocess) release() { - globalPool.markAvailable(s) + if s.alive() { + globalPool.markAvailable(s) + } } // attach attaches to the thread. @@ -748,7 +757,10 @@ func (s *subprocess) switchToApp(c *context, ac *arch.Context64) (isSyscall bool if err := s.contextQueue.add(ctx); err != nil { return false, false, err } - s.waitOnState(ctx) + + if err := s.waitOnState(ctx); err != nil { + return false, false, corruptedSharedMemoryErr(err.Error()) + } // Check if there's been an error. threadID := ctx.threadID() @@ -786,7 +798,7 @@ func (s *subprocess) switchToApp(c *context, ac *arch.Context64) (isSyscall bool return false, false, nil } -func (s *subprocess) waitOnState(ctx *sharedContext) { +func (s *subprocess) waitOnState(ctx *sharedContext) error { ctx.kicked = false slowPath := false if !s.contextQueue.fastPathEnabled() || atomic.LoadUint32(&s.contextQueue.numActiveThreads) == 0 { @@ -819,13 +831,17 @@ func (s *subprocess) waitOnState(ctx *sharedContext) { ctx.kicked = s.kickSysmsgThread() } - ctx.sleepOnState(curState) + if err := ctx.sleepOnState(curState); err != nil { + return err + } } } ctx.recordLatency() ctx.resetLatencyMeasures() ctx.enableSentryFastPath() + + return nil } // canKickSysmsgThread returns true if a new thread can be kicked. @@ -935,7 +951,7 @@ func (s *subprocess) Unmap(addr hostarch.Addr, length uint64) { unix.SYS_MUNMAP, arch.SyscallArgument{Value: uintptr(addr)}, arch.SyscallArgument{Value: uintptr(length)}) - if err != nil { + if err != nil && err != errDeadSubprocess { // We never expect this to happen. panic(fmt.Sprintf("munmap(%x, %x)) failed: %v", addr, length, err)) } diff --git a/pkg/sentry/platform/systrap/subprocess_amd64.go b/pkg/sentry/platform/systrap/subprocess_amd64.go index 82ca7a649..4a19ac59a 100644 --- a/pkg/sentry/platform/systrap/subprocess_amd64.go +++ b/pkg/sentry/platform/systrap/subprocess_amd64.go @@ -134,7 +134,6 @@ func (t *thread) adjustInitRegsRip() { // Pass the expected PPID to the child via R15 when creating stub process. func initChildProcessPPID(initregs *arch.Registers, ppid int32) { - initregs.R15 = uint64(ppid) // Rbx has to be set to 1 when creating stub process. initregs.Rbx = _NEW_STUB } diff --git a/pkg/sentry/platform/systrap/subprocess_arm64.go b/pkg/sentry/platform/systrap/subprocess_arm64.go index c055f8e08..f43c394d5 100644 --- a/pkg/sentry/platform/systrap/subprocess_arm64.go +++ b/pkg/sentry/platform/systrap/subprocess_arm64.go @@ -110,7 +110,6 @@ func (t *thread) adjustInitRegsRip() { // Pass the expected PPID to the child via X7 when creating stub process func initChildProcessPPID(initregs *arch.Registers, ppid int32) { - initregs.Regs[7] = uint64(ppid) // R9 has to be set to 1 when creating stub process. initregs.Regs[9] = _NEW_STUB } diff --git a/pkg/sentry/platform/systrap/subprocess_linux.go b/pkg/sentry/platform/systrap/subprocess_linux.go index 97bd16872..4e551823c 100644 --- a/pkg/sentry/platform/systrap/subprocess_linux.go +++ b/pkg/sentry/platform/systrap/subprocess_linux.go @@ -40,7 +40,7 @@ func createStub() (*thread, error) { // transitively) will be killed as well. It's simply not possible to // safely handle a single stub getting killed: the exact state of // execution is unknown and not recoverable. - return attachedThread(uintptr(unix.SIGKILL)|unix.CLONE_FILES, linux.SECCOMP_RET_TRAP) + return attachedThread(unix.CLONE_FILES|uintptr(unix.SIGCHLD), linux.SECCOMP_RET_TRAP) } // attachedThread returns a new attached thread. @@ -57,13 +57,15 @@ func attachedThread(flags uintptr, defaultAction linux.BPFAction) (*thread, erro Rules: seccomp.MakeSyscallRules(map[uintptr]seccomp.SyscallRule{ unix.SYS_CLONE: seccomp.Or{ // Allow creation of new subprocesses (used by the master). - seccomp.PerArg{seccomp.EqualTo(unix.CLONE_FILES | unix.SIGKILL)}, + seccomp.PerArg{seccomp.EqualTo(unix.CLONE_FILES | unix.CLONE_PARENT | unix.SIGCHLD)}, + seccomp.PerArg{seccomp.EqualTo(unix.CLONE_FILES | unix.SIGCHLD)}, // Allow creation of new sysmsg thread. seccomp.PerArg{seccomp.EqualTo( unix.CLONE_FILES | unix.CLONE_FS | unix.CLONE_VM | - unix.CLONE_PTRACE)}, + unix.CLONE_PTRACE | + linux.SIGKILL)}, // Allow creation of new threads within a single address space (used by address spaces). seccomp.PerArg{seccomp.EqualTo( unix.CLONE_FILES | @@ -256,7 +258,7 @@ func (t *thread) createStub() (*thread, error) { pid, err := t.syscallIgnoreInterrupt( ®s, unix.SYS_CLONE, - arch.SyscallArgument{Value: uintptr(unix.SIGKILL | unix.CLONE_FILES)}, + arch.SyscallArgument{Value: uintptr(unix.CLONE_FILES | unix.CLONE_PARENT | uintptr(unix.SIGCHLD))}, arch.SyscallArgument{Value: 0}, arch.SyscallArgument{Value: 0}, arch.SyscallArgument{Value: 0}, @@ -272,15 +274,7 @@ func (t *thread) createStub() (*thread, error) { // We unfortunately don't have a handy part of memory to write the wait // status. If the wait succeeds, we'll assume that it was the SIGSTOP. // If the child actually exited, the attach below will fail. - _, err = t.syscallIgnoreInterrupt( - &t.initRegs, - unix.SYS_WAIT4, - arch.SyscallArgument{Value: uintptr(pid)}, - arch.SyscallArgument{Value: 0}, - arch.SyscallArgument{Value: unix.WALL | unix.WUNTRACED}, - arch.SyscallArgument{Value: 0}, - arch.SyscallArgument{Value: 0}, - arch.SyscallArgument{Value: 0}) + _, err = unix.Wait4(int(pid), nil, unix.WALL|unix.WUNTRACED, nil) if err != nil { return nil, fmt.Errorf("waiting on stub process: %v", err) } diff --git a/pkg/sentry/platform/systrap/subprocess_unsafe.go b/pkg/sentry/platform/systrap/subprocess_unsafe.go index a3588381f..a09d25785 100644 --- a/pkg/sentry/platform/systrap/subprocess_unsafe.go +++ b/pkg/sentry/platform/systrap/subprocess_unsafe.go @@ -26,6 +26,8 @@ import ( "unsafe" "golang.org/x/sys/unix" + "gvisor.dev/gvisor/pkg/abi/linux" + "gvisor.dev/gvisor/pkg/log" "gvisor.dev/gvisor/pkg/sentry/arch" "gvisor.dev/gvisor/pkg/sentry/memmap" "gvisor.dev/gvisor/pkg/sentry/pgalloc" @@ -97,3 +99,34 @@ func restoreFPState(ctx *sharedContext, c *context, ac *arch.Context64) { dst := ctx.shared.FPState[:] copy(dst, src) } + +// alive returns true if the subprocess is alive. +func (s *subprocess) alive() bool { + if s.dead.Load() { + return false + } + + // Wait4 doesn't support WNOWAIT, but here is no other way to find out + // whether a process exited or was stopped by ptrace. + siginfo := linux.SignalInfo{} + _, _, errno := unix.Syscall6( + unix.SYS_WAITID, + unix.P_PID, + uintptr(s.syscallThread.thread.tid), + uintptr(unsafe.Pointer(&siginfo)), + uintptr(unix.WEXITED|unix.WNOHANG|unix.WNOWAIT), + 0, 0) + if errno == 0 && siginfo.PID() == 0 { + return true + } + if errno == 0 && siginfo.Code != linux.CLD_EXITED && siginfo.Code != linux.CLD_KILLED { + return true + } + + // The process is dead, let's collect its zombie. + wstatus := unix.WaitStatus(0) + pid, err := unix.Wait4(int(s.syscallThread.thread.tid), &wstatus, unix.WNOHANG, nil) + log.Warningf("the subprocess %d exited (status: %s, err %s)", pid, wstatus, err) + s.dead.Store(true) + return false +} diff --git a/pkg/sentry/platform/systrap/syscall_thread.go b/pkg/sentry/platform/systrap/syscall_thread.go index 6dd7ac1b4..dd9b80af0 100644 --- a/pkg/sentry/platform/systrap/syscall_thread.go +++ b/pkg/sentry/platform/systrap/syscall_thread.go @@ -168,6 +168,9 @@ func (t *syscallThread) attach() error { } func (t *syscallThread) syscall(sysno uintptr, args ...arch.SyscallArgument) (uintptr, error) { + if t.subproc.dead.Load() { + return 0, errDeadSubprocess + } sentryMsg := t.sentryMessage stubMsg := t.stubMessage sentryMsg.sysno = uint64(sysno) diff --git a/pkg/sentry/platform/systrap/systrap_unsafe.go b/pkg/sentry/platform/systrap/systrap_unsafe.go index 5fd679da5..eba22e920 100644 --- a/pkg/sentry/platform/systrap/systrap_unsafe.go +++ b/pkg/sentry/platform/systrap/systrap_unsafe.go @@ -98,7 +98,8 @@ func (t *thread) clone() (*thread, error) { unix.CLONE_FILES | unix.CLONE_FS | unix.CLONE_PTRACE | - unix.CLONE_VM) + unix.CLONE_VM | + linux.SIGKILL) rval, err := t.syscallIgnoreInterrupt( &t.initRegs, unix.SYS_CLONE,