Re: [ros-dev] [ros-diffs] [hbelusca] 63610: [WIN32K] There is a bug in win32k (who would have thought that?) that consists in holding a winstation spinlock while running PAGED_CODE MmCopyToCaller function, when building the...

[Hermès BÉLUSCA - MAÏTO]

It appears that we call MmPageEntireDriver in win32k’s DriverEntry function, see:

http://git.reactos.org/?p=reactos.git;a=blob;f=reactos/win32ss/user/ntuser/…

 

By the way, I’m wondering why we do have:


<http://git.reactos.org/?p=reactos.git;a=blob;f=reactos/win32ss/user/ntuser/main.c;hb=5f44954f6e32dc8269d5c89eccf896167d274b1a#l663>
663 /*


<http://git.reactos.org/?p=reactos.git;a=blob;f=reactos/win32ss/user/ntuser/main.c;hb=5f44954f6e32dc8269d5c89eccf896167d274b1a#l664>
664  * This definition doesn't work


<http://git.reactos.org/?p=reactos.git;a=blob;f=reactos/win32ss/user/ntuser/main.c;hb=5f44954f6e32dc8269d5c89eccf896167d274b1a#l665>
665  */


<http://git.reactos.org/?p=reactos.git;a=blob;f=reactos/win32ss/user/ntuser/main.c;hb=5f44954f6e32dc8269d5c89eccf896167d274b1a#l666>
666 INIT_FUNCTION


<http://git.reactos.org/?p=reactos.git;a=blob;f=reactos/win32ss/user/ntuser/main.c;hb=5f44954f6e32dc8269d5c89eccf896167d274b1a#l667>
667 NTSTATUS


<http://git.reactos.org/?p=reactos.git;a=blob;f=reactos/win32ss/user/ntuser/main.c;hb=5f44954f6e32dc8269d5c89eccf896167d274b1a#l668>
668 APIENTRY


<http://git.reactos.org/?p=reactos.git;a=blob;f=reactos/win32ss/user/ntuser/main.c;hb=5f44954f6e32dc8269d5c89eccf896167d274b1a#l669>
669 DriverEntry(

 

as a comment above DriverEntry…

 

Cheers,

Hermès

 

De : Ros-dev [mailto:[e-mail]] De la part de Alex Ionescu
Envoyé : mercredi 18 juin 2014 10:17
À : ReactOS Development List
Objet : Re: [ros-dev] [ros-diffs] [hbelusca] 63610: [WIN32K] There is a bug in win32k (who
would have thought that?) that consists in holding a winstation spinlock while running
PAGED_CODE MmCopyToCaller function, when building the...

 

We don't do *any* paging ;-)




Best regards,
Alex Ionescu

 

On Wed, Jun 18, 2014 at 1:02 AM, Hermès BÉLUSCA - MAÏTO &lt;hermes.belusca(a)sfr.fr&gt;
wrote:

Aha, thanks for your tip, so that means we don’t do it atm?

 

Hermès.

 

De : Ros-dev [mailto:[e-mail]] De la part de Alex Ionescu
Envoyé : mardi 17 juin 2014 23:14
À : ReactOS Development List
Cc : Linda Wang
Objet : Re: [ros-dev] [ros-diffs] [hbelusca] 63610: [WIN32K] There is a bug in win32k (who
would have thought that?) that consists in holding a winstation spinlock while running
PAGED_CODE MmCopyToCaller function, when building the...

 

Win32k.sys is supposed to use MmPageEntireDriver, as such, it should run 100% at passive
only.




Best regards,
Alex Ionescu

 

On Tue, Jun 17, 2014 at 10:01 PM, &lt;hbelusca(a)svn.reactos.org&gt; wrote:

Author: hbelusca
Date: Tue Jun 17 20:01:23 2014
New Revision: 63610

URL: http://svn.reactos.org/svn/reactos?rev=63610
<http://svn.reactos.org/svn/reactos?rev=63610&view=rev> &view=rev
Log:
[WIN32K]
There is a bug in win32k (who would have thought that?) that consists in holding a
winstation spinlock while running PAGED_CODE MmCopyToCaller function, when building the
list of desktops of a given window station (the bug is easily triggerable when calling
EnumDesktopsW). Since this lock is never used in anyplace but in this function, which, by
the way, is just a reader function that fills user buffer, I consider that it is safe to
remove this lock. However I want approval from win32k specialists. Hence I just disable
the code with a define USE_WINSTA_LOCK. If this lock is really needed, please rewrite the
BuildDesktopNameList function !! Otherwise remove this lock and the associated code !!
This is a blocker for the shutdown code.

Modified:
    trunk/reactos/win32ss/user/ntuser/winsta.c
    trunk/reactos/win32ss/user/ntuser/winsta.h

Modified: trunk/reactos/win32ss/user/ntuser/winsta.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/win32ss/user/ntuser/winsta…
<http://svn.reactos.org/svn/reactos/trunk/reactos/win32ss/user/ntuser/winsta.c?rev=63610&r1=63609&r2=63610&view=diff>
&r1=63609&r2=63610&view=diff
==============================================================================
--- trunk/reactos/win32ss/user/ntuser/winsta.c  [iso-8859-1] (original)
+++ trunk/reactos/win32ss/user/ntuser/winsta.c  [iso-8859-1] Tue Jun 17 20:01:23 2014
@@ -451,7 +451,9 @@
    /* Initialize the window station */
    RtlZeroMemory(WindowStationObject, sizeof(WINSTATION_OBJECT));

+#ifdef USE_WINSTA_LOCK
    KeInitializeSpinLock(&WindowStationObject->Lock);
+#endif
    InitializeListHead(&WindowStationObject->DesktopListHead);
    Status = RtlCreateAtomTable(37, &WindowStationObject->AtomTable);
    WindowStationObject->Name = WindowStationName;
@@ -1203,7 +1205,9 @@
 {
    NTSTATUS Status;
    PWINSTATION_OBJECT WindowStation;
+#ifdef USE_WINSTA_LOCK
    KIRQL OldLevel;
+#endif
    PLIST_ENTRY DesktopEntry;
    PDESKTOP DesktopObject;
    DWORD EntryCount;
@@ -1220,7 +1224,9 @@
       return Status;
    }

+#ifdef USE_WINSTA_LOCK
    KeAcquireSpinLock(&WindowStation->Lock, &OldLevel);
+#endif

    /*
     * Count the required size of buffer.
@@ -1242,7 +1248,9 @@
       Status = MmCopyToCaller(pRequiredSize, &ReturnLength, sizeof(ULONG));
       if (! NT_SUCCESS(Status))
       {
+#ifdef USE_WINSTA_LOCK
          KeReleaseSpinLock(&WindowStation->Lock, OldLevel);
+#endif
          ObDereferenceObject(WindowStation);
          return STATUS_BUFFER_TOO_SMALL;
       }
@@ -1253,7 +1261,9 @@
     */
    if (dwSize < ReturnLength)
    {
+#ifdef USE_WINSTA_LOCK
       KeReleaseSpinLock(&WindowStation->Lock, OldLevel);
+#endif
       ObDereferenceObject(WindowStation);
       return STATUS_BUFFER_TOO_SMALL;
    }
@@ -1264,7 +1274,9 @@
    Status = MmCopyToCaller(lpBuffer, &EntryCount, sizeof(DWORD));
    if (! NT_SUCCESS(Status))
    {
+#ifdef USE_WINSTA_LOCK
       KeReleaseSpinLock(&WindowStation->Lock, OldLevel);
+#endif
       ObDereferenceObject(WindowStation);
       return Status;
    }
@@ -1280,7 +1292,9 @@
       Status = MmCopyToCaller(lpBuffer, DesktopName.Buffer, DesktopName.Length);
       if (! NT_SUCCESS(Status))
       {
+#ifdef USE_WINSTA_LOCK
          KeReleaseSpinLock(&WindowStation->Lock, OldLevel);
+#endif
          ObDereferenceObject(WindowStation);
          return Status;
       }
@@ -1288,7 +1302,9 @@
       Status = MmCopyToCaller(lpBuffer, &NullWchar, sizeof(WCHAR));
       if (! NT_SUCCESS(Status))
       {
+#ifdef USE_WINSTA_LOCK
          KeReleaseSpinLock(&WindowStation->Lock, OldLevel);
+#endif
          ObDereferenceObject(WindowStation);
          return Status;
       }
@@ -1298,7 +1314,9 @@
    /*
     * Clean up
     */
+#ifdef USE_WINSTA_LOCK
    KeReleaseSpinLock(&WindowStation->Lock, OldLevel);
+#endif
    ObDereferenceObject(WindowStation);

    return STATUS_SUCCESS;

Modified: trunk/reactos/win32ss/user/ntuser/winsta.h
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/win32ss/user/ntuser/winsta…
<http://svn.reactos.org/svn/reactos/trunk/reactos/win32ss/user/ntuser/winsta.h?rev=63610&r1=63609&r2=63610&view=diff>
&r1=63609&r2=63610&view=diff
==============================================================================
--- trunk/reactos/win32ss/user/ntuser/winsta.h  [iso-8859-1] (original)
+++ trunk/reactos/win32ss/user/ntuser/winsta.h  [iso-8859-1] Tue Jun 17 20:01:23 2014
@@ -7,11 +7,16 @@
 #define WSS_LOCKED     (1)
 #define WSS_NOINTERACTIVE      (2)

+// Uncomment for using WinSta spinlock
+// #define USE_WINSTA_LOCK
+
 typedef struct _WINSTATION_OBJECT
 {
     DWORD dwSessionId;

+#ifdef USE_WINSTA_LOCK
     KSPIN_LOCK Lock;
+#endif
     UNICODE_STRING Name;
     LIST_ENTRY DesktopListHead;
     PRTL_ATOM_TABLE AtomTable;

 


_______________________________________________
Ros-dev mailing list
Ros-dev(a)reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev