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

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

Jan Beulich wrote:
>>>> 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

OK, use aggressive approach while protected by disallowing EPT when 
(!cpu_has_vmx_pat).

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