17 Sep
2022
17 Sep
'22
11:49 a.m.
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);