Re: [PATCH for-4.5 RFC v2] x86/HVM: Unconditionally crash guests on repeated vmentry failures

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 25/11/14 16:54, Andrew Cooper wrote:
> A failed vmentry is overwhelmingly likely to be caused by corrupt VMC[SB]
> state.  As a result, injecting a fault and retrying the the vmentry is likely
> to fail in the same way.
>
> With this new logic, a guest will unconditionally be crashed if it has
> suffered two repeated vmentry failures, even if it is in usermode.  This
> prevents an infinite loop in Xen where attempting to injecting a #UD is not
> sufficient to prevent the vmentry failure.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> ---
>
> This is RFC as there is still a niggle.  I tested this via a partial revert of
> the XSA-110 fix but the result is quite chatty given a double VMCB dump and
> domain crash.  However, I am not sure we want to make any vmentry failure
> quite, as any vmentry failure constitues a Xen bug.

Furthermore, my testing proves that the attempt to inject a #UD fault
does not rectify the vmentry failure caused by bad CS attributes (in
this case, L and D).

~Andrew

>
> Konrad: A hypervisor infinite loop is quite bad, so I am requesting a release
> ack for this in its eventual form.  An alternative would be to revert
> 28b4baacd5 wholesale, but most of it is good.
> ---
>  xen/arch/x86/hvm/svm/svm.c     |   16 ++++++++++++----
>  xen/arch/x86/hvm/vmx/vmx.c     |   19 +++++++++++++++----
>  xen/include/asm-x86/hvm/vcpu.h |    3 +++
>  3 files changed, 30 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 9398690..c42ec6d 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -90,13 +90,17 @@ static bool_t amd_erratum383_found __read_mostly;
>  static uint64_t osvw_length, osvw_status;
>  static DEFINE_SPINLOCK(osvw_lock);
>  
> -/* Only crash the guest if the problem originates in kernel mode. */
> +/*
> + * Only crash the guest if the problem originates in kernel mode, or we have
> + * had repeated vmentry failures.
> + */
>  static void svm_crash_or_fault(struct vcpu *v)
>  {
> -    if ( vmcb_get_cpl(v->arch.hvm_svm.vmcb) )
> -        hvm_inject_hw_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
> -    else
> +    if ( (v->arch.hvm_vcpu.vmentry_failure_count > 1) ||
> +         (vmcb_get_cpl(v->arch.hvm_svm.vmcb) == 0) )
>          domain_crash(v->domain);
> +    else
> +        hvm_inject_hw_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
>  }
>  
>  void __update_guest_eip(struct cpu_user_regs *regs, unsigned int inst_len)
> @@ -2395,9 +2399,13 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>  
>      if ( unlikely(exit_reason == VMEXIT_INVALID) )
>      {
> +        v->arch.hvm_vcpu.vmentry_failure_count++;
> +
>          svm_vmcb_dump(__func__, vmcb);
>          goto exit_and_crash;
>      }
> +    else
> +        v->arch.hvm_vcpu.vmentry_failure_count = 0;
>  
>      perfc_incra(svmexits, exit_reason);
>  
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 2907afa..e50c8a3 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -134,16 +134,21 @@ static void vmx_vcpu_destroy(struct vcpu *v)
>      passive_domain_destroy(v);
>  }
>  
> -/* Only crash the guest if the problem originates in kernel mode. */
> +/*
> + * Only crash the guest if the problem originates in kernel mode, or we have
> + * had repeated vmentry failures.
> + */
>  static void vmx_crash_or_fault(struct vcpu *v)
>  {
>      struct segment_register ss;
>  
>      vmx_get_segment_register(v, x86_seg_ss, &ss);
> -    if ( ss.attr.fields.dpl )
> -        hvm_inject_hw_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
> -    else
> +
> +    if ( (v->arch.hvm_vcpu.vmentry_failure_count > 1) ||
> +         (ss.attr.fields.dpl == 0) )
>          domain_crash(v->domain);
> +    else
> +        hvm_inject_hw_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
>  }
>  
>  static DEFINE_PER_CPU(struct vmx_msr_state, host_msr_state);
> @@ -2722,7 +2727,13 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>      }
>  
>      if ( unlikely(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY) )
> +    {
> +        v->arch.hvm_vcpu.vmentry_failure_count++;
> +
>          return vmx_failed_vmentry(exit_reason, regs);
> +    }
> +    else
> +        v->arch.hvm_vcpu.vmentry_failure_count = 0;
>  
>      if ( v->arch.hvm_vmx.vmx_realmode )
>      {
> diff --git a/xen/include/asm-x86/hvm/vcpu.h b/xen/include/asm-x86/hvm/vcpu.h
> index 01e0665..3a9d521 100644
> --- a/xen/include/asm-x86/hvm/vcpu.h
> +++ b/xen/include/asm-x86/hvm/vcpu.h
> @@ -159,6 +159,9 @@ struct hvm_vcpu {
>          struct arch_svm_struct svm;
>      } u;
>  
> +    /* Number of repeated vmentry failures. */
> +    unsigned int        vmentry_failure_count;
> +
>      struct tasklet      assert_evtchn_irq_tasklet;
>  
>      struct nestedvcpu   nvcpu;