[6/6] domctl/vNUMA: avoid arithmetic overflow

Commit Message

Jan Beulich Feb. 5, 2020, 1:17 p.m. UTC

Checking the result of a multiplication against a certain limit has no
sufficient implication on the original value's range. In the case here
it is in particular problematic that while handling the domctl we do

    if ( copy_from_guest(info->vdistance, uinfo->vdistance,
                         nr_vnodes * nr_vnodes) )
        goto vnuma_fail;

which means copying sizeof(unsigned int) * (nr_vnodes * nr_vnodes)
bytes, and the handling of XENMEM_get_vnumainfo similarly has

        tmp.vdistance = xmalloc_array(unsigned int, dom_vnodes * dom_vnodes);

which means allocating sizeof(unsigned int) * (dom_vnodes * dom_vnodes)
bytes, whereas in then goes on doing this:

        memcpy(tmp.vdistance, d->vnuma->vdistance,
               sizeof(*d->vnuma->vdistance) * dom_vnodes * dom_vnodes);

Note the lack of parentheses in the multiplication expression.

Adjust the overflow check, moving the must-not-be-zero one right next to
it to avoid questions on whether there might be division by zero.

Signed-off-by: Jan Beulich <[e-mail]>

Comments

Wei Liu Feb. 5, 2020, 3:13 p.m. UTC | #1

On Wed, Feb 05, 2020 at 02:17:02PM +0100, Jan Beulich wrote:
> Checking the result of a multiplication against a certain limit has no
> sufficient implication on the original value's range. In the case here
> it is in particular problematic that while handling the domctl we do
> 
>     if ( copy_from_guest(info->vdistance, uinfo->vdistance,
>                          nr_vnodes * nr_vnodes) )
>         goto vnuma_fail;
> 
> which means copying sizeof(unsigned int) * (nr_vnodes * nr_vnodes)
> bytes, and the handling of XENMEM_get_vnumainfo similarly has
> 
>         tmp.vdistance = xmalloc_array(unsigned int, dom_vnodes * dom_vnodes);
> 
> which means allocating sizeof(unsigned int) * (dom_vnodes * dom_vnodes)
> bytes, whereas in then goes on doing this:
> 
>         memcpy(tmp.vdistance, d->vnuma->vdistance,
>                sizeof(*d->vnuma->vdistance) * dom_vnodes * dom_vnodes);
> 
> Note the lack of parentheses in the multiplication expression.
> 
> Adjust the overflow check, moving the must-not-be-zero one right next to
> it to avoid questions on whether there might be division by zero.
> 
> Signed-off-by: Jan Beulich <[e-mail]>

Reviewed-by: Wei Liu <[e-mail]>

Patch

--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -263,7 +263,8 @@  static struct vnuma_info *vnuma_alloc(un
      * Check if any of the allocations are bigger than PAGE_SIZE.
      * See XSA-77.
      */
-    if ( nr_vnodes * nr_vnodes > (PAGE_SIZE / sizeof(*vnuma->vdistance)) ||
+    if ( nr_vnodes == 0 ||
+         nr_vnodes > (PAGE_SIZE / sizeof(*vnuma->vdistance) / nr_vnodes) ||
          nr_ranges > (PAGE_SIZE / sizeof(*vnuma->vmemrange)) )
         return ERR_PTR(-EINVAL);
 
@@ -302,7 +303,7 @@  static struct vnuma_info *vnuma_init(con
 
     nr_vnodes = uinfo->nr_vnodes;
 
-    if ( nr_vnodes == 0 || uinfo->nr_vcpus != d->max_vcpus || uinfo->pad != 0 )
+    if ( uinfo->nr_vcpus != d->max_vcpus || uinfo->pad != 0 )
         return ERR_PTR(ret);
 
     info = vnuma_alloc(nr_vnodes, uinfo->nr_vmemranges, d->max_vcpus);