[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 at 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.c?rev=63610&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.h?rev=63610&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;
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.reactos.org/pipermail/ros-dev/attachments/20140617/e2ed75b7/attachment.html>