[reactos] 01/01: [NTOS] Add some sanity checks when synchronizing PDEs

[Jérôme Gardou]

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

commit 75125228be048a899bb9d03efb2a55c8c3a8b60e
Author:     Jérôme Gardou <jerome.gardou(a)reactos.org>
AuthorDate: Fri Sep 9 19:17:14 2022 +0200
Commit:     Jérôme Gardou <zefklop(a)users.noreply.github.com>
CommitDate: Sat Sep 17 13:48:56 2022 +0200

    [NTOS] Add some sanity checks when synchronizing PDEs
---
 ntoskrnl/include/internal/mm.h |  12 ++---
 ntoskrnl/mm/ARM3/miarm.h       |  18 +++++--
 ntoskrnl/mm/i386/page.c        | 117 +++++++++++++++++++++--------------------
 ntoskrnl/mm/marea.c            |   7 ++-
 ntoskrnl/mm/rmap.c             |  18 +++++--
 ntoskrnl/mm/section.c          |  14 ++++-
 6 files changed, 112 insertions(+), 74 deletions(-)

diff --git a/ntoskrnl/include/internal/mm.h b/ntoskrnl/include/internal/mm.h
index af281b26ff3..c7bf663d1c8 100644
--- a/ntoskrnl/include/internal/mm.h
+++ b/ntoskrnl/include/internal/mm.h
@@ -1288,13 +1288,13 @@ NTSTATUS
 NTAPI
 MmGetExecuteOptions(IN PULONG ExecuteOptions);
 
-VOID
-NTAPI
+_Success_(return)
+BOOLEAN
 MmDeleteVirtualMapping(
-    struct _EPROCESS *Process,
-    PVOID Address,
-    BOOLEAN* WasDirty,
-    PPFN_NUMBER Page
+    _In_opt_ PEPROCESS Process,
+    _In_ PVOID Address,
+    _Out_opt_ BOOLEAN* WasDirty,
+    _Out_opt_ PPFN_NUMBER Page
 );
 
 /* arch/procsup.c ************************************************************/
diff --git a/ntoskrnl/mm/ARM3/miarm.h b/ntoskrnl/mm/ARM3/miarm.h
index b5fd1de3409..60197cda74e 100644
--- a/ntoskrnl/mm/ARM3/miarm.h
+++ b/ntoskrnl/mm/ARM3/miarm.h
@@ -2428,21 +2428,29 @@ FORCEINLINE
 BOOLEAN
 MiSynchronizeSystemPde(PMMPDE PointerPde)
 {
-    MMPDE SystemPde;
     ULONG Index;
 
     /* Get the Index from the PDE */
     Index = ((ULONG_PTR)PointerPde & (SYSTEM_PD_SIZE - 1)) / sizeof(MMPTE);
+    if (PointerPde->u.Hard.Valid != 0)
+    {
+        NT_ASSERT(PointerPde->u.Long == MmSystemPagePtes[Index].u.Long);
+        return TRUE;
+    }
+
+    if (MmSystemPagePtes[Index].u.Hard.Valid == 0)
+    {
+        return FALSE;
+    }
 
     /* Copy the PDE from the double-mapped system page directory */
-    SystemPde = MmSystemPagePtes[Index];
-    *PointerPde = SystemPde;
+    MI_WRITE_VALID_PDE(PointerPde, MmSystemPagePtes[Index]);
 
     /* Make sure we re-read the PDE and PTE */
     KeMemoryBarrierWithoutFence();
 
-    /* Return, if we had success */
-    return SystemPde.u.Hard.Valid != 0;
+    /* Return success */
+    return TRUE;
 }
 #endif
 
diff --git a/ntoskrnl/mm/i386/page.c b/ntoskrnl/mm/i386/page.c
index a9ba2eb1179..791aeb93490 100644
--- a/ntoskrnl/mm/i386/page.c
+++ b/ntoskrnl/mm/i386/page.c
@@ -119,7 +119,12 @@ BOOLEAN
 MiIsPageTablePresent(PVOID Address)
 {
 #if _MI_PAGING_LEVELS == 2
-    return MmWorkingSetList->UsedPageTableEntries[MiGetPdeOffset(Address)] != 0;
+    BOOLEAN Ret = MmWorkingSetList->UsedPageTableEntries[MiGetPdeOffset(Address)] !=
0;
+
+    /* Some sanity check while we're here */
+    ASSERT(Ret == (MiAddressToPde(Address)->u.Hard.Valid != 0));
+
+    return Ret;
 #else
     PMMPDE PointerPde;
     PMMPPE PointerPpe;
@@ -217,16 +222,29 @@ MmGetPfnForProcess(PEPROCESS Process,
     return Page;
 }
 
-VOID
-NTAPI
-MmDeleteVirtualMapping(PEPROCESS Process, PVOID Address,
-                       BOOLEAN* WasDirty, PPFN_NUMBER Page)
-/*
- * FUNCTION: Delete a virtual mapping
+/**
+ * @brief Deletes the virtual mapping and optionally gives back the page & dirty
bit.
+ *
+ * @param Process - The process this address belongs to, or NULL if system address.
+ * @param Address - The address to unmap.
+ * @param WasDirty - Optional param receiving the dirty bit of the PTE.
+ * @param Page - Optional param receiving the page number previously mapped to this
address.
+ *
+ * @return Whether there was actually a page mapped at the given address.
  */
+_Success_(return)
+BOOLEAN
+MmDeleteVirtualMapping(
+    _In_opt_ PEPROCESS Process,
+    _In_ PVOID Address,
+    _Out_opt_ BOOLEAN* WasDirty,
+    _Out_opt_ PPFN_NUMBER Page)
 {
     PMMPTE PointerPte;
     MMPTE OldPte;
+    BOOLEAN ValidPde;
+
+    OldPte.u.Long = 0;
 
     DPRINT("MmDeleteVirtualMapping(%p, %p, %p, %p)\n", Process, Address,
WasDirty, Page);
 
@@ -244,18 +262,10 @@ MmDeleteVirtualMapping(PEPROCESS Process, PVOID Address,
             KeBugCheck(MEMORY_MANAGEMENT);
         }
 #if (_MI_PAGING_LEVELS == 2)
-        if (!MiSynchronizeSystemPde(MiAddressToPde(Address)))
+        ValidPde = MiSynchronizeSystemPde(MiAddressToPde(Address));
 #else
-        if (!MiIsPdeForAddressValid(Address))
+        ValidPde = MiIsPdeForAddressValid(Address);
 #endif
-        {
-            /* There can't be a page if there is no PDE */
-            if (WasDirty)
-                *WasDirty = FALSE;
-            if (Page)
-                *Page = 0;
-            return;
-        }
     }
     else
     {
@@ -266,61 +276,56 @@ MmDeleteVirtualMapping(PEPROCESS Process, PVOID Address,
         }
 
         /* Only for current process !!! */
-        ASSERT(Process = PsGetCurrentProcess());
+        ASSERT(Process == PsGetCurrentProcess());
         MiLockProcessWorkingSetUnsafe(Process, PsGetCurrentThread());
 
-        /* No PDE --> No page */
-        if (!MiIsPageTablePresent(Address))
+        ValidPde = MiIsPageTablePresent(Address);
+        if (ValidPde)
         {
-            MiUnlockProcessWorkingSetUnsafe(Process, PsGetCurrentThread());
-            if (WasDirty)
-                *WasDirty = 0;
-            if (Page)
-                *Page = 0;
-            return;
+            MiMakePdeExistAndMakeValid(MiAddressToPde(Address), Process, MM_NOIRQL);
         }
-
-        MiMakePdeExistAndMakeValid(MiAddressToPde(Address), Process, MM_NOIRQL);
     }
 
-    PointerPte = MiAddressToPte(Address);
-    OldPte.u.Long = InterlockedExchangePte(PointerPte, 0);
-
-    if (OldPte.u.Long == 0)
+    /* Get the PTE if we're having anything */
+    if (ValidPde)
     {
-        /* There was nothing here */
-        if (Address < MmSystemRangeStart)
-            MiUnlockProcessWorkingSetUnsafe(Process, PsGetCurrentThread());
-        if (WasDirty)
-            *WasDirty = 0;
-        if (Page)
-            *Page = 0;
-        return;
-    }
+        PointerPte = MiAddressToPte(Address);
+        OldPte.u.Long = InterlockedExchangePte(PointerPte, 0);
 
-    /* It must have been present, or not a swap entry */
-    ASSERT(OldPte.u.Hard.Valid || !FlagOn(OldPte.u.Long, 0x800));
-
-    if (OldPte.u.Hard.Valid)
         KeInvalidateTlbEntry(Address);
 
-    if (Address < MmSystemRangeStart)
+        if (OldPte.u.Long != 0)
+        {
+            /* It must have been present, or not a swap entry */
+            ASSERT(OldPte.u.Hard.Valid || !FlagOn(OldPte.u.Long, 0x800));
+            if (WasDirty != NULL)
+            {
+                *WasDirty = !!OldPte.u.Hard.Dirty;
+            }
+            if (Page != NULL)
+            {
+                *Page = OldPte.u.Hard.PageFrameNumber;
+            }
+        }
+    }
+
+    if (Process != NULL)
     {
-        /* Remove PDE reference */
-        if (MiDecrementPageTableReferences(Address) == 0)
+        /* Remove PDE reference, if needed */
+        if (OldPte.u.Long != 0)
         {
-            KIRQL OldIrql = MiAcquirePfnLock();
-            MiDeletePde(MiAddressToPde(Address), Process);
-            MiReleasePfnLock(OldIrql);
+            if (MiDecrementPageTableReferences(Address) == 0)
+            {
+                KIRQL OldIrql = MiAcquirePfnLock();
+                MiDeletePde(MiAddressToPde(Address), Process);
+                MiReleasePfnLock(OldIrql);
+            }
         }
 
         MiUnlockProcessWorkingSetUnsafe(Process, PsGetCurrentThread());
     }
 
-    if (WasDirty)
-        *WasDirty = !!OldPte.u.Hard.Dirty;
-    if (Page)
-        *Page = OldPte.u.Hard.PageFrameNumber;
+    return OldPte.u.Long != 0;
 }
 
 
@@ -631,7 +636,7 @@ MmCreateVirtualMappingUnsafe(PEPROCESS Process,
         }
 
         /* Only for current process !!! */
-        ASSERT(Process = PsGetCurrentProcess());
+        ASSERT(Process == PsGetCurrentProcess());
         MiLockProcessWorkingSetUnsafe(Process, PsGetCurrentThread());
 
         MiMakePdeExistAndMakeValid(MiAddressToPde(Address), Process, MM_NOIRQL);
diff --git a/ntoskrnl/mm/marea.c b/ntoskrnl/mm/marea.c
index 78d07c75cd6..8bf8ab72630 100644
--- a/ntoskrnl/mm/marea.c
+++ b/ntoskrnl/mm/marea.c
@@ -314,16 +314,19 @@ MmFreeMemoryArea(
             BOOLEAN Dirty = FALSE;
             SWAPENTRY SwapEntry = 0;
             PFN_NUMBER Page = 0;
+            BOOLEAN DoFree;
 
             if (MmIsPageSwapEntry(Process, (PVOID)Address))
             {
                 MmDeletePageFileMapping(Process, (PVOID)Address, &SwapEntry);
+                /* We'll have to do some cleanup when we're on the page file */
+                DoFree = TRUE;
             }
             else
             {
-                MmDeleteVirtualMapping(Process, (PVOID)Address, &Dirty, &Page);
+                DoFree = MmDeleteVirtualMapping(Process, (PVOID)Address, &Dirty,
&Page);
             }
-            if (FreePage != NULL)
+            if (DoFree && (FreePage != NULL))
             {
                 FreePage(FreePageContext, MemoryArea, (PVOID)Address,
                          Page, SwapEntry, (BOOLEAN)Dirty);
diff --git a/ntoskrnl/mm/rmap.c b/ntoskrnl/mm/rmap.c
index e27e31de820..9c2f00eff22 100644
--- a/ntoskrnl/mm/rmap.c
+++ b/ntoskrnl/mm/rmap.c
@@ -135,6 +135,7 @@ GetEntry:
         PFN_NUMBER MapPage;
         LARGE_INTEGER Offset;
         BOOLEAN Released;
+        BOOLEAN Unmapped;
 
         Offset.QuadPart = MemoryArea->SectionData.ViewOffset +
                  ((ULONG_PTR)Address - MA_GetStartingAddress(MemoryArea));
@@ -158,10 +159,19 @@ GetEntry:
 
         /* Delete this virtual mapping in the process */
         MmDeleteRmap(Page, Process, Address);
-        MmDeleteVirtualMapping(Process, Address, &Dirty, &MapPage);
-
-        /* We checked this earlier */
-        ASSERT(MapPage == Page);
+        Unmapped = MmDeleteVirtualMapping(Process, Address, &Dirty, &MapPage);
+        if (!Unmapped || (MapPage != Page))
+        {
+            /*
+             * Something's corrupted, we got there because this process is
+             * supposed to be mapping this page there.
+             */
+            KeBugCheckEx(MEMORY_MANAGEMENT,
+                         (ULONG_PTR)Process,
+                         (ULONG_PTR)Address,
+                         (ULONG_PTR)__FILE__,
+                         __LINE__);
+        }
 
         if (Page != PFN_FROM_SSE(Entry))
         {
diff --git a/ntoskrnl/mm/section.c b/ntoskrnl/mm/section.c
index 44b5710f6f4..0ab7da4a648 100644
--- a/ntoskrnl/mm/section.c
+++ b/ntoskrnl/mm/section.c
@@ -1897,6 +1897,7 @@ MmAccessFaultSectionView(PMMSUPPORT AddressSpace,
     PMM_SECTION_SEGMENT Segment;
     PFN_NUMBER OldPage;
     PFN_NUMBER NewPage;
+    PFN_NUMBER UnmappedPage;
     PVOID PAddress;
     LARGE_INTEGER Offset;
     PMM_REGION Region;
@@ -1904,6 +1905,7 @@ MmAccessFaultSectionView(PMMSUPPORT AddressSpace,
     PEPROCESS Process = MmGetAddressSpaceOwner(AddressSpace);
     BOOLEAN Cow = FALSE;
     ULONG NewProtect;
+    BOOLEAN Unmapped;
 
     DPRINT("MmAccessFaultSectionView(%p, %p, %p)\n", AddressSpace, MemoryArea,
Address);
 
@@ -2003,7 +2005,17 @@ MmAccessFaultSectionView(PMMSUPPORT AddressSpace,
      * Unshare the old page.
      */
     DPRINT("Swapping page (Old %x New %x)\n", OldPage, NewPage);
-    MmDeleteVirtualMapping(Process, PAddress, NULL, NULL);
+    Unmapped = MmDeleteVirtualMapping(Process, PAddress, NULL, &UnmappedPage);
+    if (!Unmapped || (UnmappedPage != OldPage))
+    {
+        /* Uh , we had a page just before, but suddenly it changes. Someone corrupted us.
*/
+        KeBugCheckEx(MEMORY_MANAGEMENT,
+                    (ULONG_PTR)Process,
+                    (ULONG_PTR)PAddress,
+                    (ULONG_PTR)__FILE__,
+                    __LINE__);
+    }
+
     if (Process)
         MmDeleteRmap(OldPage, Process, PAddress);
     MmUnsharePageEntrySectionSegment(MemoryArea, Segment, &Offset, FALSE, FALSE,
NULL);