From 17563a8af926d385bc1150e2d5ac318a03e2b899 Mon Sep 17 00:00:00 2001 From: Jimmy Tran Date: Fri, 14 Feb 2025 16:08:06 -0800 Subject: [PATCH] Return EACCES when calling setpgid() after execve() From setpgid manpage, EACCES - An attempt was made to change the process group ID of one of the children of the calling process and the child had already performed an execve(2) (setpgid(), setpgrp()). This CL makes gVisor implement this rule and updates the exec test suite accordingly. TESTED: http://sponge2/7f364e8a-4f82-463e-ba62-79234c4d054d PiperOrigin-RevId: 727095560 --- pkg/sentry/kernel/sessions.go | 13 ++------ pkg/sentry/kernel/task_exec.go | 4 +-- pkg/sentry/kernel/thread_group.go | 8 +++++ pkg/sentry/syscalls/linux/sys_thread.go | 9 +++++- test/syscalls/linux/exec.cc | 42 +++++++++++++++++++++++-- 5 files changed, 60 insertions(+), 16 deletions(-) diff --git a/pkg/sentry/kernel/sessions.go b/pkg/sentry/kernel/sessions.go index 36b602e74..1575afada 100644 --- a/pkg/sentry/kernel/sessions.go +++ b/pkg/sentry/kernel/sessions.go @@ -431,13 +431,9 @@ func (tg *ThreadGroup) CreateProcessGroup() error { // JoinProcessGroup joins an existing process group. // -// This function will return EACCES if an exec has been performed since fork -// by the given ThreadGroup, and EPERM if the Sessions are not the same or the +// This function will return EPERM if the Sessions are not the same or the // group does not exist. -// -// If checkExec is set, then the join is not permitted after the process has -// executed exec at least once. -func (tg *ThreadGroup) JoinProcessGroup(pidns *PIDNamespace, pgid ProcessGroupID, checkExec bool) error { +func (tg *ThreadGroup) JoinProcessGroup(pidns *PIDNamespace, pgid ProcessGroupID) error { pidns.owner.mu.Lock() defer pidns.owner.mu.Unlock() @@ -452,11 +448,6 @@ func (tg *ThreadGroup) JoinProcessGroup(pidns *PIDNamespace, pgid ProcessGroupID return linuxerr.EPERM } - // Disallow the join if an execve has performed, per POSIX. - if checkExec && tg.execed { - return linuxerr.EACCES - } - // See if it's in the same session as ours. if pg.session != tg.processGroup.session { return linuxerr.EPERM diff --git a/pkg/sentry/kernel/task_exec.go b/pkg/sentry/kernel/task_exec.go index 170df86d2..85a662810 100644 --- a/pkg/sentry/kernel/task_exec.go +++ b/pkg/sentry/kernel/task_exec.go @@ -205,9 +205,9 @@ func (r *runSyscallAfterExecStop) execute(t *Task) taskRunState { t.signalStack = linux.SignalStack{Flags: linux.SS_DISABLE} // "The termination signal is reset to SIGCHLD (see clone(2))." t.tg.terminationSignal = linux.SIGCHLD - // execed indicates that the process can no longer join a process group + // execed indicates that the process's pgid cannot be changed // in some scenarios (namely, the parent call setpgid(2) on the child). - // See the JoinProcessGroup function in sessions.go for more context. + // See the Setpgid function in sys_thread.go for more context. t.tg.execed = true // Maximum RSS is preserved across execve(2). t.updateRSSLocked() diff --git a/pkg/sentry/kernel/thread_group.go b/pkg/sentry/kernel/thread_group.go index 26566de04..b274c5b9b 100644 --- a/pkg/sentry/kernel/thread_group.go +++ b/pkg/sentry/kernel/thread_group.go @@ -651,3 +651,11 @@ func (tg *ThreadGroup) IsInitIn(pidns *PIDNamespace) bool { func (tg *ThreadGroup) isInitInLocked(pidns *PIDNamespace) bool { return pidns.tgids[tg] == initTID } + +// Execed returns whether this ThreadGroup has execed since creation. +func (tg *ThreadGroup) Execed() bool { + ts := tg.TaskSet() + ts.mu.RLock() + defer ts.mu.RUnlock() + return tg.execed +} diff --git a/pkg/sentry/syscalls/linux/sys_thread.go b/pkg/sentry/syscalls/linux/sys_thread.go index a523b38b0..df61b96b3 100644 --- a/pkg/sentry/syscalls/linux/sys_thread.go +++ b/pkg/sentry/syscalls/linux/sys_thread.go @@ -620,6 +620,13 @@ func Setpgid(t *kernel.Task, sysno uintptr, args arch.SyscallArguments) (uintptr return 0, nil, linuxerr.EINVAL } + // Return EACCES if an attempt was made to change the process group ID of one + // of the children of the calling process and the child had + // already performed an execve(2) + if tg != t.ThreadGroup() && tg.Execed() { + return 0, nil, linuxerr.EACCES + } + // If the pgid is the same as the group, then create a new one. Otherwise, // we attempt to join an existing process group. if pgid == defaultPGID { @@ -635,7 +642,7 @@ func Setpgid(t *kernel.Task, sysno uintptr, args arch.SyscallArguments) (uintptr } } else { // Same as CreateProcessGroup, above. - if err := tg.JoinProcessGroup(t.PIDNamespace(), pgid, tg != t.ThreadGroup()); err != nil { + if err := tg.JoinProcessGroup(t.PIDNamespace(), pgid); err != nil { // See above. if t.PIDNamespace().IDOfProcessGroup(tg.ProcessGroup()) == pgid { return 0, nil, nil diff --git a/test/syscalls/linux/exec.cc b/test/syscalls/linux/exec.cc index c17d58282..e256ca901 100644 --- a/test/syscalls/linux/exec.cc +++ b/test/syscalls/linux/exec.cc @@ -17,18 +17,22 @@ #include #include #include +#include #include #include #include +#include +#include +#include +#include #include -#include #include #include +#include "gmock/gmock.h" #include "gtest/gtest.h" #include "absl/strings/match.h" -#include "absl/strings/numbers.h" #include "absl/strings/str_cat.h" #include "absl/strings/str_split.h" #include "absl/strings/string_view.h" @@ -813,6 +817,40 @@ TEST(GetpriorityTest, ExecveMaintainsPriority) { {}, W_EXITCODE(expected_exit_code, 0), ""); } +// Test that setpgid() fails on child processes after they call execve(). +TEST(ExecTest, Setpgid) { + const pid_t pid = fork(); + int status; + ASSERT_NE(pid, -1); + if (pid == 0) { + ASSERT_THAT(ptrace(PTRACE_TRACEME, 0, 0, 0), SyscallSucceeds()); + raise(SIGSTOP); + char* argv[] = {nullptr}; + char* envp[] = {nullptr}; + ASSERT_THAT(execve("/proc/self/exe", argv, envp), SyscallSucceeds()); + } + + EXPECT_THAT(setpgid(pid, pid), SyscallSucceeds()) + << "setpgid failed before execve"; + ASSERT_THAT(waitpid(pid, &status, 0), SyscallSucceedsWithValue(pid)) + << "waitpid failed"; + ASSERT_THAT(WIFSTOPPED(status), 1); + ASSERT_THAT(WSTOPSIG(status), SIGSTOP); + ASSERT_THAT( + ptrace(PTRACE_SETOPTIONS, pid, 0, PTRACE_O_EXITKILL | PTRACE_O_TRACEEXEC), + SyscallSucceeds()) + << "ptrace failed"; + ASSERT_THAT(ptrace(PTRACE_CONT, pid, 0, 0), SyscallSucceeds()) + << "ptrace (PTRACE_CONT) failed"; + ASSERT_THAT(waitpid(pid, &status, 0), SyscallSucceedsWithValue(pid)) + << "waitpid failed"; + ASSERT_THAT(WIFSTOPPED(status), 1); + ASSERT_THAT(WSTOPSIG(status), SIGTRAP); + EXPECT_THAT(setpgid(pid, pid), SyscallFailsWithErrno(EACCES)); + EXPECT_THAT(setpgid(pid, getpid()), SyscallFailsWithErrno(EACCES)); + EXPECT_THAT(setpgid(getpid(), pid), SyscallSucceeds()); +} + void ExecWithThread() { // Used to ensure that the thread has actually started. absl::Mutex mu;