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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan



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