Re: [Xen-devel] [PATCH 1/2] x86/hvm: Don't raise #GP behind the emulators back for CR accesses

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.03.17 at 11:30, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 03/03/17 10:16, Jan Beulich wrote:
>>>>> On 02.03.17 at 15:59, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> --- a/xen/arch/x86/hvm/vmx/vvmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
>>> @@ -1046,9 +1046,18 @@ static void load_shadow_guest_state(struct vcpu *v)
>>>  
>>>      nvcpu->guest_cr[0] = get_vvmcs(v, CR0_READ_SHADOW);
>>>      nvcpu->guest_cr[4] = get_vvmcs(v, CR4_READ_SHADOW);
>>> -    hvm_set_cr0(get_vvmcs(v, GUEST_CR0), 1);
>>> -    hvm_set_cr4(get_vvmcs(v, GUEST_CR4), 1);
>>> -    hvm_set_cr3(get_vvmcs(v, GUEST_CR3), 1);
>>> +
>>> +    rc = hvm_set_cr0(get_vvmcs(v, GUEST_CR0), 1);
>>> +    if ( rc == X86EMUL_EXCEPTION )
>>> +        hvm_inject_hw_exception(TRAP_gp_fault, 0);
>>> +
>>> +    rc = hvm_set_cr4(get_vvmcs(v, GUEST_CR4), 1);
>>> +    if ( rc == X86EMUL_EXCEPTION )
>>> +        hvm_inject_hw_exception(TRAP_gp_fault, 0);
>>> +
>>> +    rc = hvm_set_cr3(get_vvmcs(v, GUEST_CR3), 1);
>>> +    if ( rc == X86EMUL_EXCEPTION )
>>> +        hvm_inject_hw_exception(TRAP_gp_fault, 0);
>> While indeed not a change in behavior, this multiple raising of #GP
>> is so wrong that I wonder whether it shouldn't be fixed while you're
>> touching it: Simply accumulate the need to raise #GP, and do so
>> once at the end.
>>
>>> @@ -1237,9 +1246,17 @@ static void load_vvmcs_host_state(struct vcpu *v)
>>>          __vmwrite(vmcs_h2g_field[i].guest_field, r);
>>>      }
>>>  
>>> -    hvm_set_cr0(get_vvmcs(v, HOST_CR0), 1);
>>> -    hvm_set_cr4(get_vvmcs(v, HOST_CR4), 1);
>>> -    hvm_set_cr3(get_vvmcs(v, HOST_CR3), 1);
>>> +    rc = hvm_set_cr0(get_vvmcs(v, HOST_CR0), 1);
>>> +    if ( rc == X86EMUL_EXCEPTION )
>>> +        hvm_inject_hw_exception(TRAP_gp_fault, 0);
>>> +
>>> +    rc = hvm_set_cr4(get_vvmcs(v, HOST_CR4), 1);
>>> +    if ( rc == X86EMUL_EXCEPTION )
>>> +        hvm_inject_hw_exception(TRAP_gp_fault, 0);
>>> +
>>> +    rc = hvm_set_cr3(get_vvmcs(v, HOST_CR3), 1);
>>> +    if ( rc == X86EMUL_EXCEPTION )
>>> +        hvm_inject_hw_exception(TRAP_gp_fault, 0);
>> Same here then obviously.
> 
> In both cases, raising #GP at all is wrong.

Of course.

>  All values should have been
> properly audited at vmwrite time, so a failure here should probably be
> domain_crash().

That would be better than #DF.

>> Either way
>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Ideally I'd prefer not to mix multiple functional changes into a single
> patch.

As said - either way.

Jan


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