[ros-diffs] [tkreuzer] 74539: [NTOSKRNL] Improve S-List-Fault detection in KiTrap0EHandler to handle usermode faults as well.

Author: tkreuzer
Date: Sat May 13 20:07:39 2017
New Revision: 74539

URL: http://svn.reactos.org/svn/reactos?rev=74539&view=rev
Log:
[NTOSKRNL] Improve S-List-Fault detection in KiTrap0EHandler to handle usermode faults as
well.

Modified:
    trunk/reactos/ntoskrnl/ke/i386/traphdlr.c

Modified: trunk/reactos/ntoskrnl/ke/i386/traphdlr.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/ke/i386/traphdlr.…
==============================================================================
--- trunk/reactos/ntoskrnl/ke/i386/traphdlr.c	[iso-8859-1] (original)
+++ trunk/reactos/ntoskrnl/ke/i386/traphdlr.c	[iso-8859-1] Sat May 13 20:07:39 2017
@@ -15,6 +15,8 @@
 VOID __cdecl KiFastCallEntry(VOID);
 VOID __cdecl KiFastCallEntryWithSingleStep(VOID);
 
+extern PVOID KeUserPopEntrySListFault;
+extern PVOID KeUserPopEntrySListResume;
 extern PVOID FrRestore;
 VOID FASTCALL Ke386LoadFpuState(IN PFX_SAVE_AREA SaveArea);
 
@@ -1240,10 +1242,46 @@
                          TrapFrame);
     }
 
-    /* Check for S-LIST fault in kernel mode */
-    if (TrapFrame->Eip == (ULONG_PTR)ExpInterlockedPopEntrySListFault)
-    {
-        PSLIST_HEADER SListHeader;
+    /* Check for S-List fault
+
+       Explanation: An S-List fault can occur due to a race condition between 2
+       threads simultaneously trying to pop an element from the S-List. After
+       thread 1 has read the pointer to the top element on the S-List it is
+       preempted and thread 2 calls InterlockedPopEntrySlist on the same S-List,
+       removing the top element and freeing it's memory. After that thread 1
+       resumes and tries to read the address of the Next pointer from the top
+       element, which it assumes will be the next top element.
+       But since that memory has been freed, we get a page fault. To handle this
+       race condition, we let thread 1 repeat the operation.
+       We do NOT invoke the page fault handler in this case, since we do not
+       want to trigger any side effects, like paging or a guard page fault.
+
+       Sequence of operations:
+
+           Thread 1 : mov eax, [ebp] <= eax now points to the first element
+           Thread 1 : mov edx, [ebp + 4] <= edx is loaded with Depth and Sequence
+            *** preempted ***
+           Thread 2 : calls InterlockedPopEntrySlist, changing the top element
+           Thread 2 : frees the memory of the element that was popped
+            *** preempted ***
+           Thread 1 : checks if eax is NULL
+           Thread 1 : InterlockedPopEntrySListFault: mov ebx, [eax] <= faults
+
+        To be sure that we are dealing with exactly the case described above, we
+        check whether the ListHeader has changed. If Thread 2 only popped one
+        entry, the Next field in the S-List-header has changed.
+        If after thread 1 has faulted, thread 2 allocates a new element, by
+        chance getting the same address as the previously freed element and
+        pushes it on the list again, we will see the same top element, but the
+        Sequence member of the S-List header has changed. Therefore we check
+        both fields to make sure we catch any concurrent modification of the
+        S-List-header.
+    */
+    if ((TrapFrame->Eip == (ULONG_PTR)ExpInterlockedPopEntrySListFault) ||
+        (TrapFrame->Eip == (ULONG_PTR)KeUserPopEntrySListFault))
+    {
+        ULARGE_INTEGER SListHeader;
+        PVOID ResumeAddress;
 
         /* Sanity check that the assembly is correct:
            This must be mov ebx, [eax]
@@ -1255,30 +1293,50 @@
                (((UCHAR*)TrapFrame->Eip)[4] == 0x4D) &&
                (((UCHAR*)TrapFrame->Eip)[5] == 0x00));
 
-        /* Get the pointer to the SLIST_HEADER */
-        SListHeader = (PSLIST_HEADER)TrapFrame->Ebp;
-
-        /* Check if the Next member of the SLIST_HEADER was changed */
-        if (SListHeader->Next.Next != (PSLIST_ENTRY)TrapFrame->Eax)
-        {
+        /* Check if this is a user fault */
+        if (TrapFrame->Eip == (ULONG_PTR)KeUserPopEntrySListFault)
+        {
+            /* EBP points to the S-List-header. Copy it inside SEH, to protect
+               against a bogus pointer from user mode */
+            _SEH2_TRY
+            {
+                ProbeForRead((PVOID)TrapFrame->Ebp,
+                             sizeof(ULARGE_INTEGER),
+                             TYPE_ALIGNMENT(SLIST_HEADER));
+                SListHeader = *(PULARGE_INTEGER)TrapFrame->Ebp;
+            }
+            _SEH2_EXCEPT(EXCEPTION_EXECUTE_HANDLER)
+            {
+                /* The S-List pointer is not valid! */
+                goto NotSListFault;
+            }
+            _SEH2_END;
+            ResumeAddress = KeUserPopEntrySListResume;
+        }
+        else
+        {
+            SListHeader = *(PULARGE_INTEGER)TrapFrame->Ebp;
+            ResumeAddress = ExpInterlockedPopEntrySListResume;
+        }
+
+        /* Check if either the Next pointer or the Sequence member in the
+           S-List-header has changed. If any of these has changed, we restart
+           the operation. Otherwise we only have a bogus pointer and let the
+           page fault handler deal with it. */
+        if ((SListHeader.LowPart != TrapFrame->Eax) ||
+            (SListHeader.HighPart != TrapFrame->Edx))
+        {
+            DPRINT1("*** Got an S-List-Fault ***\n");
+            KeGetCurrentThread()->SListFaultCount++;
+
             /* Restart the operation */
-            TrapFrame->Eip = (ULONG_PTR)ExpInterlockedPopEntrySListResume;
+            TrapFrame->Eip = (ULONG_PTR)ResumeAddress;
 
             /* Continue execution */
             KiEoiHelper(TrapFrame);
         }
-        else
-        {
-#if 0
-            /* Do what windows does and issue an invalid access violation */
-            KiDispatchException2Args(KI_EXCEPTION_ACCESS_VIOLATION,
-                                     TrapFrame->Eip,
-                                     StoreInstruction,
-                                     Cr2,
-                                     TrapFrame);
-#endif
-        }
-    }
+    }
+NotSListFault:
 
     /* Call the access fault handler */
     Status = MmAccessFault(Present,