Re: [ros-dev] [ros-diffs] [sginsberg] 69393: [NTOS] Fix the Ob wait system calls to only catch the exceptions that are expected to be raised by the Ke wait functions (and not potentially silently catching *any* exception an...

[Pierre Schweitzer]

On 28/09/2015 11:01, [e-mail] wrote:
> Author: sginsberg
> Date: Mon Sep 28 09:01:11 2015
> New Revision: 69393
>
> URL: http://svn.reactos.org/svn/reactos?rev=69393&view=rev
> Log:
> [NTOS] Fix the Ob wait system calls to only catch the exceptions that are expected to be raised by the Ke wait functions (and not potentially silently catching *any* exception and corrupting everything in the process). Also fixup some code logic. SEH Mega Fixup 1/???
>
> Modified:
>     trunk/reactos/ntoskrnl/ob/obwait.c
>
> Modified: trunk/reactos/ntoskrnl/ob/obwait.c
> URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/ob/obwait.c?rev=69...
> ==============================================================================
> --- trunk/reactos/ntoskrnl/ob/obwait.c	[iso-8859-1] (original)
> +++ trunk/reactos/ntoskrnl/ob/obwait.c	[iso-8859-1] Mon Sep 28 09:01:11 2015
> @@ -49,12 +49,12 @@
>                           IN BOOLEAN Alertable,
>                           IN PLARGE_INTEGER TimeOut OPTIONAL)
>  {
> PKWAIT_BLOCK WaitBlockArray = NULL;
> PKWAIT_BLOCK WaitBlockArray;
> HANDLE Handles[MAXIMUM_WAIT_OBJECTS], KernelHandle;
> PVOID Objects[MAXIMUM_WAIT_OBJECTS];
> PVOID WaitObjects[MAXIMUM_WAIT_OBJECTS];
> ULONG i = 0, ReferencedObjects = 0, j;
> KPROCESSOR_MODE PreviousMode = ExGetPreviousMode();
> ULONG i, ReferencedObjects, j;
> KPROCESSOR_MODE PreviousMode;
> LARGE_INTEGER SafeTimeOut;
> BOOLEAN LockInUse;
> PHANDLE_TABLE_ENTRY HandleEntry;
>
> @@ -65,31 +65,26 @@
>      NTSTATUS Status;
>      PAGED_CODE();
> /* Enter a critical region since we'll play with handles */
> LockInUse = TRUE;
> KeEnterCriticalRegion();
> /* Check for valid Object Count */
>    if ((ObjectCount > MAXIMUM_WAIT_OBJECTS) || !(ObjectCount))
>    {
>        /* Fail */
>    Status = STATUS_INVALID_PARAMETER_1;
>    goto Quickie;
>    return STATUS_INVALID_PARAMETER_1;
>
> }
>
> /* Check for valid Wait Type */
>    if ((WaitType != WaitAll) && (WaitType != WaitAny))
>    {
>        /* Fail */
>    Status = STATUS_INVALID_PARAMETER_3;
>    goto Quickie;
> }
> /* Enter SEH */
> _SEH2_TRY
> {
>    /* Check if the call came from user mode */
>    if (PreviousMode != KernelMode)
>    return STATUS_INVALID_PARAMETER_3;
> }
> /* Enter SEH for user mode */
> PreviousMode = ExGetPreviousMode();
> if (PreviousMode != KernelMode)
> {
>    /* Enter SEH */
>    _SEH2_TRY

No, this is plain wrong.
This is not because you're in kernel mode that the world is marvelous
and callers trustable.
A caller can pass you buggy address and you HAVE to wrap the
RtlCopyMemory in SEH to make sure that if a buggy address is passed, the
whole system isn't brought down (that's the whole purpose of the copy
after all!).

In case you have a doubt, just put some random:
Status = ZwWaitForMultipleObjects(2, (void **)0x42424242, WaitAll,
FALSE, NULL);
In a kernel component. In w2k3, you'll get Status = STATUS_ACCESS_VIOLATION
In ReactOS, with your changes: BSOD.

Please before doing random changes that you believe are right: do testing.
Alex already told you that.

Cheers,
-- 
Pierre Schweitzer <pierre at reactos.org>
System & Network Administrator
Senior Kernel Developer
ReactOS Deutschland e.V.