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

[Tu Dinh <ngoc-tu.dinh@xxxxxxxxxx>]

On 13/10/2026 11:20, Rafał Wojdyła wrote:
> On 13.10.2026 11:12, Tu Dinh wrote:
>> On 13/10/2026 11:10, Rafał Wojdyła wrote:
>>> On 13.10.2026 10:10, Tu Dinh wrote:
>>>> On 13/10/2026 10:01, Owen Smith wrote:
>>>>>>>> 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.
>>>> I think in general it'd be nice to bound the usermode grant consumption
>>>> to the actual free grant table space (and reserve some certain amount
>>>> for the kernel), but that doesn't look simple to implement.
>>>>
>>>> SeLockMemoryPrivilege may also be worth checking in the caller path of
>>>> some ioctls.
>>> I'll finally be sending my rework of the grant/map code soon, hopefully
>>> this week. Got caught up with other projects, but I'm testing it now. It
>>> includes new interface versions with bounded batched maps/unmaps, per-
>>> page error reporting and other improvements.
>> Great to hear. I can share our WIP test suite of evtchn/gnttab ioctls as
>> well as a few private fixes we've been working on on our side.
> That would be nice, I was thinking of adding more comprehensive tests to 
> the upstream repo.

I've sent patches for issues revealed by our test suite.

Unfortunately the tests are written in C# as a companion of our own 
guest agent, so they're unlikely to fit upstream as it is. They're 
posted here https://github.com/xcp-ng/win-pv-drivers/tree/dnt/next if 
you want to check them out (they should be merged in our repo eventually)

>>> What's the policy on retaining old xenbus interface versions?
>> Currently, old xenbus interface versions have to be retained
>> indefinitely, because since b0b9cc139600 old drivers could still bind to
>> devices without being aware of compatibility changes.
> 



--
Ngoc Tu Dinh | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech