From 024d75d26394d5da7991682e74e9b2c5f23a20ac Mon Sep 17 00:00:00 2001
From: Michael Pratt <mpratt@google.com>
Date: Tue, 30 Aug 2022 15:39:58 -0400
Subject: [PATCH] Set tg.exiting on group exit via exit(2)

Setting t.tg.exiting during PrepareGroupExit prevents a SIGKILL that
arrives while the task is zombied from changing the exit status (see
ThreadGroup.applySignalSideEffectsLocked).

However, PrepareExit does not set t.tg.exiting, meaning that if the last
task exits via exit(2), then a SIGKILL _can_ change the exit status.
This does not match Linux.

Fix this by detecting that we are the last task in PrepareExit and, if
so, going through the PrepareGroupExit path.

My initial version of this CL had Task.exitThreadGroup set t.tg.exiting
if it wasn't already set. This leaves a small window between return from
exit(2) and the start of the runExit state where a SIGKILL could still
change the exit status. While this window likely doesn't matter much in
practice, I suspect it is observable via ptrace, which could see exit(2)
return and assume that the exit status is fixed. I didn't test this
case, but figured it would be cleaner to just close the race window.

Fixes #7930.
---
 pkg/sentry/kernel/task_exit.go | 16 +++++++++
 test/syscalls/linux/exit.cc    | 62 ++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+)

diff --git a/pkg/sentry/kernel/task_exit.go b/pkg/sentry/kernel/task_exit.go
index 684f67f12..bd2276e6f 100644
--- a/pkg/sentry/kernel/task_exit.go
+++ b/pkg/sentry/kernel/task_exit.go
@@ -117,8 +117,17 @@ func (t *Task) killedLocked() bool {
 //
 // Preconditions: The caller must be running on the task goroutine.
 func (t *Task) PrepareExit(ws linux.WaitStatus) {
+	t.tg.pidns.owner.mu.RLock()
+	defer t.tg.pidns.owner.mu.RUnlock()
 	t.tg.signalHandlers.mu.Lock()
 	defer t.tg.signalHandlers.mu.Unlock()
+
+	last := t.tg.activeTasks == 1
+	if last {
+		t.prepareGroupExitLocked(ws)
+		return
+	}
+
 	t.exitStatus = ws
 }
 
@@ -133,6 +142,13 @@ func (t *Task) PrepareExit(ws linux.WaitStatus) {
 func (t *Task) PrepareGroupExit(ws linux.WaitStatus) {
 	t.tg.signalHandlers.mu.Lock()
 	defer t.tg.signalHandlers.mu.Unlock()
+	t.prepareGroupExitLocked(ws)
+}
+
+// Preconditions:
+//   - The caller must be running on the task goroutine.
+//   - The signal mutex must be locked.
+func (t *Task) prepareGroupExitLocked(ws linux.WaitStatus) {
 	if t.tg.exiting || t.tg.execing != nil {
 		// Note that if t.tg.exiting is false but t.tg.execing is not nil, i.e.
 		// this "group exit" is being executed by the killed sibling of an
diff --git a/test/syscalls/linux/exit.cc b/test/syscalls/linux/exit.cc
index 37b927cfd..0d3011f53 100644
--- a/test/syscalls/linux/exit.cc
+++ b/test/syscalls/linux/exit.cc
@@ -99,6 +99,68 @@ void RunChild() {
   abort();
 }
 
+// SIGKILL of zombied thread group does not change exit status.
+TEST(ExitTest, SigkillZombieGroup) {
+  int pipe_fds[2];
+  ASSERT_THAT(pipe(pipe_fds), SyscallSucceeds());
+
+  FileDescriptor read_fd(pipe_fds[0]);
+  FileDescriptor write_fd(pipe_fds[1]);
+
+  pid_t pid = fork();
+  if (pid == 0) {
+    read_fd.reset();
+
+    _exit(0);
+  }
+
+  EXPECT_THAT(pid, SyscallSucceeds());
+  write_fd.reset();
+
+  // Wait for pipe to automatically close to indicate that the child is zombied.
+  char buf[10];
+  EXPECT_THAT(ReadFd(read_fd.get(), buf, sizeof(buf)),
+              SyscallSucceedsWithValue(0));
+
+  EXPECT_THAT(kill(pid, SIGKILL), SyscallSucceeds());
+
+  // SIGKILL did not change exit status.
+  int status;
+  EXPECT_THAT(RetryEINTR(waitpid)(pid, &status, 0), SyscallSucceeds());
+  EXPECT_TRUE(WIFEXITED(status) && WEXITSTATUS(status) == 0) << status;
+}
+
+// Variant of SigkillZombieGroup using exit(2) instead of exit_group(2).
+TEST(ExitTest, SigkillZombieThread) {
+  int pipe_fds[2];
+  ASSERT_THAT(pipe(pipe_fds), SyscallSucceeds());
+
+  FileDescriptor read_fd(pipe_fds[0]);
+  FileDescriptor write_fd(pipe_fds[1]);
+
+  pid_t pid = fork();
+  if (pid == 0) {
+    read_fd.reset();
+
+    syscall(SYS_exit, 0);
+  }
+
+  EXPECT_THAT(pid, SyscallSucceeds());
+  write_fd.reset();
+
+  // Wait for pipe to automatically close to indicate that the child is zombied.
+  char buf[10];
+  EXPECT_THAT(ReadFd(read_fd.get(), buf, sizeof(buf)),
+              SyscallSucceedsWithValue(0));
+
+  EXPECT_THAT(kill(pid, SIGKILL), SyscallSucceeds());
+
+  // SIGKILL did not change exit status.
+  int status;
+  EXPECT_THAT(RetryEINTR(waitpid)(pid, &status, 0), SyscallSucceeds());
+  EXPECT_TRUE(WIFEXITED(status) && WEXITSTATUS(status) == 0) << status;
+}
+
 }  // namespace
 
 }  // namespace testing