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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.08.18 at 11:59, <paul.durrant@xxxxxxxxxx> 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:
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

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.

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.

Jan



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