RE: [PATCH] More IOCTL parameter checks

[Owen Smith <owen.smith@xxxxxxxxxx>]

-----Original Message-----
From: win-pv-devel <win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of 
Durrant, Paul
Sent: 28 April 2022 11:44
To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
Subject: Re: [PATCH] More IOCTL parameter checks
> @@ -618,10 +621,12 @@ fail10:
>   
>   fail9:
>       Error("Fail9\n");
> -    ASSERT(NT_SUCCESS(XENBUS_GNTTAB(UnmapForeignPages,
> -                                    &Fdo->GnttabInterface,
> -                                    Context->Address
> -                                    )));
> +    {
> +        NTSTATUS status2 = XENBUS_GNTTAB(UnmapForeignPages,
> +                                         &Fdo->GnttabInterface,
> +                                         Context->Address);
> +        ASSERT(NT_SUCCESS(status2));

Why?


If the definition of ASSERT is changed, its possible that pages would not get 
unmapped in this failure case.
It feels cleaner to me to separate the cleanup operation and the ASSERTion the 
cleanup succeeded.

> +    }
>   
>   fail8:
>       Error("Fail8\n");
<snip>
 
> diff --git a/src/xeniface/ioctls.c b/src/xeniface/ioctls.c index 
> a624bd1..20a7669 100644
> --- a/src/xeniface/ioctls.c
> +++ b/src/xeniface/ioctls.c
> @@ -48,9 +48,9 @@ __CaptureUserBuffer(
>       NTSTATUS Status;
>       PVOID TempBuffer = NULL;
>   
> -    if (Length == 0) {
> +    if (Length == 0 || Buffer == NULL) {
>           *CapturedBuffer = NULL;
> -        return STATUS_SUCCESS;
> +        return STATUS_INVALID_PARAMETER;

That's a semantic change. Do we want that?

   Paul

Currently, passing Length=0 will result in a success and CapturedBuffer=NULL. 
The next operations after all calls to __CaptureUserBuffer is dereferencing 
CapturedBuffer.

Currently, passing Length!=0 and Buffer=NULL should cause ProbeForRead to throw 
an exception, which results in CapturedBuffer=NULL, but __CaptureUserBuffer 
returning a fail code.

This change will ensure that Length=0 or Buffer=NULL will both fail, and avoid 
any potential for dereferencing a NULL pointer in the operations after all 
calls to __CaptureUserBuffer


Owen

>       }
>   
>       Status = STATUS_NO_MEMORY;