From 90c0f9664ea96d3d4c23eaba8a431f6e57db9e6c Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Thu, 11 Mar 2021 13:13:05 +0100 Subject: [PATCH] Kernel: Don't keep protected Process data in a separate allocation The previous architecture had a huge flaw: the pointer to the protected data was itself unprotected, allowing you to overwrite it at any time. This patch reorganizes the protected data so it's part of the Process class itself. (Actually, it's a new ProcessBase helper class.) We use the first 4 KB of Process objects themselves as the new storage location for protected data. Then we make Process objects page-aligned using MAKE_ALIGNED_ALLOCATED. This allows us to easily turn on/off write-protection for everything in the ProcessBase portion of Process. :^) Thanks to @bugaevc for pointing out the flaw! This is still not perfect but it's an improvement. --- Kernel/Process.cpp | 46 +++++++------------ Kernel/Process.h | 88 ++++++++++++++++++++----------------- Kernel/Syscalls/disown.cpp | 3 +- Kernel/Syscalls/execve.cpp | 24 +++++----- Kernel/Syscalls/fork.cpp | 17 +++---- Kernel/Syscalls/getuid.cpp | 4 +- Kernel/Syscalls/pledge.cpp | 14 +++--- Kernel/Syscalls/process.cpp | 2 +- Kernel/Syscalls/setpgid.cpp | 3 +- Kernel/Syscalls/setuid.cpp | 49 +++++++++++---------- 10 files changed, 125 insertions(+), 125 deletions(-) diff --git a/Kernel/Process.cpp b/Kernel/Process.cpp index d0e225e14c6..0a01642a2df 100644 --- a/Kernel/Process.cpp +++ b/Kernel/Process.cpp @@ -209,27 +209,14 @@ RefPtr Process::create_kernel_process(RefPtr& first_thread, Str return process; } -const Process::ProtectedData& Process::protected_data() const -{ - return *reinterpret_cast(m_protected_data->data()); -} - void Process::protect_data() { - auto& region = m_protected_data->impl().region(); - if (!region.is_writable()) - return; - region.set_writable(false); - region.remap(); + MM.set_page_writable_direct(VirtualAddress { this }, false); } void Process::unprotect_data() { - auto& region = m_protected_data->impl().region(); - if (region.is_writable()) - return; - region.set_writable(true); - region.remap(); + MM.set_page_writable_direct(VirtualAddress { this }, true); } Process::Process(RefPtr& first_thread, const String& name, uid_t uid, gid_t gid, ProcessID ppid, bool is_kernel_process, RefPtr cwd, RefPtr executable, TTY* tty, Process* fork_parent) @@ -240,20 +227,14 @@ Process::Process(RefPtr& first_thread, const String& name, uid_t uid, gi , m_tty(tty) , m_wait_block_condition(*this) { - m_protected_data = KBuffer::try_create_with_size(sizeof(ProtectedData)); - VERIFY(m_protected_data); - - { - MutableProtectedData protected_data { *this }; - protected_data->pid = allocate_pid(); - protected_data->ppid = ppid; - protected_data->uid = uid; - protected_data->gid = gid; - protected_data->euid = uid; - protected_data->egid = gid; - protected_data->suid = uid; - protected_data->sgid = gid; - } + m_pid = allocate_pid(); + m_ppid = ppid; + m_uid = uid; + m_gid = gid; + m_euid = uid; + m_egid = gid; + m_suid = uid; + m_sgid = gid; dbgln_if(PROCESS_DEBUG, "Created new process {}({})", m_name, this->pid().value()); @@ -273,6 +254,8 @@ Process::Process(RefPtr& first_thread, const String& name, uid_t uid, gi Process::~Process() { + unprotect_data(); + VERIFY(thread_count() == 0); // all threads should have been finalized VERIFY(!m_alarm_timer); @@ -727,9 +710,10 @@ bool Process::add_thread(Thread& thread) void Process::set_dumpable(bool dumpable) { - if (dumpable == protected_data().dumpable) + if (dumpable == m_dumpable) return; - MutableProtectedData(*this)->dumpable = dumpable; + ProtectedDataMutationScope scope { *this }; + m_dumpable = dumpable; } } diff --git a/Kernel/Process.h b/Kernel/Process.h index 2259e11434a..41128a00163 100644 --- a/Kernel/Process.h +++ b/Kernel/Process.h @@ -100,47 +100,57 @@ typedef HashMap> FutexQueues; struct LoadResult; +class ProtectedProcessBase { +protected: + ProcessID m_pid { 0 }; + ProcessID m_ppid { 0 }; + SessionID m_sid { 0 }; + uid_t m_euid { 0 }; + gid_t m_egid { 0 }; + uid_t m_uid { 0 }; + gid_t m_gid { 0 }; + uid_t m_suid { 0 }; + gid_t m_sgid { 0 }; + Vector m_extra_gids; + bool m_dumpable { false }; + bool m_has_promises { false }; + u32 m_promises { 0 }; + bool m_has_execpromises { false }; + u32 m_execpromises { 0 }; +}; + +class ProcessBase : public ProtectedProcessBase { +protected: + u8 m_process_base_padding[PAGE_SIZE - sizeof(ProtectedProcessBase)]; +}; + +static_assert(sizeof(ProcessBase) == PAGE_SIZE); + class Process - : public RefCounted + : public ProcessBase + , public RefCounted , public InlineLinkedListNode , public Weakable { AK_MAKE_NONCOPYABLE(Process); AK_MAKE_NONMOVABLE(Process); + MAKE_ALIGNED_ALLOCATED(Process, PAGE_SIZE); + friend class InlineLinkedListNode; friend class Thread; friend class CoreDump; - struct ProtectedData { - ProcessID pid { 0 }; - ProcessID ppid { 0 }; - SessionID sid { 0 }; - uid_t euid { 0 }; - gid_t egid { 0 }; - uid_t uid { 0 }; - gid_t gid { 0 }; - uid_t suid { 0 }; - gid_t sgid { 0 }; - Vector extra_gids; - bool dumpable { false }; - bool has_promises { false }; - u32 promises { 0 }; - bool has_execpromises { false }; - u32 execpromises { 0 }; - }; - // Helper class to temporarily unprotect a process's protected data so you can write to it. - class MutableProtectedData { + class ProtectedDataMutationScope { public: - explicit MutableProtectedData(Process& process) + explicit ProtectedDataMutationScope(Process& process) : m_process(process) { m_process.unprotect_data(); } - ~MutableProtectedData() { m_process.protect_data(); } - ProtectedData* operator->() { return &const_cast(m_process.protected_data()); } + ~ProtectedDataMutationScope() { m_process.protect_data(); } private: Process& m_process; @@ -203,21 +213,21 @@ public: static SessionID get_sid_from_pgid(ProcessGroupID pgid); const String& name() const { return m_name; } - ProcessID pid() const { return protected_data().pid; } - SessionID sid() const { return protected_data().sid; } - bool is_session_leader() const { return protected_data().sid.value() == protected_data().pid.value(); } + ProcessID pid() const { return m_pid; } + SessionID sid() const { return m_sid; } + bool is_session_leader() const { return m_sid.value() == m_pid.value(); } ProcessGroupID pgid() const { return m_pg ? m_pg->pgid() : 0; } - bool is_group_leader() const { return pgid().value() == protected_data().pid.value(); } - const Vector& extra_gids() const { return protected_data().extra_gids; } - uid_t euid() const { return protected_data().euid; } - gid_t egid() const { return protected_data().egid; } - uid_t uid() const { return protected_data().uid; } - gid_t gid() const { return protected_data().gid; } - uid_t suid() const { return protected_data().suid; } - gid_t sgid() const { return protected_data().sgid; } - ProcessID ppid() const { return protected_data().ppid; } + bool is_group_leader() const { return pgid().value() == m_pid.value(); } + const Vector& extra_gids() const { return m_extra_gids; } + uid_t euid() const { return m_euid; } + gid_t egid() const { return m_egid; } + uid_t uid() const { return m_uid; } + gid_t gid() const { return m_gid; } + uid_t suid() const { return m_suid; } + gid_t sgid() const { return m_sgid; } + ProcessID ppid() const { return m_ppid; } - bool is_dumpable() const { return protected_data().dumpable; } + bool is_dumpable() const { return m_dumpable; } void set_dumpable(bool); mode_t umask() const { return m_umask; } @@ -445,8 +455,8 @@ public: Custody& root_directory_relative_to_global_root(); void set_root_directory(const Custody&); - bool has_promises() const { return protected_data().has_promises; } - bool has_promised(Pledge pledge) const { return protected_data().promises & (1u << (u32)pledge); } + bool has_promises() const { return m_has_promises; } + bool has_promised(Pledge pledge) const { return m_promises & (1u << (u32)pledge); } VeilState veil_state() const { @@ -537,10 +547,8 @@ private: RefPtr m_pg; - const ProtectedData& protected_data() const; void protect_data(); void unprotect_data(); - OwnPtr m_protected_data; OwnPtr m_tracer; diff --git a/Kernel/Syscalls/disown.cpp b/Kernel/Syscalls/disown.cpp index 8d9ee0754f6..14ad47cc9b0 100644 --- a/Kernel/Syscalls/disown.cpp +++ b/Kernel/Syscalls/disown.cpp @@ -36,7 +36,8 @@ KResultOr Process::sys$disown(ProcessID pid) return ESRCH; if (process->ppid() != this->pid()) return ECHILD; - MutableProtectedData(*this)->ppid = 0; + ProtectedDataMutationScope scope(*process); + process->m_ppid = 0; process->disowned_by_waiter(*this); return 0; } diff --git a/Kernel/Syscalls/execve.cpp b/Kernel/Syscalls/execve.cpp index ecdb12731f1..803f610746e 100644 --- a/Kernel/Syscalls/execve.cpp +++ b/Kernel/Syscalls/execve.cpp @@ -507,15 +507,15 @@ KResult Process::do_exec(NonnullRefPtr main_program_description if (!(main_program_description->custody()->mount_flags() & MS_NOSUID)) { if (main_program_metadata.is_setuid()) { executable_is_setid = true; - MutableProtectedData protected_data { *this }; - protected_data->euid = main_program_metadata.uid; - protected_data->suid = main_program_metadata.uid; + ProtectedDataMutationScope scope { *this }; + m_euid = main_program_metadata.uid; + m_suid = main_program_metadata.uid; } if (main_program_metadata.is_setgid()) { executable_is_setid = true; - MutableProtectedData protected_data { *this }; - protected_data->egid = main_program_metadata.gid; - protected_data->sgid = main_program_metadata.gid; + ProtectedDataMutationScope scope { *this }; + m_egid = main_program_metadata.gid; + m_sgid = main_program_metadata.gid; } } @@ -598,15 +598,15 @@ KResult Process::do_exec(NonnullRefPtr main_program_description new_main_thread->set_name(m_name); { - MutableProtectedData protected_data { *this }; - protected_data->promises = protected_data->execpromises; - protected_data->has_promises = protected_data->has_execpromises; + ProtectedDataMutationScope scope { *this }; + m_promises = m_execpromises; + m_has_promises = m_has_execpromises; - protected_data->execpromises = 0; - protected_data->has_execpromises = false; + m_execpromises = 0; + m_has_execpromises = false; // FIXME: PID/TID ISSUE - protected_data->pid = new_main_thread->tid().value(); + m_pid = new_main_thread->tid().value(); } auto tsr_result = new_main_thread->make_thread_specific_region({}); diff --git a/Kernel/Syscalls/fork.cpp b/Kernel/Syscalls/fork.cpp index 1ce770875dc..c1b1088157a 100644 --- a/Kernel/Syscalls/fork.cpp +++ b/Kernel/Syscalls/fork.cpp @@ -44,18 +44,19 @@ KResultOr Process::sys$fork(RegisterState& regs) child->m_veil_state = m_veil_state; child->m_unveiled_paths = m_unveiled_paths.deep_copy(); child->m_fds = m_fds; - child->m_pg = m_pg; child->m_umask = m_umask; child->m_signal_trampoline = m_signal_trampoline; + child->m_pg = m_pg; { - MutableProtectedData child_data { *child }; - child_data->promises = protected_data().promises; - child_data->execpromises = protected_data().execpromises; - child_data->has_promises = protected_data().has_promises; - child_data->has_execpromises = protected_data().has_execpromises; - child_data->sid = this->sid(); - child_data->extra_gids = this->extra_gids(); + child->unprotect_data(); + child->m_promises = m_promises; + child->m_execpromises = m_execpromises; + child->m_has_promises = m_has_promises; + child->m_has_execpromises = m_has_execpromises; + child->m_sid = m_sid; + child->m_extra_gids = m_extra_gids; + child->protect_data(); } dbgln_if(FORK_DEBUG, "fork: child={}", child); diff --git a/Kernel/Syscalls/getuid.cpp b/Kernel/Syscalls/getuid.cpp index f77c4f9c003..66e60cbc6c3 100644 --- a/Kernel/Syscalls/getuid.cpp +++ b/Kernel/Syscalls/getuid.cpp @@ -55,7 +55,7 @@ KResultOr Process::sys$getegid() KResultOr Process::sys$getresuid(Userspace ruid, Userspace euid, Userspace suid) { REQUIRE_PROMISE(stdio); - if (!copy_to_user(ruid, &protected_data().uid) || !copy_to_user(euid, &protected_data().euid) || !copy_to_user(suid, &protected_data().suid)) + if (!copy_to_user(ruid, &m_uid) || !copy_to_user(euid, &m_euid) || !copy_to_user(suid, &m_suid)) return EFAULT; return 0; } @@ -63,7 +63,7 @@ KResultOr Process::sys$getresuid(Userspace ruid, Userspace KResultOr Process::sys$getresgid(Userspace rgid, Userspace egid, Userspace sgid) { REQUIRE_PROMISE(stdio); - if (!copy_to_user(rgid, &protected_data().gid) || !copy_to_user(egid, &protected_data().egid) || !copy_to_user(sgid, &protected_data().sgid)) + if (!copy_to_user(rgid, &m_gid) || !copy_to_user(egid, &m_egid) || !copy_to_user(sgid, &m_sgid)) return EFAULT; return 0; } diff --git a/Kernel/Syscalls/pledge.cpp b/Kernel/Syscalls/pledge.cpp index 024fe17f457..c305d11c2b1 100644 --- a/Kernel/Syscalls/pledge.cpp +++ b/Kernel/Syscalls/pledge.cpp @@ -67,27 +67,27 @@ KResultOr Process::sys$pledge(Userspace u return true; }; - MutableProtectedData mutable_protected_data { *this }; + ProtectedDataMutationScope scope { *this }; if (!promises.is_null()) { u32 new_promises = 0; if (!parse_pledge(promises, new_promises)) return EINVAL; - if (protected_data().promises && (!new_promises || new_promises & ~protected_data().promises)) + if (m_promises && (!new_promises || new_promises & ~m_promises)) return EPERM; - mutable_protected_data->has_promises = true; - mutable_protected_data->promises = new_promises; + m_has_promises = true; + m_promises = new_promises; } if (!execpromises.is_null()) { u32 new_execpromises = 0; if (!parse_pledge(execpromises, new_execpromises)) return EINVAL; - if (protected_data().execpromises && (!new_execpromises || new_execpromises & ~protected_data().execpromises)) + if (m_execpromises && (!new_execpromises || new_execpromises & ~m_execpromises)) return EPERM; - mutable_protected_data->has_execpromises = true; - mutable_protected_data->execpromises = new_execpromises; + m_has_execpromises = true; + m_execpromises = new_execpromises; } return 0; diff --git a/Kernel/Syscalls/process.cpp b/Kernel/Syscalls/process.cpp index 06301d94f6f..e6046419657 100644 --- a/Kernel/Syscalls/process.cpp +++ b/Kernel/Syscalls/process.cpp @@ -38,7 +38,7 @@ KResultOr Process::sys$getpid() KResultOr Process::sys$getppid() { REQUIRE_PROMISE(stdio); - return protected_data().ppid.value(); + return m_ppid.value(); } KResultOr Process::sys$get_process_name(Userspace buffer, size_t buffer_size) diff --git a/Kernel/Syscalls/setpgid.cpp b/Kernel/Syscalls/setpgid.cpp index f10fec2ebf5..219297be9b4 100644 --- a/Kernel/Syscalls/setpgid.cpp +++ b/Kernel/Syscalls/setpgid.cpp @@ -55,9 +55,10 @@ KResultOr Process::sys$setsid() if (found_process_with_same_pgid_as_my_pid) return EPERM; // Create a new Session and a new ProcessGroup. - MutableProtectedData(*this)->sid = pid().value(); m_pg = ProcessGroup::create(ProcessGroupID(pid().value())); m_tty = nullptr; + ProtectedDataMutationScope scope { *this }; + m_sid = pid().value(); return sid().value(); } diff --git a/Kernel/Syscalls/setuid.cpp b/Kernel/Syscalls/setuid.cpp index 023be08e57f..870e172cbeb 100644 --- a/Kernel/Syscalls/setuid.cpp +++ b/Kernel/Syscalls/setuid.cpp @@ -37,7 +37,10 @@ KResultOr Process::sys$seteuid(uid_t new_euid) if (euid() != new_euid) set_dumpable(false); - MutableProtectedData(*this)->euid = new_euid; + + ProtectedDataMutationScope scope { *this }; + + m_euid = new_euid; return 0; } @@ -51,7 +54,8 @@ KResultOr Process::sys$setegid(gid_t new_egid) if (egid() != new_egid) set_dumpable(false); - MutableProtectedData(*this)->egid = new_egid; + ProtectedDataMutationScope scope { *this }; + m_egid = new_egid; return 0; } @@ -65,10 +69,10 @@ KResultOr Process::sys$setuid(uid_t new_uid) if (euid() != new_uid) set_dumpable(false); - MutableProtectedData protected_data { *this }; - protected_data->uid = new_uid; - protected_data->euid = new_uid; - protected_data->suid = new_uid; + ProtectedDataMutationScope scope { *this }; + m_uid = new_uid; + m_euid = new_uid; + m_suid = new_uid; return 0; } @@ -82,10 +86,10 @@ KResultOr Process::sys$setgid(gid_t new_gid) if (egid() != new_gid) set_dumpable(false); - MutableProtectedData protected_data { *this }; - protected_data->gid = new_gid; - protected_data->egid = new_gid; - protected_data->sgid = new_gid; + ProtectedDataMutationScope scope { *this }; + m_gid = new_gid; + m_egid = new_gid; + m_sgid = new_gid; return 0; } @@ -107,10 +111,10 @@ KResultOr Process::sys$setresuid(uid_t new_ruid, uid_t new_euid, uid_t new_ if (euid() != new_euid) set_dumpable(false); - MutableProtectedData protected_data { *this }; - protected_data->uid = new_ruid; - protected_data->euid = new_euid; - protected_data->suid = new_suid; + ProtectedDataMutationScope scope { *this }; + m_uid = new_ruid; + m_euid = new_euid; + m_suid = new_suid; return 0; } @@ -132,10 +136,10 @@ KResultOr Process::sys$setresgid(gid_t new_rgid, gid_t new_egid, gid_t new_ if (egid() != new_egid) set_dumpable(false); - MutableProtectedData protected_data { *this }; - protected_data->gid = new_rgid; - protected_data->egid = new_egid; - protected_data->sgid = new_sgid; + ProtectedDataMutationScope scope { *this }; + m_gid = new_rgid; + m_egid = new_egid; + m_sgid = new_sgid; return 0; } @@ -148,7 +152,8 @@ KResultOr Process::sys$setgroups(ssize_t count, Userspace use return EPERM; if (!count) { - MutableProtectedData(*this)->extra_gids.clear(); + ProtectedDataMutationScope scope { *this }; + m_extra_gids.clear(); return 0; } @@ -163,13 +168,13 @@ KResultOr Process::sys$setgroups(ssize_t count, Userspace use unique_extra_gids.set(extra_gid); } - MutableProtectedData protected_data { *this }; - protected_data->extra_gids.resize(unique_extra_gids.size()); + ProtectedDataMutationScope scope { *this }; + m_extra_gids.resize(unique_extra_gids.size()); size_t i = 0; for (auto& extra_gid : unique_extra_gids) { if (extra_gid == gid()) continue; - protected_data->extra_gids[i++] = extra_gid; + m_extra_gids[i++] = extra_gid; } return 0; }