x86/HVM: prevent infinite VM entry retries

PATCH In review Jan Beulich 2 participants 3 messages Nov 27, 2014

Share & tools

Jan Beulich

author

2014-11-27 12:40 UTC #0 lore raw

PATCH 1/1 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 <[email protected]>

Signed-off-by Jan Beulich <[email protected]>

Signed-off-by Tim Deegan <[email protected]>

---
v2: Replace SVM change by what Tim suggested.

xen/arch/x86/hvm/svm/svm.c +11 -8

@@ -2371,8 +2371,8 @@ 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;
-
+                domain_crash(v->domain);
+                goto out;
             default:
                 BUG();
             case NESTEDHVM_VMEXIT_ERROR:

@@ -2385,18 +2385,21 @@ void svm_vmexit_handler(struct cpu_user_
         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);

@@ -2431,13 +2434,13 @@ void svm_vmexit_handler(struct cpu_user_
 
     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;

@@ -2684,7 +2687,7 @@ void svm_vmexit_handler(struct cpu_user_
         break;
 
     default:
-    exit_and_crash:
+    unexpected_exit_type:
         gdprintk(XENLOG_ERR, "unexpected VMEXIT: exit reason = %#"PRIx64", "
                  "exitinfo1 = %#"PRIx64", exitinfo2 = %#"PRIx64"\n",
                  exit_reason,

xen/arch/x86/hvm/vmx/vmx.c +14 ~~-15~~

@@ -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;
     }

x86/HVM: prevent infinite VM entry retries

"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 <[email protected]>

Signed-off-by Jan Beulich <[email protected]>

Signed-off-by Tim Deegan <[email protected]>

---
v2: Replace SVM change by what Tim suggested.

xen/arch/x86/hvm/svm/svm.c +11 -8

@@ -2371,8 +2371,8 @@ 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;
-
+                domain_crash(v->domain);
+                goto out;
             default:
                 BUG();
             case NESTEDHVM_VMEXIT_ERROR:

@@ -2385,18 +2385,21 @@ void svm_vmexit_handler(struct cpu_user_
         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);

@@ -2431,13 +2434,13 @@ void svm_vmexit_handler(struct cpu_user_
 
     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;

@@ -2684,7 +2687,7 @@ void svm_vmexit_handler(struct cpu_user_
         break;
 
     default:
-    exit_and_crash:
+    unexpected_exit_type:
         gdprintk(XENLOG_ERR, "unexpected VMEXIT: exit reason = %#"PRIx64", "
                  "exitinfo1 = %#"PRIx64", exitinfo2 = %#"PRIx64"\n",
                  exit_reason,

xen/arch/x86/hvm/vmx/vmx.c +14 ~~-15~~

@@ -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;
     }

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Andrew Cooper

2014-11-27 12:46 UTC #1 lore raw

Quoted 35 lines · Jan Beulich

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 <[email protected]>
Signed-off-by Jan Beulich <[email protected]>
Signed-off-by Tim Deegan <[email protected]>
---
v2: Replace SVM change by what Tim suggested.
xen/arch/x86/hvm/svm/svm.c +4 -3
@@ -2371,8 +2371,8 @@ 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;
-
+                domain_crash(v->domain);
+                goto out;
             default:
                 BUG();
             case NESTEDHVM_VMEXIT_ERROR:
@@ -2385,18 +2385,21 @@ void svm_vmexit_handler(struct cpu_user_
         case NESTEDHVM_VMEXIT_FATALERROR:
             gdprintk(XENLOG_ERR,
                 "unexpected nestedsvm_check_intercepts() error\n");
-            goto exit_and_crash;
+            domain_crash(v->domain);
+            goto out;
         default:

xen/arch/x86/hvm/svm/svm.c void svm_vmexit_handler(struct cpu_user_ · line 2399

Quoted 10 lines

xen/arch/x86/hvm/svm/svm.c +2 -1

             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) )
     {

I think it would be good to retain the printk here from previous versions of the patch, specifically identifies the below vmcb dump as caused by a failed vmentry. As it stands, it is a vmcb dump with no other context.

Either way, Reviewed-by: Andrew Cooper <[email protected]>

Quoted 86 lines

xen/arch/x86/hvm/svm/svm.c +5 -4

         svm_vmcb_dump(__func__, vmcb);
-        goto exit_and_crash;
+        domain_crash(v->domain);
+        goto out;
     }
 
     perfc_incra(svmexits, exit_reason);

@@ -2431,13 +2434,13 @@ void svm_vmexit_handler(struct cpu_user_
 
     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;

@@ -2684,7 +2687,7 @@ void svm_vmexit_handler(struct cpu_user_
         break;
 
     default:
-    exit_and_crash:
+    unexpected_exit_type:
         gdprintk(XENLOG_ERR, "unexpected VMEXIT: exit reason = %#"PRIx64", "
                  "exitinfo1 = %#"PRIx64", exitinfo2 = %#"PRIx64"\n",
                  exit_reason,

xen/arch/x86/hvm/vmx/vmx.c +14 ~~-15~~

@@ -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;
     }

Jan Beulich

author

2014-11-27 12:58 UTC #2 lore raw

Quoted 11 lines

Quoted 1 line
Quoted 1 line · On 27.11.14 at 13:46, <[email protected]> wrote:
Quoted 2 lines · Jan Beulich
     if ( unlikely(exit_reason == VMEXIT_INVALID) )
     {
I think it would be good to retain the printk here from previous versions of the patch, specifically identifies the below vmcb dump as caused by a failed vmentry. As it stands, it is a vmcb dump with no other context.
Either way, Reviewed-by: Andrew Cooper <[email protected]>

I meant to do so and then forgot. Added to the to be committed version.

Jan