From 0bb7c8f4c4757ed1f1abb77a46ba32e9cf72106d Mon Sep 17 00:00:00 2001 From: Liav A Date: Sat, 26 Nov 2022 14:42:35 +0200 Subject: [PATCH] Kernel+SystemServer: Don't hardcode coredump directory path Instead, allow userspace to decide on the coredump directory path. By default, SystemServer sets it to the /tmp/coredump directory, but users can now change this by writing a new path to the sysfs node at /sys/kernel/variables/coredump_directory, and also to read this node to check where coredumps are currently generated at. --- Kernel/CMakeLists.txt | 2 + Kernel/Coredump.cpp | 8 ++++ Kernel/Coredump.h | 2 + .../Kernel/Variables/CoredumpDirectory.cpp | 45 ++++++++++++++++++ .../Kernel/Variables/CoredumpDirectory.h | 30 ++++++++++++ .../Subsystems/Kernel/Variables/Directory.cpp | 2 + .../Kernel/Variables/StringVariable.cpp | 44 ++++++++++++++++++ .../Kernel/Variables/StringVariable.h | 46 +++++++++++++++++++ Kernel/Process.cpp | 11 ++++- Userland/Services/SystemServer/main.cpp | 14 ++++++ 10 files changed, 203 insertions(+), 1 deletion(-) create mode 100644 Kernel/FileSystem/SysFS/Subsystems/Kernel/Variables/CoredumpDirectory.cpp create mode 100644 Kernel/FileSystem/SysFS/Subsystems/Kernel/Variables/CoredumpDirectory.h create mode 100644 Kernel/FileSystem/SysFS/Subsystems/Kernel/Variables/StringVariable.cpp create mode 100644 Kernel/FileSystem/SysFS/Subsystems/Kernel/Variables/StringVariable.h diff --git a/Kernel/CMakeLists.txt b/Kernel/CMakeLists.txt index da64657a43..49f2d93093 100644 --- a/Kernel/CMakeLists.txt +++ b/Kernel/CMakeLists.txt @@ -196,8 +196,10 @@ set(KERNEL_SOURCES FileSystem/SysFS/Subsystems/Kernel/Network/UDP.cpp FileSystem/SysFS/Subsystems/Kernel/Variables/BooleanVariable.cpp FileSystem/SysFS/Subsystems/Kernel/Variables/CapsLockRemap.cpp + FileSystem/SysFS/Subsystems/Kernel/Variables/CoredumpDirectory.cpp FileSystem/SysFS/Subsystems/Kernel/Variables/Directory.cpp FileSystem/SysFS/Subsystems/Kernel/Variables/DumpKmallocStack.cpp + FileSystem/SysFS/Subsystems/Kernel/Variables/StringVariable.cpp FileSystem/SysFS/Subsystems/Kernel/Variables/UBSANDeadly.cpp FileSystem/TmpFS/FileSystem.cpp FileSystem/TmpFS/Inode.cpp diff --git a/Kernel/Coredump.cpp b/Kernel/Coredump.cpp index 82c99329f1..4aa70c5230 100644 --- a/Kernel/Coredump.cpp +++ b/Kernel/Coredump.cpp @@ -9,6 +9,7 @@ #include #include +#include #include #include #include @@ -22,8 +23,15 @@ #define INCLUDE_USERSPACE_HEAP_MEMORY_IN_COREDUMPS 0 +static Singleton>> s_coredump_directory_path; + namespace Kernel { +SpinlockProtected>& Coredump::directory_path() +{ + return s_coredump_directory_path; +} + bool Coredump::FlatRegionData::looks_like_userspace_heap_region() const { return name().starts_with("LibJS:"sv) || name().starts_with("malloc:"sv); diff --git a/Kernel/Coredump.h b/Kernel/Coredump.h index 89567da718..d1acea68d8 100644 --- a/Kernel/Coredump.h +++ b/Kernel/Coredump.h @@ -11,6 +11,7 @@ #include #include #include +#include #include namespace Kernel { @@ -18,6 +19,7 @@ namespace Kernel { class Coredump { public: static ErrorOr> try_create(NonnullLockRefPtr, StringView output_path); + static SpinlockProtected>& directory_path(); ~Coredump() = default; ErrorOr write(); diff --git a/Kernel/FileSystem/SysFS/Subsystems/Kernel/Variables/CoredumpDirectory.cpp b/Kernel/FileSystem/SysFS/Subsystems/Kernel/Variables/CoredumpDirectory.cpp new file mode 100644 index 0000000000..b889ce02e4 --- /dev/null +++ b/Kernel/FileSystem/SysFS/Subsystems/Kernel/Variables/CoredumpDirectory.cpp @@ -0,0 +1,45 @@ +/* + * Copyright (c) 2022, Liav A. + * + * SPDX-License-Identifier: BSD-2-Clause + */ + +#include +#include +#include + +namespace Kernel { + +UNMAP_AFTER_INIT SysFSCoredumpDirectory::SysFSCoredumpDirectory(SysFSDirectory const& parent_directory) + : SysFSSystemStringVariable(parent_directory) +{ +} + +UNMAP_AFTER_INIT NonnullLockRefPtr SysFSCoredumpDirectory::must_create(SysFSDirectory const& parent_directory) +{ + return adopt_lock_ref_if_nonnull(new (nothrow) SysFSCoredumpDirectory(parent_directory)).release_nonnull(); +} + +ErrorOr> SysFSCoredumpDirectory::value() const +{ + return Coredump::directory_path().with([&](auto& coredump_directory_path) -> ErrorOr> { + if (coredump_directory_path) + return KString::try_create(coredump_directory_path->view()); + return KString::try_create(""sv); + }); +} +void SysFSCoredumpDirectory::set_value(NonnullOwnPtr new_value) +{ + Coredump::directory_path().with([&](auto& coredump_directory_path) { + coredump_directory_path = move(new_value); + }); +} + +mode_t SysFSCoredumpDirectory::permissions() const +{ + // NOTE: Let's not allow users to randomly change the coredump path, but only + // allow this for the root user. + return S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH; +} + +} diff --git a/Kernel/FileSystem/SysFS/Subsystems/Kernel/Variables/CoredumpDirectory.h b/Kernel/FileSystem/SysFS/Subsystems/Kernel/Variables/CoredumpDirectory.h new file mode 100644 index 0000000000..2bc6fefa0a --- /dev/null +++ b/Kernel/FileSystem/SysFS/Subsystems/Kernel/Variables/CoredumpDirectory.h @@ -0,0 +1,30 @@ +/* + * Copyright (c) 2022, Liav A. + * + * SPDX-License-Identifier: BSD-2-Clause + */ + +#pragma once + +#include +#include +#include +#include + +namespace Kernel { + +class SysFSCoredumpDirectory final : public SysFSSystemStringVariable { +public: + virtual StringView name() const override { return "coredump_directory"sv; } + static NonnullLockRefPtr must_create(SysFSDirectory const&); + +private: + virtual ErrorOr> value() const override; + virtual void set_value(NonnullOwnPtr new_value) override; + + explicit SysFSCoredumpDirectory(SysFSDirectory const&); + + virtual mode_t permissions() const override; +}; + +} diff --git a/Kernel/FileSystem/SysFS/Subsystems/Kernel/Variables/Directory.cpp b/Kernel/FileSystem/SysFS/Subsystems/Kernel/Variables/Directory.cpp index 06d265347e..f930dc8a6f 100644 --- a/Kernel/FileSystem/SysFS/Subsystems/Kernel/Variables/Directory.cpp +++ b/Kernel/FileSystem/SysFS/Subsystems/Kernel/Variables/Directory.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -21,6 +22,7 @@ UNMAP_AFTER_INIT NonnullLockRefPtr SysFSGlo list.append(SysFSCapsLockRemap::must_create(*global_variables_directory)); list.append(SysFSDumpKmallocStacks::must_create(*global_variables_directory)); list.append(SysFSUBSANDeadly::must_create(*global_variables_directory)); + list.append(SysFSCoredumpDirectory::must_create(*global_variables_directory)); return {}; })); return global_variables_directory; diff --git a/Kernel/FileSystem/SysFS/Subsystems/Kernel/Variables/StringVariable.cpp b/Kernel/FileSystem/SysFS/Subsystems/Kernel/Variables/StringVariable.cpp new file mode 100644 index 0000000000..b4aa97ba20 --- /dev/null +++ b/Kernel/FileSystem/SysFS/Subsystems/Kernel/Variables/StringVariable.cpp @@ -0,0 +1,44 @@ +/* + * Copyright (c) 2022, Liav A. + * + * SPDX-License-Identifier: BSD-2-Clause + */ + +#include +#include +#include + +namespace Kernel { + +ErrorOr SysFSSystemStringVariable::try_generate(KBufferBuilder& builder) +{ + auto string_value = TRY(value()); + return builder.appendff("{}\n", string_value->view()); +} + +ErrorOr SysFSSystemStringVariable::write_bytes(off_t, size_t count, UserOrKernelBuffer const& buffer, OpenFileDescription*) +{ + MutexLocker locker(m_refresh_lock); + // Note: We do all of this code before taking the spinlock because then we disable + // interrupts so page faults will not work. + char* value = nullptr; + auto new_value = TRY(KString::try_create_uninitialized(count, value)); + TRY(buffer.read(value, count)); + auto new_value_without_possible_newlines = TRY(KString::try_create(new_value->view().trim("\n"sv))); + return Process::current().jail().with([&](auto& my_jail) -> ErrorOr { + // Note: If we are in a jail, don't let the current process to change the variable. + if (my_jail) + return Error::from_errno(EPERM); + set_value(move(new_value_without_possible_newlines)); + return count; + }); +} + +ErrorOr SysFSSystemStringVariable::truncate(u64 size) +{ + if (size != 0) + return EPERM; + return {}; +} + +} diff --git a/Kernel/FileSystem/SysFS/Subsystems/Kernel/Variables/StringVariable.h b/Kernel/FileSystem/SysFS/Subsystems/Kernel/Variables/StringVariable.h new file mode 100644 index 0000000000..addba7e803 --- /dev/null +++ b/Kernel/FileSystem/SysFS/Subsystems/Kernel/Variables/StringVariable.h @@ -0,0 +1,46 @@ +/* + * Copyright (c) 2022, Liav A. + * + * SPDX-License-Identifier: BSD-2-Clause + */ + +#pragma once + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace Kernel { + +class SysFSSystemStringVariable : public SysFSGlobalInformation { +protected: + explicit SysFSSystemStringVariable(SysFSDirectory const& parent_directory) + : SysFSGlobalInformation(parent_directory) + { + } + virtual ErrorOr> value() const = 0; + virtual void set_value(NonnullOwnPtr new_value) = 0; + +private: + // ^SysFSGlobalInformation + virtual ErrorOr try_generate(KBufferBuilder&) override final; + + // ^SysFSExposedComponent + virtual ErrorOr write_bytes(off_t, size_t, UserOrKernelBuffer const&, OpenFileDescription*) override final; + virtual mode_t permissions() const override { return 0644; } + virtual ErrorOr truncate(u64) override final; +}; + +} diff --git a/Kernel/Process.cpp b/Kernel/Process.cpp index 8fe1b42961..4e21066044 100644 --- a/Kernel/Process.cpp +++ b/Kernel/Process.cpp @@ -696,7 +696,16 @@ ErrorOr Process::dump_core() VERIFY(is_dumpable()); VERIFY(should_generate_coredump()); dbgln("Generating coredump for pid: {}", pid().value()); - auto coredump_path = TRY(KString::formatted("/tmp/coredump/{}_{}_{}", name(), pid().value(), kgettimeofday().to_truncated_seconds())); + auto coredump_directory_path = TRY(Coredump::directory_path().with([&](auto& coredump_directory_path) -> ErrorOr> { + if (coredump_directory_path) + return KString::try_create(coredump_directory_path->view()); + return KString::try_create(""sv); + })); + if (coredump_directory_path->view() == ""sv) { + dbgln("Generating coredump for pid {} failed because coredump directory was not set.", pid().value()); + return {}; + } + auto coredump_path = TRY(KString::formatted("{}/{}_{}_{}", coredump_directory_path->view(), name(), pid().value(), kgettimeofday().to_truncated_seconds())); auto coredump = TRY(Coredump::try_create(*this, coredump_path->view())); return coredump->write(); } diff --git a/Userland/Services/SystemServer/main.cpp b/Userland/Services/SystemServer/main.cpp index fc2beb4694..7ca03237df 100644 --- a/Userland/Services/SystemServer/main.cpp +++ b/Userland/Services/SystemServer/main.cpp @@ -469,6 +469,19 @@ static ErrorOr create_tmp_coredump_directory() return {}; } +static ErrorOr set_default_coredump_directory() +{ + dbgln("Setting /tmp/coredump as the coredump directory"); + auto sysfs_coredump_directory_variable_fd = TRY(Core::System::open("/sys/kernel/variables/coredump_directory"sv, O_RDWR)); + ScopeGuard close_on_exit([&] { + close(sysfs_coredump_directory_variable_fd); + }); + auto tmp_coredump_directory_path = "/tmp/coredump"sv; + auto nwritten = TRY(Core::System::write(sysfs_coredump_directory_variable_fd, tmp_coredump_directory_path.bytes())); + VERIFY(static_cast(nwritten) == tmp_coredump_directory_path.length()); + return {}; +} + static ErrorOr create_tmp_semaphore_directory() { dbgln("Creating /tmp/semaphore directory"); @@ -494,6 +507,7 @@ ErrorOr serenity_main(Main::Arguments arguments) if (!user) { TRY(create_tmp_coredump_directory()); + TRY(set_default_coredump_directory()); TRY(create_tmp_semaphore_directory()); TRY(determine_system_mode()); }