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

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

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.
> > 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.
--

Rafał Wojdyła

Invisible Things Lab