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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 03/03/17 10:16, Jan Beulich wrote:
>>>> On 02.03.17 at 15:59, <andrew.cooper3@xxxxxxxxxx> wrote:
>> hvm_set_cr{0,4}() are reachable from the emulator, but use
>> hvm_inject_hw_exception() directly.
>>
>> Alter the API to make the callers of hvm_set_cr{0,3,4}() responsible for
>> raising #GP, and apply this change to all existing callers.
> As you're touching CR-write paths only, would you mind changing
> the title to say "writes" instead of "accesses"?

Ok.

>
>> --- 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.  All values should have been
properly audited at vmwrite time, so a failure here should probably be
domain_crash().

>
> Either way
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Ideally I'd prefer not to mix multiple functional changes into a single
patch.

~Andrew

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