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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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"?

> --- 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.

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

Jan


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