Kernel: Wrap process address spaces in SpinlockProtected

This forces anyone who wants to look into and/or manipulate an address space to lock it. And this replaces the previous, more flimsy, manual spinlock use. Note that pointers *into* the address space are not safe to use after you unlock the space. We've got many issues like this, and we'll have to track those down as wlel.
cubiclove · Aug 24, 2022 · cf16b2c · cf16b2c
1 parent d6ef18f
commit cf16b2c
Show file tree

Hide file tree

Showing 38 changed files with 712 additions and 631 deletions.
diff --git a/Kernel/Arch/x86/common/CrashHandler.cpp b/Kernel/Arch/x86/common/CrashHandler.cpp
@@ -35,7 +35,7 @@ void handle_crash(Kernel::RegisterState const& regs, char const* description, in
  dump_registers(regs);
 
  if (crashed_in_kernel) {
- process.address_space().dump_regions();
+ process.address_space().with([&](auto& space) { space->dump_regions(); });
  PANIC("Crash in ring 0");
  }
 

diff --git a/Kernel/Arch/x86/common/Interrupts.cpp b/Kernel/Arch/x86/common/Interrupts.cpp
@@ -303,9 +303,15 @@ void page_fault_handler(TrapFrame* trap)
  };
 
  VirtualAddress userspace_sp = VirtualAddress { regs.userspace_sp() };
- if (!faulted_in_kernel && !MM.validate_user_stack(current_thread->process().address_space(), userspace_sp)) {
- dbgln("Invalid stack pointer: {}", userspace_sp);
- return handle_crash(regs, "Bad stack on page fault", SIGSEGV);
+
+ if (!faulted_in_kernel) {
+ bool has_valid_stack_pointer = current_thread->process().address_space().with([&](auto& space) {
+ return MM.validate_user_stack(*space, userspace_sp);
+ });
+ if (!has_valid_stack_pointer) {
+ dbgln("Invalid stack pointer: {}", userspace_sp);
+ return handle_crash(regs, "Bad stack on page fault", SIGSEGV);
+ }
  }
 
  PageFault fault { regs.exception_code, VirtualAddress { fault_address } };

diff --git a/Kernel/Arch/x86/common/Spinlock.cpp b/Kernel/Arch/x86/common/Spinlock.cpp
@@ -55,6 +55,7 @@ u32 RecursiveSpinlock::lock()
 
 void RecursiveSpinlock::unlock(u32 prev_flags)
 {
+ VERIFY_INTERRUPTS_DISABLED();
  VERIFY(m_recursions > 0);
  VERIFY(m_lock.load(AK::memory_order_relaxed) == FlatPtr(&Processor::current()));
  if (--m_recursions == 0) {

diff --git a/Kernel/Coredump.cpp b/Kernel/Coredump.cpp
@@ -46,17 +46,19 @@ Coredump::Coredump(NonnullLockRefPtr<Process> process, NonnullLockRefPtr<OpenFil
  , m_description(move(description))
 {
  m_num_program_headers = 0;
- m_process->address_space().region_tree().with([&](auto& region_tree) {
- for (auto& region : region_tree.regions()) {
+ m_process->address_space().with([&](auto& space) {
+ space->region_tree().with([&](auto& region_tree) {
+ for (auto& region : region_tree.regions()) {
 #if !INCLUDE_USERSPACE_HEAP_MEMORY_IN_COREDUMPS
- if (looks_like_userspace_heap_region(region))
- continue;
+  if (looks_like_userspace_heap_region(region))
+  continue;
 #endif
 
- if (region.access() == Memory::Region::Access::None)
- continue;
- ++m_num_program_headers;
- }
+ if (region.access() == Memory::Region::Access::None)
+ continue;
+ ++m_num_program_headers;
+ }
+ });
  });
  ++m_num_program_headers; // +1 for NOTE segment
 }
@@ -135,38 +137,40 @@ ErrorOr<void> Coredump::write_elf_header()
 ErrorOr<void> Coredump::write_program_headers(size_t notes_size)
 {
  size_t offset = sizeof(ElfW(Ehdr)) + m_num_program_headers * sizeof(ElfW(Phdr));
- m_process->address_space().region_tree().with([&](auto& region_tree) {
- for (auto& region : region_tree.regions()) {
+ m_process->address_space().with([&](auto& space) {
+ space->region_tree().with([&](auto& region_tree) {
+ for (auto& region : region_tree.regions()) {
 
 #if !INCLUDE_USERSPACE_HEAP_MEMORY_IN_COREDUMPS
- if (looks_like_userspace_heap_region(region))
- continue;
+  if (looks_like_userspace_heap_region(region))
+  continue;
 #endif
 
- if (region.access() == Memory::Region::Access::None)
- continue;
+  if (region.access() == Memory::Region::Access::None)
+  continue;
 
- ElfW(Phdr) phdr {};
+  ElfW(Phdr) phdr {};
 
- phdr.p_type = PT_LOAD;
- phdr.p_offset = offset;
- phdr.p_vaddr = region.vaddr().get();
- phdr.p_paddr = 0;
+  phdr.p_type = PT_LOAD;
+  phdr.p_offset = offset;
+  phdr.p_vaddr = region.vaddr().get();
+  phdr.p_paddr = 0;
 
- phdr.p_filesz = region.page_count() * PAGE_SIZE;
- phdr.p_memsz = region.page_count() * PAGE_SIZE;
- phdr.p_align = 0;
+  phdr.p_filesz = region.page_count() * PAGE_SIZE;
+  phdr.p_memsz = region.page_count() * PAGE_SIZE;
+  phdr.p_align = 0;
 
- phdr.p_flags = region.is_readable() ? PF_R : 0;
- if (region.is_writable())
- phdr.p_flags |= PF_W;
- if (region.is_executable())
- phdr.p_flags |= PF_X;
+  phdr.p_flags = region.is_readable() ? PF_R : 0;
+  if (region.is_writable())
+  phdr.p_flags |= PF_W;
+  if (region.is_executable())
+  phdr.p_flags |= PF_X;
 
- offset += phdr.p_filesz;
+  offset += phdr.p_filesz;
 
- [[maybe_unused]] auto rc = m_description->write(UserOrKernelBuffer::for_kernel_buffer(reinterpret_cast<uint8_t*>(&phdr)), sizeof(ElfW(Phdr)));
- }
+ [[maybe_unused]] auto rc = m_description->write(UserOrKernelBuffer::for_kernel_buffer(reinterpret_cast<uint8_t*>(&phdr)), sizeof(ElfW(Phdr)));
+ }
+ });
  });
 
  ElfW(Phdr) notes_pheader {};
@@ -188,37 +192,39 @@ ErrorOr<void> Coredump::write_regions()
 {
  u8 zero_buffer[PAGE_SIZE] = {};
 
- return m_process->address_space().region_tree().with([&](auto& region_tree) -> ErrorOr<void> {
- for (auto& region : region_tree.regions()) {
- VERIFY(!region.is_kernel());
+ return m_process->address_space().with([&](auto& space) {
+ return space->region_tree().with([&](auto& region_tree) -> ErrorOr<void> {
+ for (auto& region : region_tree.regions()) {
+ VERIFY(!region.is_kernel());
 
 #if !INCLUDE_USERSPACE_HEAP_MEMORY_IN_COREDUMPS
- if (looks_like_userspace_heap_region(region))
- continue;
+  if (looks_like_userspace_heap_region(region))
+  continue;
 #endif
 
- if (region.access() == Memory::Region::Access::None)
- continue;
-
- // If we crashed in the middle of mapping in Regions, they do not have a page directory yet, and will crash on a remap() call
- if (!region.is_mapped())
- continue;
-
- region.set_readable(true);
- region.remap();
-
- for (size_t i = 0; i < region.page_count(); i++) {
- auto page = region.physical_page(i);
- auto src_buffer = [&]() -> ErrorOr<UserOrKernelBuffer> {
- if (page)
- return UserOrKernelBuffer::for_user_buffer(reinterpret_cast<uint8_t*>((region.vaddr().as_ptr() + (i * PAGE_SIZE))), PAGE_SIZE);
- // If the current page is not backed by a physical page, we zero it in the coredump file.
- return UserOrKernelBuffer::for_kernel_buffer(zero_buffer);
- }();
- TRY(m_description->write(src_buffer.value(), PAGE_SIZE));
+ if (region.access() == Memory::Region::Access::None)
+ continue;
+
+ // If we crashed in the middle of mapping in Regions, they do not have a page directory yet, and will crash on a remap() call
+ if (!region.is_mapped())
+ continue;
+
+ region.set_readable(true);
+ region.remap();
+
+ for (size_t i = 0; i < region.page_count(); i++) {
+ auto page = region.physical_page(i);
+ auto src_buffer = [&]() -> ErrorOr<UserOrKernelBuffer> {
+ if (page)
+ return UserOrKernelBuffer::for_user_buffer(reinterpret_cast<uint8_t*>((region.vaddr().as_ptr() + (i * PAGE_SIZE))), PAGE_SIZE);
+ // If the current page is not backed by a physical page, we zero it in the coredump file.
+ return UserOrKernelBuffer::for_kernel_buffer(zero_buffer);
+ }();
+ TRY(m_description->write(src_buffer.value(), PAGE_SIZE));
+ }
  }
- }
- return {};
+  return {};
+ });
  });
 }
 
@@ -279,34 +285,36 @@ ErrorOr<void> Coredump::create_notes_threads_data(auto& builder) const
 ErrorOr<void> Coredump::create_notes_regions_data(auto& builder) const
 {
  size_t region_index = 0;
- return m_process->address_space().region_tree().with([&](auto& region_tree) -> ErrorOr<void> {
- for (auto const& region : region_tree.regions()) {
+ return m_process->address_space().with([&](auto& space) {
+ return space->region_tree().with([&](auto& region_tree) -> ErrorOr<void> {
+ for (auto const& region : region_tree.regions()) {
 
 #if !INCLUDE_USERSPACE_HEAP_MEMORY_IN_COREDUMPS
- if (looks_like_userspace_heap_region(region))
- continue;
+  if (looks_like_userspace_heap_region(region))
+  continue;
 #endif
 
- if (region.access() == Memory::Region::Access::None)
- continue;
+  if (region.access() == Memory::Region::Access::None)
+  continue;
 
- ELF::Core::MemoryRegionInfo info {};
- info.header.type = ELF::Core::NotesEntryHeader::Type::MemoryRegionInfo;
+  ELF::Core::MemoryRegionInfo info {};
+  info.header.type = ELF::Core::NotesEntryHeader::Type::MemoryRegionInfo;
 
- info.region_start = region.vaddr().get();
- info.region_end = region.vaddr().offset(region.size()).get();
- info.program_header_index = region_index++;
+  info.region_start = region.vaddr().get();
+  info.region_end = region.vaddr().offset(region.size()).get();
+  info.program_header_index = region_index++;
 
- TRY(builder.append_bytes(ReadonlyBytes { (void*)&info, sizeof(info) }));
+  TRY(builder.append_bytes(ReadonlyBytes { (void*)&info, sizeof(info) }));
 
- // NOTE: The region name *is* null-terminated, so the following is ok:
- auto name = region.name();
- if (name.is_empty())
- TRY(builder.append('\0'));
- else
- TRY(builder.append(name.characters_without_null_termination(), name.length() + 1));
- }
- return {};
+ // NOTE: The region name *is* null-terminated, so the following is ok:
+ auto name = region.name();
+ if (name.is_empty())
+ TRY(builder.append('\0'));
+ else
+ TRY(builder.append(name.characters_without_null_termination(), name.length() + 1));
+ }
+ return {};
+ });
  });
 }
 
@@ -344,7 +352,6 @@ ErrorOr<void> Coredump::create_notes_segment_data(auto& builder) const
 
 ErrorOr<void> Coredump::write()
 {
- SpinlockLocker lock(m_process->address_space().get_lock());
  ScopedAddressSpaceSwitcher switcher(m_process);
 
  auto builder = TRY(KBufferBuilder::try_create());

diff --git a/Kernel/Devices/KCOVDevice.cpp b/Kernel/Devices/KCOVDevice.cpp
@@ -116,7 +116,7 @@ ErrorOr<void> KCOVDevice::ioctl(OpenFileDescription&, unsigned request, Userspac
  }
 }
 
-ErrorOr<Memory::Region*> KCOVDevice::mmap(Process& process, OpenFileDescription&, Memory::VirtualRange const& range, u64 offset, int prot, bool shared)
+ErrorOr<Memory::Region*> KCOVDevice::mmap(Process& process, Memory::AddressSpace& address_space, OpenFileDescription&, Memory::VirtualRange const& range, u64 offset, int prot, bool shared)
 {
  auto pid = process.pid();
  auto maybe_kcov_instance = proc_instance->get(pid);
@@ -126,8 +126,7 @@ ErrorOr<Memory::Region*> KCOVDevice::mmap(Process& process, OpenFileDescription&
  if (!kcov_instance->vmobject())
  return ENOBUFS; // mmaped, before KCOV_SETBUFSIZE
 
- return process.address_space().allocate_region_with_vmobject(
- range, *kcov_instance->vmobject(), offset, {}, prot, shared);
+ return address_space.allocate_region_with_vmobject(range, *kcov_instance->vmobject(), offset, {}, prot, shared);
 }
 
 }
diff --git a/Kernel/Devices/KCOVDevice.h b/Kernel/Devices/KCOVDevice.h
@@ -22,7 +22,7 @@ class KCOVDevice final : public CharacterDevice {
  static void free_process();
 
  // ^File
- ErrorOr<Memory::Region*> mmap(Process&, OpenFileDescription&, Memory::VirtualRange const&, u64 offset, int prot, bool shared) override;
+ ErrorOr<Memory::Region*> mmap(Process&, Memory::AddressSpace&, OpenFileDescription&, Memory::VirtualRange const&, u64 offset, int prot, bool shared) override;
  ErrorOr<NonnullLockRefPtr<OpenFileDescription>> open(int options) override;
 
 protected:

diff --git a/Kernel/Devices/MemoryDevice.cpp b/Kernel/Devices/MemoryDevice.cpp
@@ -42,7 +42,7 @@ ErrorOr<size_t> MemoryDevice::read(OpenFileDescription&, u64 offset, UserOrKerne
  return length;
 }
 
-ErrorOr<Memory::Region*> MemoryDevice::mmap(Process& process, OpenFileDescription&, Memory::VirtualRange const& range, u64 offset, int prot, bool shared)
+ErrorOr<Memory::Region*> MemoryDevice::mmap(Process&, Memory::AddressSpace& address_space, OpenFileDescription&, Memory::VirtualRange const& range, u64 offset, int prot, bool shared)
 {
  auto viewed_address = PhysicalAddress(offset);
 
@@ -63,7 +63,7 @@ ErrorOr<Memory::Region*> MemoryDevice::mmap(Process& process, OpenFileDescriptio
 
  auto vmobject = TRY(Memory::AnonymousVMObject::try_create_for_physical_range(viewed_address, range.size()));
 
- return process.address_space().allocate_region_with_vmobject(
+ return address_space.allocate_region_with_vmobject(
  range,
  move(vmobject),
  0,

diff --git a/Kernel/Devices/MemoryDevice.h b/Kernel/Devices/MemoryDevice.h
@@ -19,7 +19,7 @@ class MemoryDevice final : public CharacterDevice {
  static NonnullLockRefPtr<MemoryDevice> must_create();
  ~MemoryDevice();
 
- virtual ErrorOr<Memory::Region*> mmap(Process&, OpenFileDescription&, Memory::VirtualRange const&, u64 offset, int prot, bool shared) override;
+ virtual ErrorOr<Memory::Region*> mmap(Process&, Memory::AddressSpace&, OpenFileDescription&, Memory::VirtualRange const&, u64 offset, int prot, bool shared) override;
 
 private:
  MemoryDevice();

diff --git a/Kernel/FileSystem/AnonymousFile.cpp b/Kernel/FileSystem/AnonymousFile.cpp
@@ -17,12 +17,12 @@ AnonymousFile::AnonymousFile(NonnullLockRefPtr<Memory::AnonymousVMObject> vmobje
 
 AnonymousFile::~AnonymousFile() = default;
 
-ErrorOr<Memory::Region*> AnonymousFile::mmap(Process& process, OpenFileDescription&, Memory::VirtualRange const& range, u64 offset, int prot, bool shared)
+ErrorOr<Memory::Region*> AnonymousFile::mmap(Process&, Memory::AddressSpace& address_space, OpenFileDescription&, Memory::VirtualRange const& range, u64 offset, int prot, bool shared)
 {
  if (offset != 0)
  return EINVAL;
 
- return process.address_space().allocate_region_with_vmobject(range, m_vmobject, offset, {}, prot, shared);
+ return address_space.allocate_region_with_vmobject(range, m_vmobject, offset, {}, prot, shared);
 }
 
 ErrorOr<NonnullOwnPtr<KString>> AnonymousFile::pseudo_path(OpenFileDescription const&) const

diff --git a/Kernel/FileSystem/AnonymousFile.h b/Kernel/FileSystem/AnonymousFile.h
@@ -20,7 +20,7 @@ class AnonymousFile final : public File {
 
  virtual ~AnonymousFile() override;
 
- virtual ErrorOr<Memory::Region*> mmap(Process&, OpenFileDescription&, Memory::VirtualRange const&, u64 offset, int prot, bool shared) override;
+ virtual ErrorOr<Memory::Region*> mmap(Process&, Memory::AddressSpace&, OpenFileDescription&, Memory::VirtualRange const&, u64 offset, int prot, bool shared) override;
 
 private:
  virtual StringView class_name() const override { return "AnonymousFile"sv; }

diff --git a/Kernel/FileSystem/File.cpp b/Kernel/FileSystem/File.cpp
@@ -35,7 +35,7 @@ ErrorOr<void> File::ioctl(OpenFileDescription&, unsigned, Userspace<void*>)
  return ENOTTY;
 }
 
-ErrorOr<Memory::Region*> File::mmap(Process&, OpenFileDescription&, Memory::VirtualRange const&, u64, int, bool)
+ErrorOr<Memory::Region*> File::mmap(Process&, Memory::AddressSpace&, OpenFileDescription&, Memory::VirtualRange const&, u64, int, bool)
 {
  return ENODEV;
 }

diff --git a/Kernel/FileSystem/File.h b/Kernel/FileSystem/File.h
@@ -90,7 +90,7 @@ class File
  virtual ErrorOr<size_t> read(OpenFileDescription&, u64, UserOrKernelBuffer&, size_t) = 0;
  virtual ErrorOr<size_t> write(OpenFileDescription&, u64, UserOrKernelBuffer const&, size_t) = 0;
  virtual ErrorOr<void> ioctl(OpenFileDescription&, unsigned request, Userspace<void*> arg);
- virtual ErrorOr<Memory::Region*> mmap(Process&, OpenFileDescription&, Memory::VirtualRange const&, u64 offset, int prot, bool shared);
+ virtual ErrorOr<Memory::Region*> mmap(Process&, Memory::AddressSpace&, OpenFileDescription&, Memory::VirtualRange const&, u64 offset, int prot, bool shared);
  virtual ErrorOr<struct stat> stat() const { return EBADF; }
 
  // Although this might be better described "name" or "description", these terms already have other meanings.

diff --git a/Kernel/FileSystem/InodeFile.cpp b/Kernel/FileSystem/InodeFile.cpp
@@ -85,7 +85,7 @@ ErrorOr<void> InodeFile::ioctl(OpenFileDescription& description, unsigned reques
  }
 }
 
-ErrorOr<Memory::Region*> InodeFile::mmap(Process& process, OpenFileDescription& description, Memory::VirtualRange const& range, u64 offset, int prot, bool shared)
+ErrorOr<Memory::Region*> InodeFile::mmap(Process&, Memory::AddressSpace& address_space, OpenFileDescription& description, Memory::VirtualRange const& range, u64 offset, int prot, bool shared)
 {
  // FIXME: If PROT_EXEC, check that the underlying file system isn't mounted noexec.
  LockRefPtr<Memory::InodeVMObject> vmobject;
@@ -94,7 +94,7 @@ ErrorOr<Memory::Region*> InodeFile::mmap(Process& process, OpenFileDescription&
  else
  vmobject = TRY(Memory::PrivateInodeVMObject::try_create_with_inode(inode()));
  auto path = TRY(description.pseudo_path());
- return process.address_space().allocate_region_with_vmobject(range, vmobject.release_nonnull(), offset, path->view(), prot, shared);
+ return address_space.allocate_region_with_vmobject(range, vmobject.release_nonnull(), offset, path->view(), prot, shared);
 }
 
 ErrorOr<NonnullOwnPtr<KString>> InodeFile::pseudo_path(OpenFileDescription const&) const

diff --git a/Kernel/FileSystem/InodeFile.h b/Kernel/FileSystem/InodeFile.h
@@ -33,7 +33,7 @@ class InodeFile final : public File {
  virtual ErrorOr<size_t> read(OpenFileDescription&, u64, UserOrKernelBuffer&, size_t) override;
  virtual ErrorOr<size_t> write(OpenFileDescription&, u64, UserOrKernelBuffer const&, size_t) override;
  virtual ErrorOr<void> ioctl(OpenFileDescription&, unsigned request, Userspace<void*> arg) override;
- virtual ErrorOr<Memory::Region*> mmap(Process&, OpenFileDescription&, Memory::VirtualRange const&, u64 offset, int prot, bool shared) override;
+ virtual ErrorOr<Memory::Region*> mmap(Process&, Memory::AddressSpace&, OpenFileDescription&, Memory::VirtualRange const&, u64 offset, int prot, bool shared) override;
  virtual ErrorOr<struct stat> stat() const override { return inode().metadata().stat(); }
 
  virtual ErrorOr<NonnullOwnPtr<KString>> pseudo_path(OpenFileDescription const&) const override;

diff --git a/Kernel/FileSystem/OpenFileDescription.cpp b/Kernel/FileSystem/OpenFileDescription.cpp
@@ -374,9 +374,9 @@ InodeMetadata OpenFileDescription::metadata() const
  return {};
 }
 
-ErrorOr<Memory::Region*> OpenFileDescription::mmap(Process& process, Memory::VirtualRange const& range, u64 offset, int prot, bool shared)
+ErrorOr<Memory::Region*> OpenFileDescription::mmap(Process& process, Memory::AddressSpace& address_space, Memory::VirtualRange const& range, u64 offset, int prot, bool shared)
 {
- return m_file->mmap(process, *this, range, offset, prot, shared);
+ return m_file->mmap(process, address_space, *this, range, offset, prot, shared);
 }
 
 ErrorOr<void> OpenFileDescription::truncate(u64 length)