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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.10.13 at 11:28, "Liu, Jinsong" <jinsong.liu@xxxxxxxxx> wrote:
> Jan Beulich wrote:
>>>>> On 11.10.13 at 20:25, "Liu, Jinsong" <jinsong.liu@xxxxxxxxx> wrote:
>>> +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);
>> 
>> So you continue to use the problematic function, and you also
>> don't remove the problematic code from it's VMX backend - what
>> am I missing here? If there was no room for getting here with
>> !paging_mode_hap(), the problematic code should be removed
>> from vmx_set_uc_mode(). And if we still can get here with
>> !paging_mode_hap(), then what's the purpose of the patch?
>> 
> 
> The new solution w/ PAT depends on whether Intel processor support 'load 
> IA32_PAT' VM-entry control bit (which in very old Intel processors it indeed 
> didn't support it).
> 
> Though I'm pretty sure Intel processors w/ EPT capability also support 'load 
> IA32_PAT' VM-entry control bit, Intel SDM didn't explicitly guarantee it. 
> Hence we still keep old function, though I'm sure it will never be called, 
> just for safety or logic integration.

If any such processor exists, the security issue would still be there
with the patch as is. Hence either use of EPT needs to be
suppressed in that case, or some other solution allowing the broken
code to be deleted (or replaced) needs to be found.

>>> +    /*
>>> +     * 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) &&
>> 
>> has_arch_mmios() has been wrong (too weak) here originally, so I
>> strongly suggest replacing it here as you modify this logic anyway:
>> This check must consider not only non-empty iomem_caps, but also
>> non-empty ioport_caps.
> 
> OK, or, how about has_arch_pdevs()?

Yes, precisely that one.

>>> +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;
>>> +        }
>> 
>> All this is done only on the current vCPU, and I don't see any
>> mechanism by which the other vCPU-s of the domain would
>> similarly get their PAT overridden (yet the whole domain gets
>> transitioned to UC mode). This minimally needs more cleanup (i.e.
>> it should be uniformly just the local vCPU or uniformly the whole
>> domain that is affected here).
>> 
>> And if going the local-vCPU-only route you need to make clear
>> (via code comment I would think) that this is safe to do when
>> two of the guest's vCPU-s with different CR0.CD settings run
>> on two hyperthreads on the same core.
>> 
> 
> For Intel processors CR0.CD operation is per vCPU, not per domain. It's OK 
> running w/ different CR0.CD and different cacheability for different CPUs 
> (For AMD CR0.CD seems should be identical but AMD has not XSA-60 issue we are 
> talking about).
> 
> IA32_PAT is separate MSR per each logic processor, so it's safe running two 
> vCPUs on two hyperthreads on same core. I will add code comments.

So are you saying that no conflicts will arise in TLBs getting
populated with different cachabilty information from the two
hyperthreads?

Jan

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