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

[Keir Fraser <keir@xxxxxxx>]

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