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");