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

@@ -209,27 +209,14 @@ RefPtr<Process> Process::create_kernel_process(RefPtr<Thread>& first_thread, Str
     return process;
 }
 
-const Process::ProtectedData& Process::protected_data() const
-{
-    return *reinterpret_cast<const ProtectedData*>(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<Thread>& first_thread, const String& name, uid_t uid, gid_t gid, ProcessID ppid, bool is_kernel_process, RefPtr<Custody> cwd, RefPtr<Custody> executable, TTY* tty, Process* fork_parent)
@@ -240,20 +227,14 @@ Process::Process(RefPtr<Thread>& 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<Thread>& 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;
 }
 
 }

Kernel/Process.h

@@ -100,47 +100,57 @@ 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 };
+    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<Process>
+    : public ProcessBase
+    , public RefCounted<Process>
     , public InlineLinkedListNode<Process>
     , public Weakable<Process> {
 
     AK_MAKE_NONCOPYABLE(Process);
     AK_MAKE_NONMOVABLE(Process);
 
+    MAKE_ALIGNED_ALLOCATED(Process, PAGE_SIZE);
+
     friend class InlineLinkedListNode<Process>;
     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<gid_t> 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<ProtectedData&>(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<gid_t>& 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_dumpable() const { return protected_data().dumpable; }
+    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_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<ProcessGroup> m_pg;
 
-    const ProtectedData& protected_data() const;
     void protect_data();
     void unprotect_data();
-    OwnPtr<KBuffer> m_protected_data;
 
     OwnPtr<ThreadTracer> m_tracer;
 

Kernel/Syscalls/disown.cpp

@@ -36,7 +36,8 @@ KResultOr<int> 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;
 }

Kernel/Syscalls/execve.cpp

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

Kernel/Syscalls/fork.cpp

@@ -44,18 +44,19 @@ KResultOr<pid_t> 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);

Kernel/Syscalls/getuid.cpp

@@ -55,7 +55,7 @@ KResultOr<gid_t> Process::sys$getegid()
 KResultOr<int> Process::sys$getresuid(Userspace<uid_t*> ruid, Userspace<uid_t*> euid, Userspace<uid_t*> 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<int> Process::sys$getresuid(Userspace<uid_t*> ruid, Userspace<uid_t*>
 KResultOr<int> Process::sys$getresgid(Userspace<gid_t*> rgid, Userspace<gid_t*> egid, Userspace<gid_t*> 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;
 }

Kernel/Syscalls/pledge.cpp

@@ -67,27 +67,27 @@ KResultOr<int> Process::sys$pledge(Userspace<const Syscall::SC_pledge_params*> 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;

Kernel/Syscalls/process.cpp

@@ -38,7 +38,7 @@ KResultOr<pid_t> Process::sys$getpid()
 KResultOr<pid_t> Process::sys$getppid()
 {
     REQUIRE_PROMISE(stdio);
-    return protected_data().ppid.value();
+    return m_ppid.value();
 }
 
 KResultOr<int> Process::sys$get_process_name(Userspace<char*> buffer, size_t buffer_size)

Kernel/Syscalls/setpgid.cpp

@@ -55,9 +55,10 @@ KResultOr<pid_t> 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();
 }
 

Kernel/Syscalls/setuid.cpp

@@ -37,7 +37,10 @@ KResultOr<int> 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<int> 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<int> 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<int> 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<int> 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<int> 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<int> Process::sys$setgroups(ssize_t count, Userspace<const gid_t*> use
         return EPERM;
 
     if (!count) {
-        MutableProtectedData(*this)->extra_gids.clear();
+        ProtectedDataMutationScope scope { *this };
+        m_extra_gids.clear();
         return 0;
     }
 
@@ -163,13 +168,13 @@ KResultOr<int> Process::sys$setgroups(ssize_t count, Userspace<const gid_t*> 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;
 }