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

[Alex Ionescu]

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, <hbelusca(a)svn.reactos.org> 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;
>
>
>