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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.10.13 at 18:47, "Liu, Jinsong" <jinsong.liu@xxxxxxxxx> wrote:
> Jan Beulich wrote:
>>>>> 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.
>> 
> 
> Well, I have to go back to Intel library:) a long search for Intel VT 
> history: both 'load IA32_PAT' and 'EPT' features does not exist at Intel SDM 
> Order Number: 253669-026US (Feb 2008), but co-exist as early as Intel SDM 
> Order 
> Number: 253669-027US (July 2008).
> 
> Per my understanding 'load IA32_PAT' co-exist/co-work with EPT:
> 1. shadow don't need 'load IA32_PAT' feature
> 2. EPT, from its earliest version, has 'iPAT' bit to control guest PAT and 
> host EPT memory type combining -- without 'load IA32_PAT' how can guest PAT 
> memory type take effect?
> 
> So we have 2 choices for 'load IA32_PAT':
> 1. aggressive choice: remove vmx_set_uc_mode logic
>     if ( shadow )
>         drop shadows and new ones would be created on demand;
>     else  /* Intel EPT */
>         IA32_PAT solution
> 2. conservative choice: as what the patch did, use if (!cpu_has_vmx_pat) to 
> keep logic integration -- it didn't make thing worse.

As said before - the code in question needs to go away or be
modified in a way that's safe. If there are extra restrictions
that need to be enforced (like cpu_has_vmx_pat being a prereq
for EPT+non-snooped-IOMMU), then so be it (i.e. your patch
would need to be extended to do that).

Jan


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