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<Process>, 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

@@ -216,14 +216,14 @@ RefPtr<Process> Process::create_kernel_process(RefPtr<Thread>& 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<KString> key, NonnullOwnPtr<KString> value)

Kernel/Process.h

@@ -11,6 +11,7 @@
 #include <AK/IntrusiveList.h>
 #include <AK/IntrusiveListRelaxedConst.h>
 #include <AK/NonnullRefPtrVector.h>
+#include <AK/OwnPtr.h>
 #include <AK/String.h>
 #include <AK/Userspace.h>
 #include <AK/WeakPtr.h>
@@ -82,45 +83,36 @@ typedef HashMap<FlatPtr, RefPtr<FutexQueue>> 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<gid_t> m_extra_gids;
-    bool m_dumpable { false };
-    Atomic<bool> m_has_promises { false };
-    Atomic<u32> m_promises { 0 };
-    Atomic<bool> m_has_execpromises { false };
-    Atomic<u32> m_execpromises { 0 };
-    mode_t m_umask { 022 };
-    VirtualAddress m_signal_trampoline;
-    Atomic<u32> 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<Process>
+    : public RefCounted<Process>
     , public Weakable<Process> {
 
+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<gid_t> extra_gids;
+        bool dumpable { false };
+        Atomic<bool> has_promises { false };
+        Atomic<u32> promises { 0 };
+        Atomic<bool> has_execpromises { false };
+        Atomic<u32> execpromises { 0 };
+        mode_t umask { 022 };
+        VirtualAddress signal_trampoline;
+        Atomic<u32> 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<gid_t>& 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<gid_t>& 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<Thread> 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, RawPtr<Process>, &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<Process::List>& processes();

Kernel/Syscalls/disown.cpp

@@ -18,7 +18,7 @@ KResultOr<FlatPtr> 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;
 }

Kernel/Syscalls/execve.cpp

@@ -540,14 +540,14 @@ KResult Process::do_exec(NonnullRefPtr<FileDescription> 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<FileDescription> 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({});

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();

Kernel/Syscalls/fork.cpp

@@ -30,15 +30,15 @@ KResultOr<FlatPtr> 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);

Kernel/Syscalls/getuid.cpp

@@ -40,7 +40,7 @@ KResultOr<FlatPtr> Process::sys$getresuid(Userspace<uid_t*> ruid, Userspace<uid_
 {
     VERIFY_PROCESS_BIG_LOCK_ACQUIRED(this)
     REQUIRE_PROMISE(stdio);
-    if (!copy_to_user(ruid, &m_uid) || !copy_to_user(euid, &m_euid) || !copy_to_user(suid, &m_suid))
+    if (!copy_to_user(ruid, &m_protected_values.uid) || !copy_to_user(euid, &m_protected_values.euid) || !copy_to_user(suid, &m_protected_values.suid))
         return EFAULT;
     return 0;
 }
@@ -49,7 +49,7 @@ KResultOr<FlatPtr> Process::sys$getresgid(Userspace<gid_t*> rgid, Userspace<gid_
 {
     VERIFY_PROCESS_BIG_LOCK_ACQUIRED(this)
     REQUIRE_PROMISE(stdio);
-    if (!copy_to_user(rgid, &m_gid) || !copy_to_user(egid, &m_egid) || !copy_to_user(sgid, &m_sgid))
+    if (!copy_to_user(rgid, &m_protected_values.gid) || !copy_to_user(egid, &m_protected_values.egid) || !copy_to_user(sgid, &m_protected_values.sgid))
         return EFAULT;
     return 0;
 }

Kernel/Syscalls/pledge.cpp

@@ -56,7 +56,7 @@ KResultOr<FlatPtr> Process::sys$pledge(Userspace<const Syscall::SC_pledge_params
     if (promises) {
         if (!parse_pledge(promises->view(), 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<FlatPtr> Process::sys$pledge(Userspace<const Syscall::SC_pledge_params
     if (execpromises) {
         if (!parse_pledge(execpromises->view(), 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<FlatPtr> Process::sys$pledge(Userspace<const Syscall::SC_pledge_params
     // leave the caller in an unexpected state.
 
     if (promises) {
-        m_promises = new_promises;
-        m_has_promises = true;
+        m_protected_values.has_promises = true;
+        m_protected_values.promises = new_promises;
     }
 
     if (execpromises) {
-        m_execpromises = new_execpromises;
-        m_has_execpromises = true;
+        m_protected_values.has_execpromises = true;
+        m_protected_values.execpromises = new_execpromises;
     }
 
     return 0;

Kernel/Syscalls/process.cpp

@@ -20,7 +20,7 @@ KResultOr<FlatPtr> Process::sys$getppid()
 {
     VERIFY_PROCESS_BIG_LOCK_ACQUIRED(this)
     REQUIRE_PROMISE(stdio);
-    return m_ppid.value();
+    return m_protected_values.ppid.value();
 }
 
 KResultOr<FlatPtr> Process::sys$get_process_name(Userspace<char*> buffer, size_t buffer_size)

Kernel/Syscalls/setpgid.cpp

@@ -40,7 +40,7 @@ KResultOr<FlatPtr> 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();
 }
 

Kernel/Syscalls/setuid.cpp

@@ -21,7 +21,7 @@ KResultOr<FlatPtr> 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<FlatPtr> 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<FlatPtr> 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<FlatPtr> 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<FlatPtr> 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<FlatPtr> 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<FlatPtr> 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<FlatPtr> Process::sys$setgroups(size_t count, Userspace<const gid_t*>
 
     if (!count) {
         ProtectedDataMutationScope scope { *this };
-        m_extra_gids.clear();
+        m_protected_values.extra_gids.clear();
         return 0;
     }
 
@@ -181,13 +181,13 @@ KResultOr<FlatPtr> Process::sys$setgroups(size_t count, Userspace<const gid_t*>
     }
 
     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;
 }

Kernel/Syscalls/umask.cpp

@@ -12,9 +12,9 @@ KResultOr<FlatPtr> 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;
 }