From d8b514873f8d0cfd2413b074a4c383278e125408 Mon Sep 17 00:00:00 2001 From: Liav A Date: Mon, 17 Jul 2023 19:22:01 +0300 Subject: [PATCH] Kernel: Use FixedStringBuffer for fixed-length strings in syscalls Using the kernel stack is preferable, especially when the examined strings should be limited to a reasonable length. This is a small improvement, because if we don't actually move these strings then we don't need to own heap allocations for them during the syscall handler function scope. In addition to that, some kernel strings are known to be limited, like the hostname string, for these strings we also can use FixedStringBuffer to store and copy to and from these buffers, without using any heap allocations at all. --- Base/usr/share/man/man2/pledge.md | 1 + Base/usr/share/man/man2/unveil.md | 1 + Kernel/FileSystem/MountFile.cpp | 17 +++++++------ Kernel/Net/IPv4Socket.cpp | 5 ++-- Kernel/Net/Socket.cpp | 4 +-- Kernel/Syscalls/hostname.cpp | 16 +++++++----- Kernel/Syscalls/mount.cpp | 6 +++-- Kernel/Syscalls/pledge.cpp | 31 ++++++++++++----------- Kernel/Syscalls/uname.cpp | 7 +++--- Kernel/Syscalls/unveil.cpp | 7 ++---- Kernel/Tasks/Process.cpp | 6 ++--- Kernel/Tasks/Process.h | 41 ++++++++++++++++++++++++++++++- Tests/Kernel/TestKernelUnveil.cpp | 4 +++ 13 files changed, 100 insertions(+), 46 deletions(-) diff --git a/Base/usr/share/man/man2/pledge.md b/Base/usr/share/man/man2/pledge.md index 979b06c577..c399c7b1f2 100644 --- a/Base/usr/share/man/man2/pledge.md +++ b/Base/usr/share/man/man2/pledge.md @@ -65,6 +65,7 @@ Promises marked with an asterisk (\*) are SerenityOS specific extensions not sup * `EFAULT`: `promises` and/or `execpromises` are not null and not in readable memory. * `EINVAL`: One or more invalid promises were specified. * `EPERM`: An attempt to increase capabilities was rejected. +* `E2BIG`: `promises` string or `execpromises `string are longer than all known promises strings together. ## History diff --git a/Base/usr/share/man/man2/unveil.md b/Base/usr/share/man/man2/unveil.md index 2c1c7c38a3..2afbd9b389 100644 --- a/Base/usr/share/man/man2/unveil.md +++ b/Base/usr/share/man/man2/unveil.md @@ -68,6 +68,7 @@ the error. * `EPERM`: The veil is locked, or an attempt to add more permissions for an already unveiled path was rejected. * `EINVAL`: `path` is not an absolute path, or `permissions` are malformed. +* `E2BIG`: `permissions` string is longer than 5 characters. All of the usual path resolution errors may also occur. diff --git a/Kernel/FileSystem/MountFile.cpp b/Kernel/FileSystem/MountFile.cpp index b2b6d81e6f..8f1f79d8af 100644 --- a/Kernel/FileSystem/MountFile.cpp +++ b/Kernel/FileSystem/MountFile.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -16,6 +17,7 @@ #include #include #include +#include namespace Kernel { @@ -51,8 +53,10 @@ ErrorOr MountFile::ioctl(OpenFileDescription&, unsigned request, Userspace auto mount_specific_data = TRY(copy_typed_from_user(user_mount_specific_data)); if ((mount_specific_data.value_type == MountSpecificFlag::ValueType::SignedInteger || mount_specific_data.value_type == MountSpecificFlag::ValueType::UnsignedInteger) && mount_specific_data.value_length != 8) return EDOM; - if (mount_specific_data.key_string_length > MOUNT_SPECIFIC_FLAG_KEY_STRING_MAX_LENGTH) - return ENAMETOOLONG; + + Syscall::StringArgument user_key_string { reinterpret_cast(mount_specific_data.key_string_addr), static_cast(mount_specific_data.key_string_length) }; + auto key_string = TRY(Process::get_syscall_name_string_fixed_buffer(user_key_string)); + if (mount_specific_data.value_type != MountSpecificFlag::ValueType::Boolean && mount_specific_data.value_length == 0) return EINVAL; if (mount_specific_data.value_type != MountSpecificFlag::ValueType::Boolean && mount_specific_data.value_addr == nullptr) @@ -71,7 +75,6 @@ ErrorOr MountFile::ioctl(OpenFileDescription&, unsigned request, Userspace // NOTE: We enforce that the passed argument will be either i64 or u64, so it will always be // exactly 8 bytes. We do that to simplify handling of integers as well as to ensure ABI correctness // in all possible cases. - auto key_string = TRY(try_copy_kstring_from_user(reinterpret_cast(mount_specific_data.key_string_addr), static_cast(mount_specific_data.key_string_length))); switch (mount_specific_data.value_type) { // NOTE: This is actually considered as simply boolean flag. case MountSpecificFlag::ValueType::Boolean: { @@ -81,27 +84,27 @@ ErrorOr MountFile::ioctl(OpenFileDescription&, unsigned request, Userspace if (value_integer != 0 && value_integer != 1) return EDOM; bool value = (value_integer == 1) ? true : false; - TRY(m_file_system_initializer.handle_mount_boolean_flag(our_mount_specific_data->bytes(), key_string->view(), value)); + TRY(m_file_system_initializer.handle_mount_boolean_flag(our_mount_specific_data->bytes(), key_string.representable_view(), value)); return {}; } case MountSpecificFlag::ValueType::UnsignedInteger: { VERIFY(m_file_system_initializer.handle_mount_unsigned_integer_flag); Userspace user_value_addr(reinterpret_cast(mount_specific_data.value_addr)); auto value_integer = TRY(copy_typed_from_user(user_value_addr)); - TRY(m_file_system_initializer.handle_mount_unsigned_integer_flag(our_mount_specific_data->bytes(), key_string->view(), value_integer)); + TRY(m_file_system_initializer.handle_mount_unsigned_integer_flag(our_mount_specific_data->bytes(), key_string.representable_view(), value_integer)); return {}; } case MountSpecificFlag::ValueType::SignedInteger: { VERIFY(m_file_system_initializer.handle_mount_signed_integer_flag); Userspace user_value_addr(reinterpret_cast(mount_specific_data.value_addr)); auto value_integer = TRY(copy_typed_from_user(user_value_addr)); - TRY(m_file_system_initializer.handle_mount_signed_integer_flag(our_mount_specific_data->bytes(), key_string->view(), value_integer)); + TRY(m_file_system_initializer.handle_mount_signed_integer_flag(our_mount_specific_data->bytes(), key_string.representable_view(), value_integer)); return {}; } case MountSpecificFlag::ValueType::ASCIIString: { VERIFY(m_file_system_initializer.handle_mount_ascii_string_flag); auto value_string = TRY(try_copy_kstring_from_user(reinterpret_cast(mount_specific_data.value_addr), static_cast(mount_specific_data.value_length))); - TRY(m_file_system_initializer.handle_mount_ascii_string_flag(our_mount_specific_data->bytes(), key_string->view(), value_string->view())); + TRY(m_file_system_initializer.handle_mount_ascii_string_flag(our_mount_specific_data->bytes(), key_string.representable_view(), value_string->view())); return {}; } default: diff --git a/Kernel/Net/IPv4Socket.cpp b/Kernel/Net/IPv4Socket.cpp index 9c80ac6ff0..ad1bad00f8 100644 --- a/Kernel/Net/IPv4Socket.cpp +++ b/Kernel/Net/IPv4Socket.cpp @@ -622,9 +622,8 @@ ErrorOr IPv4Socket::ioctl(OpenFileDescription&, unsigned request, Userspac TRY(copy_from_user(&route, user_route)); Userspace user_rt_dev((FlatPtr)route.rt_dev); - auto ifname = TRY(try_copy_kstring_from_user(user_rt_dev, IFNAMSIZ)); - - auto adapter = NetworkingManagement::the().lookup_by_name(ifname->view()); + auto ifname = TRY(Process::get_syscall_name_string_fixed_buffer(user_rt_dev)); + auto adapter = NetworkingManagement::the().lookup_by_name(ifname.representable_view()); if (!adapter) return ENODEV; diff --git a/Kernel/Net/Socket.cpp b/Kernel/Net/Socket.cpp index d8a4e4cd8f..078bce0280 100644 --- a/Kernel/Net/Socket.cpp +++ b/Kernel/Net/Socket.cpp @@ -96,8 +96,8 @@ ErrorOr Socket::setsockopt(int level, int option, Userspace u if (user_value_size != IFNAMSIZ) return EINVAL; auto user_string = static_ptr_cast(user_value); - auto ifname = TRY(try_copy_kstring_from_user(user_string, user_value_size)); - auto device = NetworkingManagement::the().lookup_by_name(ifname->view()); + auto ifname = TRY(Process::get_syscall_name_string_fixed_buffer(user_string, user_value_size)); + auto device = NetworkingManagement::the().lookup_by_name(ifname.representable_view()); if (!device) return ENODEV; m_bound_interface.with([&device](auto& bound_device) { diff --git a/Kernel/Syscalls/hostname.cpp b/Kernel/Syscalls/hostname.cpp index 988583ca7a..c1603d4500 100644 --- a/Kernel/Syscalls/hostname.cpp +++ b/Kernel/Syscalls/hostname.cpp @@ -15,9 +15,15 @@ ErrorOr Process::sys$gethostname(Userspace buffer, size_t size) if (size > NumericLimits::max()) return EINVAL; return hostname().with_shared([&](auto const& name) -> ErrorOr { - if (size < (name->length() + 1)) + // NOTE: To be able to copy a null-terminated string, we need at most + // 65 characters to store and copy and not 64 here, to store the whole + // hostname string + null terminator. + FixedStringBuffer current_hostname {}; + current_hostname.store_characters(name.representable_view()); + auto name_view = current_hostname.representable_view(); + if (size < (name_view.length() + 1)) return ENAMETOOLONG; - TRY(copy_to_user(buffer, name->characters(), name->length() + 1)); + TRY(copy_to_user(buffer, name_view.characters_without_null_termination(), name_view.length() + 1)); return 0; }); } @@ -30,11 +36,9 @@ ErrorOr Process::sys$sethostname(Userspace buffer, size_t auto credentials = this->credentials(); if (!credentials->is_superuser()) return EPERM; - if (length > 64) - return ENAMETOOLONG; - auto new_name = TRY(try_copy_kstring_from_user(buffer, length)); + auto new_hostname = TRY(get_syscall_name_string_fixed_buffer(buffer, length)); hostname().with_exclusive([&](auto& name) { - name = move(new_name); + name.store_characters(new_hostname.representable_view()); }); return 0; } diff --git a/Kernel/Syscalls/mount.cpp b/Kernel/Syscalls/mount.cpp index 80fbd32367..58586c9e0c 100644 --- a/Kernel/Syscalls/mount.cpp +++ b/Kernel/Syscalls/mount.cpp @@ -5,6 +5,7 @@ * SPDX-License-Identifier: BSD-2-Clause */ +#include #include #include #include @@ -20,14 +21,15 @@ ErrorOr Process::sys$fsopen(Userspace if (!credentials->is_superuser()) return Error::from_errno(EPERM); auto params = TRY(copy_typed_from_user(user_params)); - auto fs_type_string = TRY(try_copy_kstring_from_user(params.fs_type)); + // NOTE: 16 characters should be enough for any fstype today and in the future. + auto fs_type_string = TRY(get_syscall_name_string_fixed_buffer<16>(params.fs_type)); // NOTE: If some userspace program uses MS_REMOUNT, return EINVAL to indicate that we never want this // flag to appear in the mount table... if (params.flags & MS_REMOUNT || params.flags & MS_BIND) return Error::from_errno(EINVAL); - auto const* fs_type_initializer = TRY(VirtualFileSystem::find_filesystem_type_initializer(fs_type_string->view())); + auto const* fs_type_initializer = TRY(VirtualFileSystem::find_filesystem_type_initializer(fs_type_string.representable_view())); VERIFY(fs_type_initializer); auto mount_file = TRY(MountFile::create(*fs_type_initializer, params.flags)); auto description = TRY(OpenFileDescription::try_create(move(mount_file))); diff --git a/Kernel/Syscalls/pledge.cpp b/Kernel/Syscalls/pledge.cpp index 9610a74715..93b7084ba7 100644 --- a/Kernel/Syscalls/pledge.cpp +++ b/Kernel/Syscalls/pledge.cpp @@ -4,6 +4,7 @@ * SPDX-License-Identifier: BSD-2-Clause */ +#include #include #include @@ -14,17 +15,19 @@ ErrorOr Process::sys$pledge(Userspace VERIFY_NO_PROCESS_BIG_LOCK(this); auto params = TRY(copy_typed_from_user(user_params)); - if (params.promises.length > 1024 || params.execpromises.length > 1024) - return E2BIG; + FixedStringBuffer promises {}; + bool promises_provided { false }; + FixedStringBuffer execpromises {}; + bool execpromises_provided { false }; - OwnPtr promises; if (params.promises.characters) { - promises = TRY(try_copy_kstring_from_user(params.promises)); + promises_provided = true; + promises = TRY(get_syscall_string_fixed_buffer(params.promises)); } - OwnPtr execpromises; if (params.execpromises.characters) { - execpromises = TRY(try_copy_kstring_from_user(params.execpromises)); + execpromises_provided = true; + execpromises = TRY(get_syscall_string_fixed_buffer(params.execpromises)); } auto parse_pledge = [&](auto pledge_spec, u32& mask) { @@ -43,19 +46,19 @@ ErrorOr Process::sys$pledge(Userspace }; u32 new_promises = 0; - if (promises) { - if (!parse_pledge(promises->view(), new_promises)) + if (promises_provided) { + if (!parse_pledge(promises.representable_view(), new_promises)) return EINVAL; } u32 new_execpromises = 0; - if (execpromises) { - if (!parse_pledge(execpromises->view(), new_execpromises)) + if (execpromises_provided) { + if (!parse_pledge(execpromises.representable_view(), new_execpromises)) return EINVAL; } return with_mutable_protected_data([&](auto& protected_data) -> ErrorOr { - if (promises) { + if (promises_provided) { if (protected_data.has_promises && (new_promises & ~protected_data.promises)) { if (!(protected_data.promises & (1u << (u32)Pledge::no_error))) return EPERM; @@ -63,7 +66,7 @@ ErrorOr Process::sys$pledge(Userspace } } - if (execpromises) { + if (execpromises_provided) { if (protected_data.has_execpromises && (new_execpromises & ~protected_data.execpromises)) { if (!(protected_data.promises & (1u << (u32)Pledge::no_error))) return EPERM; @@ -76,12 +79,12 @@ ErrorOr Process::sys$pledge(Userspace // erroring out when parsing the exec promises later. Such bugs silently // leave the caller in an unexpected state. - if (promises) { + if (promises_provided) { protected_data.has_promises = true; protected_data.promises = new_promises; } - if (execpromises) { + if (execpromises_provided) { protected_data.has_execpromises = true; protected_data.execpromises = new_execpromises; } diff --git a/Kernel/Syscalls/uname.cpp b/Kernel/Syscalls/uname.cpp index 9da13d0e6b..7cf1e5087c 100644 --- a/Kernel/Syscalls/uname.cpp +++ b/Kernel/Syscalls/uname.cpp @@ -38,9 +38,10 @@ ErrorOr Process::sys$uname(Userspace user_buf) AK::TypedTransfer::copy(reinterpret_cast(buf.version), SERENITY_VERSION.bytes().data(), min(SERENITY_VERSION.length(), UTSNAME_ENTRY_LEN - 1)); hostname().with_shared([&](auto const& name) { - auto length = min(name->length(), UTSNAME_ENTRY_LEN - 1); - AK::TypedTransfer::copy(reinterpret_cast(buf.nodename), name->characters(), length); - buf.nodename[length] = '\0'; + auto name_length = name.representable_view().length(); + VERIFY(name_length <= (UTSNAME_ENTRY_LEN - 1)); + AK::TypedTransfer::copy(reinterpret_cast(buf.nodename), name.representable_view().characters_without_null_termination(), name_length); + buf.nodename[name_length] = '\0'; }); TRY(copy_to_user(user_buf, &buf)); diff --git a/Kernel/Syscalls/unveil.cpp b/Kernel/Syscalls/unveil.cpp index dc9332b84c..0a36981a79 100644 --- a/Kernel/Syscalls/unveil.cpp +++ b/Kernel/Syscalls/unveil.cpp @@ -101,19 +101,16 @@ ErrorOr Process::sys$unveil(Userspace if (!params.path.characters || !params.permissions.characters) return EINVAL; - if (params.permissions.length > 5) - return EINVAL; - auto path = TRY(get_syscall_path_argument(params.path)); if (path->is_empty() || !path->view().starts_with('/')) return EINVAL; - auto permissions = TRY(try_copy_kstring_from_user(params.permissions)); + auto permissions = TRY(get_syscall_string_fixed_buffer<5>(params.permissions)); // Let's work out permissions first... unsigned new_permissions = 0; - for (char const permission : permissions->view()) { + for (char const permission : permissions.representable_view()) { switch (permission) { case 'r': new_permissions |= UnveilAccess::Read; diff --git a/Kernel/Tasks/Process.cpp b/Kernel/Tasks/Process.cpp index 2c7651c59b..db41cab486 100644 --- a/Kernel/Tasks/Process.cpp +++ b/Kernel/Tasks/Process.cpp @@ -53,9 +53,9 @@ static Atomic next_pid; static Singleton> s_all_instances; READONLY_AFTER_INIT Memory::Region* g_signal_trampoline_region; -static Singleton>> s_hostname; +static Singleton>> s_hostname; -MutexProtected>& hostname() +MutexProtected>& hostname() { return *s_hostname; } @@ -161,7 +161,7 @@ UNMAP_AFTER_INIT void Process::initialize() // Note: This is called before scheduling is initialized, and before APs are booted. // So we can "safely" bypass the lock here. - reinterpret_cast&>(hostname()) = KString::must_create("courage"sv); + reinterpret_cast&>(hostname()).store_characters("courage"sv); create_signal_trampoline(); } diff --git a/Kernel/Tasks/Process.h b/Kernel/Tasks/Process.h index ffb3e42646..c127a470ef 100644 --- a/Kernel/Tasks/Process.h +++ b/Kernel/Tasks/Process.h @@ -41,7 +41,7 @@ namespace Kernel { -MutexProtected>& hostname(); +MutexProtected>& hostname(); UnixDateTime kgettimeofday(); #define ENUMERATE_PLEDGE_PROMISES \ @@ -74,6 +74,15 @@ UnixDateTime kgettimeofday(); __ENUMERATE_PLEDGE_PROMISE(mount) \ __ENUMERATE_PLEDGE_PROMISE(no_error) +#define __ENUMERATE_PLEDGE_PROMISE(x) sizeof(#x) + 1 + +// NOTE: We truncate the last space from the string as it's not needed (with 0 - 1). +constexpr static unsigned all_promises_strings_length_with_spaces = ENUMERATE_PLEDGE_PROMISES 0 - 1; +#undef __ENUMERATE_PLEDGE_PROMISE + +// NOTE: This is a sanity check because length of more than 1024 characters +// is not reasonable. +static_assert(all_promises_strings_length_with_spaces <= 1024); + enum class Pledge : u32 { #define __ENUMERATE_PLEDGE_PROMISE(x) x, ENUMERATE_PLEDGE_PROMISES @@ -605,6 +614,36 @@ public: ErrorOr validate_mmap_prot(int prot, bool map_stack, bool map_anonymous, Memory::Region const* region = nullptr) const; ErrorOr validate_inode_mmap_prot(int prot, bool description_readable, bool description_writable, bool map_shared) const; + template + static ErrorOr> get_syscall_string_fixed_buffer(Syscall::StringArgument const& argument) + { + // NOTE: If the string is too much big for the FixedStringBuffer, + // we return E2BIG error here. + FixedStringBuffer buffer; + TRY(try_copy_string_from_user_into_fixed_string_buffer(reinterpret_cast(argument.characters), buffer, argument.length)); + return buffer; + } + + template + static ErrorOr> get_syscall_name_string_fixed_buffer(Userspace user_buffer, size_t user_length = Size) + { + // NOTE: If the string is too much big for the FixedStringBuffer, + // we return E2BIG error here. + FixedStringBuffer buffer; + TRY(try_copy_string_from_user_into_fixed_string_buffer(user_buffer, buffer, user_length)); + return buffer; + } + + template + static ErrorOr> get_syscall_name_string_fixed_buffer(Syscall::StringArgument const& argument) + { + // NOTE: If the string is too much big for the FixedStringBuffer, + // we return ENAMETOOLONG error here. + FixedStringBuffer buffer; + TRY(try_copy_name_from_user_into_fixed_string_buffer(reinterpret_cast(argument.characters), buffer, argument.length)); + return buffer; + } + private: friend class MemoryManager; friend class Scheduler; diff --git a/Tests/Kernel/TestKernelUnveil.cpp b/Tests/Kernel/TestKernelUnveil.cpp index 04d6d9a737..5d8d79e661 100644 --- a/Tests/Kernel/TestKernelUnveil.cpp +++ b/Tests/Kernel/TestKernelUnveil.cpp @@ -12,6 +12,10 @@ TEST_CASE(test_argument_validation) { auto res = unveil("/etc", "aaaaaaaaaaaa"); EXPECT_EQ(res, -1); + EXPECT_EQ(errno, E2BIG); + + res = unveil("/etc", "aaaaa"); + EXPECT_EQ(res, -1); EXPECT_EQ(errno, EINVAL); res = unveil(nullptr, "r");