Re: [Xen-devel] [PATCH v6] Sanity check xsave area when migrating or restoring from older Xen verions

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.10.14 at 16:53, <dkoch@xxxxxxxxxxx> wrote:
> @@ -2011,15 +2012,8 @@ static int hvm_load_cpu_xsave_states(struct domain *d, 
> hvm_domain_context_t *h)
>                          save_area) + XSTATE_AREA_MIN_SIZE);
>          return -EINVAL;
>      }
> -    size = HVM_CPU_XSAVE_SIZE(xfeature_mask);
> -    if ( desc->length > size )
> -    {
> -        printk(XENLOG_G_WARNING
> -               "HVM%d.%d restore mismatch: xsave length %u > %u\n",
> -               d->domain_id, vcpuid, desc->length, size);
> -        return -EOPNOTSUPP;
> -    }
>      h->cur += sizeof (*desc);
> +    overflow_start = h->cur;

This variable badly named: What it points to is the payload, not the
excess data.

> @@ -2038,10 +2032,23 @@ static int hvm_load_cpu_xsave_states(struct domain 
> *d, hvm_domain_context_t *h)
>      size = HVM_CPU_XSAVE_SIZE(ctxt->xcr0_accum);
>      if ( desc->length > size )
>      {
> +        /*
> +         * Xen-4.3 and older used to send longer-than-needed xsave regions. 

4.3.0 please (also in the patch description), since from 4.3.1
onwards this isn't the case anymore.

> +         * Permit loading the record if the extra data is all zero.
> +         */
> +        for ( i = size; i < desc->length; i++ )
> +        {
> +            if ( h->data[overflow_start + i] )
> +            {
> +                printk(XENLOG_G_WARNING
> +                       "HVM%d.%u restore mismatch: xsave length %#x > %#x 
> and has non-zero data at %#x\n",

"... %#x > %#x (non-zero data at %#x)\n"

> +                       d->domain_id, vcpuid, desc->length, size, i);
> +                return -EOPNOTSUPP;
> +            }
> +        }
>          printk(XENLOG_G_WARNING
> -               "HVM%d.%d restore mismatch: xsave length %u > %u\n",
> +               "HVM%d.%u restore mismatch: xsave length %#x > %#x\n",
>                 d->domain_id, vcpuid, desc->length, size);
> -        return -EOPNOTSUPP;
>      }
>      /* Checking finished */
>  

But the main problem is with the code that follows further down:

    memcpy(v->arch.xsave_area, &ctxt->save_area,
           desc->length - offsetof(struct hvm_hw_cpu_xsave, save_area));

I.e. without this getting changed to use min(desc->length, size)
or something else to this effect you may end up corrupting memory.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel