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

[Don Koch <dkoch@xxxxxxxxxxx>]

On Thu, 23 Oct 2014 08:38:12 +0100
Jan Beulich <JBeulich@xxxxxxxx> wrote:

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

Wasn't fond of the name, anyway. (I'm horrid at picking variable names.)
Propose changing it to desc_start.

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

OK. I was unaware this had been ported to 4.3.1. Will change.

Are there any versions of 4.2.x that said patch has been backported to?

> > +         * 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"

Shorter, better. Will do.

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

True. Even if it wasn't, it's a nice optimization.

Will change. (Was debating setting desc->length = size at the end of the
check block, but that seems a bit hackish.)

> Jan

-d


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