From 301c1a3a58c354828533b60bb821deb3e945ffd7 Mon Sep 17 00:00:00 2001 From: Idan Horowitz Date: Sun, 4 Jul 2021 20:23:26 +0300 Subject: [PATCH] Everywhere: Fix incorrect usages of AK::Checked Specifically, explicitly specify the checked type, use the resulting value instead of doing the same calculation twice, and break down calculations to discrete operations to ensure no intermediary overflows are missed. --- Kernel/StdLib.h | 16 ++++++++-------- Kernel/Syscalls/execve.cpp | 4 ++-- Kernel/Syscalls/select.cpp | 2 +- Userland/Libraries/LibJS/Runtime/TypedArray.cpp | 3 ++- .../LibJS/Runtime/TypedArrayPrototype.cpp | 4 ++-- .../AbstractMachine/BytecodeInterpreter.cpp | 2 +- 6 files changed, 16 insertions(+), 15 deletions(-) diff --git a/Kernel/StdLib.h b/Kernel/StdLib.h index 0a7a6ec00e..08ed115b89 100644 --- a/Kernel/StdLib.h +++ b/Kernel/StdLib.h @@ -131,42 +131,42 @@ template [[nodiscard]] inline bool copy_n_from_user(T* dest, const T* src, size_t count) { static_assert(IsTriviallyCopyable); - Checked size = sizeof(T); + Checked size = sizeof(T); size *= count; if (size.has_overflow()) return false; - return copy_from_user(dest, src, sizeof(T) * count); + return copy_from_user(dest, src, size.value()); } template [[nodiscard]] inline bool copy_n_to_user(T* dest, const T* src, size_t count) { static_assert(IsTriviallyCopyable); - Checked size = sizeof(T); + Checked size = sizeof(T); size *= count; if (size.has_overflow()) return false; - return copy_to_user(dest, src, sizeof(T) * count); + return copy_to_user(dest, src, size.value()); } template [[nodiscard]] inline bool copy_n_from_user(T* dest, Userspace src, size_t count) { static_assert(IsTriviallyCopyable); - Checked size = sizeof(T); + Checked size = sizeof(T); size *= count; if (size.has_overflow()) return false; - return copy_from_user(dest, src.unsafe_userspace_ptr(), sizeof(T) * count); + return copy_from_user(dest, src.unsafe_userspace_ptr(), size.value()); } template [[nodiscard]] inline bool copy_n_to_user(Userspace dest, const T* src, size_t count) { static_assert(IsTriviallyCopyable); - Checked size = sizeof(T); + Checked size = sizeof(T); size *= count; if (size.has_overflow()) return false; - return copy_to_user(dest.unsafe_userspace_ptr(), src, sizeof(T) * count); + return copy_to_user(dest.unsafe_userspace_ptr(), src, size.value()); } diff --git a/Kernel/Syscalls/execve.cpp b/Kernel/Syscalls/execve.cpp index adf31e7a7c..663fd956e2 100644 --- a/Kernel/Syscalls/execve.cpp +++ b/Kernel/Syscalls/execve.cpp @@ -942,14 +942,14 @@ KResultOr Process::sys$execve(Userspace size = sizeof(*list.strings); size *= list.length; if (size.has_overflow()) return false; Vector strings; if (!strings.try_resize(list.length)) return false; - if (!copy_from_user(strings.data(), list.strings, list.length * sizeof(*list.strings))) + if (!copy_from_user(strings.data(), list.strings, size.value())) return false; for (size_t i = 0; i < list.length; ++i) { auto string = copy_string_from_user(strings[i]); diff --git a/Kernel/Syscalls/select.cpp b/Kernel/Syscalls/select.cpp index 04758cf94f..ebbb18dcff 100644 --- a/Kernel/Syscalls/select.cpp +++ b/Kernel/Syscalls/select.cpp @@ -152,7 +152,7 @@ KResultOr Process::sys$poll(Userspace u Vector fds_copy; if (params.nfds > 0) { - Checked nfds_checked = sizeof(pollfd); + Checked nfds_checked = sizeof(pollfd); nfds_checked *= params.nfds; if (nfds_checked.has_overflow()) return EFAULT; diff --git a/Userland/Libraries/LibJS/Runtime/TypedArray.cpp b/Userland/Libraries/LibJS/Runtime/TypedArray.cpp index a757bfd8eb..61154f7362 100644 --- a/Userland/Libraries/LibJS/Runtime/TypedArray.cpp +++ b/Userland/Libraries/LibJS/Runtime/TypedArray.cpp @@ -97,7 +97,8 @@ static void initialize_typed_array_from_typed_array(GlobalObject& global_object, auto element_length = src_array.array_length(); auto element_size = dest_array.element_size(); - Checked byte_length = element_size * element_length; + Checked byte_length = element_size; + byte_length *= element_length; if (byte_length.has_overflow()) { vm.throw_exception(global_object, ErrorType::InvalidLength, "typed array"); return; diff --git a/Userland/Libraries/LibJS/Runtime/TypedArrayPrototype.cpp b/Userland/Libraries/LibJS/Runtime/TypedArrayPrototype.cpp index e7b3e5bab1..ef719d62f8 100644 --- a/Userland/Libraries/LibJS/Runtime/TypedArrayPrototype.cpp +++ b/Userland/Libraries/LibJS/Runtime/TypedArrayPrototype.cpp @@ -362,7 +362,7 @@ JS_DEFINE_NATIVE_FUNCTION(TypedArrayPrototype::set) return {}; } - Checked checked { source_length }; + Checked checked = source_length; checked += static_cast(target_offset); if (checked.has_overflow() || checked.value() > target_length) { vm.throw_exception(global_object, "Overflow or out of bounds in target length"); @@ -436,7 +436,7 @@ JS_DEFINE_NATIVE_FUNCTION(TypedArrayPrototype::set) return {}; } - Checked checked { source_length }; + Checked checked = source_length; checked += static_cast(target_offset); if (checked.has_overflow() || checked.value() > target_length) { vm.throw_exception(global_object, "Overflow or out of bounds in target length"); diff --git a/Userland/Libraries/LibWasm/AbstractMachine/BytecodeInterpreter.cpp b/Userland/Libraries/LibWasm/AbstractMachine/BytecodeInterpreter.cpp index 7244b591c5..868e1a7113 100644 --- a/Userland/Libraries/LibWasm/AbstractMachine/BytecodeInterpreter.cpp +++ b/Userland/Libraries/LibWasm/AbstractMachine/BytecodeInterpreter.cpp @@ -191,7 +191,7 @@ void BytecodeInterpreter::call_address(Configuration& configuration, FunctionAdd TRAP_IF_NOT(rhs.has_value()); \ dbgln_if(WASM_TRACE_DEBUG, "{} {} {} = ??", ulhs.value(), #operator, rhs.value()); \ __VA_ARGS__; \ - Checked lhs = ulhs.value(); \ + Checked lhs = ulhs.value(); \ lhs operator##= rhs.value(); \ TRAP_IF_NOT(!lhs.has_overflow()); \ auto result = lhs.value(); \