From 0b8226811f0340316e4b0b6c05342598f86e5cd1 Mon Sep 17 00:00:00 2001
From: Andreas Kling <kling@serenityos.org>
Date: Sun, 4 Apr 2021 20:11:54 +0200
Subject: [PATCH] Kernel+CrashReporter: Add metadata about page faults to crash
 reports

Crash reports for page faults now tell you what kind of memory access
failed and where. :^)
---
 Kernel/Arch/i386/CPU.cpp                     | 16 ++++++++++++++--
 Kernel/Process.cpp                           |  5 +++++
 Kernel/Process.h                             |  1 +
 Userland/Applications/CrashReporter/main.cpp |  7 +++++++
 4 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/Kernel/Arch/i386/CPU.cpp b/Kernel/Arch/i386/CPU.cpp
index 5f3ecfc1293..4a3dba9529a 100644
--- a/Kernel/Arch/i386/CPU.cpp
+++ b/Kernel/Arch/i386/CPU.cpp
@@ -284,7 +284,8 @@ void page_fault_handler(TrapFrame* trap)
         PANIC("Attempt to access UNMAP_AFTER_INIT section");
     }
 
-    auto response = MM.handle_page_fault(PageFault(regs.exception_code, VirtualAddress(fault_address)));
+    PageFault fault { regs.exception_code, VirtualAddress { fault_address } };
+    auto response = MM.handle_page_fault(fault);
 
     if (response == PageFaultResponse::ShouldCrash || response == PageFaultResponse::OutOfMemory) {
         if (faulted_in_kernel && handle_safe_access_fault(regs, fault_address)) {
@@ -328,6 +329,18 @@ void page_fault_handler(TrapFrame* trap)
             dbgln("Note: Address {} looks like a possible nullptr dereference", VirtualAddress(fault_address));
         }
 
+        auto& current_process = current_thread->process();
+        if (current_process.is_user_process()) {
+            current_process.set_coredump_metadata("fault_address", String::formatted("{:p}", fault_address));
+            current_process.set_coredump_metadata("fault_type", fault.type() == PageFault::Type::PageNotPresent ? "NotPresent" : "ProtectionViolation");
+            String fault_access;
+            if (fault.is_instruction_fetch())
+                fault_access = "Execute";
+            else
+                fault_access = fault.access() == PageFault::Access::Read ? "Read" : "Write";
+            current_process.set_coredump_metadata("fault_access", fault_access);
+        }
+
         handle_crash(regs, "Page Fault", SIGSEGV, response == PageFaultResponse::OutOfMemory);
     } else if (response == PageFaultResponse::Continue) {
 #if PAGE_FAULT_DEBUG
@@ -2397,7 +2410,6 @@ void copy_ptrace_registers_into_kernel_registers(RegisterState& kernel_regs, con
     kernel_regs.eip = ptrace_regs.eip;
     kernel_regs.eflags = (kernel_regs.eflags & ~safe_eflags_mask) | (ptrace_regs.eflags & safe_eflags_mask);
 }
-
 }
 
 #ifdef DEBUG
diff --git a/Kernel/Process.cpp b/Kernel/Process.cpp
index 7899f5dacb8..fb18f737961 100644
--- a/Kernel/Process.cpp
+++ b/Kernel/Process.cpp
@@ -727,4 +727,9 @@ void Process::set_dumpable(bool dumpable)
     m_dumpable = dumpable;
 }
 
+void Process::set_coredump_metadata(const String& key, String value)
+{
+    m_coredump_metadata.set(key, move(value));
+}
+
 }
diff --git a/Kernel/Process.h b/Kernel/Process.h
index b1e864817dc..7848baa3121 100644
--- a/Kernel/Process.h
+++ b/Kernel/Process.h
@@ -494,6 +494,7 @@ public:
 
     HashMap<String, String>& coredump_metadata() { return m_coredump_metadata; }
     const HashMap<String, String>& coredump_metadata() const { return m_coredump_metadata; }
+    void set_coredump_metadata(const String& key, String value);
 
     const NonnullRefPtrVector<Thread>& threads_for_coredump(Badge<CoreDump>) const { return m_threads_for_coredump; }
 
diff --git a/Userland/Applications/CrashReporter/main.cpp b/Userland/Applications/CrashReporter/main.cpp
index f5e1c899d58..aa71be18454 100644
--- a/Userland/Applications/CrashReporter/main.cpp
+++ b/Userland/Applications/CrashReporter/main.cpp
@@ -79,6 +79,13 @@ static TitleAndText build_backtrace(const CoreDump::Reader& coredump, const ELF:
     else if (metadata.contains("pledge_violation"))
         prepend_metadata("pledge_violation", "Has not pledged {}");
 
+    auto fault_address = metadata.get("fault_address");
+    auto fault_type = metadata.get("fault_type");
+    auto fault_access = metadata.get("fault_access");
+    if (fault_address.has_value() && fault_type.has_value() && fault_access.has_value()) {
+        builder.appendff("{} fault on {} at address {}\n\n", fault_type.value(), fault_access.value(), fault_address.value());
+    }
+
     auto first_entry = true;
     for (auto& entry : backtrace.entries()) {
         if (first_entry)