Re: [Xen-devel] [PATCH] XSA-60 security hole: cr0.cd handling

["Liu, Jinsong" <jinsong.liu@xxxxxxxxx>]

Sorry, just find a little bug of this patch -- We should not let guest direct 
access IA32_PAT MSR when cr0.cd setting.
I will fix it and then re-send out later.

Thanks,
Jinsong

Liu, Jinsong wrote:
> From 1bdeea81e0ec151ec10c3341802f814956a2ff8d Mon Sep 17 00:00:00 2001
> From: Liu Jinsong <jinsong.liu@xxxxxxxxx>
> Date: Fri, 11 Oct 2013 23:22:05 +0800
> Subject: [PATCH] XSA-60 security hole: cr0.cd handling
> 
> Recently Oracle developers found a Xen security issue as DOS
> affecting, 
> named as XSA-60. Please refer
> http://xenbits.xen.org/xsa/advisory-60.html 
> 
> Basically it involves how to handle guest cr0.cd setting, which under
> some environment it consumes much time resulting in DOS-like behavior.
> 
> This patch solves XSA-60 security hole:
> 1. For guest w/o VT-d, and for guest with VT-d but snooped, Xen need
> do nothing, since hardware snoop mechanism has ensured cache
> coherency. 
> 
> 2. For guest with VT-d but non-snooped, cache coherency can not be
> guaranteed by h/w snoop, therefore it need emulate UC type to guest:
> 2.1). if it works w/ Intel EPT, set guest IA32_PAT fields as UC so
> that 
> guest memory type are all UC.
> 2.2). if it works w/ shadow, it works as currently Xen does, dropping
> all shadows so that any new ones would be created on demand w/ UC.
> 
> Singed-off-by: Liu Jinsong <jinsong.liu@xxxxxxxxx>
> ---
>  xen/arch/x86/hvm/hvm.c            |   71
>  ++++++++++++++++++++++--------------- xen/arch/x86/hvm/vmx/vmx.c    
>  |   57 ++++++++++++++++++++++++++++- xen/include/asm-x86/hvm/hvm.h  
>  |    1 + xen/include/asm-x86/hvm/support.h |    2 +
>  4 files changed, 100 insertions(+), 31 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index de81e45..0e14f49 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1703,6 +1703,36 @@ int hvm_mov_from_cr(unsigned int cr, unsigned
>      int gpr) return X86EMUL_UNHANDLEABLE;
>  }
> 
> +void hvm_handle_cd_traditional(struct vcpu *v, unsigned long value)
> +{
> +    if ( (value & X86_CR0_CD) && !(value & X86_CR0_NW) )
> +    {
> +        /* Entering no fill cache mode. */
> +        spin_lock(&v->domain->arch.hvm_domain.uc_lock);
> +        v->arch.hvm_vcpu.cache_mode = NO_FILL_CACHE_MODE;
> +
> +        if ( !v->domain->arch.hvm_domain.is_in_uc_mode )
> +        {
> +            /* Flush physical caches. */
> +            on_each_cpu(local_flush_cache, NULL, 1);
> +            hvm_set_uc_mode(v, 1);
> +        }
> +        spin_unlock(&v->domain->arch.hvm_domain.uc_lock);
> +    }
> +    else if ( !(value & (X86_CR0_CD | X86_CR0_NW)) &&
> +              (v->arch.hvm_vcpu.cache_mode == NO_FILL_CACHE_MODE) )
> +    {
> +        /* Exit from no fill cache mode. */
> +        spin_lock(&v->domain->arch.hvm_domain.uc_lock);
> +        v->arch.hvm_vcpu.cache_mode = NORMAL_CACHE_MODE;
> +
> +        if ( domain_exit_uc_mode(v) )
> +            hvm_set_uc_mode(v, 0);
> +
> +        spin_unlock(&v->domain->arch.hvm_domain.uc_lock);
> +    }
> +}
> +
>  int hvm_set_cr0(unsigned long value)
>  {
>      struct vcpu *v = current;
> @@ -1786,35 +1816,18 @@ int hvm_set_cr0(unsigned long value)
>          }
>      }
> 
> -    if ( has_arch_mmios(v->domain) )
> -    {
> -        if ( (value & X86_CR0_CD) && !(value & X86_CR0_NW) )
> -        {
> -            /* Entering no fill cache mode. */
> -            spin_lock(&v->domain->arch.hvm_domain.uc_lock);
> -            v->arch.hvm_vcpu.cache_mode = NO_FILL_CACHE_MODE;
> -
> -            if ( !v->domain->arch.hvm_domain.is_in_uc_mode )
> -            {
> -                /* Flush physical caches. */
> -                on_each_cpu(local_flush_cache, NULL, 1);
> -                hvm_set_uc_mode(v, 1);
> -            }
> -            spin_unlock(&v->domain->arch.hvm_domain.uc_lock);
> -        }
> -        else if ( !(value & (X86_CR0_CD | X86_CR0_NW)) &&
> -                  (v->arch.hvm_vcpu.cache_mode ==
> NO_FILL_CACHE_MODE) ) 
> -        {
> -            /* Exit from no fill cache mode. */
> -            spin_lock(&v->domain->arch.hvm_domain.uc_lock);
> -            v->arch.hvm_vcpu.cache_mode = NORMAL_CACHE_MODE;
> -
> -            if ( domain_exit_uc_mode(v) )
> -                hvm_set_uc_mode(v, 0);
> -
> -            spin_unlock(&v->domain->arch.hvm_domain.uc_lock);
> -        }
> -    }
> +    /*
> +     * When cr0.cd setting
> +     * 1. For guest w/o VT-d, and for guest with VT-d but snooped,
> Xen need not +     * do anything, since hardware snoop mechanism has
> ensured cache coherency; +     * 2. For guest with VT-d but
> non-snooped, cache coherency cannot be +     * guaranteed by h/w so
> need emulate UC memory type to guest. +     */
> +    if ( ((value ^ old_value) & X86_CR0_CD) &&
> +           has_arch_mmios(v->domain) &&
> +           iommu_enabled && !iommu_snoop &&
> +           hvm_funcs.handle_cd )
> +        hvm_funcs.handle_cd(v, value);
> 
>      v->arch.hvm_vcpu.guest_cr[0] = value;
>      hvm_update_guest_cr(v, 0);
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 9ca8632..1e0269b 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -642,6 +642,10 @@ static void vmx_ctxt_switch_to(struct vcpu *v)
>              __invept(INVEPT_SINGLE_CONTEXT, ept_get_eptp(ept_data),
>      0); }
> 
> +    /* For guest cr0.cd setting, do not use potentially polluted
> cache */ +    if ( unlikely(v->arch.hvm_vcpu.cache_mode ==
> NO_FILL_CACHE_MODE) ) +        wbinvd();
> +
>      vmx_restore_guest_msrs(v);
>      vmx_restore_dr(v);
>  }
> @@ -908,7 +912,8 @@ static unsigned long
> vmx_get_shadow_gs_base(struct vcpu *v) 
> 
>  static int vmx_set_guest_pat(struct vcpu *v, u64 gpat)
>  {
> -    if ( !cpu_has_vmx_pat || !paging_mode_hap(v->domain) )
> +    if ( !cpu_has_vmx_pat || !paging_mode_hap(v->domain) ||
> +        unlikely(v->arch.hvm_vcpu.cache_mode == NO_FILL_CACHE_MODE) )
>          return 0;
> 
>      vmx_vmcs_enter(v);
> @@ -919,7 +924,8 @@ static int vmx_set_guest_pat(struct vcpu *v, u64
> gpat) 
> 
>  static int vmx_get_guest_pat(struct vcpu *v, u64 *gpat)
>  {
> -    if ( !cpu_has_vmx_pat || !paging_mode_hap(v->domain) )
> +    if ( !cpu_has_vmx_pat || !paging_mode_hap(v->domain) ||
> +        unlikely(v->arch.hvm_vcpu.cache_mode == NO_FILL_CACHE_MODE) )
>          return 0;
> 
>      vmx_vmcs_enter(v);
> @@ -1400,6 +1406,52 @@ static void vmx_set_uc_mode(struct vcpu *v)
>      hvm_asid_flush_vcpu(v);
>  }
> 
> +static void vmx_handle_cd(struct vcpu *v, unsigned long value)
> +{
> +    if ( !paging_mode_hap(v->domain) ||
> +         unlikely(!cpu_has_vmx_pat) )    /* Too old for EPT, just
> for safe */ +    {
> +        /*
> +         * For shadow, 'load IA32_PAT' VM-entry control is 0, so it
> cannot +         * set guest memory type as UC via IA32_PAT. Xen drop
> all shadows +         * so that any new ones would be created on
> demand. +         */
> +        hvm_handle_cd_traditional(v, value);
> +    }
> +    else
> +    {
> +        u64 *pat = &v->arch.hvm_vcpu.pat_cr;
> +
> +        if ( value & X86_CR0_CD )
> +        {
> +            /*
> +             * For EPT, set guest IA32_PAT fields as UC so that guest
> +             * memory type are all UC.
> +             */
> +            u64 uc_pat =
> +                ((uint64_t)PAT_TYPE_UNCACHABLE)       |       /*
> PAT0 */ +                ((uint64_t)PAT_TYPE_UNCACHABLE << 8)  |     
> /* PAT1 */ +                ((uint64_t)PAT_TYPE_UNCACHABLE << 16) |  
> /* PAT2 */ +                ((uint64_t)PAT_TYPE_UNCACHABLE << 24) |  
> /* PAT3 */ +                ((uint64_t)PAT_TYPE_UNCACHABLE << 32) |  
> /* PAT4 */ +                ((uint64_t)PAT_TYPE_UNCACHABLE << 40) |  
> /* PAT5 */ +                ((uint64_t)PAT_TYPE_UNCACHABLE << 48) |  
> /* PAT6 */ +                ((uint64_t)PAT_TYPE_UNCACHABLE << 56);   
> /* PAT7 */ +
> +            vmx_get_guest_pat(v, pat);
> +            vmx_set_guest_pat(v, uc_pat);
> +
> +            wbinvd();
> +            v->arch.hvm_vcpu.cache_mode = NO_FILL_CACHE_MODE;
> +        }
> +        else
> +        {
> +            vmx_set_guest_pat(v, *pat);
> +            v->arch.hvm_vcpu.cache_mode = NORMAL_CACHE_MODE;
> +        }
> +    }
> +}
> +
>  static void vmx_set_info_guest(struct vcpu *v)
>  {
>      unsigned long intr_shadow;
> @@ -1558,6 +1610,7 @@ static struct hvm_function_table __initdata
>      vmx_function_table = { .msr_read_intercept   =
>      vmx_msr_read_intercept, .msr_write_intercept  =
>      vmx_msr_write_intercept, .invlpg_intercept     =
> vmx_invlpg_intercept, +    .handle_cd            = vmx_handle_cd,
>      .set_uc_mode          = vmx_set_uc_mode,
>      .set_info_guest       = vmx_set_info_guest,
>      .set_rdtsc_exiting    = vmx_set_rdtsc_exiting,
> diff --git a/xen/include/asm-x86/hvm/hvm.h
> b/xen/include/asm-x86/hvm/hvm.h 
> index 3376418..6306587 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -156,6 +156,7 @@ struct hvm_function_table {
>      int (*msr_read_intercept)(unsigned int msr, uint64_t
>      *msr_content); int (*msr_write_intercept)(unsigned int msr,
>      uint64_t msr_content); void (*invlpg_intercept)(unsigned long
> vaddr); +    void (*handle_cd)(struct vcpu *v, unsigned long value);
>      void (*set_uc_mode)(struct vcpu *v);
>      void (*set_info_guest)(struct vcpu *v);
>      void (*set_rdtsc_exiting)(struct vcpu *v, bool_t);
> diff --git a/xen/include/asm-x86/hvm/support.h
> b/xen/include/asm-x86/hvm/support.h index 7ddc806..b291168 100644
> --- a/xen/include/asm-x86/hvm/support.h
> +++ b/xen/include/asm-x86/hvm/support.h
> @@ -130,6 +130,8 @@ void hvm_rdtsc_intercept(struct cpu_user_regs
> *regs); 
> 
>  int __must_check hvm_handle_xsetbv(u32 index, u64 new_bv);
> 
> +void hvm_handle_cd_traditional(struct vcpu *v, unsigned long value);
> +
>  /* These functions all return X86EMUL return codes. */
>  int hvm_set_efer(uint64_t value);
>  int hvm_set_cr0(unsigned long value);


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