Re: [PATCH] Fix double-free on error condition in GnttabPermitForeignAccess()

[Rafał Wojdyła <omeg@xxxxxxxxxxxxxxxxxxxxxx>]

On 7 mar 2024 10:36, Owen Smith wrote:
> Is there other cleanup needed in other fail cases?
> Context->Grants, Context->KernelVa, Context->Mdl are allocated and pages 
> could need revoking access during the cleanup operation in a failure case.
> Calling GnttabStopSharing() should do this, but its not called in the 
> case where GnttabPermitForeignAccess returns a failure code.
> Owen
Good catch, the GnttabStopSharing() call should remain there to clean up 
the contents of the context. I'll send the updated version.
> On Thu, Mar 7, 2024 at 8:51 AM Rafał Wojdyła 
> <omeg@xxxxxxxxxxxxxxxxxxxxxx <mailto:omeg@xxxxxxxxxxxxxxxxxxxxxx>> wrote:
>     XENIFACE_GNTTAB_CONTEXT associated with the request was incorrectly
>     freed
>     by GnttabPermitForeignAccess() when a failure occured. The context
>     is also
>     freed by the parent function, IoctlGnttabPermitForeignAccess(),
>     which led
>     to a double-free and kernel heap corruption.
>
>     Signed-off-by: Rafał Wojdyła <omeg@xxxxxxxxxxxxxxxxxxxxxx
>     <mailto:omeg@xxxxxxxxxxxxxxxxxxxxxx>>
>     ---
>       src/xeniface/ioctl_gnttab.c | 7 -------
>       1 file changed, 7 deletions(-)
>
>     diff --git a/src/xeniface/ioctl_gnttab.c b/src/xeniface/ioctl_gnttab.c
>     index 8ab2099..083c29d 100644
>     --- a/src/xeniface/ioctl_gnttab.c
>     +++ b/src/xeniface/ioctl_gnttab.c
>     @@ -303,13 +303,6 @@ fail2:
>
>       fail1:
>           Error("Fail1\n");
>     -    GnttabStopSharing(Fdo, Context, Page);
>     -
>     -    if (Context != NULL) {
>     -        RtlZeroMemory(Context, sizeof(*Context));
>     -        __FreePoolWithTag(Context, XENIFACE_POOL_TAG);
>     -    }
>     -
>           return Status;
>       }
>
>     -- 
>     2.43.0.windows.1
--
Rafał Wojdyła
Invisible Things Lab

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature