From 1c0aa51684dd45b6a46d3df46a14bf85ebd525e5 Mon Sep 17 00:00:00 2001 From: Liav A Date: Sat, 19 Aug 2023 21:10:25 +0300 Subject: [PATCH] Kernel+Userland: Remove the {get,set}_thread_name syscalls These syscalls are not necessary on their own, and they give the false impression that a caller could set or get the thread name of any process in the system, which is not true. Therefore, move the functionality of these syscalls to be options in the prctl syscall, which makes it abundantly clear that these operations could only occur from a running thread in a process that sees other threads in that process only. --- Kernel/API/Syscall.h | 2 - Kernel/API/prctl_numbers.h | 2 + Kernel/Syscalls/prctl.cpp | 27 +++++++++++- Kernel/Syscalls/thread.cpp | 41 +------------------ Kernel/Tasks/Process.cpp | 13 ++++++ Kernel/Tasks/Process.h | 5 +-- Tests/Kernel/TestProcFSWrite.cpp | 2 +- Tests/LibJS/test262-runner.cpp | 2 +- .../DevTools/UserspaceEmulator/Emulator.h | 1 - .../UserspaceEmulator/Emulator_syscalls.cpp | 9 ---- Userland/Libraries/LibC/pthread.cpp | 5 ++- Userland/Libraries/LibC/sys/prctl.cpp | 3 +- Userland/Libraries/LibCore/Process.cpp | 3 +- Userland/Libraries/LibTest/CrashTest.cpp | 2 +- 14 files changed, 54 insertions(+), 63 deletions(-) diff --git a/Kernel/API/Syscall.h b/Kernel/API/Syscall.h index fbeedeef28..33eafe3545 100644 --- a/Kernel/API/Syscall.h +++ b/Kernel/API/Syscall.h @@ -91,7 +91,6 @@ enum class NeedsBigProcessLock { S(get_dir_entries, NeedsBigProcessLock::No) \ S(get_root_session_id, NeedsBigProcessLock::No) \ S(get_stack_bounds, NeedsBigProcessLock::No) \ - S(get_thread_name, NeedsBigProcessLock::No) \ S(getcwd, NeedsBigProcessLock::No) \ S(getegid, NeedsBigProcessLock::No) \ S(geteuid, NeedsBigProcessLock::No) \ @@ -162,7 +161,6 @@ enum class NeedsBigProcessLock { S(sendfd, NeedsBigProcessLock::No) \ S(sendmsg, NeedsBigProcessLock::Yes) \ S(set_mmap_name, NeedsBigProcessLock::No) \ - S(set_thread_name, NeedsBigProcessLock::No) \ S(setegid, NeedsBigProcessLock::No) \ S(seteuid, NeedsBigProcessLock::No) \ S(setgid, NeedsBigProcessLock::No) \ diff --git a/Kernel/API/prctl_numbers.h b/Kernel/API/prctl_numbers.h index 20976f2abd..1165712562 100644 --- a/Kernel/API/prctl_numbers.h +++ b/Kernel/API/prctl_numbers.h @@ -13,3 +13,5 @@ #define PR_SET_COREDUMP_METADATA_VALUE 5 #define PR_SET_PROCESS_NAME 6 #define PR_GET_PROCESS_NAME 7 +#define PR_SET_THREAD_NAME 8 +#define PR_GET_THREAD_NAME 9 diff --git a/Kernel/Syscalls/prctl.cpp b/Kernel/Syscalls/prctl.cpp index 116a447cc9..f783506dd9 100644 --- a/Kernel/Syscalls/prctl.cpp +++ b/Kernel/Syscalls/prctl.cpp @@ -9,7 +9,7 @@ namespace Kernel { -ErrorOr Process::sys$prctl(int option, FlatPtr arg1, FlatPtr arg2) +ErrorOr Process::sys$prctl(int option, FlatPtr arg1, FlatPtr arg2, FlatPtr arg3) { VERIFY_NO_PROCESS_BIG_LOCK(this); return with_mutable_protected_data([&](auto& protected_data) -> ErrorOr { @@ -71,6 +71,31 @@ ErrorOr Process::sys$prctl(int option, FlatPtr arg1, FlatPtr arg2) })); return 0; } + case PR_SET_THREAD_NAME: { + TRY(require_promise(Pledge::stdio)); + int thread_id = static_cast(arg1); + Userspace buffer = arg2; + size_t buffer_size = static_cast(arg3); + + Thread::Name thread_name {}; + TRY(try_copy_name_from_user_into_fixed_string_buffer(buffer, thread_name, buffer_size)); + auto thread = TRY(get_thread_from_thread_list(thread_id)); + thread->set_name(thread_name.representable_view()); + return 0; + } + case PR_GET_THREAD_NAME: { + TRY(require_promise(Pledge::thread)); + int thread_id = static_cast(arg1); + Userspace buffer = arg2; + size_t buffer_size = static_cast(arg3); + auto thread = TRY(get_thread_from_thread_list(thread_id)); + + TRY(thread->name().with([&](auto& thread_name) -> ErrorOr { + VERIFY(!thread_name.representable_view().is_null()); + return copy_fixed_string_buffer_including_null_char_to_user(buffer, buffer_size, thread_name); + })); + return 0; + } } return EINVAL; diff --git a/Kernel/Syscalls/thread.cpp b/Kernel/Syscalls/thread.cpp index 7c6f2ae317..ac67899f31 100644 --- a/Kernel/Syscalls/thread.cpp +++ b/Kernel/Syscalls/thread.cpp @@ -46,7 +46,7 @@ ErrorOr Process::sys$create_thread(void* (*entry)(void*), Userspacetid().value()); })); @@ -189,45 +189,6 @@ ErrorOr Process::sys$kill_thread(pid_t tid, int signal) return 0; } -ErrorOr Process::sys$set_thread_name(pid_t tid, Userspace user_name, size_t user_name_length) -{ - VERIFY_NO_PROCESS_BIG_LOCK(this); - TRY(require_promise(Pledge::stdio)); - - Thread::Name thread_name {}; - TRY(try_copy_name_from_user_into_fixed_string_buffer<64>(user_name, thread_name, user_name_length)); - - auto thread = Thread::from_tid(tid); - if (!thread || thread->pid() != pid()) - return ESRCH; - - thread->set_name(thread_name.representable_view()); - return 0; -} - -ErrorOr Process::sys$get_thread_name(pid_t tid, Userspace buffer, size_t buffer_size) -{ - VERIFY_NO_PROCESS_BIG_LOCK(this); - TRY(require_promise(Pledge::thread)); - if (buffer_size == 0) - return EINVAL; - - auto thread = Thread::from_tid(tid); - if (!thread || thread->pid() != pid()) - return ESRCH; - - TRY(thread->name().with([&](auto& thread_name) -> ErrorOr { - VERIFY(!thread_name.representable_view().is_null()); - auto thread_name_view = thread_name.representable_view(); - if (thread_name_view.length() + 1 > buffer_size) - return ENAMETOOLONG; - - return copy_to_user(buffer, thread_name_view.characters_without_null_termination(), thread_name_view.length() + 1); - })); - - return 0; -} - ErrorOr Process::sys$gettid() { VERIFY_NO_PROCESS_BIG_LOCK(this); diff --git a/Kernel/Tasks/Process.cpp b/Kernel/Tasks/Process.cpp index 597b073e54..40a0901559 100644 --- a/Kernel/Tasks/Process.cpp +++ b/Kernel/Tasks/Process.cpp @@ -639,6 +639,19 @@ size_t Process::OpenFileDescriptions::open_count() const return count; } +ErrorOr> Process::get_thread_from_thread_list(pid_t tid) +{ + if (tid < 0) + return ESRCH; + return m_thread_list.with([tid](auto& list) -> ErrorOr> { + for (auto& thread : list) { + if (thread.tid() == tid) + return thread; + } + return ESRCH; + }); +} + ErrorOr Process::OpenFileDescriptions::allocate(int first_candidate_fd) { for (size_t i = first_candidate_fd; i < max_open(); ++i) { diff --git a/Kernel/Tasks/Process.h b/Kernel/Tasks/Process.h index c3578a841a..02c6883351 100644 --- a/Kernel/Tasks/Process.h +++ b/Kernel/Tasks/Process.h @@ -434,8 +434,6 @@ public: [[noreturn]] void sys$exit_thread(Userspace, Userspace, size_t); ErrorOr sys$join_thread(pid_t tid, Userspace exit_value); ErrorOr sys$detach_thread(pid_t tid); - ErrorOr sys$set_thread_name(pid_t tid, Userspace buffer, size_t buffer_size); - ErrorOr sys$get_thread_name(pid_t tid, Userspace buffer, size_t buffer_size); ErrorOr sys$kill_thread(pid_t tid, int signal); ErrorOr sys$rename(Userspace); ErrorOr sys$mknod(Userspace); @@ -459,7 +457,7 @@ public: ErrorOr sys$sysconf(int name); ErrorOr sys$disown(ProcessID); ErrorOr sys$allocate_tls(Userspace initial_data, size_t); - ErrorOr sys$prctl(int option, FlatPtr arg1, FlatPtr arg2); + ErrorOr sys$prctl(int option, FlatPtr arg1, FlatPtr arg2, FlatPtr arg3); ErrorOr sys$anon_create(size_t, int options); ErrorOr sys$statvfs(Userspace user_params); ErrorOr sys$fstatvfs(int fd, statvfs* buf); @@ -901,6 +899,7 @@ private: SpinlockProtected const& thread_list() const { return m_thread_list; } ErrorOr> get_thread_from_pid_or_tid(pid_t pid_or_tid, Syscall::SchedulerParametersMode mode); + ErrorOr> get_thread_from_thread_list(pid_t tid); SpinlockProtected m_thread_list {}; diff --git a/Tests/Kernel/TestProcFSWrite.cpp b/Tests/Kernel/TestProcFSWrite.cpp index 2559543777..0245e99138 100644 --- a/Tests/Kernel/TestProcFSWrite.cpp +++ b/Tests/Kernel/TestProcFSWrite.cpp @@ -21,7 +21,7 @@ TEST_CASE(check_root) // If running as setuid, the process is automatically marked as non-dumpable, which bars access to /proc/self/. // However, that is the easiest guess for a /proc/$PID/ directory, so we'd like to use that. // In order to do so, mark this process as dumpable: - EXPECT_EQ(prctl(PR_SET_DUMPABLE, 1, 0), 0); + EXPECT_EQ(prctl(PR_SET_DUMPABLE, 1, 0, 0), 0); } TEST_CASE(root_writes_to_procfs) diff --git a/Tests/LibJS/test262-runner.cpp b/Tests/LibJS/test262-runner.cpp index 9477a78801..beb69b8b09 100644 --- a/Tests/LibJS/test262-runner.cpp +++ b/Tests/LibJS/test262-runner.cpp @@ -574,7 +574,7 @@ int main(int argc, char** argv) args_parser.parse(arguments); #if !defined(AK_OS_MACOS) && !defined(AK_OS_EMSCRIPTEN) - if (disable_core_dumping && prctl(PR_SET_DUMPABLE, 0, 0) < 0) { + if (disable_core_dumping && prctl(PR_SET_DUMPABLE, 0, 0, 0) < 0) { perror("prctl(PR_SET_DUMPABLE)"); return exit_wrong_arguments; } diff --git a/Userland/DevTools/UserspaceEmulator/Emulator.h b/Userland/DevTools/UserspaceEmulator/Emulator.h index 0ffef1c154..f867ea4434 100644 --- a/Userland/DevTools/UserspaceEmulator/Emulator.h +++ b/Userland/DevTools/UserspaceEmulator/Emulator.h @@ -229,7 +229,6 @@ private: int virt$sendmsg(int sockfd, FlatPtr msg_addr, int flags); int virt$set_mmap_name(FlatPtr); int virt$set_process_name(FlatPtr buffer, int size); - int virt$set_thread_name(pid_t, FlatPtr, size_t); int virt$setgid(gid_t); int virt$setgroups(ssize_t count, FlatPtr); int virt$setpgid(pid_t pid, pid_t pgid); diff --git a/Userland/DevTools/UserspaceEmulator/Emulator_syscalls.cpp b/Userland/DevTools/UserspaceEmulator/Emulator_syscalls.cpp index 32217a2335..82e469ce44 100644 --- a/Userland/DevTools/UserspaceEmulator/Emulator_syscalls.cpp +++ b/Userland/DevTools/UserspaceEmulator/Emulator_syscalls.cpp @@ -214,8 +214,6 @@ u32 Emulator::virt_syscall(u32 function, u32 arg1, u32 arg2, u32 arg3) return virt$sendmsg(arg1, arg2, arg3); case SC_set_mmap_name: return virt$set_mmap_name(arg1); - case SC_set_thread_name: - return virt$set_thread_name(arg1, arg2, arg3); case SC_setgid: return virt$setgid(arg2); case SC_setgroups: @@ -1516,13 +1514,6 @@ int Emulator::virt$scheduler_set_parameters(FlatPtr user_addr) return syscall(SC_scheduler_set_parameters, &user_param); } -int Emulator::virt$set_thread_name(pid_t pid, FlatPtr name_addr, size_t name_length) -{ - auto user_name = mmu().copy_buffer_from_vm(name_addr, name_length); - auto name = DeprecatedString::formatted("(UE) {}", StringView { user_name.data(), user_name.size() }); - return syscall(SC_set_thread_name, pid, name.characters(), name.length()); -} - pid_t Emulator::virt$setsid() { return syscall(SC_setsid); diff --git a/Userland/Libraries/LibC/pthread.cpp b/Userland/Libraries/LibC/pthread.cpp index 5e7947c9fa..cd2760fa71 100644 --- a/Userland/Libraries/LibC/pthread.cpp +++ b/Userland/Libraries/LibC/pthread.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -547,13 +548,13 @@ int pthread_setname_np(pthread_t thread, char const* name) { if (!name) return EFAULT; - int rc = syscall(SC_set_thread_name, thread, name, strlen(name)); + int rc = prctl(PR_SET_THREAD_NAME, thread, name, strlen(name)); __RETURN_PTHREAD_ERROR(rc); } int pthread_getname_np(pthread_t thread, char* buffer, size_t buffer_size) { - int rc = syscall(SC_get_thread_name, thread, buffer, buffer_size); + int rc = prctl(PR_GET_THREAD_NAME, thread, buffer, buffer_size); __RETURN_PTHREAD_ERROR(rc); } diff --git a/Userland/Libraries/LibC/sys/prctl.cpp b/Userland/Libraries/LibC/sys/prctl.cpp index 88d0afcd36..7042fc81ef 100644 --- a/Userland/Libraries/LibC/sys/prctl.cpp +++ b/Userland/Libraries/LibC/sys/prctl.cpp @@ -19,10 +19,11 @@ int prctl(int option, ...) uintptr_t arg1 = va_arg(args, uintptr_t); uintptr_t arg2 = va_arg(args, uintptr_t); + uintptr_t arg3 = va_arg(args, uintptr_t); va_end(args); - int rc = syscall(SC_prctl, option, arg1, arg2); + int rc = syscall(SC_prctl, option, arg1, arg2, arg3); __RETURN_WITH_ERRNO(rc, rc, -1); } } diff --git a/Userland/Libraries/LibCore/Process.cpp b/Userland/Libraries/LibCore/Process.cpp index d638d8d9dd..8ab9596093 100644 --- a/Userland/Libraries/LibCore/Process.cpp +++ b/Userland/Libraries/LibCore/Process.cpp @@ -19,6 +19,7 @@ #if defined(AK_OS_SERENITY) # include +# include # include #elif defined(AK_OS_BSD_GENERIC) && !defined(AK_OS_SOLARIS) # include @@ -137,7 +138,7 @@ ErrorOr Process::set_name([[maybe_unused]] StringView name, [[maybe_unused if (set_thread_name == SetThreadName::No) return {}; - rc = syscall(SC_set_thread_name, gettid(), name.characters_without_null_termination(), name.length()); + rc = prctl(PR_SET_THREAD_NAME, gettid(), name.characters_without_null_termination(), name.length()); if (rc != 0) return Error::from_syscall("set_thread_name"sv, -rc); return {}; diff --git a/Userland/Libraries/LibTest/CrashTest.cpp b/Userland/Libraries/LibTest/CrashTest.cpp index d3f221ddf4..8a674c12b4 100644 --- a/Userland/Libraries/LibTest/CrashTest.cpp +++ b/Userland/Libraries/LibTest/CrashTest.cpp @@ -39,7 +39,7 @@ bool Crash::run(RunType run_type) VERIFY_NOT_REACHED(); } else if (pid == 0) { #if !defined(AK_OS_MACOS) && !defined(AK_OS_EMSCRIPTEN) - if (prctl(PR_SET_DUMPABLE, 0, 0) < 0) + if (prctl(PR_SET_DUMPABLE, 0, 0, 0) < 0) perror("prctl(PR_SET_DUMPABLE)"); #endif exit((int)m_crash_function());