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, sginsberg(a)svn.reactos.org 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=6…
>  ==============================================================================
>  --- 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.