Re: [Xen-devel] [PATCH v21 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 08 August 2018 11:47
> To: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Paul Durrant
> <Paul.Durrant@xxxxxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>;
> Stefano Stabellini <sstabellini@xxxxxxxxxx>; xen-devel <xen-
> devel@xxxxxxxxxxxxxxxxxxxx>; Konrad Rzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>
> Subject: Re: [PATCH v21 1/2] common: add a new mappable resource type:
> XENMEM_resource_grant_table
> 
> >>> On 08.08.18 at 12:10, <andrew.cooper3@xxxxxxxxxx> wrote:
> > On 08/08/18 10:00, Paul Durrant wrote:
> >> +static int gnttab_get_status_frame_mfn(struct domain *d,
> >> +                                       unsigned long idx, mfn_t *mfn)
> >> +{
> >> +    struct grant_table *gt = d->grant_table;
> >> +
> >> +    ASSERT(gt->gt_version == 2);
> >> +
> >> +    if ( idx >= nr_status_frames(gt) )
> >> +    {
> >> +        unsigned long nr = status_to_grant_frames(idx + 1);
> >
> > Why the +1 ? Won't that cause a failure if you attempt to map the
> > maximum valid index?
> 
> That's the normal index-of-something to count-of-something
> conversion (or else the table may get grown by too little). I've
> instead been considering the badness of overflow here, but
> decided to leave it uncommented as the check further down
> would at least not make this insecure. However, with ...
> 
> >> +
> >> +        if ( nr <= gt->max_grant_frames )
> >> +            gnttab_grow_table(d, nr);
> >
> > You want to capture the return value of grow_table(), which at least
> > distinguishes between ENODEV and ENOMEM.
> >
> >> +
> >> +        if ( idx >= nr_status_frames(gt) )
> >> +            return -EINVAL;
> >
> > This can probably(?) be asserted if gnttab_grow_table() returns
> > successfully.
> 
> ... these two a potential overflow above would then have a
> chance of triggering the assertion you suggest to add.
> 
> As to the grow_table() return value check - I'd prefer if the
> patch here didn't alter original behavior. If we want it altered,
> better in a separate patch.

Ok. I'll leave the return value of gnttab_grow_table() unchecked as-is.

> 
> Jan
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel