> -----Original Message----- > From: Jan Beulich [mailto:[email protected]] > Sent: 09 August 2018 11:24 > To: Paul Durrant <[email protected]> > Cc: Andrew Cooper <[email protected]>; Wei Liu > <[email protected]>; George Dunlap <[email protected]>; Ian > Jackson <[email protected]>; Stefano Stabellini > <[email protected]>; xen-devel <[email protected]>; > Konrad Rzeszutek Wilk <[email protected]>; Tim (Xen.org) > <[email protected]> > Subject: Re: [PATCH v23 1/2] common: add a new mappable resource type: > XENMEM_resource_grant_table > > >>> On 09.08.18 at 11:59, <[email protected]> wrote: > > +static int gnttab_get_status_frame_mfn(struct domain *d, > > + unsigned long idx, mfn_t *mfn) > > +{ > > + const struct grant_table *gt = d->grant_table; > > + > > + ASSERT(gt->gt_version == 2); > > + > > + if ( idx >= nr_status_frames(gt) ) > > + { > > + unsigned long nr_status; > > + unsigned long nr_grant; > > + > > + nr_status = idx + 1; /* sufficient frames to make idx valid */ > > + > > + if ( nr_status <= nr_status_frames(gt) ) /* overflow check */ > > + return -EINVAL; > > Still pretty odd a check, even if now at least correct. Why not simply > check nr_status to be zero? Let me know if you're fine with me making > this adjustment while committing:
Yes, I'm happy for you to adjust overflow checking as you see fit. (I thought just doing a similar check as the outer if was kind of more obvious, but given it's a + 1 then clearly checking against 0 would be fine too). > Reviewed-by: Jan Beulich <[email protected]> > > That said though - idx being -1UL is not really "invalid". In an abstract > world it simply means a fully populated table of maximum size. But of > course the table can't grow this large in practice, because each entry > is more than one byte (i.e. we'd still get -EINVAL further down). > > > + nr_grant = status_to_grant_frames(nr_status); > > Irrespective of the R-b above: This is the real source of possible > overflows, as here nr_status gets multiplied by a value larger than 1. > I therefore wonder whether it wouldn't be better to check here > that the reverse translation yields nr_status again. Once again I'd > be fine adding this while committing, provided you agree. > Yes, that sounds like a worthwhile check. > Otoh I'm not convinced all this overflow checking does much good > here anyway: Anyone setting the maximum table size so absurdly > high that this would start to matter is going to have bigger trouble > anyway afaict. True. Paul > > Jan > _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
