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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 26/01/18 10:58, Jan Beulich wrote:
>>>> On 26.01.18 at 11:49, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> 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/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.
> As can be implied from the bit not being stored into the actual
> register, we should either consistently store the bit and mask
> it where needed or - following your patch - mask the bit before
> storing a value. This latter approach also has the advantage
> of properly causing failure when e.g. the incoming migration
> stream has the bit set.

Luckily, we won't ever see this bit set in migration.  Introspection
currently needs to detach from the domain around migration, which means
we will never end up having a vcpu paused on its introspection-cr3 exit
with the bit set.

What is however a problem for migration is that we don't know that the
incoming domain has PCID available when auditing %cr3's content, and
there is still a large quantity of work required until we can fix this.

~Andrew

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