Kernel: Make copy_to/from_user safe and remove unnecessary checks

Since the CPU already does almost all necessary validation steps
for us, we don't really need to attempt to do this. Doing it
ourselves doesn't really work very reliably, because we'd have to
account for other processors modifying virtual memory, and we'd
have to account for e.g. pages not being able to be allocated
due to insufficient resources.

So change the copy_to/from_user (and associated helper functions)
to use the new safe_memcpy, which will return whether it succeeded
or not. The only manual validation step needed (which the CPU
can't perform for us) is making sure the pointers provided by user
mode aren't pointing to kernel mappings.

To make it easier to read/write from/to either kernel or user mode
data add the UserOrKernelBuffer helper class, which will internally
either use copy_from/to_user or directly memcpy, or pass the data
through directly using a temporary buffer on the stack.

Last but not least we need to keep syscall params trivial as we
need to copy them from/to user mode using copy_from/to_user.

DevTools/UserspaceEmulator/Emulator.cpp

@@ -930,7 +930,7 @@ int Emulator::virt$execve(FlatPtr params_addr)
  auto copy_string_list = [this](auto& output_vector, auto& string_list) {
  for (size_t i = 0; i < string_list.length; ++i) {
  Syscall::StringArgument string;
- mmu().copy_from_vm(&string, (FlatPtr)&string_list.strings.ptr()[i], sizeof(string));
+ mmu().copy_from_vm(&string, (FlatPtr)&string_list.strings[i], sizeof(string));
  output_vector.append(String::copy(mmu().copy_buffer_from_vm((FlatPtr)string.characters, string.length)));
  }
  };
@@ -1307,7 +1307,7 @@ int Emulator::virt$waitid(FlatPtr params_addr)
  }
 
  if (params.infop)
- mmu().copy_to_vm(params.infop, &info, sizeof(info));
+ mmu().copy_to_vm((FlatPtr)params.infop, &info, sizeof(info));
 
  return rc;
 }

Kernel/API/Syscall.h

@@ -222,7 +222,7 @@ inline constexpr const char* to_string(Function function)
 
 #ifdef __serenity__
 struct StringArgument {
- Userspace<const char*> characters;
+ const char* characters;
  size_t length { 0 };
 };
 
@@ -239,7 +239,7 @@ struct ImmutableBufferArgument {
 };
 
 struct StringListArgument {
- Userspace<StringArgument*> strings {};
+ StringArgument* strings {};
  size_t length { 0 };
 };
 
@@ -273,73 +273,73 @@ struct SC_select_params {
 struct SC_poll_params {
  struct pollfd* fds;
  unsigned nfds;
- Userspace<const struct timespec*> timeout;
- Userspace<const u32*> sigmask;
+ const struct timespec* timeout;
+ const u32* sigmask;
 };
 
 struct SC_clock_nanosleep_params {
  int clock_id;
  int flags;
- Userspace<const struct timespec*> requested_sleep;
- Userspace<struct timespec*> remaining_sleep;
+ const struct timespec* requested_sleep;
+ struct timespec* remaining_sleep;
 };
 
 struct SC_sendto_params {
  int sockfd;
  ImmutableBufferArgument<void, size_t> data;
  int flags;
- Userspace<const sockaddr*> addr;
+ const sockaddr* addr;
  socklen_t addr_length;
 };
 
 struct SC_recvfrom_params {
  int sockfd;
  MutableBufferArgument<void, size_t> buffer;
  int flags;
- Userspace<sockaddr*> addr;
- Userspace<socklen_t*> addr_length;
+ sockaddr* addr;
+ socklen_t* addr_length;
 };
 
 struct SC_getsockopt_params {
  int sockfd;
  int level;
  int option;
- Userspace<void*> value;
- Userspace<socklen_t*> value_size;
+ void* value;
+ socklen_t* value_size;
 };
 
 struct SC_setsockopt_params {
  int sockfd;
  int level;
  int option;
- Userspace<const void*> value;
+ const void* value;
  socklen_t value_size;
 };
 
 struct SC_getsockname_params {
  int sockfd;
- Userspace<sockaddr*> addr;
- Userspace<socklen_t*> addrlen;
+ sockaddr* addr;
+ socklen_t* addrlen;
 };
 
 struct SC_getpeername_params {
  int sockfd;
- Userspace<sockaddr*> addr;
- Userspace<socklen_t*> addrlen;
+ sockaddr* addr;
+ socklen_t* addrlen;
 };
 
 struct SC_futex_params {
- Userspace<const i32*> userspace_address;
+ const i32* userspace_address;
  int futex_op;
  i32 val;
- Userspace<const timespec*> timeout;
+ const timespec* timeout;
 };
 
 struct SC_setkeymap_params {
- Userspace<const u32*> map;
- Userspace<const u32*> shift_map;
- Userspace<const u32*> alt_map;
- Userspace<const u32*> altgr_map;
+ const u32* map;
+ const u32* shift_map;
+ const u32* alt_map;
+ const u32* altgr_map;
  StringArgument map_name;
 };
 
@@ -354,7 +354,7 @@ struct SC_create_thread_params {
  unsigned int m_guard_page_size = 0; // Rounded up to PAGE_SIZE
  unsigned int m_reported_guard_page_size = 0; // The lie we tell callers
  unsigned int m_stack_size = 4 * MiB; // Default PTHREAD_STACK_MIN
- Userspace<void*> m_stack_location; // nullptr means any, o.w. process virtual address
+ void* m_stack_location; // nullptr means any, o.w. process virtual address
 };
 
 struct SC_realpath_params {
@@ -426,26 +426,26 @@ struct SC_unveil_params {
 struct SC_waitid_params {
  int idtype;
  int id;
- Userspace<struct siginfo*> infop;
+ struct siginfo* infop;
  int options;
 };
 
 struct SC_stat_params {
  StringArgument path;
- Userspace<struct stat*> statbuf;
+ struct stat* statbuf;
  bool follow_symlinks;
 };
 
 struct SC_ptrace_params {
  int request;
  pid_t tid;
- Userspace<u8*> addr;
+ u8* addr;
  int data;
 };
 
 struct SC_ptrace_peek_params {
- Userspace<const u32*> address;
- Userspace<u32*> out_data;
+ const u32* address;
+ u32* out_data;
 };
 
 void initialize();

Kernel/Arch/i386/CPU.cpp

@@ -226,14 +226,14 @@ extern "C" u8* safe_memset_2_faulted;
 
 bool safe_memcpy(void* dest_ptr, const void* src_ptr, size_t n, void*& fault_at)
 {
+ fault_at = nullptr;
  size_t dest = (size_t)dest_ptr;
  size_t src = (size_t)src_ptr;
  size_t remainder;
  // FIXME: Support starting at an unaligned address.
  if (!(dest & 0x3) && !(src & 0x3) && n >= 12) {
  size_t size_ts = n / sizeof(size_t);
  asm volatile(
- "xor %[fault_at], %[fault_at] \n"
  ".global safe_memcpy_ins_1 \n"
  "safe_memcpy_ins_1: \n"
  "rep movsl \n"
@@ -256,7 +256,6 @@ bool safe_memcpy(void* dest_ptr, const void* src_ptr, size_t n, void*& fault_at)
  }
  }
  asm volatile(
- "xor %[fault_at], %[fault_at] \n"
  ".global safe_memcpy_ins_2 \n"
  "safe_memcpy_ins_2: \n"
  "rep movsb \n"
@@ -277,8 +276,8 @@ bool safe_memcpy(void* dest_ptr, const void* src_ptr, size_t n, void*& fault_at)
 ssize_t safe_strnlen(const char* str, size_t max_n, void*& fault_at)
 {
  ssize_t count = 0;
+ fault_at = nullptr;
  asm volatile(
- "xor %[fault_at], %[fault_at] \n"
  "1: \n"
  "test %[max_n], %[max_n] \n"
  "je 2f \n"
@@ -307,6 +306,7 @@ ssize_t safe_strnlen(const char* str, size_t max_n, void*& fault_at)
 
 bool safe_memset(void* dest_ptr, int c, size_t n, void*& fault_at)
 {
+ fault_at = nullptr;
  size_t dest = (size_t)dest_ptr;
  size_t remainder;
  // FIXME: Support starting at an unaligned address.
@@ -316,7 +316,6 @@ bool safe_memset(void* dest_ptr, int c, size_t n, void*& fault_at)
  expanded_c |= expanded_c << 8;
  expanded_c |= expanded_c << 16;
  asm volatile(
- "xor %[fault_at], %[fault_at] \n"
  ".global safe_memset_ins_1 \n"
  "safe_memset_ins_1: \n"
  "rep stosl \n"
@@ -338,7 +337,6 @@ bool safe_memset(void* dest_ptr, int c, size_t n, void*& fault_at)
  }
  }
  asm volatile(
- "xor %[fault_at], %[fault_at] \n"
  ".global safe_memset_ins_2 \n"
  "safe_memset_ins_2: \n"
  "rep stosb \n"

Kernel/CMakeLists.txt

@@ -176,6 +176,7 @@ set(KERNEL_SOURCES
  Time/RTC.cpp
  Time/TimeManagement.cpp
  TimerQueue.cpp
+ UserOrKernelBuffer.cpp
  VM/AnonymousVMObject.cpp
  VM/ContiguousVMObject.cpp
  VM/InodeVMObject.cpp

Kernel/Console.cpp

@@ -65,20 +65,26 @@ bool Console::can_read(const Kernel::FileDescription&, size_t) const
  return false;
 }
 
-Kernel::KResultOr<size_t> Console::read(Kernel::FileDescription&, size_t, u8*, size_t)
+Kernel::KResultOr<size_t> Console::read(Kernel::FileDescription&, size_t, Kernel::UserOrKernelBuffer&, size_t)
 {
  // FIXME: Implement reading from the console.
  // Maybe we could use a ring buffer for this device?
  return 0;
 }
 
-Kernel::KResultOr<size_t> Console::write(Kernel::FileDescription&, size_t, const u8* data, size_t size)
+Kernel::KResultOr<size_t> Console::write(Kernel::FileDescription&, size_t, const Kernel::UserOrKernelBuffer& data, size_t size)
 {
  if (!size)
  return 0;
- for (size_t i = 0; i < size; ++i)
- put_char(data[i]);
- return size;
+
+ ssize_t nread = data.read_buffered<256>(size, [&](const u8* bytes, size_t bytes_count) {
+ for (size_t i = 0; i < bytes_count; i++)
+ put_char((char)bytes[i]);
+ return (ssize_t)bytes_count;
+ });
+ if (nread < 0)
+ return Kernel::KResult(nread);
+ return (size_t)nread;
 }
 
 void Console::put_char(char ch)

Kernel/Console.h

@@ -43,8 +43,8 @@ class Console final : public Kernel::CharacterDevice {
  // ^CharacterDevice
  virtual bool can_read(const Kernel::FileDescription&, size_t) const override;
  virtual bool can_write(const Kernel::FileDescription&, size_t) const override { return true; }
- virtual Kernel::KResultOr<size_t> read(Kernel::FileDescription&, size_t, u8*, size_t) override;
- virtual Kernel::KResultOr<size_t> write(Kernel::FileDescription&, size_t, const u8*, size_t) override;
+ virtual Kernel::KResultOr<size_t> read(Kernel::FileDescription&, size_t, Kernel::UserOrKernelBuffer&, size_t) override;
+ virtual Kernel::KResultOr<size_t> write(Kernel::FileDescription&, size_t, const Kernel::UserOrKernelBuffer&, size_t) override;
  virtual const char* class_name() const override { return "Console"; }
 
  void put_char(char);

Kernel/Devices/BXVGADevice.cpp

@@ -203,18 +203,16 @@ int BXVGADevice::ioctl(FileDescription&, unsigned request, FlatPtr arg)
  switch (request) {
  case FB_IOCTL_GET_SIZE_IN_BYTES: {
  auto* out = (size_t*)arg;
- if (!Process::current()->validate_write_typed(out))
- return -EFAULT;
  size_t value = framebuffer_size_in_bytes();
- copy_to_user(out, &value);
+ if (!copy_to_user(out, &value))
+ return -EFAULT;
  return 0;
  }
  case FB_IOCTL_GET_BUFFER: {
  auto* index = (int*)arg;
- if (!Process::current()->validate_write_typed(index))
- return -EFAULT;
  int value = m_y_offset == 0 ? 0 : 1;
- copy_to_user(index, &value);
+ if (!copy_to_user(index, &value))
+ return -EFAULT;
  return 0;
  }
  case FB_IOCTL_SET_BUFFER: {
@@ -225,21 +223,18 @@ int BXVGADevice::ioctl(FileDescription&, unsigned request, FlatPtr arg)
  }
  case FB_IOCTL_GET_RESOLUTION: {
  auto* user_resolution = (FBResolution*)arg;
- if (!Process::current()->validate_write_typed(user_resolution))
- return -EFAULT;
  FBResolution resolution;
  resolution.pitch = m_framebuffer_pitch;
  resolution.width = m_framebuffer_width;
  resolution.height = m_framebuffer_height;
- copy_to_user(user_resolution, &resolution);
+ if (!copy_to_user(user_resolution, &resolution))
+ return -EFAULT;
  return 0;
  }
  case FB_IOCTL_SET_RESOLUTION: {
  auto* user_resolution = (FBResolution*)arg;
- if (!Process::current()->validate_write_typed(user_resolution))
- return -EFAULT;
  FBResolution resolution;
- if (!Process::current()->validate_read_and_copy_typed(&resolution, user_resolution))
+ if (!copy_from_user(&resolution, user_resolution))
  return -EFAULT;
  if (resolution.width > MAX_RESOLUTION_WIDTH || resolution.height > MAX_RESOLUTION_HEIGHT)
  return -EINVAL;
@@ -250,7 +245,8 @@ int BXVGADevice::ioctl(FileDescription&, unsigned request, FlatPtr arg)
  resolution.pitch = m_framebuffer_pitch;
  resolution.width = m_framebuffer_width;
  resolution.height = m_framebuffer_height;
- copy_to_user(user_resolution, &resolution);
+ if (!copy_to_user(user_resolution, &resolution))
+ return -EFAULT;
  return -EINVAL;
  }
 #ifdef BXVGA_DEBUG
@@ -259,7 +255,8 @@ int BXVGADevice::ioctl(FileDescription&, unsigned request, FlatPtr arg)
  resolution.pitch = m_framebuffer_pitch;
  resolution.width = m_framebuffer_width;
  resolution.height = m_framebuffer_height;
- copy_to_user(user_resolution, &resolution);
+ if (!copy_to_user(user_resolution, &resolution))
+ return -EFAULT;
  return 0;
  }
  default:

Kernel/Devices/BXVGADevice.h

@@ -48,10 +48,10 @@ class BXVGADevice final : public BlockDevice {
  virtual const char* class_name() const override { return "BXVGA"; }
  virtual bool can_read(const FileDescription&, size_t) const override { return true; }
  virtual bool can_write(const FileDescription&, size_t) const override { return true; }
- virtual KResultOr<size_t> read(FileDescription&, size_t, u8*, size_t) override { return -EINVAL; }
- virtual KResultOr<size_t> write(FileDescription&, size_t, const u8*, size_t) override { return -EINVAL; }
- virtual bool read_blocks(unsigned, u16, u8*) override { return false; }
- virtual bool write_blocks(unsigned, u16, const u8*) override { return false; }
+ virtual KResultOr<size_t> read(FileDescription&, size_t, UserOrKernelBuffer&, size_t) override { return -EINVAL; }
+ virtual KResultOr<size_t> write(FileDescription&, size_t, const UserOrKernelBuffer&, size_t) override { return -EINVAL; }
+ virtual bool read_blocks(unsigned, u16, UserOrKernelBuffer&) override { return false; }
+ virtual bool write_blocks(unsigned, u16, const UserOrKernelBuffer&) override { return false; }
 
  void set_safe_resolution();
 

Kernel/Devices/BlockDevice.cpp

@@ -32,17 +32,17 @@ BlockDevice::~BlockDevice()
 {
 }
 
-bool BlockDevice::read_block(unsigned index, u8* buffer) const
+bool BlockDevice::read_block(unsigned index, UserOrKernelBuffer& buffer) const
 {
  return const_cast<BlockDevice*>(this)->read_blocks(index, 1, buffer);
 }
 
-bool BlockDevice::write_block(unsigned index, const u8* data)
+bool BlockDevice::write_block(unsigned index, const UserOrKernelBuffer& data)
 {
  return write_blocks(index, 1, data);
 }
 
-bool BlockDevice::read_raw(u32 offset, unsigned length, u8* out) const
+bool BlockDevice::read_raw(u32 offset, unsigned length, UserOrKernelBuffer& out) const
 {
  ASSERT((offset % block_size()) == 0);
  ASSERT((length % block_size()) == 0);
@@ -51,7 +51,7 @@ bool BlockDevice::read_raw(u32 offset, unsigned length, u8* out) const
  return const_cast<BlockDevice*>(this)->read_blocks(first_block, end_block - first_block, out);
 }
 
-bool BlockDevice::write_raw(u32 offset, unsigned length, const u8* in)
+bool BlockDevice::write_raw(u32 offset, unsigned length, const UserOrKernelBuffer& in)
 {
  ASSERT((offset % block_size()) == 0);
  ASSERT((length % block_size()) == 0);