From 01b79910b3b881c26a972487fddb4251682438f5 Mon Sep 17 00:00:00 2001 From: Liav A Date: Sat, 7 Aug 2021 22:30:06 +0300 Subject: [PATCH] Kernel/Process: Move protected values to the end of the object The compiler can re-order the structure (class) members if that's necessary, so if we make Process to inherit from ProcFSExposedComponent, even if the declaration is to inherit first from ProcessBase, then from ProcFSExposedComponent and last from Weakable, the members of class ProcFSExposedComponent (including the Ref-counted parts) are the first members of the Process class. This problem made it impossible to safely use the current toggling method with the write-protection bit on the ProcessBase members, so instead of inheriting from it, we make its members the last ones in the Process class so we can safely locate and modify the corresponding page write protection bit of these values. We make sure that the Process class doesn't expand beyond 8192 bytes and the protected values are always aligned on a page boundary. --- Kernel/Process.cpp | 40 ++++++------- Kernel/Process.h | 112 ++++++++++++++++++------------------ Kernel/Syscalls/disown.cpp | 2 +- Kernel/Syscalls/execve.cpp | 20 +++---- Kernel/Syscalls/exit.cpp | 4 +- Kernel/Syscalls/fork.cpp | 18 +++--- Kernel/Syscalls/getuid.cpp | 4 +- Kernel/Syscalls/pledge.cpp | 12 ++-- Kernel/Syscalls/process.cpp | 2 +- Kernel/Syscalls/setpgid.cpp | 2 +- Kernel/Syscalls/setuid.cpp | 38 ++++++------ Kernel/Syscalls/umask.cpp | 4 +- 12 files changed, 130 insertions(+), 128 deletions(-) diff --git a/Kernel/Process.cpp b/Kernel/Process.cpp index 26cd0c49550..94ca8f0c8a2 100644 --- a/Kernel/Process.cpp +++ b/Kernel/Process.cpp @@ -216,14 +216,14 @@ RefPtr Process::create_kernel_process(RefPtr& first_thread, Str void Process::protect_data() { m_protected_data_refs.unref([&]() { - MM.set_page_writable_direct(VirtualAddress { this }, false); + MM.set_page_writable_direct(VirtualAddress { &this->m_protected_values }, false); }); } void Process::unprotect_data() { m_protected_data_refs.ref([&]() { - MM.set_page_writable_direct(VirtualAddress { this }, true); + MM.set_page_writable_direct(VirtualAddress { &this->m_protected_values }, true); }); } @@ -249,14 +249,14 @@ Process::Process(const String& name, uid_t uid, gid_t gid, ProcessID ppid, bool // Ensure that we protect the process data when exiting the constructor. ProtectedDataMutationScope scope { *this }; - 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; + m_protected_values.pid = allocate_pid(); + m_protected_values.ppid = ppid; + m_protected_values.uid = uid; + m_protected_values.gid = gid; + m_protected_values.euid = uid; + m_protected_values.egid = gid; + m_protected_values.suid = uid; + m_protected_values.sgid = gid; dbgln_if(PROCESS_DEBUG, "Created new process {}({})", m_name, this->pid().value()); } @@ -386,7 +386,7 @@ void Process::crash(int signal, FlatPtr ip, bool out_of_memory) } { ProtectedDataMutationScope scope { *this }; - m_termination_signal = signal; + m_protected_values.termination_signal = signal; } set_dump_core(!out_of_memory); address_space().dump_regions(); @@ -482,11 +482,11 @@ siginfo_t Process::wait_info() siginfo.si_pid = pid().value(); siginfo.si_uid = uid(); - if (m_termination_signal) { - siginfo.si_status = m_termination_signal; + if (m_protected_values.termination_signal) { + siginfo.si_status = m_protected_values.termination_signal; siginfo.si_code = CLD_KILLED; } else { - siginfo.si_status = m_termination_status; + siginfo.si_status = m_protected_values.termination_status; siginfo.si_code = CLD_EXITED; } return siginfo; @@ -690,8 +690,8 @@ void Process::terminate_due_to_signal(u8 signal) dbgln("Terminating {} due to signal {}", *this, signal); { ProtectedDataMutationScope scope { *this }; - m_termination_status = 0; - m_termination_signal = signal; + m_protected_values.termination_status = 0; + m_protected_values.termination_signal = signal; } die(); } @@ -832,7 +832,7 @@ void Process::delete_perf_events_buffer() bool Process::remove_thread(Thread& thread) { ProtectedDataMutationScope scope { *this }; - auto thread_cnt_before = m_thread_count.fetch_sub(1, AK::MemoryOrder::memory_order_acq_rel); + auto thread_cnt_before = m_protected_values.thread_count.fetch_sub(1, AK::MemoryOrder::memory_order_acq_rel); VERIFY(thread_cnt_before != 0); thread_list().with([&](auto& thread_list) { thread_list.remove(thread); @@ -843,7 +843,7 @@ bool Process::remove_thread(Thread& thread) bool Process::add_thread(Thread& thread) { ProtectedDataMutationScope scope { *this }; - bool is_first = m_thread_count.fetch_add(1, AK::MemoryOrder::memory_order_relaxed) == 0; + bool is_first = m_protected_values.thread_count.fetch_add(1, AK::MemoryOrder::memory_order_relaxed) == 0; thread_list().with([&](auto& thread_list) { thread_list.append(thread); }); @@ -852,10 +852,10 @@ bool Process::add_thread(Thread& thread) void Process::set_dumpable(bool dumpable) { - if (dumpable == m_dumpable) + if (dumpable == m_protected_values.dumpable) return; ProtectedDataMutationScope scope { *this }; - m_dumpable = dumpable; + m_protected_values.dumpable = dumpable; } KResult Process::set_coredump_property(NonnullOwnPtr key, NonnullOwnPtr value) diff --git a/Kernel/Process.h b/Kernel/Process.h index 26dcc49d110..bea511b1065 100644 --- a/Kernel/Process.h +++ b/Kernel/Process.h @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -82,45 +83,36 @@ 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 }; - Atomic m_has_promises { false }; - Atomic m_promises { 0 }; - Atomic m_has_execpromises { false }; - Atomic m_execpromises { 0 }; - mode_t m_umask { 022 }; - VirtualAddress m_signal_trampoline; - Atomic m_thread_count { 0 }; - u8 m_termination_status { 0 }; - u8 m_termination_signal { 0 }; -}; - -class ProcessBase : public ProtectedProcessBase { -protected: - // Without the alignas specifier here the compiler places this class into - // the parent class' padding which then causes the members for the RefCounted - // class to be placed within the first page of the Process class. - alignas(ProtectedProcessBase) u8 m_process_base_padding[PAGE_SIZE - sizeof(ProtectedProcessBase)]; -}; - -static_assert(sizeof(ProcessBase) == PAGE_SIZE); - class Process - : public ProcessBase - , public RefCounted + : public RefCounted , public Weakable { +private: + class ProtectedValues { + public: + 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 }; + Atomic has_promises { false }; + Atomic promises { 0 }; + Atomic has_execpromises { false }; + Atomic execpromises { 0 }; + mode_t umask { 022 }; + VirtualAddress signal_trampoline; + Atomic thread_count { 0 }; + u8 termination_status { 0 }; + u8 termination_signal { 0 }; + }; + +public: AK_MAKE_NONCOPYABLE(Process); AK_MAKE_NONMOVABLE(Process); @@ -205,24 +197,24 @@ public: static SessionID get_sid_from_pgid(ProcessGroupID pgid); const String& name() const { return m_name; } - ProcessID pid() const { return m_pid; } - SessionID sid() const { return m_sid; } - bool is_session_leader() const { return m_sid.value() == m_pid.value(); } + ProcessID pid() const { return m_protected_values.pid; } + SessionID sid() const { return m_protected_values.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() == 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_group_leader() const { return pgid().value() == pid().value(); } + const Vector& extra_gids() const { return m_protected_values.extra_gids; } + uid_t euid() const { return m_protected_values.euid; } + gid_t egid() const { return m_protected_values.egid; } + uid_t uid() const { return m_protected_values.uid; } + gid_t gid() const { return m_protected_values.gid; } + uid_t suid() const { return m_protected_values.suid; } + gid_t sgid() const { return m_protected_values.sgid; } + ProcessID ppid() const { return m_protected_values.ppid; } - bool is_dumpable() const { return m_dumpable; } + bool is_dumpable() const { return m_protected_values.dumpable; } void set_dumpable(bool); - mode_t umask() const { return m_umask; } + mode_t umask() const { return m_protected_values.umask; } bool in_group(gid_t) const; @@ -448,11 +440,11 @@ public: void terminate_due_to_signal(u8 signal); KResult send_signal(u8 signal, Process* sender); - u8 termination_signal() const { return m_termination_signal; } + u8 termination_signal() const { return m_protected_values.termination_signal; } u16 thread_count() const { - return m_thread_count.load(AK::MemoryOrder::memory_order_relaxed); + return m_protected_values.thread_count.load(AK::MemoryOrder::memory_order_relaxed); } Mutex& big_lock() { return m_big_lock; } @@ -462,8 +454,8 @@ public: Custody& root_directory_relative_to_global_root(); void set_root_directory(const Custody&); - bool has_promises() const { return m_has_promises; } - bool has_promised(Pledge pledge) const { return m_promises & (1u << (u32)pledge); } + bool has_promises() const { return m_protected_values.has_promises; } + bool has_promised(Pledge pledge) const { return m_protected_values.promises & (1u << (u32)pledge); } VeilState veil_state() const { @@ -509,7 +501,7 @@ public: Memory::AddressSpace& address_space() { return *m_space; } Memory::AddressSpace const& address_space() const { return *m_space; } - VirtualAddress signal_trampoline() const { return m_signal_trampoline; } + VirtualAddress signal_trampoline() const { return m_protected_values.signal_trampoline; } private: friend class MemoryManager; @@ -763,10 +755,20 @@ private: NonnullRefPtrVector m_threads_for_coredump; + static_assert(sizeof(ProtectedValues) < (PAGE_SIZE)); + alignas(4096) ProtectedValues m_protected_values; + u8 m_protected_values_padding[PAGE_SIZE - sizeof(ProtectedValues)]; + public: using List = IntrusiveListRelaxedConst, &Process::m_list_node>; }; +// Note: Process object should be 2 pages of 4096 bytes each. +// It's not expected that the Process object will expand further because the first +// page is used for all unprotected values (which should be plenty of space for them). +// The second page is being used exclusively for write-protected values. +static_assert(sizeof(Process) == (PAGE_SIZE * 2)); + extern RecursiveSpinLock g_profiling_lock; ProtectedValue& processes(); diff --git a/Kernel/Syscalls/disown.cpp b/Kernel/Syscalls/disown.cpp index 4e5cdbc8dc1..09ac972dc39 100644 --- a/Kernel/Syscalls/disown.cpp +++ b/Kernel/Syscalls/disown.cpp @@ -18,7 +18,7 @@ KResultOr Process::sys$disown(ProcessID pid) if (process->ppid() != this->pid()) return ECHILD; ProtectedDataMutationScope scope(*process); - process->m_ppid = 0; + process->m_protected_values.ppid = 0; process->disowned_by_waiter(*this); return 0; } diff --git a/Kernel/Syscalls/execve.cpp b/Kernel/Syscalls/execve.cpp index 4b55c7f2e7f..eef9d081de1 100644 --- a/Kernel/Syscalls/execve.cpp +++ b/Kernel/Syscalls/execve.cpp @@ -540,14 +540,14 @@ KResult Process::do_exec(NonnullRefPtr main_program_description if (main_program_metadata.is_setuid()) { executable_is_setid = true; ProtectedDataMutationScope scope { *this }; - m_euid = main_program_metadata.uid; - m_suid = main_program_metadata.uid; + m_protected_values.euid = main_program_metadata.uid; + m_protected_values.suid = main_program_metadata.uid; } if (main_program_metadata.is_setgid()) { executable_is_setid = true; ProtectedDataMutationScope scope { *this }; - m_egid = main_program_metadata.gid; - m_sgid = main_program_metadata.gid; + m_protected_values.egid = main_program_metadata.gid; + m_protected_values.sgid = main_program_metadata.gid; } } @@ -641,16 +641,16 @@ KResult Process::do_exec(NonnullRefPtr main_program_description { ProtectedDataMutationScope scope { *this }; - m_promises = m_execpromises.load(); - m_has_promises = m_has_execpromises.load(); + m_protected_values.promises = m_protected_values.execpromises.load(); + m_protected_values.has_promises = m_protected_values.has_execpromises.load(); - m_execpromises = 0; - m_has_execpromises = false; + m_protected_values.execpromises = 0; + m_protected_values.has_execpromises = false; - m_signal_trampoline = signal_trampoline_region.value()->vaddr(); + m_protected_values.signal_trampoline = signal_trampoline_region.value()->vaddr(); // FIXME: PID/TID ISSUE - m_pid = new_main_thread->tid().value(); + m_protected_values.pid = new_main_thread->tid().value(); } auto tsr_result = new_main_thread->make_thread_specific_region({}); diff --git a/Kernel/Syscalls/exit.cpp b/Kernel/Syscalls/exit.cpp index b4c2d80e54c..df046179d5e 100644 --- a/Kernel/Syscalls/exit.cpp +++ b/Kernel/Syscalls/exit.cpp @@ -20,8 +20,8 @@ void Process::sys$exit(int status) { ProtectedDataMutationScope scope { *this }; - m_termination_status = status; - m_termination_signal = 0; + m_protected_values.termination_status = status; + m_protected_values.termination_signal = 0; } auto* current_thread = Thread::current(); diff --git a/Kernel/Syscalls/fork.cpp b/Kernel/Syscalls/fork.cpp index cab53f8a19f..867575e4a0d 100644 --- a/Kernel/Syscalls/fork.cpp +++ b/Kernel/Syscalls/fork.cpp @@ -30,15 +30,15 @@ KResultOr Process::sys$fork(RegisterState& regs) { ProtectedDataMutationScope scope { *child }; - child->m_promises = m_promises.load(); - child->m_execpromises = m_execpromises.load(); - child->m_has_promises = m_has_promises.load(); - child->m_has_execpromises = m_has_execpromises.load(); - child->m_sid = m_sid; - child->m_extra_gids = m_extra_gids; - child->m_umask = m_umask; - child->m_signal_trampoline = m_signal_trampoline; - child->m_dumpable = m_dumpable; + 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.extra_gids = m_protected_values.extra_gids; + 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; } dbgln_if(FORK_DEBUG, "fork: child={}", child); diff --git a/Kernel/Syscalls/getuid.cpp b/Kernel/Syscalls/getuid.cpp index b94711e3dd1..24a578b063f 100644 --- a/Kernel/Syscalls/getuid.cpp +++ b/Kernel/Syscalls/getuid.cpp @@ -40,7 +40,7 @@ KResultOr Process::sys$getresuid(Userspace ruid, Userspace Process::sys$getresgid(Userspace rgid, Userspace Process::sys$pledge(Userspaceview(), new_promises)) return EINVAL; - if (m_has_promises && (new_promises & ~m_promises)) + if (m_protected_values.has_promises && (new_promises & ~m_protected_values.promises)) return EPERM; } @@ -64,7 +64,7 @@ KResultOr Process::sys$pledge(Userspaceview(), new_execpromises)) return EINVAL; - if (m_has_execpromises && (new_execpromises & ~m_execpromises)) + if (m_protected_values.has_execpromises && (new_execpromises & ~m_protected_values.execpromises)) return EPERM; } @@ -74,13 +74,13 @@ KResultOr Process::sys$pledge(Userspace Process::sys$getppid() { VERIFY_PROCESS_BIG_LOCK_ACQUIRED(this) REQUIRE_PROMISE(stdio); - return m_ppid.value(); + return m_protected_values.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 f9832b9f58b..7800c5b3a9c 100644 --- a/Kernel/Syscalls/setpgid.cpp +++ b/Kernel/Syscalls/setpgid.cpp @@ -40,7 +40,7 @@ KResultOr Process::sys$setsid() m_pg = ProcessGroup::create(ProcessGroupID(pid().value())); m_tty = nullptr; ProtectedDataMutationScope scope { *this }; - m_sid = pid().value(); + m_protected_values.sid = pid().value(); return sid().value(); } diff --git a/Kernel/Syscalls/setuid.cpp b/Kernel/Syscalls/setuid.cpp index 0a9e7e607cd..a16c0eeedb5 100644 --- a/Kernel/Syscalls/setuid.cpp +++ b/Kernel/Syscalls/setuid.cpp @@ -21,7 +21,7 @@ KResultOr Process::sys$seteuid(uid_t new_euid) ProtectedDataMutationScope scope { *this }; - m_euid = new_euid; + m_protected_values.euid = new_euid; return 0; } @@ -37,7 +37,7 @@ KResultOr Process::sys$setegid(gid_t new_egid) set_dumpable(false); ProtectedDataMutationScope scope { *this }; - m_egid = new_egid; + m_protected_values.egid = new_egid; return 0; } @@ -53,9 +53,9 @@ KResultOr Process::sys$setuid(uid_t new_uid) set_dumpable(false); ProtectedDataMutationScope scope { *this }; - m_uid = new_uid; - m_euid = new_uid; - m_suid = new_uid; + m_protected_values.uid = new_uid; + m_protected_values.euid = new_uid; + m_protected_values.suid = new_uid; return 0; } @@ -71,9 +71,9 @@ KResultOr Process::sys$setgid(gid_t new_gid) set_dumpable(false); ProtectedDataMutationScope scope { *this }; - m_gid = new_gid; - m_egid = new_gid; - m_sgid = new_gid; + m_protected_values.gid = new_gid; + m_protected_values.egid = new_gid; + m_protected_values.sgid = new_gid; return 0; } @@ -98,8 +98,8 @@ KResultOr Process::sys$setreuid(uid_t new_ruid, uid_t new_euid) set_dumpable(false); ProtectedDataMutationScope scope { *this }; - m_uid = new_ruid; - m_euid = new_euid; + m_protected_values.uid = new_ruid; + m_protected_values.euid = new_euid; return 0; } @@ -123,9 +123,9 @@ KResultOr Process::sys$setresuid(uid_t new_ruid, uid_t new_euid, uid_t set_dumpable(false); ProtectedDataMutationScope scope { *this }; - m_uid = new_ruid; - m_euid = new_euid; - m_suid = new_suid; + m_protected_values.uid = new_ruid; + m_protected_values.euid = new_euid; + m_protected_values.suid = new_suid; return 0; } @@ -149,9 +149,9 @@ KResultOr Process::sys$setresgid(gid_t new_rgid, gid_t new_egid, gid_t set_dumpable(false); ProtectedDataMutationScope scope { *this }; - m_gid = new_rgid; - m_egid = new_egid; - m_sgid = new_sgid; + m_protected_values.gid = new_rgid; + m_protected_values.egid = new_egid; + m_protected_values.sgid = new_sgid; return 0; } @@ -164,7 +164,7 @@ KResultOr Process::sys$setgroups(size_t count, Userspace if (!count) { ProtectedDataMutationScope scope { *this }; - m_extra_gids.clear(); + m_protected_values.extra_gids.clear(); return 0; } @@ -181,13 +181,13 @@ KResultOr Process::sys$setgroups(size_t count, Userspace } ProtectedDataMutationScope scope { *this }; - if (!m_extra_gids.try_resize(unique_extra_gids.size())) + if (!m_protected_values.extra_gids.try_resize(unique_extra_gids.size())) return ENOMEM; size_t i = 0; for (auto& extra_gid : unique_extra_gids) { if (extra_gid == gid()) continue; - m_extra_gids[i++] = extra_gid; + m_protected_values.extra_gids[i++] = extra_gid; } return 0; } diff --git a/Kernel/Syscalls/umask.cpp b/Kernel/Syscalls/umask.cpp index 16819f02154..e019fb03446 100644 --- a/Kernel/Syscalls/umask.cpp +++ b/Kernel/Syscalls/umask.cpp @@ -12,9 +12,9 @@ KResultOr Process::sys$umask(mode_t mask) { VERIFY_PROCESS_BIG_LOCK_ACQUIRED(this) REQUIRE_PROMISE(stdio); - auto old_mask = m_umask; + auto old_mask = m_protected_values.umask; ProtectedDataMutationScope scope { *this }; - m_umask = mask & 0777; + m_protected_values.umask = mask & 0777; return old_mask; }