[PATCH] x86/HVM: only kill guest when unknown VM exit occurred in guest kernel mode

[PATCH] x86/HVM: only kill guest when unknown VM exit occurred in guest kernel mode

[Jan Beulich]

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

A recent KVM change by Nadav Amit <namit@cs.technion.ac.il> pointed out
that unconditional VM exits (like VMX'es ones for the INVEPT, INVVPID,
and XSETBV instructions) may result from guest user mode activity (in
the example cases, e.g. prior to a privilege level check being done).
Consequently convert the unconditional domain_crash() to a conditional
one (when guest is in kernel mode) with the alternative of injecting
#UD (when in user mode).

This is meant to be a precaution against in-guest security issues
introduced when any such VM exit becomes possible (on newer hardware)
without the hypervisor immediately being aware of it. There are no such
unhandled VM exits currently (and hence this is not an active security
issue), but old (no longer security maintained) versions exhibit issues
in the cases given as examples above.

Suggested-by: Tim Deegan <tim@xen.org>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2680,7 +2680,11 @@ void svm_vmexit_handler(struct cpu_user_
                  "exitinfo1 = %#"PRIx64", exitinfo2 = %#"PRIx64"\n",
                  exit_reason, 
                  (u64)vmcb->exitinfo1, (u64)vmcb->exitinfo2);
-        domain_crash(v->domain);
+        if ( vmcb_get_cpl(vmcb) )
+            hvm_inject_hw_exception(TRAP_invalid_op,
+                                    HVM_DELIVER_NO_ERROR_CODE);
+        else
+            domain_crash(v->domain);
         break;
     }
 
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3157,8 +3157,19 @@ void vmx_vmexit_handler(struct cpu_user_
     /* fall through */
     default:
     exit_and_crash:
-        gdprintk(XENLOG_ERR, "Bad vmexit (reason %#lx)\n", exit_reason);
-        domain_crash(v->domain);
+        {
+            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-unknown-exits-user-mode.patch --]
[-- Type: text/plain, Size: 2400 bytes --]

x86/HVM: only kill guest when unknown VM exit occurred in guest kernel mode

A recent KVM change by Nadav Amit <namit@cs.technion.ac.il> pointed out
that unconditional VM exits (like VMX'es ones for the INVEPT, INVVPID,
and XSETBV instructions) may result from guest user mode activity (in
the example cases, e.g. prior to a privilege level check being done).
Consequently convert the unconditional domain_crash() to a conditional
one (when guest is in kernel mode) with the alternative of injecting
#UD (when in user mode).

This is meant to be a precaution against in-guest security issues
introduced when any such VM exit becomes possible (on newer hardware)
without the hypervisor immediately being aware of it. There are no such
unhandled VM exits currently (and hence this is not an active security
issue), but old (no longer security maintained) versions exhibit issues
in the cases given as examples above.

Suggested-by: Tim Deegan <tim@xen.org>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2680,7 +2680,11 @@ void svm_vmexit_handler(struct cpu_user_
                  "exitinfo1 = %#"PRIx64", exitinfo2 = %#"PRIx64"\n",
                  exit_reason, 
                  (u64)vmcb->exitinfo1, (u64)vmcb->exitinfo2);
-        domain_crash(v->domain);
+        if ( vmcb_get_cpl(vmcb) )
+            hvm_inject_hw_exception(TRAP_invalid_op,
+                                    HVM_DELIVER_NO_ERROR_CODE);
+        else
+            domain_crash(v->domain);
         break;
     }
 
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3157,8 +3157,19 @@ void vmx_vmexit_handler(struct cpu_user_
     /* fall through */
     default:
     exit_and_crash:
-        gdprintk(XENLOG_ERR, "Bad vmexit (reason %#lx)\n", exit_reason);
-        domain_crash(v->domain);
+        {
+            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] x86/HVM: only kill guest when unknown VM exit occurred in guest kernel mode

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 2840 bytes --]

On 30/10/14 14:43, Jan Beulich wrote:
> A recent KVM change by Nadav Amit <namit@cs.technion.ac.il> pointed out
> that unconditional VM exits (like VMX'es ones for the INVEPT, INVVPID,
> and XSETBV instructions) may result from guest user mode activity (in
> the example cases, e.g. prior to a privilege level check being done).
> Consequently convert the unconditional domain_crash() to a conditional
> one (when guest is in kernel mode) with the alternative of injecting
> #UD (when in user mode).
>
> This is meant to be a precaution against in-guest security issues
> introduced when any such VM exit becomes possible (on newer hardware)
> without the hypervisor immediately being aware of it. There are no such
> unhandled VM exits currently (and hence this is not an active security
> issue), but old (no longer security maintained) versions exhibit issues
> in the cases given as examples above.
>
> Suggested-by: Tim Deegan <tim@xen.org>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

The gdprintk() in vmx.c is not true for some entries via the
exit_and_crash label, but it is probably worth deferring fixing it to a
separate patch.

>
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2680,7 +2680,11 @@ void svm_vmexit_handler(struct cpu_user_
>                   "exitinfo1 = %#"PRIx64", exitinfo2 = %#"PRIx64"\n",
>                   exit_reason, 
>                   (u64)vmcb->exitinfo1, (u64)vmcb->exitinfo2);
> -        domain_crash(v->domain);
> +        if ( vmcb_get_cpl(vmcb) )
> +            hvm_inject_hw_exception(TRAP_invalid_op,
> +                                    HVM_DELIVER_NO_ERROR_CODE);
> +        else
> +            domain_crash(v->domain);
>          break;
>      }
>  
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3157,8 +3157,19 @@ void vmx_vmexit_handler(struct cpu_user_
>      /* fall through */
>      default:
>      exit_and_crash:
> -        gdprintk(XENLOG_ERR, "Bad vmexit (reason %#lx)\n", exit_reason);
> -        domain_crash(v->domain);
> +        {
> +            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
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 3772 bytes --]

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

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

Re: [PATCH] x86/HVM: only kill guest when unknown VM exit occurred in guest kernel mode

[Jan Beulich]

>>> On 30.10.14 at 15:57, <andrew.cooper3@citrix.com> wrote:
> On 30/10/14 14:43, Jan Beulich wrote:
>> A recent KVM change by Nadav Amit <namit@cs.technion.ac.il> pointed out
>> that unconditional VM exits (like VMX'es ones for the INVEPT, INVVPID,
>> and XSETBV instructions) may result from guest user mode activity (in
>> the example cases, e.g. prior to a privilege level check being done).
>> Consequently convert the unconditional domain_crash() to a conditional
>> one (when guest is in kernel mode) with the alternative of injecting
>> #UD (when in user mode).
>>
>> This is meant to be a precaution against in-guest security issues
>> introduced when any such VM exit becomes possible (on newer hardware)
>> without the hypervisor immediately being aware of it. There are no such
>> unhandled VM exits currently (and hence this is not an active security
>> issue), but old (no longer security maintained) versions exhibit issues
>> in the cases given as examples above.
>>
>> Suggested-by: Tim Deegan <tim@xen.org>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> The gdprintk() in vmx.c is not true for some entries via the
> exit_and_crash label, but it is probably worth deferring fixing it to a
> separate patch.

Right - several would better bypass the logging. The same applies
to svm.c afaict. And yes, logically a separate change, so better
also a separate patch.

Jan

Re: [PATCH] x86/HVM: only kill guest when unknown VM exit occurred in guest kernel mode

[Boris Ostrovsky]

On 10/30/2014 10:43 AM, Jan Beulich wrote:
> A recent KVM change by Nadav Amit <namit@cs.technion.ac.il> pointed out
> that unconditional VM exits (like VMX'es ones for the INVEPT, INVVPID,
> and XSETBV instructions) may result from guest user mode activity (in
> the example cases, e.g. prior to a privilege level check being done).
> Consequently convert the unconditional domain_crash() to a conditional
> one (when guest is in kernel mode) with the alternative of injecting
> #UD (when in user mode).
>
> This is meant to be a precaution against in-guest security issues
> introduced when any such VM exit becomes possible (on newer hardware)
> without the hypervisor immediately being aware of it. There are no such
> unhandled VM exits currently (and hence this is not an active security
> issue), but old (no longer security maintained) versions exhibit issues
> in the cases given as examples above.
>
> Suggested-by: Tim Deegan <tim@xen.org>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>


>
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2680,7 +2680,11 @@ void svm_vmexit_handler(struct cpu_user_
>                    "exitinfo1 = %#"PRIx64", exitinfo2 = %#"PRIx64"\n",
>                    exit_reason,
>                    (u64)vmcb->exitinfo1, (u64)vmcb->exitinfo2);
> -        domain_crash(v->domain);
> +        if ( vmcb_get_cpl(vmcb) )
> +            hvm_inject_hw_exception(TRAP_invalid_op,
> +                                    HVM_DELIVER_NO_ERROR_CODE);
> +        else
> +            domain_crash(v->domain);
>           break;
>       }
>   
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3157,8 +3157,19 @@ void vmx_vmexit_handler(struct cpu_user_
>       /* fall through */
>       default:
>       exit_and_crash:
> -        gdprintk(XENLOG_ERR, "Bad vmexit (reason %#lx)\n", exit_reason);
> -        domain_crash(v->domain);
> +        {
> +            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;
>       }
>   
>
>
>

Re: [PATCH] x86/HVM: only kill guest when unknown VM exit occurred in guest kernel mode

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Thursday, October 30, 2014 10:43 PM
> 
> A recent KVM change by Nadav Amit <namit@cs.technion.ac.il> pointed out
> that unconditional VM exits (like VMX'es ones for the INVEPT, INVVPID,
> and XSETBV instructions) may result from guest user mode activity (in
> the example cases, e.g. prior to a privilege level check being done).
> Consequently convert the unconditional domain_crash() to a conditional
> one (when guest is in kernel mode) with the alternative of injecting
> #UD (when in user mode).
> 
> This is meant to be a precaution against in-guest security issues
> introduced when any such VM exit becomes possible (on newer hardware)
> without the hypervisor immediately being aware of it. There are no such
> unhandled VM exits currently (and hence this is not an active security
> issue), but old (no longer security maintained) versions exhibit issues
> in the cases given as examples above.
> 
> Suggested-by: Tim Deegan <tim@xen.org>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

> 
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2680,7 +2680,11 @@ void svm_vmexit_handler(struct cpu_user_
>                   "exitinfo1 = %#"PRIx64", exitinfo2 = %#"PRIx64"\n",
>                   exit_reason,
>                   (u64)vmcb->exitinfo1, (u64)vmcb->exitinfo2);
> -        domain_crash(v->domain);
> +        if ( vmcb_get_cpl(vmcb) )
> +            hvm_inject_hw_exception(TRAP_invalid_op,
> +
> HVM_DELIVER_NO_ERROR_CODE);
> +        else
> +            domain_crash(v->domain);
>          break;
>      }
> 
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3157,8 +3157,19 @@ void vmx_vmexit_handler(struct cpu_user_
>      /* fall through */
>      default:
>      exit_and_crash:
> -        gdprintk(XENLOG_ERR, "Bad vmexit (reason %#lx)\n", exit_reason);
> -        domain_crash(v->domain);
> +        {
> +            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;
>      }
> 
> 
>