Re: [PATCH 10/10] xencontrol: guard against NumberPages overflow in gnttab size calculation

[Owen Smith <owen.smith@xxxxxxxxxx>]

>>> diff --git a/src/xencontrol/xencontrol.c b/src/xencontrol/xencontrol.c
>>> index a68dcf6..8f21867 100644
>>> --- a/src/xencontrol/xencontrol.c
>>> +++ b/src/xencontrol/xencontrol.c
>>> @@ -407,6 +407,11 @@ XcGnttabPermitForeignAccess2(
>>>        BOOL                                            Success;
>>>        DWORD                                           Status;
>>>
>>> +    if (NumberPages == 0 || NumberPages > XENCONTROL_GNTTAB_MAX_PAGES) {
>>
>> I believe the boundary condition of NumberPages ==
>> XENCONTROL_GNTTAB_MAX_PAGES still causes an overflow? So
>> XENCONTROL_GNTTAB_MAX_PAGES should be (1024 * 1024 - 1) instead.
>
>MAXULONG / PAGE_SIZE would be a better formulation still, only problem
>being PAGE_SIZE is not defined for usermode targets.

Maximum number of pages allowed is also checked in IoctlGnttabMapForeignPages
against a hard coded 1024*1024.
It would be good to have the same constant/#define referred to in both places.

Eventually, this ends up in FdoPciHoleAllocate or FdoMemoryHoleAllocate, 
depending
on the "UseMemoryHole" setting.
FdoPciHoleAllocate is limited to 4GiB - PAGE_SIZE bytes (i.e. 1024*1024 - 1 
pages)
FdoMemoryHoleAllocate's allocations doesnt specify a maximum size, but the 
chances
of allocating a contiguous 4GB set of pages would be problematic.

Its likely this check is excessively large, but I'm unsure as to what the safe 
correct
maximum would be.

>
>>
>>> +        Log(XLL_ERROR, L"Invalid NumberPages %lu", NumberPages);
>>> +        return ERROR_INVALID_PARAMETER;
>>> +    }
>>> +
>>>        In.RemoteDomain = RemoteDomain;
>>>        In.Address = Address;
>>>        In.NumberPages = NumberPages;
>>> @@ -508,6 +513,11 @@ XcGnttabMapForeignPages(

Owen