From 8ed06ad814734430193f3673b5e00861eda9aa47 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Sun, 21 Aug 2022 12:18:26 +0200 Subject: [PATCH] Kernel: Guard Process "protected data" with a spinlock This ensures that both mutable and immutable access to the protected data of a process is serialized. Note that there may still be multiple TOCTOU issues around this, as we have a bunch of convenience accessors that make it easy to introduce them. We'll need to audit those as well. --- Kernel/Process.cpp | 76 ++++---- Kernel/Process.h | 86 ++++++--- Kernel/Syscalls/disown.cpp | 5 +- Kernel/Syscalls/execve.cpp | 26 ++- Kernel/Syscalls/exit.cpp | 9 +- Kernel/Syscalls/fork.cpp | 25 +-- Kernel/Syscalls/pledge.cpp | 55 +++--- Kernel/Syscalls/process.cpp | 2 +- Kernel/Syscalls/setpgid.cpp | 7 +- Kernel/Syscalls/setuid.cpp | 348 ++++++++++++++++++------------------ Kernel/Syscalls/umask.cpp | 9 +- 11 files changed, 348 insertions(+), 300 deletions(-) diff --git a/Kernel/Process.cpp b/Kernel/Process.cpp index c069d0f0ed..5d922a3552 100644 --- a/Kernel/Process.cpp +++ b/Kernel/Process.cpp @@ -211,14 +211,14 @@ LockRefPtr Process::create_kernel_process(LockRefPtr& first_thr void Process::protect_data() { m_protected_data_refs.unref([&]() { - MM.set_page_writable_direct(VirtualAddress { &this->m_protected_values }, false); + MM.set_page_writable_direct(VirtualAddress { &this->m_protected_values_do_not_access_directly }, false); }); } void Process::unprotect_data() { m_protected_data_refs.ref([&]() { - MM.set_page_writable_direct(VirtualAddress { &this->m_protected_values }, true); + MM.set_page_writable_direct(VirtualAddress { &this->m_protected_values_do_not_access_directly }, true); }); } @@ -234,6 +234,7 @@ ErrorOr> Process::try_create(LockRefPtr& firs Process::Process(NonnullOwnPtr name, NonnullRefPtr credentials, ProcessID ppid, bool is_kernel_process, RefPtr current_directory, RefPtr executable, TTY* tty, UnveilNode unveil_tree) : m_name(move(name)) + , m_protected_data_lock(LockRank::None) , m_is_kernel_process(is_kernel_process) , m_executable(LockRank::None, move(executable)) , m_current_directory(LockRank::None, move(current_directory)) @@ -242,11 +243,11 @@ Process::Process(NonnullOwnPtr name, NonnullRefPtr credent , m_wait_blocker_set(*this) { // Ensure that we protect the process data when exiting the constructor. - ProtectedDataMutationScope scope { *this }; - - m_protected_values.pid = allocate_pid(); - m_protected_values.ppid = ppid; - m_protected_values.credentials = move(credentials); + with_mutable_protected_data([&](auto& protected_data) { + protected_data.pid = allocate_pid(); + protected_data.ppid = ppid; + protected_data.credentials = move(credentials); + }); dbgln_if(PROCESS_DEBUG, "Created new process {}({})", m_name, this->pid().value()); } @@ -402,10 +403,9 @@ void Process::crash(int signal, FlatPtr ip, bool out_of_memory) } dump_backtrace(); } - { - ProtectedDataMutationScope scope { *this }; - m_protected_values.termination_signal = signal; - } + with_mutable_protected_data([&](auto& protected_data) { + protected_data.termination_signal = signal; + }); set_should_generate_coredump(!out_of_memory); address_space().dump_regions(); VERIFY(is_user_process()); @@ -527,13 +527,15 @@ siginfo_t Process::wait_info() const siginfo.si_pid = pid().value(); siginfo.si_uid = uid().value(); - if (m_protected_values.termination_signal != 0) { - siginfo.si_status = m_protected_values.termination_signal; - siginfo.si_code = CLD_KILLED; - } else { - siginfo.si_status = m_protected_values.termination_status; - siginfo.si_code = CLD_EXITED; - } + with_protected_data([&](auto& protected_data) { + if (protected_data.termination_signal != 0) { + siginfo.si_status = protected_data.termination_signal; + siginfo.si_code = CLD_KILLED; + } else { + siginfo.si_status = protected_data.termination_status; + siginfo.si_code = CLD_EXITED; + } + }); return siginfo; } @@ -619,7 +621,7 @@ void Process::finalize() dbgln("\x1b[01;31mProcess '{}' exited with the veil left open\x1b[0m", name()); if (g_init_pid != 0 && pid() == g_init_pid) - PANIC("Init process quit unexpectedly. Exit code: {}", m_protected_values.termination_status); + PANIC("Init process quit unexpectedly. Exit code: {}", termination_status()); if (is_dumpable()) { if (m_should_generate_coredump) { @@ -741,11 +743,10 @@ void Process::terminate_due_to_signal(u8 signal) VERIFY(signal < NSIG); VERIFY(&Process::current() == this); dbgln("Terminating {} due to signal {}", *this, signal); - { - ProtectedDataMutationScope scope { *this }; - m_protected_values.termination_status = 0; - m_protected_values.termination_signal = signal; - } + with_mutable_protected_data([&](auto& protected_data) { + protected_data.termination_status = 0; + protected_data.termination_signal = signal; + }); die(); } @@ -851,31 +852,34 @@ void Process::delete_perf_events_buffer() bool Process::remove_thread(Thread& thread) { - ProtectedDataMutationScope scope { *this }; - auto thread_cnt_before = m_protected_values.thread_count.fetch_sub(1, AK::MemoryOrder::memory_order_acq_rel); - VERIFY(thread_cnt_before != 0); + u32 thread_count_before = 0; thread_list().with([&](auto& thread_list) { thread_list.remove(thread); + with_mutable_protected_data([&](auto& protected_data) { + thread_count_before = protected_data.thread_count.fetch_sub(1, AK::MemoryOrder::memory_order_acq_rel); + VERIFY(thread_count_before != 0); + }); }); - return thread_cnt_before == 1; + return thread_count_before == 1; } bool Process::add_thread(Thread& thread) { - ProtectedDataMutationScope scope { *this }; - bool is_first = m_protected_values.thread_count.fetch_add(1, AK::MemoryOrder::memory_order_relaxed) == 0; + bool is_first = false; thread_list().with([&](auto& thread_list) { thread_list.append(thread); + with_mutable_protected_data([&](auto& protected_data) { + is_first = protected_data.thread_count.fetch_add(1, AK::MemoryOrder::memory_order_relaxed) == 0; + }); }); return is_first; } void Process::set_dumpable(bool dumpable) { - if (dumpable == m_protected_values.dumpable) - return; - ProtectedDataMutationScope scope { *this }; - m_protected_values.dumpable = dumpable; + with_mutable_protected_data([&](auto& protected_data) { + protected_data.dumpable = dumpable; + }); } ErrorOr Process::set_coredump_property(NonnullOwnPtr key, NonnullOwnPtr value) @@ -968,7 +972,9 @@ GroupID Process::sgid() const NonnullRefPtr Process::credentials() const { - return *m_protected_values.credentials; + return with_protected_data([&](auto& protected_data) -> NonnullRefPtr { + return *protected_data.credentials; + }); } RefPtr Process::executable() diff --git a/Kernel/Process.h b/Kernel/Process.h index 345cc4e343..43c35c463a 100644 --- a/Kernel/Process.h +++ b/Kernel/Process.h @@ -135,20 +135,19 @@ public: friend class Thread; friend class Coredump; - // Helper class to temporarily unprotect a process's protected data so you can write to it. - class ProtectedDataMutationScope { - public: - explicit ProtectedDataMutationScope(Process& process) - : m_process(process) - { - m_process.unprotect_data(); - } + auto with_protected_data(auto&& callback) const + { + SpinlockLocker locker(m_protected_data_lock); + return callback(m_protected_values_do_not_access_directly); + } - ~ProtectedDataMutationScope() { m_process.protect_data(); } - - private: - Process& m_process; - }; + auto with_mutable_protected_data(auto&& callback) + { + SpinlockLocker locker(m_protected_data_lock); + unprotect_data(); + auto guard = ScopeGuard([&] { protect_data(); }); + return callback(m_protected_values_do_not_access_directly); + } enum class State : u8 { Running = 0, @@ -218,12 +217,21 @@ public: static SessionID get_sid_from_pgid(ProcessGroupID pgid); StringView name() const { return m_name->view(); } - ProcessID pid() const { return m_protected_values.pid; } - SessionID sid() const { return m_protected_values.sid; } + ProcessID pid() const + { + return with_protected_data([](auto& protected_data) { return protected_data.pid; }); + } + SessionID sid() const + { + return with_protected_data([](auto& protected_data) { return protected_data.sid; }); + } bool is_session_leader() const { return sid().value() == pid().value(); } ProcessGroupID pgid() const { return m_pg ? m_pg->pgid() : 0; } bool is_group_leader() const { return pgid().value() == pid().value(); } - ProcessID ppid() const { return m_protected_values.ppid; } + ProcessID ppid() const + { + return with_protected_data([](auto& protected_data) { return protected_data.ppid; }); + } NonnullRefPtr credentials() const; @@ -234,10 +242,16 @@ public: UserID suid() const; GroupID sgid() const; - bool is_dumpable() const { return m_protected_values.dumpable; } + bool is_dumpable() const + { + return with_protected_data([](auto& protected_data) { return protected_data.dumpable; }); + } void set_dumpable(bool); - mode_t umask() const { return m_protected_values.umask; } + mode_t umask() const + { + return with_protected_data([](auto& protected_data) { return protected_data.umask; }); + } bool in_group(GroupID) const; @@ -467,19 +481,37 @@ public: void terminate_due_to_signal(u8 signal); ErrorOr send_signal(u8 signal, Process* sender); - u8 termination_signal() const { return m_protected_values.termination_signal; } - u8 termination_status() const { return m_protected_values.termination_status; } + u8 termination_signal() const + { + return with_protected_data([](auto& protected_data) -> u8 { + return protected_data.termination_signal; + }); + } + u8 termination_status() const + { + return with_protected_data([](auto& protected_data) { return protected_data.termination_status; }); + } u16 thread_count() const { - return m_protected_values.thread_count.load(AK::MemoryOrder::memory_order_relaxed); + return with_protected_data([](auto& protected_data) { + return protected_data.thread_count.load(AK::MemoryOrder::memory_order_relaxed); + }); } Mutex& big_lock() { return m_big_lock; } Mutex& ptrace_lock() { return m_ptrace_lock; } - bool has_promises() const { return m_protected_values.has_promises; } - bool has_promised(Pledge pledge) const { return (m_protected_values.promises & (1U << (u32)pledge)) != 0; } + bool has_promises() const + { + return with_protected_data([](auto& protected_data) { return protected_data.has_promises.load(); }); + } + bool has_promised(Pledge pledge) const + { + return with_protected_data([&](auto& protected_data) { + return (protected_data.promises & (1U << (u32)pledge)) != 0; + }); + } VeilState veil_state() const { @@ -539,7 +571,10 @@ public: Memory::AddressSpace& address_space() { return *m_space; } Memory::AddressSpace const& address_space() const { return *m_space; } - VirtualAddress signal_trampoline() const { return m_protected_values.signal_trampoline; } + VirtualAddress signal_trampoline() const + { + return with_protected_data([](auto& protected_data) { return protected_data.signal_trampoline; }); + } ErrorOr require_promise(Pledge); ErrorOr require_no_promises() const; @@ -635,6 +670,7 @@ private: LockRefPtr m_pg; + RecursiveSpinlock mutable m_protected_data_lock; AtomicEdgeAction m_protected_data_refs; void protect_data(); void unprotect_data(); @@ -870,7 +906,7 @@ private: Array m_signal_action_data; static_assert(sizeof(ProtectedValues) < (PAGE_SIZE)); - alignas(4096) ProtectedValues m_protected_values; + alignas(4096) ProtectedValues m_protected_values_do_not_access_directly; u8 m_protected_values_padding[PAGE_SIZE - sizeof(ProtectedValues)]; public: diff --git a/Kernel/Syscalls/disown.cpp b/Kernel/Syscalls/disown.cpp index 46bb38f3dc..55c5f3d05f 100644 --- a/Kernel/Syscalls/disown.cpp +++ b/Kernel/Syscalls/disown.cpp @@ -17,8 +17,9 @@ ErrorOr Process::sys$disown(ProcessID pid) return ESRCH; if (process->ppid() != this->pid()) return ECHILD; - ProtectedDataMutationScope scope(*process); - process->m_protected_values.ppid = 0; + process->with_mutable_protected_data([](auto& protected_data) { + protected_data.ppid = 0; + }); process->disowned_by_waiter(*this); return 0; } diff --git a/Kernel/Syscalls/execve.cpp b/Kernel/Syscalls/execve.cpp index 405c9dbc8c..ff4c1cb718 100644 --- a/Kernel/Syscalls/execve.cpp +++ b/Kernel/Syscalls/execve.cpp @@ -542,11 +542,10 @@ ErrorOr Process::do_exec(NonnullLockRefPtr main_progr kill_threads_except_self(); - { - ProtectedDataMutationScope scope { *this }; - m_protected_values.credentials = move(new_credentials); - } - set_dumpable(!executable_is_setid); + with_mutable_protected_data([&](auto& protected_data) { + protected_data.credentials = move(new_credentials); + protected_data.dumpable = !executable_is_setid; + }); // We make sure to enter the new address space before destroying the old one. // This ensures that the process always has a valid page directory. @@ -627,19 +626,18 @@ ErrorOr Process::do_exec(NonnullLockRefPtr main_progr // NOTE: Be careful to not trigger any page faults below! - { - ProtectedDataMutationScope scope { *this }; - m_protected_values.promises = m_protected_values.execpromises.load(); - m_protected_values.has_promises = m_protected_values.has_execpromises.load(); + with_mutable_protected_data([&](auto& protected_data) { + protected_data.promises = protected_data.execpromises.load(); + protected_data.has_promises = protected_data.has_execpromises.load(); - m_protected_values.execpromises = 0; - m_protected_values.has_execpromises = false; + protected_data.execpromises = 0; + protected_data.has_execpromises = false; - m_protected_values.signal_trampoline = signal_trampoline_region->vaddr(); + protected_data.signal_trampoline = signal_trampoline_region->vaddr(); // FIXME: PID/TID ISSUE - m_protected_values.pid = new_main_thread->tid().value(); - } + protected_data.pid = new_main_thread->tid().value(); + }); auto tsr_result = new_main_thread->make_thread_specific_region({}); if (tsr_result.is_error()) { diff --git a/Kernel/Syscalls/exit.cpp b/Kernel/Syscalls/exit.cpp index 087ac29fae..cf1f0c400e 100644 --- a/Kernel/Syscalls/exit.cpp +++ b/Kernel/Syscalls/exit.cpp @@ -18,11 +18,10 @@ void Process::sys$exit(int status) VERIFY_PROCESS_BIG_LOCK_ACQUIRED(this); } - { - ProtectedDataMutationScope scope { *this }; - m_protected_values.termination_status = status; - m_protected_values.termination_signal = 0; - } + with_mutable_protected_data([status](auto& protected_data) { + protected_data.termination_status = status; + protected_data.termination_signal = 0; + }); auto* current_thread = Thread::current(); current_thread->set_profiling_suppressed(); diff --git a/Kernel/Syscalls/fork.cpp b/Kernel/Syscalls/fork.cpp index 2009f743a7..a0e209fa4a 100644 --- a/Kernel/Syscalls/fork.cpp +++ b/Kernel/Syscalls/fork.cpp @@ -49,18 +49,19 @@ ErrorOr Process::sys$fork(RegisterState& regs) child->m_pg = m_pg; - { - ProtectedDataMutationScope scope { *child }; - child->m_protected_values.promises = m_protected_values.promises.load(); - child->m_protected_values.execpromises = m_protected_values.execpromises.load(); - child->m_protected_values.has_promises = m_protected_values.has_promises.load(); - child->m_protected_values.has_execpromises = m_protected_values.has_execpromises.load(); - child->m_protected_values.sid = m_protected_values.sid; - child->m_protected_values.credentials = m_protected_values.credentials; - child->m_protected_values.umask = m_protected_values.umask; - child->m_protected_values.signal_trampoline = m_protected_values.signal_trampoline; - child->m_protected_values.dumpable = m_protected_values.dumpable; - } + with_protected_data([&](auto& my_protected_data) { + child->with_mutable_protected_data([&](auto& child_protected_data) { + child_protected_data.promises = my_protected_data.promises.load(); + child_protected_data.execpromises = my_protected_data.execpromises.load(); + child_protected_data.has_promises = my_protected_data.has_promises.load(); + child_protected_data.has_execpromises = my_protected_data.has_execpromises.load(); + child_protected_data.sid = my_protected_data.sid; + child_protected_data.credentials = my_protected_data.credentials; + child_protected_data.umask = my_protected_data.umask; + child_protected_data.signal_trampoline = my_protected_data.signal_trampoline; + child_protected_data.dumpable = my_protected_data.dumpable; + }); + }); dbgln_if(FORK_DEBUG, "fork: child={}", child); child->address_space().set_enforces_syscall_regions(address_space().enforces_syscall_regions()); diff --git a/Kernel/Syscalls/pledge.cpp b/Kernel/Syscalls/pledge.cpp index c32399bba9..8b2412196e 100644 --- a/Kernel/Syscalls/pledge.cpp +++ b/Kernel/Syscalls/pledge.cpp @@ -46,43 +46,48 @@ ErrorOr Process::sys$pledge(Userspace if (promises) { if (!parse_pledge(promises->view(), new_promises)) return EINVAL; - - if (m_protected_values.has_promises && (new_promises & ~m_protected_values.promises)) { - if (!(m_protected_values.promises & (1u << (u32)Pledge::no_error))) - return EPERM; - new_promises &= m_protected_values.promises; - } } u32 new_execpromises = 0; if (execpromises) { if (!parse_pledge(execpromises->view(), new_execpromises)) return EINVAL; - if (m_protected_values.has_execpromises && (new_execpromises & ~m_protected_values.execpromises)) { - if (!(m_protected_values.promises & (1u << (u32)Pledge::no_error))) - return EPERM; - new_execpromises &= m_protected_values.execpromises; + } + + return with_mutable_protected_data([&](auto& protected_data) -> ErrorOr { + if (promises) { + if (protected_data.has_promises && (new_promises & ~protected_data.promises)) { + if (!(protected_data.promises & (1u << (u32)Pledge::no_error))) + return EPERM; + new_promises &= protected_data.promises; + } } - } - // Only apply promises after all validation has occurred, this ensures - // we don't introduce logic bugs like applying the promises, and then - // erroring out when parsing the exec promises later. Such bugs silently - // leave the caller in an unexpected state. + if (execpromises) { + if (protected_data.has_execpromises && (new_execpromises & ~protected_data.execpromises)) { + if (!(protected_data.promises & (1u << (u32)Pledge::no_error))) + return EPERM; + new_execpromises &= protected_data.execpromises; + } + } - ProtectedDataMutationScope scope { *this }; + // Only apply promises after all validation has occurred, this ensures + // we don't introduce logic bugs like applying the promises, and then + // erroring out when parsing the exec promises later. Such bugs silently + // leave the caller in an unexpected state. - if (promises) { - m_protected_values.has_promises = true; - m_protected_values.promises = new_promises; - } + if (promises) { + protected_data.has_promises = true; + protected_data.promises = new_promises; + } - if (execpromises) { - m_protected_values.has_execpromises = true; - m_protected_values.execpromises = new_execpromises; - } + if (execpromises) { + protected_data.has_execpromises = true; + protected_data.execpromises = new_execpromises; + } - return 0; + return 0; + }); } } diff --git a/Kernel/Syscalls/process.cpp b/Kernel/Syscalls/process.cpp index 209098ce6c..338e05b413 100644 --- a/Kernel/Syscalls/process.cpp +++ b/Kernel/Syscalls/process.cpp @@ -20,7 +20,7 @@ ErrorOr Process::sys$getppid() { VERIFY_PROCESS_BIG_LOCK_ACQUIRED(this); TRY(require_promise(Pledge::stdio)); - return m_protected_values.ppid.value(); + return with_protected_data([](auto& protected_data) { return protected_data.ppid.value(); }); } ErrorOr Process::sys$get_process_name(Userspace buffer, size_t buffer_size) diff --git a/Kernel/Syscalls/setpgid.cpp b/Kernel/Syscalls/setpgid.cpp index 92dbc681ba..b6473484e7 100644 --- a/Kernel/Syscalls/setpgid.cpp +++ b/Kernel/Syscalls/setpgid.cpp @@ -40,9 +40,10 @@ ErrorOr Process::sys$setsid() m_pg = TRY(ProcessGroup::try_create(ProcessGroupID(pid().value()))); m_tty = nullptr; - ProtectedDataMutationScope scope { *this }; - m_protected_values.sid = pid().value(); - return sid().value(); + return with_mutable_protected_data([&](auto& protected_data) -> ErrorOr { + protected_data.sid = pid().value(); + return protected_data.sid.value(); + }); } ErrorOr Process::sys$getpgid(pid_t pid) diff --git a/Kernel/Syscalls/setuid.cpp b/Kernel/Syscalls/setuid.cpp index 2039df0839..5193a4b77b 100644 --- a/Kernel/Syscalls/setuid.cpp +++ b/Kernel/Syscalls/setuid.cpp @@ -17,27 +17,27 @@ ErrorOr Process::sys$seteuid(UserID new_euid) if (new_euid == (uid_t)-1) return EINVAL; - auto credentials = this->credentials(); + return with_mutable_protected_data([&](auto& protected_data) -> ErrorOr { + auto credentials = this->credentials(); - if (new_euid != credentials->uid() && new_euid != credentials->suid() && !credentials->is_superuser()) - return EPERM; + if (new_euid != credentials->uid() && new_euid != credentials->suid() && !credentials->is_superuser()) + return EPERM; - auto new_credentials = TRY(Credentials::create( - credentials->uid(), - credentials->gid(), - new_euid, - credentials->egid(), - credentials->suid(), - credentials->sgid(), - credentials->extra_gids())); + auto new_credentials = TRY(Credentials::create( + credentials->uid(), + credentials->gid(), + new_euid, + credentials->egid(), + credentials->suid(), + credentials->sgid(), + credentials->extra_gids())); - ProtectedDataMutationScope scope { *this }; + if (credentials->euid() != new_euid) + protected_data.dumpable = false; - if (credentials->euid() != new_euid) - set_dumpable(false); - - m_protected_values.credentials = move(new_credentials); - return 0; + protected_data.credentials = move(new_credentials); + return 0; + }); } ErrorOr Process::sys$setegid(GroupID new_egid) @@ -48,27 +48,27 @@ ErrorOr Process::sys$setegid(GroupID new_egid) if (new_egid == (uid_t)-1) return EINVAL; - auto credentials = this->credentials(); + return with_mutable_protected_data([&](auto& protected_data) -> ErrorOr { + auto credentials = this->credentials(); - if (new_egid != credentials->gid() && new_egid != credentials->sgid() && !credentials->is_superuser()) - return EPERM; + if (new_egid != credentials->gid() && new_egid != credentials->sgid() && !credentials->is_superuser()) + return EPERM; - auto new_credentials = TRY(Credentials::create( - credentials->uid(), - credentials->gid(), - credentials->euid(), - new_egid, - credentials->suid(), - credentials->sgid(), - credentials->extra_gids())); + auto new_credentials = TRY(Credentials::create( + credentials->uid(), + credentials->gid(), + credentials->euid(), + new_egid, + credentials->suid(), + credentials->sgid(), + credentials->extra_gids())); - ProtectedDataMutationScope scope { *this }; + if (credentials->egid() != new_egid) + protected_data.dumpable = false; - if (credentials->egid() != new_egid) - set_dumpable(false); - - m_protected_values.credentials = move(new_credentials); - return 0; + protected_data.credentials = move(new_credentials); + return 0; + }); } ErrorOr Process::sys$setuid(UserID new_uid) @@ -79,27 +79,27 @@ ErrorOr Process::sys$setuid(UserID new_uid) if (new_uid == (uid_t)-1) return EINVAL; - auto credentials = this->credentials(); + return with_mutable_protected_data([&](auto& protected_data) -> ErrorOr { + auto credentials = this->credentials(); - if (new_uid != credentials->uid() && new_uid != credentials->euid() && !credentials->is_superuser()) - return EPERM; + if (new_uid != credentials->uid() && new_uid != credentials->euid() && !credentials->is_superuser()) + return EPERM; - auto new_credentials = TRY(Credentials::create( - new_uid, - credentials->gid(), - new_uid, - credentials->egid(), - new_uid, - credentials->sgid(), - credentials->extra_gids())); + auto new_credentials = TRY(Credentials::create( + new_uid, + credentials->gid(), + new_uid, + credentials->egid(), + new_uid, + credentials->sgid(), + credentials->extra_gids())); - ProtectedDataMutationScope scope { *this }; + if (credentials->euid() != new_uid) + protected_data.dumpable = false; - if (credentials->euid() != new_uid) - set_dumpable(false); - - m_protected_values.credentials = move(new_credentials); - return 0; + protected_data.credentials = move(new_credentials); + return 0; + }); } ErrorOr Process::sys$setgid(GroupID new_gid) @@ -110,27 +110,27 @@ ErrorOr Process::sys$setgid(GroupID new_gid) if (new_gid == (uid_t)-1) return EINVAL; - auto credentials = this->credentials(); + return with_mutable_protected_data([&](auto& protected_data) -> ErrorOr { + auto credentials = this->credentials(); - if (new_gid != credentials->gid() && new_gid != credentials->egid() && !credentials->is_superuser()) - return EPERM; + if (new_gid != credentials->gid() && new_gid != credentials->egid() && !credentials->is_superuser()) + return EPERM; - auto new_credentials = TRY(Credentials::create( - credentials->uid(), - new_gid, - credentials->euid(), - new_gid, - credentials->suid(), - new_gid, - credentials->extra_gids())); + auto new_credentials = TRY(Credentials::create( + credentials->uid(), + new_gid, + credentials->euid(), + new_gid, + credentials->suid(), + new_gid, + credentials->extra_gids())); - ProtectedDataMutationScope scope { *this }; + if (credentials->egid() != new_gid) + protected_data.dumpable = false; - if (credentials->egid() != new_gid) - set_dumpable(false); - - m_protected_values.credentials = move(new_credentials); - return 0; + protected_data.credentials = move(new_credentials); + return 0; + }); } ErrorOr Process::sys$setreuid(UserID new_ruid, UserID new_euid) @@ -138,36 +138,36 @@ ErrorOr Process::sys$setreuid(UserID new_ruid, UserID new_euid) VERIFY_NO_PROCESS_BIG_LOCK(this); TRY(require_promise(Pledge::id)); - auto credentials = this->credentials(); + return with_mutable_protected_data([&](auto& protected_data) -> ErrorOr { + auto credentials = this->credentials(); - if (new_ruid == (uid_t)-1) - new_ruid = credentials->uid(); - if (new_euid == (uid_t)-1) - new_euid = credentials->euid(); + if (new_ruid == (uid_t)-1) + new_ruid = credentials->uid(); + if (new_euid == (uid_t)-1) + new_euid = credentials->euid(); - auto ok = [&credentials](UserID id) { return id == credentials->uid() || id == credentials->euid() || id == credentials->suid(); }; - if (!ok(new_ruid) || !ok(new_euid)) - return EPERM; + auto ok = [&credentials](UserID id) { return id == credentials->uid() || id == credentials->euid() || id == credentials->suid(); }; + if (!ok(new_ruid) || !ok(new_euid)) + return EPERM; - if (new_ruid < (uid_t)-1 || new_euid < (uid_t)-1) - return EINVAL; + if (new_ruid < (uid_t)-1 || new_euid < (uid_t)-1) + return EINVAL; - auto new_credentials = TRY(Credentials::create( - new_ruid, - credentials->gid(), - new_euid, - credentials->egid(), - credentials->suid(), - credentials->sgid(), - credentials->extra_gids())); + auto new_credentials = TRY(Credentials::create( + new_ruid, + credentials->gid(), + new_euid, + credentials->egid(), + credentials->suid(), + credentials->sgid(), + credentials->extra_gids())); - ProtectedDataMutationScope scope { *this }; + if (credentials->euid() != new_euid) + protected_data.dumpable = false; - if (credentials->euid() != new_euid) - set_dumpable(false); - - m_protected_values.credentials = move(new_credentials); - return 0; + protected_data.credentials = move(new_credentials); + return 0; + }); } ErrorOr Process::sys$setresuid(UserID new_ruid, UserID new_euid, UserID new_suid) @@ -175,35 +175,35 @@ ErrorOr Process::sys$setresuid(UserID new_ruid, UserID new_euid, UserID VERIFY_NO_PROCESS_BIG_LOCK(this); TRY(require_promise(Pledge::id)); - auto credentials = this->credentials(); + return with_mutable_protected_data([&](auto& protected_data) -> ErrorOr { + auto credentials = this->credentials(); - if (new_ruid == (uid_t)-1) - new_ruid = credentials->uid(); - if (new_euid == (uid_t)-1) - new_euid = credentials->euid(); - if (new_suid == (uid_t)-1) - new_suid = credentials->suid(); + if (new_ruid == (uid_t)-1) + new_ruid = credentials->uid(); + if (new_euid == (uid_t)-1) + new_euid = credentials->euid(); + if (new_suid == (uid_t)-1) + new_suid = credentials->suid(); - auto ok = [&credentials](UserID id) { return id == credentials->uid() || id == credentials->euid() || id == credentials->suid(); }; - if ((!ok(new_ruid) || !ok(new_euid) || !ok(new_suid)) && !credentials->is_superuser()) - return EPERM; + auto ok = [&credentials](UserID id) { return id == credentials->uid() || id == credentials->euid() || id == credentials->suid(); }; + if ((!ok(new_ruid) || !ok(new_euid) || !ok(new_suid)) && !credentials->is_superuser()) + return EPERM; - auto new_credentials = TRY(Credentials::create( - new_ruid, - credentials->gid(), - new_euid, - credentials->egid(), - new_suid, - credentials->sgid(), - credentials->extra_gids())); + auto new_credentials = TRY(Credentials::create( + new_ruid, + credentials->gid(), + new_euid, + credentials->egid(), + new_suid, + credentials->sgid(), + credentials->extra_gids())); - ProtectedDataMutationScope scope { *this }; + if (credentials->euid() != new_euid) + protected_data.dumpable = false; - if (credentials->euid() != new_euid) - set_dumpable(false); - - m_protected_values.credentials = move(new_credentials); - return 0; + protected_data.credentials = move(new_credentials); + return 0; + }); } ErrorOr Process::sys$setresgid(GroupID new_rgid, GroupID new_egid, GroupID new_sgid) @@ -211,35 +211,35 @@ ErrorOr Process::sys$setresgid(GroupID new_rgid, GroupID new_egid, Grou VERIFY_NO_PROCESS_BIG_LOCK(this); TRY(require_promise(Pledge::id)); - auto credentials = this->credentials(); + return with_mutable_protected_data([&](auto& protected_data) -> ErrorOr { + auto credentials = this->credentials(); - if (new_rgid == (gid_t)-1) - new_rgid = credentials->gid(); - if (new_egid == (gid_t)-1) - new_egid = credentials->egid(); - if (new_sgid == (gid_t)-1) - new_sgid = credentials->sgid(); + if (new_rgid == (gid_t)-1) + new_rgid = credentials->gid(); + if (new_egid == (gid_t)-1) + new_egid = credentials->egid(); + if (new_sgid == (gid_t)-1) + new_sgid = credentials->sgid(); - auto ok = [&credentials](GroupID id) { return id == credentials->gid() || id == credentials->egid() || id == credentials->sgid(); }; - if ((!ok(new_rgid) || !ok(new_egid) || !ok(new_sgid)) && !credentials->is_superuser()) - return EPERM; + auto ok = [&credentials](GroupID id) { return id == credentials->gid() || id == credentials->egid() || id == credentials->sgid(); }; + if ((!ok(new_rgid) || !ok(new_egid) || !ok(new_sgid)) && !credentials->is_superuser()) + return EPERM; - auto new_credentials = TRY(Credentials::create( - credentials->uid(), - new_rgid, - credentials->euid(), - new_egid, - credentials->suid(), - new_sgid, - credentials->extra_gids())); + auto new_credentials = TRY(Credentials::create( + credentials->uid(), + new_rgid, + credentials->euid(), + new_egid, + credentials->suid(), + new_sgid, + credentials->extra_gids())); - ProtectedDataMutationScope scope { *this }; + if (credentials->egid() != new_egid) + protected_data.dumpable = false; - if (credentials->egid() != new_egid) - set_dumpable(false); - - m_protected_values.credentials = move(new_credentials); - return 0; + protected_data.credentials = move(new_credentials); + return 0; + }); } ErrorOr Process::sys$setgroups(size_t count, Userspace user_gids) @@ -250,49 +250,49 @@ ErrorOr Process::sys$setgroups(size_t count, Userspace if (count > NGROUPS_MAX) return EINVAL; - auto credentials = this->credentials(); + return with_mutable_protected_data([&](auto& protected_data) -> ErrorOr { + auto credentials = this->credentials(); - if (!credentials->is_superuser()) - return EPERM; + if (!credentials->is_superuser()) + return EPERM; - if (!count) { - ProtectedDataMutationScope scope { *this }; - m_protected_values.credentials = TRY(Credentials::create( + if (!count) { + protected_data.credentials = TRY(Credentials::create( + credentials->uid(), + credentials->gid(), + credentials->euid(), + credentials->egid(), + credentials->suid(), + credentials->sgid(), + {})); + return 0; + } + + Vector new_extra_gids; + TRY(new_extra_gids.try_resize(count)); + TRY(copy_n_from_user(new_extra_gids.data(), user_gids, count)); + + HashTable unique_extra_gids; + for (auto& extra_gid : new_extra_gids) { + if (extra_gid != credentials->gid()) + TRY(unique_extra_gids.try_set(extra_gid)); + } + + new_extra_gids.clear_with_capacity(); + for (auto extra_gid : unique_extra_gids) { + TRY(new_extra_gids.try_append(extra_gid)); + } + + protected_data.credentials = TRY(Credentials::create( credentials->uid(), credentials->gid(), credentials->euid(), credentials->egid(), credentials->suid(), credentials->sgid(), - {})); + new_extra_gids.span())); return 0; - } - - Vector new_extra_gids; - TRY(new_extra_gids.try_resize(count)); - TRY(copy_n_from_user(new_extra_gids.data(), user_gids, count)); - - HashTable unique_extra_gids; - for (auto& extra_gid : new_extra_gids) { - if (extra_gid != credentials->gid()) - TRY(unique_extra_gids.try_set(extra_gid)); - } - - new_extra_gids.clear_with_capacity(); - for (auto extra_gid : unique_extra_gids) { - TRY(new_extra_gids.try_append(extra_gid)); - } - - ProtectedDataMutationScope scope { *this }; - m_protected_values.credentials = TRY(Credentials::create( - credentials->uid(), - credentials->gid(), - credentials->euid(), - credentials->egid(), - credentials->suid(), - credentials->sgid(), - new_extra_gids.span())); - return 0; + }); } } diff --git a/Kernel/Syscalls/umask.cpp b/Kernel/Syscalls/umask.cpp index 0ec8a97c3b..d7031c0568 100644 --- a/Kernel/Syscalls/umask.cpp +++ b/Kernel/Syscalls/umask.cpp @@ -12,10 +12,11 @@ ErrorOr Process::sys$umask(mode_t mask) { VERIFY_PROCESS_BIG_LOCK_ACQUIRED(this); TRY(require_promise(Pledge::stdio)); - auto old_mask = m_protected_values.umask; - ProtectedDataMutationScope scope { *this }; - m_protected_values.umask = mask & 0777; - return old_mask; + return with_mutable_protected_data([&](auto& protected_data) -> ErrorOr { + auto old_mask = protected_data.umask; + protected_data.umask = mask & 0777; + return old_mask; + }); } }