[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 at 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=69393&r1=69392&r2=69393&view=diff
> ==============================================================================
> --- 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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 3960 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://www.reactos.org/pipermail/ros-dev/attachments/20150928/af81133b/attachment.bin>