New subject: [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...

17 Jun 2014

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&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…

 ==============================================================================
 --- 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…

 ==============================================================================
 --- 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;

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...