Kernel: Put Process unveil state in a SpinlockProtected container

This makes path resolution safe to perform without holding the big lock.

Kernel/FileSystem/VirtualFileSystem.cpp

@@ -748,10 +748,10 @@ UnveilNode const& VirtualFileSystem::find_matching_unveiled_path(StringView path
 {
     auto& current_process = Process::current();
     VERIFY(current_process.veil_state() != VeilState::None);
-    auto& unveil_root = current_process.unveiled_paths();
+    return current_process.unveil_data().with([&](auto const& unveil_data) -> UnveilNode const& {
-
+        auto path_parts = KLexicalPath::parts(path);
-    auto path_parts = KLexicalPath::parts(path);
+        return unveil_data.paths.traverse_until_last_accessible_node(path_parts.begin(), path_parts.end());
-    return unveil_root.traverse_until_last_accessible_node(path_parts.begin(), path_parts.end());
+    });
 }
 
 ErrorOr<void> VirtualFileSystem::validate_path_against_process_veil(Custody const& custody, int options)

Kernel/Process.cpp

@@ -224,13 +224,13 @@ ErrorOr<NonnullRefPtr<Process>> Process::try_create(RefPtr<Thread>& first_thread
     return process;
 }
 
-Process::Process(NonnullOwnPtr<KString> name, UserID uid, GroupID gid, ProcessID ppid, bool is_kernel_process, NonnullRefPtr<Custody> current_directory, RefPtr<Custody> executable, TTY* tty, UnveilNode unveil_tree)
+Process::Process(NonnullOwnPtr<KString> name, UserID uid, GroupID gid, ProcessID ppid, bool is_kernel_process, RefPtr<Custody> current_directory, RefPtr<Custody> executable, TTY* tty, UnveilNode unveil_tree)
     : m_name(move(name))
     , m_is_kernel_process(is_kernel_process)
     , m_executable(move(executable))
     , m_current_directory(move(current_directory))
     , m_tty(tty)
-    , m_unveiled_paths(move(unveil_tree))
+    , m_unveil_data(move(unveil_tree))
     , m_wait_blocker_set(*this)
 {
     // Ensure that we protect the process data when exiting the constructor.

Kernel/Process.h

@@ -466,13 +466,21 @@ public:
 
     VeilState veil_state() const
     {
-        return m_veil_state;
+        return m_unveil_data.with([&](auto const& unveil_data) { return unveil_data.state; });
-    }
-    const UnveilNode& unveiled_paths() const
-    {
-        return m_unveiled_paths;
     }
 
+    struct UnveilData {
+        explicit UnveilData(UnveilNode&& p)
+            : paths(move(p))
+        {
+        }
+        VeilState state { VeilState::None };
+        UnveilNode paths;
+    };
+
+    auto& unveil_data() { return m_unveil_data; }
+    auto const& unveil_data() const { return m_unveil_data; }
+
     bool wait_for_tracer_at_next_execve() const
     {
         return m_wait_for_tracer_at_next_execve;
@@ -805,8 +813,7 @@ private:
 
     RefPtr<Timer> m_alarm_timer;
 
-    VeilState m_veil_state { VeilState::None };
+    SpinlockProtected<UnveilData> m_unveil_data;
-    UnveilNode m_unveiled_paths;
 
     OwnPtr<PerformanceEventBuffer> m_perf_event_buffer;
 

Kernel/ProcessSpecificExposed.cpp

@@ -145,25 +145,28 @@ ErrorOr<void> Process::procfs_get_pledge_stats(KBufferBuilder& builder) const
 ErrorOr<void> Process::procfs_get_unveil_stats(KBufferBuilder& builder) const
 {
     auto array = TRY(JsonArraySerializer<>::try_create(builder));
-    TRY(unveiled_paths().for_each_node_in_tree_order([&](auto const& unveiled_path) -> ErrorOr<IterationDecision> {
+    TRY(m_unveil_data.with([&](auto& unveil_data) -> ErrorOr<void> {
-        if (!unveiled_path.was_explicitly_unveiled())
+        TRY(unveil_data.paths.for_each_node_in_tree_order([&](auto const& unveiled_path) -> ErrorOr<IterationDecision> {
+            if (!unveiled_path.was_explicitly_unveiled())
+                return IterationDecision::Continue;
+            auto obj = TRY(array.add_object());
+            TRY(obj.add("path", unveiled_path.path()));
+            StringBuilder permissions_builder;
+            if (unveiled_path.permissions() & UnveilAccess::Read)
+                permissions_builder.append('r');
+            if (unveiled_path.permissions() & UnveilAccess::Write)
+                permissions_builder.append('w');
+            if (unveiled_path.permissions() & UnveilAccess::Execute)
+                permissions_builder.append('x');
+            if (unveiled_path.permissions() & UnveilAccess::CreateOrRemove)
+                permissions_builder.append('c');
+            if (unveiled_path.permissions() & UnveilAccess::Browse)
+                permissions_builder.append('b');
+            TRY(obj.add("permissions", permissions_builder.string_view()));
+            TRY(obj.finish());
             return IterationDecision::Continue;
-        auto obj = TRY(array.add_object());
+        }));
-        TRY(obj.add("path", unveiled_path.path()));
+        return {};
-        StringBuilder permissions_builder;
-        if (unveiled_path.permissions() & UnveilAccess::Read)
-            permissions_builder.append('r');
-        if (unveiled_path.permissions() & UnveilAccess::Write)
-            permissions_builder.append('w');
-        if (unveiled_path.permissions() & UnveilAccess::Execute)
-            permissions_builder.append('x');
-        if (unveiled_path.permissions() & UnveilAccess::CreateOrRemove)
-            permissions_builder.append('c');
-        if (unveiled_path.permissions() & UnveilAccess::Browse)
-            permissions_builder.append('b');
-        TRY(obj.add("permissions", permissions_builder.string_view()));
-        TRY(obj.finish());
-        return IterationDecision::Continue;
     }));
     TRY(array.finish());
     return {};

Kernel/Syscalls/execve.cpp

@@ -523,9 +523,12 @@ ErrorOr<void> Process::do_exec(NonnullRefPtr<OpenFileDescription> main_program_d
     m_arguments = move(arguments);
     m_environment = move(environment);
 
-    m_veil_state = VeilState::None;
+    TRY(m_unveil_data.with([&](auto& unveil_data) -> ErrorOr<void> {
-    m_unveiled_paths.clear();
+        unveil_data.state = VeilState::None;
-    m_unveiled_paths.set_metadata({ TRY(KString::try_create("/"sv)), UnveilAccess::None, false });
+        unveil_data.paths.clear();
+        unveil_data.paths.set_metadata({ TRY(KString::try_create("/"sv)), UnveilAccess::None, false });
+        return {};
+    }));
 
     for (auto& property : m_coredump_properties)
         property = {};

Kernel/Syscalls/fork.cpp

@@ -20,8 +20,13 @@ ErrorOr<FlatPtr> Process::sys$fork(RegisterState& regs)
     RefPtr<Thread> child_first_thread;
     auto child_name = TRY(m_name->try_clone());
     auto child = TRY(Process::try_create(child_first_thread, move(child_name), uid(), gid(), pid(), m_is_kernel_process, current_directory(), m_executable, m_tty, this));
-    child->m_veil_state = m_veil_state;
+    TRY(m_unveil_data.with([&](auto& parent_unveil_data) -> ErrorOr<void> {
-    child->m_unveiled_paths = TRY(m_unveiled_paths.deep_copy());
+        return child->m_unveil_data.with([&](auto& child_unveil_data) -> ErrorOr<void> {
+            child_unveil_data.state = parent_unveil_data.state;
+            child_unveil_data.paths = TRY(parent_unveil_data.paths.deep_copy());
+            return {};
+        });
+    }));
 
     TRY(child->m_fds.with_exclusive([&](auto& child_fds) {
         return m_fds.with_exclusive([&](auto& parent_fds) {

Kernel/Syscalls/unveil.cpp

@@ -30,11 +30,11 @@ ErrorOr<FlatPtr> Process::sys$unveil(Userspace<const Syscall::SC_unveil_params*>
     auto params = TRY(copy_typed_from_user(user_params));
 
     if (!params.path.characters && !params.permissions.characters) {
-        m_veil_state = VeilState::Locked;
+        m_unveil_data.with([&](auto& unveil_data) { unveil_data.state = VeilState::Locked; });
         return 0;
     }
 
-    if (m_veil_state == VeilState::Locked)
+    if (veil_state() == VeilState::Locked)
         return EPERM;
 
     if (!params.path.characters || !params.permissions.characters)
@@ -94,39 +94,41 @@ ErrorOr<FlatPtr> Process::sys$unveil(Userspace<const Syscall::SC_unveil_params*>
 
     auto path_parts = KLexicalPath::parts(new_unveiled_path->view());
     auto it = path_parts.begin();
-    auto& matching_node = m_unveiled_paths.traverse_until_last_accessible_node(it, path_parts.end());
+    return m_unveil_data.with([&](auto& unveil_data) -> ErrorOr<FlatPtr> {
-    if (it.is_end()) {
+        auto& matching_node = unveil_data.paths.traverse_until_last_accessible_node(it, path_parts.end());
-        // If the path has already been explicitly unveiled, do not allow elevating its permissions.
+        if (it.is_end()) {
-        if (matching_node.was_explicitly_unveiled()) {
+            // If the path has already been explicitly unveiled, do not allow elevating its permissions.
-            if (new_permissions & ~matching_node.permissions())
+            if (matching_node.was_explicitly_unveiled()) {
-                return EPERM;
+                if (new_permissions & ~matching_node.permissions())
+                    return EPERM;
+            }
+
+            // It is possible that nodes that are "grandchildren" of the matching node have already been unveiled.
+            // This means that there may be intermediate nodes between this one and the unveiled "grandchildren"
+            // that inherited the current node's previous permissions. Those nodes now need their permissions
+            // updated to match the current node.
+            if (matching_node.permissions() != new_permissions)
+                update_intermediate_node_permissions(matching_node, (UnveilAccess)new_permissions);
+
+            matching_node.metadata_value().explicitly_unveiled = true;
+            matching_node.metadata_value().permissions = (UnveilAccess)new_permissions;
+            unveil_data.state = VeilState::Dropped;
+            return 0;
         }
 
-        // It is possible that nodes that are "grandchildren" of the matching node have already been unveiled.
+        TRY(matching_node.insert(
-        // This means that there may be intermediate nodes between this one and the unveiled "grandchildren"
+            it,
-        // that inherited the current node's previous permissions. Those nodes now need their permissions
+            path_parts.end(),
-        // updated to match the current node.
+            { new_unveiled_path.release_nonnull(), (UnveilAccess)new_permissions, true },
-        if (matching_node.permissions() != new_permissions)
+            [](auto& parent, auto& it) -> ErrorOr<Optional<UnveilMetadata>> {
-            update_intermediate_node_permissions(matching_node, (UnveilAccess)new_permissions);
+                auto path = TRY(KString::formatted("{}/{}", parent.path(), *it));
+                return UnveilMetadata(move(path), parent.permissions(), false);
+            }));
 
-        matching_node.metadata_value().explicitly_unveiled = true;
+        VERIFY(unveil_data.state != VeilState::Locked);
-        matching_node.metadata_value().permissions = (UnveilAccess)new_permissions;
+        unveil_data.state = VeilState::Dropped;
-        m_veil_state = VeilState::Dropped;
         return 0;
-    }
+    });
-
-    TRY(matching_node.insert(
-        it,
-        path_parts.end(),
-        { new_unveiled_path.release_nonnull(), (UnveilAccess)new_permissions, true },
-        [](auto& parent, auto& it) -> ErrorOr<Optional<UnveilMetadata>> {
-            auto path = TRY(KString::formatted("{}/{}", parent.path(), *it));
-            return UnveilMetadata(move(path), parent.permissions(), false);
-        }));
-
-    VERIFY(m_veil_state != VeilState::Locked);
-    m_veil_state = VeilState::Dropped;
-    return 0;
 }
 
 }