WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] Re: [PATCH] x86: add strictly sanity check for XSAVE/XRS

To: "Kay, Allen M" <allen.m.kay@xxxxxxxxx>, "Wei, Gang" <gang.wei@xxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Subject: Re: [Xen-devel] Re: [PATCH] x86: add strictly sanity check for XSAVE/XRSTOR
From: Keir Fraser <keir@xxxxxxx>
Date: Sat, 19 Feb 2011 08:35:23 +0000
Cc: "wei.huang2@xxxxxxx" <wei.huang2@xxxxxxx>
Delivery-date: Sat, 19 Feb 2011 00:36:25 -0800
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:sender:user-agent:date:subject:from:to:cc :message-id:thread-topic:thread-index:in-reply-to:mime-version :content-type:content-transfer-encoding; bh=cHbvU9Xmai5hhi/yNtuvJg/cF+LGH6hW5j0HAO2eQcc=; b=T8OFXeOXOU9lP/CsHTSFhOSdW/SK84a2L17qGdwAShz7eHG5d7TkVF4TMzTY4y5889 8eRdlGIYPVuYh9KmXX9/bfe0Gb5851XyKkQOnBI+GwtfODO6YOlAj60dGMAPmUNNrOsp o5u2cG5t7nEhOj7SRPlz/oY+dLBi+EDrx1pZE=
Domainkey-signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=sender:user-agent:date:subject:from:to:cc:message-id:thread-topic :thread-index:in-reply-to:mime-version:content-type :content-transfer-encoding; b=TyZqUYBOCKi8OMmwEggpnA+ottoY6RnjRhuERqaWwbWttEafwTB+S8IsqgrT6nwolV /cSCidSNRNnKJVUShhsUY09rIbSZRWb/pysvPrAwqd+9vKymRSvmOVJPZ1iLCSmo17Cf mhhve+ejIWJC58XPF6s//gZAh7StQXBIDn+IY=
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <987664A83D2D224EAE907B061CE93D53019D58FB7D@xxxxxxxxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
Thread-index: AcvOSPtNY1iWQhS8TKOXCx9nhUW9CAAy6UcgAAmsdQUAJbhjMAAPb83Y
Thread-topic: [Xen-devel] Re: [PATCH] x86: add strictly sanity check for XSAVE/XRSTOR
User-agent: Microsoft-Entourage/12.28.0.101117
On 19/02/2011 01:21, "Kay, Allen M" <allen.m.kay@xxxxxxxxx> wrote:

> This patch was meant to address that checking cpu_has_xsave is not enough.

Well obviously it is, else the new checks would not be coded as assertions.

> Since it only checks the availability of the feature but it does not check
> whether memory has allocated properly or not.  It is possible that xsave can
> be used without memory being properly allocated and results in clobbering of
> memory.  We have already encountered two random boot failures caused by xsave
> patch in the past due to this so we want to put some safeguard to ensure this
> will not happen again.
> 
> Maybe the proper thing to do is to have a new function call xsave_enabled(),
> this function then checks for whether memory has allocated properly in
> addition to checking cpu_has_xsave.
> 
> What do you think or do you have a better suggestion?

Yeah, a new function xsave_enabled() which encapsulates the check of
cpu_has_xsave, plus your new assertions, would be acceptable I think.

 -- Keir

> Allen
> 
> -----Original Message-----
> From: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
> [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxx] On Behalf Of Keir Fraser
> Sent: Thursday, February 17, 2011 11:13 PM
> To: Wei, Gang; xen-devel@xxxxxxxxxxxxxxxxxxx
> Cc: wei.huang2@xxxxxxx
> Subject: [Xen-devel] Re: [PATCH] x86: add strictly sanity check for
> XSAVE/XRSTOR
> 
> On 18/02/2011 02:45, "Wei, Gang" <gang.wei@xxxxxxxxx> wrote:
> 
>> This patch is trying to make issues around XSAVE/XRSTOR induced in future
>> easy
>> to be exposed.
> 
> The fact that xsave_alloc_save_area() is called unconditionally on the vcpu
> allocation path suffices I think. It's pretty easy to eyeball that no
> successfully initialised non-idle vcpu can have an xsave area smaller than
> min_size.
> 
> I like assertions a lot, but not carpet bombed all over the code.
> 
>  -- Keir
> 
> 
>> Jimmy
>> 
>> x86: add strictly sanity check for XSAVE/XRSTOR
>> 
>> Signed-off-by: Wei Gang <gang.wei@xxxxxxxxx>
>> 
>> diff -r 137ad3347504 xen/arch/x86/domctl.c
>> --- a/xen/arch/x86/domctl.c Mon Feb 14 17:02:55 2011 +0000
>> +++ b/xen/arch/x86/domctl.c Fri Feb 18 16:00:41 2011 +0800
>> @@ -1604,8 +1604,13 @@ void arch_get_info_guest(struct vcpu *v,
>>  
>>      /* Fill legacy context from xsave area first */
>>      if ( cpu_has_xsave )
>> +    {
>> +        ASSERT(xsave_cntxt_size >= XSAVE_AREA_MIN_SIZE);
>> +        ASSERT(v->arch.xsave_area);
>> +
>>          memcpy(v->arch.xsave_area, &v->arch.guest_context.fpu_ctxt,
>>                 sizeof(v->arch.guest_context.fpu_ctxt));
>> +    }
>>  
>>      if ( !is_pv_32on64_domain(v->domain) )
>>          memcpy(c.nat, &v->arch.guest_context, sizeof(*c.nat)); diff -r
>> 137ad3347504 xen/arch/x86/hvm/hvm.c
>> --- a/xen/arch/x86/hvm/hvm.c Mon Feb 14 17:02:55 2011 +0000
>> +++ b/xen/arch/x86/hvm/hvm.c Fri Feb 18 16:03:23 2011 +0800
>> @@ -777,6 +777,9 @@ static int hvm_load_cpu_ctxt(struct doma
>>      {
>>          struct xsave_struct *xsave_area = v->arch.xsave_area;
>>  
>> +        ASSERT(xsave_cntxt_size >= XSAVE_AREA_MIN_SIZE);
>> +        ASSERT(v->arch.xsave_area);
>> +
>>          memcpy(v->arch.xsave_area, ctxt.fpu_regs, sizeof(ctxt.fpu_regs));
>>          xsave_area->xsave_hdr.xstate_bv = XSTATE_FP_SSE;
>>          v->arch.xcr0_accum = XSTATE_FP_SSE; @@ -834,6 +837,7 @@ static int
>> hvm_save_cpu_xsave_states(str
>>      if ( !cpu_has_xsave )
>>          return 0;   /* do nothing */
>>  
>> +    ASSERT(xsave_cntxt_size >= XSAVE_AREA_MIN_SIZE);
>>      for_each_vcpu ( d, v )
>>      {
>>          if ( _hvm_init_entry(h, CPU_XSAVE_CODE, v->vcpu_id,
>> HVM_CPU_XSAVE_SIZE) ) @@ -846,8 +850,12 @@ static int
>> hvm_save_cpu_xsave_states(str
>>          ctxt->xcr0 = v->arch.xcr0;
>>          ctxt->xcr0_accum = v->arch.xcr0_accum;
>>          if ( v->fpu_initialised )
>> +        {
>> +            ASSERT(v->arch.xsave_area);
>> +
>>              memcpy(&ctxt->save_area,
>>                  v->arch.xsave_area, xsave_cntxt_size);
>> +        }
>>      }
>>  
>>      return 0;
>> @@ -873,6 +881,9 @@ static int hvm_load_cpu_xsave_states(str
>>          gdprintk(XENLOG_ERR, "HVM restore: domain has no vcpu %u\n",
>> vcpuid);
>>          return -EINVAL;
>>      }
>> +
>> +    ASSERT(xsave_cntxt_size >= XSAVE_AREA_MIN_SIZE);
>> +    ASSERT(v->arch.xsave_area);
>>  
>>      /* Customized checking for entry since our entry is of variable length
>> */
>>      desc = (struct hvm_save_descriptor *)&h->data[h->cur]; diff -r
>> 137ad3347504 xen/arch/x86/i387.c
>> --- a/xen/arch/x86/i387.c Mon Feb 14 17:02:55 2011 +0000
>> +++ b/xen/arch/x86/i387.c Fri Feb 18 16:00:41 2011 +0800
>> @@ -71,6 +71,9 @@ void setup_fpu(struct vcpu *v)
>>  
>>      if ( cpu_has_xsave )
>>      {
>> +        ASSERT(xsave_cntxt_size >= XSAVE_AREA_MIN_SIZE);
>> +        ASSERT(v->arch.xsave_area);
>> +
>>          /*
>>           * XCR0 normally represents what guest OS set. In case of Xen
>> itself,
>>           * we set all supported feature mask before doing save/restore.
>> @@ -118,6 +121,9 @@ void save_init_fpu(struct vcpu *v)
>>  
>>      if ( cpu_has_xsave )
>>      {
>> +        ASSERT(xsave_cntxt_size >= XSAVE_AREA_MIN_SIZE);
>> +        ASSERT(v->arch.xsave_area);
>> +
>>          /* XCR0 normally represents what guest OS set. In case of Xen
>> itself,
>>           * we set all accumulated feature mask before doing save/restore.
>>           */
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel