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

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

[Andrew Cooper]

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.

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;
-- 
1.7.10.4

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

[Andrew Cooper]

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;

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

[Jan Beulich]

>>> On 25.11.14 at 17:54, <andrew.cooper3@citrix.com> wrote:
> 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.

I think that double printing would be tolerable, but I've had yet
another idea: Couldn't we make the second exception a #DF,
thus having the guest killed via triple fault in the worst case at
the third recurring failure (via hvm_combine_hw_exceptions())?

Also your test results point out that we're delivering such an
exception with wrong context to the guest: Machine state should
match that before the results from the emulation got committed.
But doing so would be a pretty significant change for almost no
benefit.

Jan

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

[Andrew Cooper]

On 25/11/14 17:25, Jan Beulich wrote:
>>>> On 25.11.14 at 17:54, <andrew.cooper3@citrix.com> wrote:
>> 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.
> I think that double printing would be tolerable, but I've had yet
> another idea: Couldn't we make the second exception a #DF,
> thus having the guest killed via triple fault in the worst case at
> the third recurring failure (via hvm_combine_hw_exceptions())?

That still won't catch a large class of vmentry failures from bad host
state.  There needs to be some cutoff where we simply give up and crash
the domain.  In the end, this is just an exercise in how much we attempt
to recover in the case that we have already hit a hypervisor bug, and
can't be completely certain about any state.

Furthermore, guest userspace being able to exploit a Xen bug to result
in a triple fault is no better than an instant domain_crash(), from a
VMs point of view.  However, from a toolstacks point of view, it is even
worse, as malicious userspace could tie up toolstack resource in
domain_kill() and domain_create() (as a triple fault is modelled as a
clean reboot).

>
> Also your test results point out that we're delivering such an
> exception with wrong context to the guest: Machine state should
> match that before the results from the emulation got committed.
> But doing so would be a pretty significant change for almost no
> benefit.

Agreed, on both counts.

~Andrew

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

[Jan Beulich]

>>> Andrew Cooper <andrew.cooper3@citrix.com> 11/25/14 7:14 PM >>>
>On 25/11/14 17:25, Jan Beulich wrote:
>>>>> On 25.11.14 at 17:54, <andrew.cooper3@citrix.com> wrote:
>>> 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.
>> I think that double printing would be tolerable, but I've had yet
>> another idea: Couldn't we make the second exception a #DF,
>> thus having the guest killed via triple fault in the worst case at
>> the third recurring failure (via hvm_combine_hw_exceptions())?
>
>That still won't catch a large class of vmentry failures from bad host
>state.  There needs to be some cutoff where we simply give up and crash
>the domain.  In the end, this is just an exercise in how much we attempt
>to recover in the case that we have already hit a hypervisor bug, and
>can't be completely certain about any state.
>
>Furthermore, guest userspace being able to exploit a Xen bug to result
>in a triple fault is no better than an instant domain_crash(), from a
>VMs point of view.  However, from a toolstacks point of view, it is even
>worse, as malicious userspace could tie up toolstack resource in
>domain_kill() and domain_create() (as a triple fault is modelled as a
>clean reboot).

Right - the more I think about it, the more I'm inclined to take your v1 patch...

Jan

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

[Andrew Cooper]

On 26/11/14 16:53, Jan Beulich wrote:
>>>> Andrew Cooper <andrew.cooper3@citrix.com> 11/25/14 7:14 PM >>>
>> On 25/11/14 17:25, Jan Beulich wrote:
>>>>>> On 25.11.14 at 17:54, <andrew.cooper3@citrix.com> wrote:
>>>> 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.
>>> I think that double printing would be tolerable, but I've had yet
>>> another idea: Couldn't we make the second exception a #DF,
>>> thus having the guest killed via triple fault in the worst case at
>>> the third recurring failure (via hvm_combine_hw_exceptions())?
>> That still won't catch a large class of vmentry failures from bad host
>> state.  There needs to be some cutoff where we simply give up and crash
>> the domain.  In the end, this is just an exercise in how much we attempt
>> to recover in the case that we have already hit a hypervisor bug, and
>> can't be completely certain about any state.
>>
>> Furthermore, guest userspace being able to exploit a Xen bug to result
>> in a triple fault is no better than an instant domain_crash(), from a
>> VMs point of view.  However, from a toolstacks point of view, it is even
>> worse, as malicious userspace could tie up toolstack resource in
>> domain_kill() and domain_create() (as a triple fault is modelled as a
>> clean reboot).
> Right - the more I think about it, the more I'm inclined to take your v1 patch...
>
> Jan
>

My v1 patch only fixes the VMX side of things.  SVM doesn't explicitly
identify a failed vmentry and lets it fall into the default case of the
vmexit handler.  As such, with v1, the infinite loop still affects AMD
hardware.

~Andrew

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

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 5041 bytes --]

>>> On 26.11.14 at 18:43, <andrew.cooper3@citrix.com> wrote:
> My v1 patch only fixes the VMX side of things.  SVM doesn't explicitly
> identify a failed vmentry and lets it fall into the default case of the
> vmexit handler.  As such, with v1, the infinite loop still affects AMD
> hardware.

Right; I should have said "something along the lines of v1". An SVM
part is needed, but that should probably extend beyond what you
proposed in v2: There are a number of "goto exit_and_crash"
statements ahead of where you place your addition. I think they
all need to be treated similarly.

I therefore think we should revert the VMX part of 28b4baacd5
and make SVM behavior consistent with what results for VMX:
Crash the guest unconditionally on VMEXIT_INVALID, without
looking for recurring VM entry failures. See below/attached.

Jan

x86/HVM: prevent infinite VM entry retries

This reverts the VMX side of commit 28b4baac ("x86/HVM: don't crash
guest upon problems occurring in user mode") and gets SVM in line with
the resulting VMX behavior. This is because Andrew validly says

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

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2338,6 +2338,7 @@ void svm_vmexit_handler(struct cpu_user_
         struct nestedvcpu *nv = &vcpu_nestedhvm(v);
         struct vmcb_struct *ns_vmcb = nv->nv_vvmcx;
         uint64_t exitinfo1, exitinfo2;
+        bool_t crash = 0;
 
         paging_update_nestedmode(v);
 
@@ -2371,13 +2372,16 @@ void svm_vmexit_handler(struct cpu_user_
                 goto out;
             case NESTEDHVM_VMEXIT_FATALERROR:
                 gdprintk(XENLOG_ERR, "unexpected nestedsvm_vmexit() error\n");
-                goto exit_and_crash;
-
+                crash = 1;
+                break;
             default:
                 BUG();
             case NESTEDHVM_VMEXIT_ERROR:
                 break;
             }
+            if ( crash )
+                break;
+            /* fall through */
         case NESTEDHVM_VMEXIT_ERROR:
             gdprintk(XENLOG_ERR,
                 "nestedsvm_check_intercepts() returned NESTEDHVM_VMEXIT_ERROR\n");
@@ -2385,18 +2389,25 @@ void svm_vmexit_handler(struct cpu_user_
         case NESTEDHVM_VMEXIT_FATALERROR:
             gdprintk(XENLOG_ERR,
                 "unexpected nestedsvm_check_intercepts() error\n");
-            goto exit_and_crash;
+            crash = 1;
+            break;
         default:
             gdprintk(XENLOG_INFO, "nestedsvm_check_intercepts() returned %i\n",
                 nsret);
-            goto exit_and_crash;
+            crash = 1;
+            break;
         }
+
+        if ( unlikely(crash) && exit_reason != VMEXIT_INVALID )
+            goto exit_and_crash;
     }
 
     if ( unlikely(exit_reason == VMEXIT_INVALID) )
     {
+        gdprintk(XENLOG_ERR, "invalid VMCB state:\n");
         svm_vmcb_dump(__func__, vmcb);
-        goto exit_and_crash;
+        domain_crash(v->domain);
+        return;
     }
 
     perfc_incra(svmexits, exit_reason);
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -134,18 +134,6 @@ static void vmx_vcpu_destroy(struct vcpu
     passive_domain_destroy(v);
 }
 
-/* Only crash the guest if the problem originates in kernel mode. */
-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
-        domain_crash(v->domain);
-}
-
 static DEFINE_PER_CPU(struct vmx_msr_state, host_msr_state);
 
 static const u32 msr_index[] =
@@ -2520,7 +2508,7 @@ static void vmx_failed_vmentry(unsigned 
     vmcs_dump_vcpu(curr);
     printk("**************************************\n");
 
-    vmx_crash_or_fault(curr);
+    domain_crash(curr->domain);
 }
 
 void vmx_enter_realmode(struct cpu_user_regs *regs)
@@ -3173,8 +3161,19 @@ void vmx_vmexit_handler(struct cpu_user_
     /* fall through */
     default:
     exit_and_crash:
-        gdprintk(XENLOG_WARNING, "Bad vmexit (reason %#lx)\n", exit_reason);
-        vmx_crash_or_fault(v);
+        {
+            struct segment_register ss;
+
+            gdprintk(XENLOG_WARNING, "Bad vmexit (reason %#lx)\n",
+                     exit_reason);
+
+            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
+                domain_crash(v->domain);
+        }
         break;
     }
 



[-- Attachment #2: x86-HVM-prevent-infinite-VMentry-loop.patch --]
[-- Type: text/plain, Size: 4185 bytes --]

x86/HVM: prevent infinite VM entry retries

This reverts the VMX side of commit 28b4baac ("x86/HVM: don't crash
guest upon problems occurring in user mode") and gets SVM in line with
the resulting VMX behavior. This is because Andrew validly says

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

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2338,6 +2338,7 @@ void svm_vmexit_handler(struct cpu_user_
         struct nestedvcpu *nv = &vcpu_nestedhvm(v);
         struct vmcb_struct *ns_vmcb = nv->nv_vvmcx;
         uint64_t exitinfo1, exitinfo2;
+        bool_t crash = 0;
 
         paging_update_nestedmode(v);
 
@@ -2371,13 +2372,16 @@ void svm_vmexit_handler(struct cpu_user_
                 goto out;
             case NESTEDHVM_VMEXIT_FATALERROR:
                 gdprintk(XENLOG_ERR, "unexpected nestedsvm_vmexit() error\n");
-                goto exit_and_crash;
-
+                crash = 1;
+                break;
             default:
                 BUG();
             case NESTEDHVM_VMEXIT_ERROR:
                 break;
             }
+            if ( crash )
+                break;
+            /* fall through */
         case NESTEDHVM_VMEXIT_ERROR:
             gdprintk(XENLOG_ERR,
                 "nestedsvm_check_intercepts() returned NESTEDHVM_VMEXIT_ERROR\n");
@@ -2385,18 +2389,25 @@ void svm_vmexit_handler(struct cpu_user_
         case NESTEDHVM_VMEXIT_FATALERROR:
             gdprintk(XENLOG_ERR,
                 "unexpected nestedsvm_check_intercepts() error\n");
-            goto exit_and_crash;
+            crash = 1;
+            break;
         default:
             gdprintk(XENLOG_INFO, "nestedsvm_check_intercepts() returned %i\n",
                 nsret);
-            goto exit_and_crash;
+            crash = 1;
+            break;
         }
+
+        if ( unlikely(crash) && exit_reason != VMEXIT_INVALID )
+            goto exit_and_crash;
     }
 
     if ( unlikely(exit_reason == VMEXIT_INVALID) )
     {
+        gdprintk(XENLOG_ERR, "invalid VMCB state:\n");
         svm_vmcb_dump(__func__, vmcb);
-        goto exit_and_crash;
+        domain_crash(v->domain);
+        return;
     }
 
     perfc_incra(svmexits, exit_reason);
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -134,18 +134,6 @@ static void vmx_vcpu_destroy(struct vcpu
     passive_domain_destroy(v);
 }
 
-/* Only crash the guest if the problem originates in kernel mode. */
-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
-        domain_crash(v->domain);
-}
-
 static DEFINE_PER_CPU(struct vmx_msr_state, host_msr_state);
 
 static const u32 msr_index[] =
@@ -2520,7 +2508,7 @@ static void vmx_failed_vmentry(unsigned 
     vmcs_dump_vcpu(curr);
     printk("**************************************\n");
 
-    vmx_crash_or_fault(curr);
+    domain_crash(curr->domain);
 }
 
 void vmx_enter_realmode(struct cpu_user_regs *regs)
@@ -3173,8 +3161,19 @@ void vmx_vmexit_handler(struct cpu_user_
     /* fall through */
     default:
     exit_and_crash:
-        gdprintk(XENLOG_WARNING, "Bad vmexit (reason %#lx)\n", exit_reason);
-        vmx_crash_or_fault(v);
+        {
+            struct segment_register ss;
+
+            gdprintk(XENLOG_WARNING, "Bad vmexit (reason %#lx)\n",
+                     exit_reason);
+
+            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
+                domain_crash(v->domain);
+        }
         break;
     }
 

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

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

[Tim Deegan]

At 08:42 +0000 on 27 Nov (1417074133), Jan Beulich wrote:
> >>> On 26.11.14 at 18:43, <andrew.cooper3@citrix.com> wrote:
> > My v1 patch only fixes the VMX side of things.  SVM doesn't explicitly
> > identify a failed vmentry and lets it fall into the default case of the
> > vmexit handler.  As such, with v1, the infinite loop still affects AMD
> > hardware.
> 
> Right; I should have said "something along the lines of v1". An SVM
> part is needed, but that should probably extend beyond what you
> proposed in v2: There are a number of "goto exit_and_crash"
> statements ahead of where you place your addition. I think they
> all need to be treated similarly.
> 
> I therefore think we should revert the VMX part of 28b4baacd5
> and make SVM behavior consistent with what results for VMX:
> Crash the guest unconditionally on VMEXIT_INVALID, without
> looking for recurring VM entry failures. See below/attached.
> 
> Jan
> 
> x86/HVM: prevent infinite VM entry retries
> 
> This reverts the VMX side of commit 28b4baac ("x86/HVM: don't crash
> guest upon problems occurring in user mode") and gets SVM in line with
> the resulting VMX behavior. This is because Andrew validly says
> 
> "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."
> 
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Looking at the SVM side, AFAICT you're trying to filter out
VMEXIT_INVALID exits that couldn't be handled by nested SVM, but I
think it's fine just to always crash on nested-SVM failures: we know
the guest wasn't in user mode because it successfully executed VMRUN.
And looking at it, the other users of that label are for unexpected
debugging exits, which can't be caused by the guest userspace either.

So how about this for the SVM side, reverting to crashing for
everything except new, unsupported exit types?

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 8aca6e6..8d28578 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2675,16 +2675,18 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
         break;
 
     default:
-    exit_and_crash:
         gdprintk(XENLOG_ERR, "unexpected VMEXIT: exit reason = %#"PRIx64", "
                  "exitinfo1 = %#"PRIx64", exitinfo2 = %#"PRIx64"\n",
                  exit_reason, 
                  (u64)vmcb->exitinfo1, (u64)vmcb->exitinfo2);
-        if ( vmcb_get_cpl(vmcb) )
+        if ( vmcb_get_cpl(vmcb) ) {
             hvm_inject_hw_exception(TRAP_invalid_op,
                                     HVM_DELIVER_NO_ERROR_CODE);
-        else
-            domain_crash(v->domain);
+            break;
+        }
+        /* else fall through */
+    exit_and_crash:
+        domain_crash(v->domain);
         break;
     }

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

[Jan Beulich]

>>> On 27.11.14 at 11:26, <tim@xen.org> wrote:
> At 08:42 +0000 on 27 Nov (1417074133), Jan Beulich wrote:
>> >>> On 26.11.14 at 18:43, <andrew.cooper3@citrix.com> wrote:
>> > My v1 patch only fixes the VMX side of things.  SVM doesn't explicitly
>> > identify a failed vmentry and lets it fall into the default case of the
>> > vmexit handler.  As such, with v1, the infinite loop still affects AMD
>> > hardware.
>> 
>> Right; I should have said "something along the lines of v1". An SVM
>> part is needed, but that should probably extend beyond what you
>> proposed in v2: There are a number of "goto exit_and_crash"
>> statements ahead of where you place your addition. I think they
>> all need to be treated similarly.
>> 
>> I therefore think we should revert the VMX part of 28b4baacd5
>> and make SVM behavior consistent with what results for VMX:
>> Crash the guest unconditionally on VMEXIT_INVALID, without
>> looking for recurring VM entry failures. See below/attached.
>> 
>> Jan
>> 
>> x86/HVM: prevent infinite VM entry retries
>> 
>> This reverts the VMX side of commit 28b4baac ("x86/HVM: don't crash
>> guest upon problems occurring in user mode") and gets SVM in line with
>> the resulting VMX behavior. This is because Andrew validly says
>> 
>> "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."
>> 
>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Looking at the SVM side, AFAICT you're trying to filter out
> VMEXIT_INVALID exits that couldn't be handled by nested SVM, but I
> think it's fine just to always crash on nested-SVM failures: we know
> the guest wasn't in user mode because it successfully executed VMRUN.
> And looking at it, the other users of that label are for unexpected
> debugging exits, which can't be caused by the guest userspace either.
> 
> So how about this for the SVM side, reverting to crashing for
> everything except new, unsupported exit types?

Generally a good idea, but there are two paths to exit_and_crash
(for VMEXIT_EXCEPTION_DB and VMEXIT_EXCEPTION_BP) which I
think would better crash only conditionally.

> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2675,16 +2675,18 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>          break;
>  
>      default:
> -    exit_and_crash:
>          gdprintk(XENLOG_ERR, "unexpected VMEXIT: exit reason = %#"PRIx64", 
> "
>                   "exitinfo1 = %#"PRIx64", exitinfo2 = %#"PRIx64"\n",
>                   exit_reason, 
>                   (u64)vmcb->exitinfo1, (u64)vmcb->exitinfo2);
> -        if ( vmcb_get_cpl(vmcb) )
> +        if ( vmcb_get_cpl(vmcb) ) {
>              hvm_inject_hw_exception(TRAP_invalid_op,
>                                      HVM_DELIVER_NO_ERROR_CODE);
> -        else
> -            domain_crash(v->domain);
> +            break;
> +        }
> +        /* else fall through */
> +    exit_and_crash:
> +        domain_crash(v->domain);
>          break;
>      }

Additionally this re-arrangement would make some domain_crash()
invocations "silent" (i.e. no other accompanying message), but that's
of course easy to fix.

And finally, if doing it that way we might better remove the
exit_and_crash label altogether and call domain_crash() directly
in the places we mean it to be called.

Jan

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

[Andrew Cooper]

On 27/11/14 11:16, Jan Beulich wrote:
>>>> On 27.11.14 at 11:26, <tim@xen.org> wrote:
>> At 08:42 +0000 on 27 Nov (1417074133), Jan Beulich wrote:
>>>>>> On 26.11.14 at 18:43, <andrew.cooper3@citrix.com> wrote:
>>>> My v1 patch only fixes the VMX side of things.  SVM doesn't explicitly
>>>> identify a failed vmentry and lets it fall into the default case of the
>>>> vmexit handler.  As such, with v1, the infinite loop still affects AMD
>>>> hardware.
>>> Right; I should have said "something along the lines of v1". An SVM
>>> part is needed, but that should probably extend beyond what you
>>> proposed in v2: There are a number of "goto exit_and_crash"
>>> statements ahead of where you place your addition. I think they
>>> all need to be treated similarly.
>>>
>>> I therefore think we should revert the VMX part of 28b4baacd5
>>> and make SVM behavior consistent with what results for VMX:
>>> Crash the guest unconditionally on VMEXIT_INVALID, without
>>> looking for recurring VM entry failures. See below/attached.
>>>
>>> Jan
>>>
>>> x86/HVM: prevent infinite VM entry retries
>>>
>>> This reverts the VMX side of commit 28b4baac ("x86/HVM: don't crash
>>> guest upon problems occurring in user mode") and gets SVM in line with
>>> the resulting VMX behavior. This is because Andrew validly says
>>>
>>> "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."
>>>
>>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Looking at the SVM side, AFAICT you're trying to filter out
>> VMEXIT_INVALID exits that couldn't be handled by nested SVM, but I
>> think it's fine just to always crash on nested-SVM failures: we know
>> the guest wasn't in user mode because it successfully executed VMRUN.
>> And looking at it, the other users of that label are for unexpected
>> debugging exits, which can't be caused by the guest userspace either.
>>
>> So how about this for the SVM side, reverting to crashing for
>> everything except new, unsupported exit types?
> Generally a good idea, but there are two paths to exit_and_crash
> (for VMEXIT_EXCEPTION_DB and VMEXIT_EXCEPTION_BP) which I
> think would better crash only conditionally.

I would agree - these two should only be conditional crashes. If the
intercepts are enabled, userspace is perfectly capable of generating the
conditions behind the back of the kernel.

~Andrew

>
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -2675,16 +2675,18 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>>          break;
>>  
>>      default:
>> -    exit_and_crash:
>>          gdprintk(XENLOG_ERR, "unexpected VMEXIT: exit reason = %#"PRIx64", 
>> "
>>                   "exitinfo1 = %#"PRIx64", exitinfo2 = %#"PRIx64"\n",
>>                   exit_reason, 
>>                   (u64)vmcb->exitinfo1, (u64)vmcb->exitinfo2);
>> -        if ( vmcb_get_cpl(vmcb) )
>> +        if ( vmcb_get_cpl(vmcb) ) {
>>              hvm_inject_hw_exception(TRAP_invalid_op,
>>                                      HVM_DELIVER_NO_ERROR_CODE);
>> -        else
>> -            domain_crash(v->domain);
>> +            break;
>> +        }
>> +        /* else fall through */
>> +    exit_and_crash:
>> +        domain_crash(v->domain);
>>          break;
>>      }
> Additionally this re-arrangement would make some domain_crash()
> invocations "silent" (i.e. no other accompanying message), but that's
> of course easy to fix.
>
> And finally, if doing it that way we might better remove the
> exit_and_crash label altogether and call domain_crash() directly
> in the places we mean it to be called.
>
> Jan
>

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

[Tim Deegan]

At 11:16 +0000 on 27 Nov (1417083414), Jan Beulich wrote:
> >>> On 27.11.14 at 11:26, <tim@xen.org> wrote:
> > At 08:42 +0000 on 27 Nov (1417074133), Jan Beulich wrote:
> >> >>> On 26.11.14 at 18:43, <andrew.cooper3@citrix.com> wrote:
> >> > My v1 patch only fixes the VMX side of things.  SVM doesn't explicitly
> >> > identify a failed vmentry and lets it fall into the default case of the
> >> > vmexit handler.  As such, with v1, the infinite loop still affects AMD
> >> > hardware.
> >> 
> >> Right; I should have said "something along the lines of v1". An SVM
> >> part is needed, but that should probably extend beyond what you
> >> proposed in v2: There are a number of "goto exit_and_crash"
> >> statements ahead of where you place your addition. I think they
> >> all need to be treated similarly.
> >> 
> >> I therefore think we should revert the VMX part of 28b4baacd5
> >> and make SVM behavior consistent with what results for VMX:
> >> Crash the guest unconditionally on VMEXIT_INVALID, without
> >> looking for recurring VM entry failures. See below/attached.
> >> 
> >> Jan
> >> 
> >> x86/HVM: prevent infinite VM entry retries
> >> 
> >> This reverts the VMX side of commit 28b4baac ("x86/HVM: don't crash
> >> guest upon problems occurring in user mode") and gets SVM in line with
> >> the resulting VMX behavior. This is because Andrew validly says
> >> 
> >> "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."
> >> 
> >> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > Looking at the SVM side, AFAICT you're trying to filter out
> > VMEXIT_INVALID exits that couldn't be handled by nested SVM, but I
> > think it's fine just to always crash on nested-SVM failures: we know
> > the guest wasn't in user mode because it successfully executed VMRUN.
> > And looking at it, the other users of that label are for unexpected
> > debugging exits, which can't be caused by the guest userspace either.
> > 
> > So how about this for the SVM side, reverting to crashing for
> > everything except new, unsupported exit types?
> 
> Generally a good idea, but there are two paths to exit_and_crash
> (for VMEXIT_EXCEPTION_DB and VMEXIT_EXCEPTION_BP) which I
> think would better crash only conditionally.

Those are catching Xen bugs -- we don't (or at least shouldn't) enable
those exit types when the debugger is not attached.  I think that,
like with the p2m ENOMEM case, turning them into #UD is not really
helpful.  But if you prefer, those could be made into 'goto default'
to preserve the current behaviour, which would also retain the
debugging output.

> And finally, if doing it that way we might better remove the
> exit_and_crash label altogether and call domain_crash() directly
> in the places we mean it to be called.

Indeed.  How's this, then?

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 8aca6e6..213fd6e 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2362,7 +2362,8 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
                 goto out;
             case NESTEDHVM_VMEXIT_FATALERROR:
                 gdprintk(XENLOG_ERR, "unexpected nestedsvm_vmexit() error\n");
-                goto exit_and_crash;
+                domain_crash(v->domain);
+                goto out;
 
             default:
                 BUG();
@@ -2376,18 +2377,21 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
         case NESTEDHVM_VMEXIT_FATALERROR:
             gdprintk(XENLOG_ERR,
                 "unexpected nestedsvm_check_intercepts() error\n");
-            goto exit_and_crash;
+            domain_crash(v->domain);
+            goto out;
         default:
             gdprintk(XENLOG_INFO, "nestedsvm_check_intercepts() returned %i\n",
                 nsret);
-            goto exit_and_crash;
+            domain_crash(v->domain);
+            goto out;
         }
     }
 
     if ( unlikely(exit_reason == VMEXIT_INVALID) )
     {
         svm_vmcb_dump(__func__, vmcb);
-        goto exit_and_crash;
+        domain_crash(v->domain);
+        goto out;
     }
 
     perfc_incra(svmexits, exit_reason);
@@ -2422,13 +2426,13 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
 
     case VMEXIT_EXCEPTION_DB:
         if ( !v->domain->debugger_attached )
-            goto exit_and_crash;
+            goto unexpected_exit_type;
         domain_pause_for_debugger();
         break;
 
     case VMEXIT_EXCEPTION_BP:
         if ( !v->domain->debugger_attached )
-            goto exit_and_crash;
+            goto unexpected_exit_type;
         /* AMD Vol2, 15.11: INT3, INTO, BOUND intercepts do not update RIP. */
         if ( (inst_len = __get_instruction_length(v, INSTR_INT3)) == 0 )
             break;
@@ -2675,7 +2679,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
         break;
 
     default:
-    exit_and_crash:
+    unexpected_exit_type:
         gdprintk(XENLOG_ERR, "unexpected VMEXIT: exit reason = %#"PRIx64", "
                  "exitinfo1 = %#"PRIx64", exitinfo2 = %#"PRIx64"\n",
                  exit_reason, 

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

[Jan Beulich]

>>> On 27.11.14 at 12:29, <tim@xen.org> wrote:
> At 11:16 +0000 on 27 Nov (1417083414), Jan Beulich wrote:
>> >>> On 27.11.14 at 11:26, <tim@xen.org> wrote:
>> > At 08:42 +0000 on 27 Nov (1417074133), Jan Beulich wrote:
>> >> >>> On 26.11.14 at 18:43, <andrew.cooper3@citrix.com> wrote:
>> >> > My v1 patch only fixes the VMX side of things.  SVM doesn't explicitly
>> >> > identify a failed vmentry and lets it fall into the default case of the
>> >> > vmexit handler.  As such, with v1, the infinite loop still affects AMD
>> >> > hardware.
>> >> 
>> >> Right; I should have said "something along the lines of v1". An SVM
>> >> part is needed, but that should probably extend beyond what you
>> >> proposed in v2: There are a number of "goto exit_and_crash"
>> >> statements ahead of where you place your addition. I think they
>> >> all need to be treated similarly.
>> >> 
>> >> I therefore think we should revert the VMX part of 28b4baacd5
>> >> and make SVM behavior consistent with what results for VMX:
>> >> Crash the guest unconditionally on VMEXIT_INVALID, without
>> >> looking for recurring VM entry failures. See below/attached.
>> >> 
>> >> Jan
>> >> 
>> >> x86/HVM: prevent infinite VM entry retries
>> >> 
>> >> This reverts the VMX side of commit 28b4baac ("x86/HVM: don't crash
>> >> guest upon problems occurring in user mode") and gets SVM in line with
>> >> the resulting VMX behavior. This is because Andrew validly says
>> >> 
>> >> "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."
>> >> 
>> >> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> > 
>> > Looking at the SVM side, AFAICT you're trying to filter out
>> > VMEXIT_INVALID exits that couldn't be handled by nested SVM, but I
>> > think it's fine just to always crash on nested-SVM failures: we know
>> > the guest wasn't in user mode because it successfully executed VMRUN.
>> > And looking at it, the other users of that label are for unexpected
>> > debugging exits, which can't be caused by the guest userspace either.
>> > 
>> > So how about this for the SVM side, reverting to crashing for
>> > everything except new, unsupported exit types?
>> 
>> Generally a good idea, but there are two paths to exit_and_crash
>> (for VMEXIT_EXCEPTION_DB and VMEXIT_EXCEPTION_BP) which I
>> think would better crash only conditionally.
> 
> Those are catching Xen bugs -- we don't (or at least shouldn't) enable
> those exit types when the debugger is not attached.  I think that,
> like with the p2m ENOMEM case, turning them into #UD is not really
> helpful.  But if you prefer, those could be made into 'goto default'
> to preserve the current behaviour, which would also retain the
> debugging output.
> 
>> And finally, if doing it that way we might better remove the
>> exit_and_crash label altogether and call domain_crash() directly
>> in the places we mean it to be called.
> 
> Indeed.  How's this, then?

Looks good - if you don't mind I'll replace the SVM part of the earlier
patch with this, add your S-o-b alongside mine, and send again for
final review.

Jan

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

[Tim Deegan]

At 11:38 +0000 on 27 Nov (1417084691), Jan Beulich wrote:
> >>> On 27.11.14 at 12:29, <tim@xen.org> wrote:
> > At 11:16 +0000 on 27 Nov (1417083414), Jan Beulich wrote:
> >> >>> On 27.11.14 at 11:26, <tim@xen.org> wrote:
> >> > At 08:42 +0000 on 27 Nov (1417074133), Jan Beulich wrote:
> >> >> >>> On 26.11.14 at 18:43, <andrew.cooper3@citrix.com> wrote:
> >> >> > My v1 patch only fixes the VMX side of things.  SVM doesn't explicitly
> >> >> > identify a failed vmentry and lets it fall into the default case of the
> >> >> > vmexit handler.  As such, with v1, the infinite loop still affects AMD
> >> >> > hardware.
> >> >> 
> >> >> Right; I should have said "something along the lines of v1". An SVM
> >> >> part is needed, but that should probably extend beyond what you
> >> >> proposed in v2: There are a number of "goto exit_and_crash"
> >> >> statements ahead of where you place your addition. I think they
> >> >> all need to be treated similarly.
> >> >> 
> >> >> I therefore think we should revert the VMX part of 28b4baacd5
> >> >> and make SVM behavior consistent with what results for VMX:
> >> >> Crash the guest unconditionally on VMEXIT_INVALID, without
> >> >> looking for recurring VM entry failures. See below/attached.
> >> >> 
> >> >> Jan
> >> >> 
> >> >> x86/HVM: prevent infinite VM entry retries
> >> >> 
> >> >> This reverts the VMX side of commit 28b4baac ("x86/HVM: don't crash
> >> >> guest upon problems occurring in user mode") and gets SVM in line with
> >> >> the resulting VMX behavior. This is because Andrew validly says
> >> >> 
> >> >> "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."
> >> >> 
> >> >> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> > 
> >> > Looking at the SVM side, AFAICT you're trying to filter out
> >> > VMEXIT_INVALID exits that couldn't be handled by nested SVM, but I
> >> > think it's fine just to always crash on nested-SVM failures: we know
> >> > the guest wasn't in user mode because it successfully executed VMRUN.
> >> > And looking at it, the other users of that label are for unexpected
> >> > debugging exits, which can't be caused by the guest userspace either.
> >> > 
> >> > So how about this for the SVM side, reverting to crashing for
> >> > everything except new, unsupported exit types?
> >> 
> >> Generally a good idea, but there are two paths to exit_and_crash
> >> (for VMEXIT_EXCEPTION_DB and VMEXIT_EXCEPTION_BP) which I
> >> think would better crash only conditionally.
> > 
> > Those are catching Xen bugs -- we don't (or at least shouldn't) enable
> > those exit types when the debugger is not attached.  I think that,
> > like with the p2m ENOMEM case, turning them into #UD is not really
> > helpful.  But if you prefer, those could be made into 'goto default'
> > to preserve the current behaviour, which would also retain the
> > debugging output.
> > 
> >> And finally, if doing it that way we might better remove the
> >> exit_and_crash label altogether and call domain_crash() directly
> >> in the places we mean it to be called.
> > 
> > Indeed.  How's this, then?
> 
> Looks good - if you don't mind I'll replace the SVM part of the earlier
> patch with this, add your S-o-b alongside mine, and send again for
> final review.

Sure, that sounds great.

Tim.