Re: [Xen-devel] [PATCH] x86/hvm: fix domain crash when CR3 has the noflush bit set

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 01/26/2018 12:27 PM, Jan Beulich wrote:
>>>> On 26.01.18 at 10:39, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -2324,6 +2324,9 @@ int hvm_set_cr3(unsigned long value, bool_t may_defer)
>>          }
>>      }
>>  
>> +    if ( hvm_pcid_enabled(v) ) /* Clear the noflush bit. */
>> +        value &= ((1ull << 63) - 1);
> 
> This needs a proper X86_CR3_* constant defined.

Agreed, I'll add one.

> Did you investigate what it would take to actually _act_ upon the
> bit being set (besides clearing it for the value to be stored to be
> sane)? Afaict it would require handing a boolean through
> paging_update_cr3() -> ... -> {svm,vmx}_update_guest_cr() to
> allow suppressing the hvm_asid_flush_vcpu() etc there.

I did not, I'll look into that as well.

>> --- a/xen/arch/x86/hvm/monitor.c
>> +++ b/xen/arch/x86/hvm/monitor.c
>> @@ -36,6 +36,12 @@ bool hvm_monitor_cr(unsigned int index, unsigned long 
>> value, unsigned long old)
>>      struct arch_domain *ad = &curr->domain->arch;
>>      unsigned int ctrlreg_bitmask = monitor_ctrlreg_bitmask(index);
>>  
>> +    if ( index == 3 && hvm_pcid_enabled(curr) ) /* Clear the noflush bit. */
>> +    {
>> +        value &= ((1ull << 63) - 1);
>> +        old &= ((1ull << 63) - 1);
> 
> Why would "old" need clearing the bit? It being set during write
> doesn't actually get stored into the register, aiui.

v->arch.hvm_vcpu.guest_cr[3] (which is used for "old") does get set in a
number of places:

$ grep -IRn "hvm_vcpu.guest_cr\[3\] ="
arch/x86/hvm/hvm.c:2343:    v->arch.hvm_vcpu.guest_cr[3] = value;
arch/x86/hvm/hvm.c:3914:    v->arch.hvm_vcpu.guest_cr[3] = 0;
arch/x86/hvm/vmx/vmx.c:790:    v->arch.hvm_vcpu.guest_cr[3] = cr3;
arch/x86/hvm/vmx/vmx.c:3522:            v->arch.hvm_vcpu.guest_cr[3] =
v->arch.hvm_vcpu.hw_cr[3];
arch/x86/hvm/domain.c:207:        v->arch.hvm_vcpu.guest_cr[3] = regs->cr3;
arch/x86/hvm/domain.c:258:        v->arch.hvm_vcpu.guest_cr[3] = regs->cr3;
arch/x86/hvm/svm/svm.c:316:    v->arch.hvm_vcpu.guest_cr[3] = c->cr3;
arch/x86/hvm/svm/svm.c:2476:        v->arch.hvm_vcpu.guest_cr[3] =
v->arch.hvm_vcpu.hw_cr[3] =

and I thought it's better to be safe than sorry.


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel