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

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

On 09/10/2026 12:12, Tu Dinh wrote:
> On 08/10/2026 14:05, david ambu wrote:
>> FIELD_OFFSET(T, References[NumberPages]) with a caller-controlled
>> NumberPages can overflow ULONG, producing a Size smaller than the
>> actual allocation needed, leading to a heap buffer overrun.
>>
>> Add a guard rejecting NumberPages == 0 or > XENCONTROL_GNTTAB_MAX_PAGES
>> (1M pages = 4 GB) in both XcGnttabPermitForeignAccess2 and
>> XcGnttabMapForeignPages. Define the limit as a named constant in
>> xencontrol.h.
>>
>> Signed-off-by: david ambu <david.preetham@xxxxxxxxxx>
>> Assisted by Claude Sonnet 4.6
>> ---
>>    include/xencontrol.h        |  3 +++
>>    src/xencontrol/xencontrol.c | 10 ++++++++++
>>    2 files changed, 13 insertions(+)
>>
>> diff --git a/include/xencontrol.h b/include/xencontrol.h
>> index 4b916ec..e95e257 100644
>> --- a/include/xencontrol.h
>> +++ b/include/xencontrol.h
>> @@ -163,6 +163,9 @@ XcEvtchnUnmask(
>>        _In_ ULONG                  LocalPort
>>        );
>>    
>> +/*! Maximum NumberPages accepted by gnttab permit/map functions */
>> +#define XENCONTROL_GNTTAB_MAX_PAGES (1024 * 1024)
>> +
>>    /*! \brief Grant a \a RemoteDomain permission to access local newly 
>> allocated memory pages
>>        \param Xc Xencontrol handle returned by XcOpen()
>>        \param RemoteDomain ID of a remote domain that is being granted access
>> 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.

> 
>> +        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(
>>        BOOL                                        Success;
>>        DWORD                                       Status = 
>> ERROR_OUTOFMEMORY;
>>    
>> +    if (NumberPages == 0 || NumberPages > XENCONTROL_GNTTAB_MAX_PAGES) {
>> +        Log(XLL_ERROR, L"Invalid NumberPages %lu", NumberPages);
>> +        return ERROR_INVALID_PARAMETER;
>> +    }
>> +
>>        Size = (ULONG)FIELD_OFFSET(XENIFACE_GNTTAB_MAP_FOREIGN_PAGES_IN_V2, 
>> References[NumberPages]);
>>        In = malloc(Size);
>>        if (!In)
> 
> 
> 



--
Ngoc Tu Dinh | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech