Re: [PATCH v23 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 09 August 2018 11:24
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; Ian
> Jackson <Ian.Jackson@citrix.com>; Stefano Stabellini
> <sstabellini@kernel.org>; xen-devel <xen-devel@lists.xenproject.org>;
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Tim (Xen.org)
> <tim@xen.org>
> Subject: Re: [PATCH v23 1/2] common: add a new mappable resource type:
> XENMEM_resource_grant_table
> 
> >>> On 09.08.18 at 11:59, <paul.durrant@citrix.com> 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 <jbeulich@suse.com>
> 
> 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
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel