[ros-diffs] [reactos] 02/02: [NTOS:KE/MM] Some bug-fixes in the bugcheck code.

[Hermès Bélusca-Maïto]

https://git.reactos.org/?p=reactos.git;a=commitdiff;h=f694d12f0c499819cba85…

commit f694d12f0c499819cba854c51017f9c9242a03bd
Author:     Hermès Bélusca-Maïto <hermes.belusca-maito(a)reactos.org>
AuthorDate: Tue Jun 25 20:45:23 2019 +0200
Commit:     Hermès Bélusca-Maïto <hermes.belusca-maito(a)reactos.org>
CommitDate: Tue Jun 25 21:01:49 2019 +0200

    [NTOS:KE/MM] Some bug-fixes in the bugcheck code.
    
    - Introduce the MmMakeKernelResourceSectionWritable() helper for
      making the kernel resource memory section writable, and use it
      in KeGetBugMessageText(). Indeed, this latter function patches
      in place the bugcheck resource message to trim any trailing
      newlines before displaying the message on screen.
    
      See also
https://github.com/osresearch/uxen/blob/83bad53/dm/introspection-win7.c#L286
      that mentions it too.
    
      This fixes bugcheck text display (e.g. the MANUALLY_INITIATED_CRASH one)
      when using (at least) MSVC-built ReactOS, avoiding a Page-Fault
      exception during the bugcheck.
    
    - Cover KeGetBugMessageText() in SEH since we are accessing kernel
      resources that could also be corrupted in bugcheck scenarii, and we
      don't want to further bugcheck.
    
    - Fix newline trimming loop.
    
    - KiDoBugCheckCallbacks():
      * Wrap the bugcheck CallbackRoutine call in SEH.
      * Add a FIXME concerning the need of further memory validation of CurrentRecord.
    
    - Add a FIXME concerning the need to run the bugcheck-reason callbacks
      with the KbCallbackReserved1 reason, in KeBugCheckWithTf().
      Mentioned in
http://blog.ptsecurity.com/2012/06/customizing-blue-screen-of-death.html
---
 ntoskrnl/include/internal/mm.h |  6 ++-
 ntoskrnl/ke/bug.c              | 88 ++++++++++++++++++++++++++++++------------
 ntoskrnl/mm/ARM3/sysldr.c      | 40 +++++++++++++++++++
 3 files changed, 108 insertions(+), 26 deletions(-)

diff --git a/ntoskrnl/include/internal/mm.h b/ntoskrnl/include/internal/mm.h
index b4d7ad85bcc..da3a720d4dc 100644
--- a/ntoskrnl/include/internal/mm.h
+++ b/ntoskrnl/include/internal/mm.h
@@ -794,7 +794,7 @@ NTAPI
 MmDeleteKernelStack(PVOID Stack,
                     BOOLEAN GuiStack);
 
-/* balace.c ******************************************************************/
+/* balance.c *****************************************************************/
 
 INIT_FUNCTION
 VOID
@@ -1370,6 +1370,10 @@ MiInitializeLoadedModuleList(
     IN PLOADER_PARAMETER_BLOCK LoaderBlock
 );
 
+VOID
+NTAPI
+MmMakeKernelResourceSectionWritable(VOID);
+
 NTSTATUS
 NTAPI
 MmLoadSystemImage(
diff --git a/ntoskrnl/ke/bug.c b/ntoskrnl/ke/bug.c
index f1e7a50376f..a92eb9297b9 100644
--- a/ntoskrnl/ke/bug.c
+++ b/ntoskrnl/ke/bug.c
@@ -406,47 +406,65 @@ NTAPI
 KeGetBugMessageText(IN ULONG BugCheckCode,
                     OUT PANSI_STRING OutputString OPTIONAL)
 {
-    ULONG i, j;
+    ULONG i;
     ULONG IdOffset;
-    ULONG_PTR MessageEntry;
+    PMESSAGE_RESOURCE_ENTRY MessageEntry;
     PCHAR BugCode;
-    BOOLEAN Result = FALSE;
     USHORT Length;
+    BOOLEAN Result = FALSE;
 
     /* Make sure we're not bugchecking too early */
     if (!KiBugCodeMessages) return Result;
 
+    /*
+     * Globally protect in SEH as we are trying to access data in
+     * dire situations, and potentially going to patch it (see below).
+     */
+    _SEH2_TRY
+    {
+
+    /*
+     * Make the kernel resource section writable, as we are going to manually
+     * trim the trailing newlines in the bugcheck resource message in place,
+     * when OutputString is NULL and before displaying it on screen.
+     */
+    MmMakeKernelResourceSectionWritable();
+
     /* Find the message. This code is based on RtlFindMesssage */
     for (i = 0; i < KiBugCodeMessages->NumberOfBlocks; i++)
     {
-        /* Check if the ID Matches */
+        /* Check if the ID matches */
         if ((BugCheckCode >= KiBugCodeMessages->Blocks[i].LowId) &&
             (BugCheckCode <= KiBugCodeMessages->Blocks[i].HighId))
         {
-            /* Get Offset to Entry */
-            MessageEntry = KiBugCodeMessages->Blocks[i].OffsetToEntries +
-                           (ULONG_PTR)KiBugCodeMessages;
+            /* Get offset to entry */
+            MessageEntry = (PMESSAGE_RESOURCE_ENTRY)
+                ((ULONG_PTR)KiBugCodeMessages +
KiBugCodeMessages->Blocks[i].OffsetToEntries);
             IdOffset = BugCheckCode - KiBugCodeMessages->Blocks[i].LowId;
 
-            /* Get offset to ID */
-            for (j = 0; j < IdOffset; j++)
+            /* Advance in the entries until finding it */
+            while (IdOffset--)
             {
-                /* Advance in the Entries */
-                MessageEntry += ((PMESSAGE_RESOURCE_ENTRY)MessageEntry)->
-                                Length;
+                MessageEntry = (PMESSAGE_RESOURCE_ENTRY)
+                    ((ULONG_PTR)MessageEntry + MessageEntry->Length);
             }
 
-            /* Get the final Code */
-            BugCode = (PCHAR)((PMESSAGE_RESOURCE_ENTRY)MessageEntry)->Text;
+            /* Make sure it's not Unicode */
+            ASSERT(!(MessageEntry->Flags & MESSAGE_RESOURCE_UNICODE));
+
+            /* Get the final code */
+            BugCode = (PCHAR)MessageEntry->Text;
             Length = (USHORT)strlen(BugCode);
 
             /* Handle trailing newlines */
-            while ((Length > 0) && ((BugCode[Length] == '\n') ||
-                                    (BugCode[Length] == '\r') ||
-                                    (BugCode[Length] == ANSI_NULL)))
+            while ((Length > 0) && ((BugCode[Length - 1] == '\n') ||
+                                    (BugCode[Length - 1] == '\r') ||
+                                    (BugCode[Length - 1] == ANSI_NULL)))
             {
-                /* Check if we have a string to return */
-                if (!OutputString) BugCode[Length] = ANSI_NULL;
+                /* Directly trim the newline in place if we don't return the string
*/
+                if (!OutputString) BugCode[Length - 1] = ANSI_NULL;
+
+                /* Skip the trailing newline */
                 Length--;
             }
 
@@ -455,12 +473,12 @@ KeGetBugMessageText(IN ULONG BugCheckCode,
             {
                 /* Return it in the OutputString */
                 OutputString->Buffer = BugCode;
-                OutputString->Length = Length + 1;
-                OutputString->MaximumLength = Length + 1;
+                OutputString->Length = Length;
+                OutputString->MaximumLength = Length;
             }
             else
             {
-                /* Direct Output to Screen */
+                /* Direct output to screen */
                 InbvDisplayString(BugCode);
                 InbvDisplayString("\r");
             }
@@ -471,6 +489,12 @@ KeGetBugMessageText(IN ULONG BugCheckCode,
         }
     }
 
+    }
+    _SEH2_EXCEPT(EXCEPTION_EXECUTE_HANDLER)
+    {
+    }
+    _SEH2_END;
+
     /* Return the result */
     return Result;
 }
@@ -499,6 +523,8 @@ KiDoBugCheckCallbacks(VOID)
                                           Entry);
 
         /* Validate it */
+        // TODO/FIXME: Check whether the memory CurrentRecord points to
+        // is still accessible and valid!
         if (CurrentRecord->Entry.Blink != LastEntry) return;
         Checksum = (ULONG_PTR)CurrentRecord->CallbackRoutine;
         Checksum += (ULONG_PTR)CurrentRecord->Buffer;
@@ -511,9 +537,17 @@ KiDoBugCheckCallbacks(VOID)
         {
             /* Call the routine */
             CurrentRecord->State = BufferStarted;
-            (CurrentRecord->CallbackRoutine)(CurrentRecord->Buffer,
-                                             CurrentRecord->Length);
-            CurrentRecord->State = BufferFinished;
+            _SEH2_TRY
+            {
+                (CurrentRecord->CallbackRoutine)(CurrentRecord->Buffer,
+                                                 CurrentRecord->Length);
+                CurrentRecord->State = BufferFinished;
+            }
+            _SEH2_EXCEPT(EXCEPTION_EXECUTE_HANDLER)
+            {
+                CurrentRecord->State = BufferIncomplete;
+            }
+            _SEH2_END;
         }
 
         /* Go to the next entry */
@@ -1145,6 +1179,10 @@ KeBugCheckWithTf(IN ULONG BugCheckCode,
                             HardErrMessage,
                             AnsiName);
 
+        // TODO/FIXME: Run the registered reason-callbacks from
+        // the KeBugcheckReasonCallbackListHead list with the
+        // KbCallbackReserved1 reason.
+
         /* Check if the debugger is disabled but we can enable it */
         if (!(KdDebuggerEnabled) && !(KdPitchDebugger))
         {
diff --git a/ntoskrnl/mm/ARM3/sysldr.c b/ntoskrnl/mm/ARM3/sysldr.c
index 640856620a7..8b34fea69fb 100644
--- a/ntoskrnl/mm/ARM3/sysldr.c
+++ b/ntoskrnl/mm/ARM3/sysldr.c
@@ -2284,6 +2284,46 @@ MiInitializeLoadedModuleList(IN PLOADER_PARAMETER_BLOCK
LoaderBlock)
     return TRUE;
 }
 
+VOID
+NTAPI
+MmMakeKernelResourceSectionWritable(VOID)
+{
+    PMMPTE PointerPte;
+    MMPTE TempPte;
+
+    /* Don't do anything if the resource section is already writable */
+    if (MiKernelResourceStartPte == NULL || MiKernelResourceEndPte == NULL)
+        return;
+
+    /* If the resource section is physical, we cannot change its protection */
+    if (MI_IS_PHYSICAL_ADDRESS(MiPteToAddress(MiKernelResourceStartPte)))
+        return;
+
+    /* Loop the PTEs */
+    for (PointerPte = MiKernelResourceStartPte; PointerPte <= MiKernelResourceEndPte;
PointerPte++)
+    {
+        /* Read the PTE */
+        TempPte = *PointerPte;
+
+        /* Make sure it's valid */
+        ASSERT(TempPte.u.Hard.Valid == 1);
+
+        /* Update the protection */
+        MI_MAKE_WRITE_PAGE(&TempPte);
+        MI_UPDATE_VALID_PTE(PointerPte, TempPte);
+    }
+
+    /*
+     * Invalidate the cached resource section PTEs
+     * so as to not change its protection again later.
+     */
+    MiKernelResourceStartPte = NULL;
+    MiKernelResourceEndPte = NULL;
+
+    /* Only flush the current processor's TLB */
+    KeFlushCurrentTb();
+}
+
 LOGICAL
 NTAPI
 MiUseLargeDriverPage(IN ULONG NumberOfPtes,