Re: [PATCH] More IOCTL parameter checks

["Durrant, Paul" <xadimgnik@xxxxxxxxx>]

On 28/04/2022 13:10, Owen Smith wrote:
> -----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.
Ok. Fair enough.

> > +    }
> >    
> >    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
Thanks for the explanation; it would be nice to see that in the commit 
message :-)
  Paul

> Owen
>
> >        }
> >    
> >        Status = STATUS_NO_MEMORY;