[PATCH 0/4] x86/vvmx: XSA-278 followup fixes

[PATCH 0/4] x86/vvmx: XSA-278 followup fixes

[Andrew Cooper]

Here are some of the easier fixes following on from the XSA-278 investigation.
This series removes the duplicated checks left over from the security fix.  I
did have some further plans, but the embargo breaking early means I haven't
had time to get them ready for posting.

A longer term plan is to model nested virt as an X86_EMU_ flag, but that
requires a fair amount of untangling of various toolstack actions during
create and migrate.

Andrew Cooper (4):
  x86/vvmx: Unconditionally initialise vmxon_region_pa during vcpu construction
  x86/vvmx: Drop the now-obsolete vmx_inst_check_privilege()
  x86/vvmx: INVVPID instructions should be handled at by L1
  x86/vvmx: Don't handle unknown nested vmexit reasons at L0

 xen/arch/x86/hvm/vmx/vmx.c  |  2 +
 xen/arch/x86/hvm/vmx/vvmx.c | 90 ++++++++++++---------------------------------
 2 files changed, 25 insertions(+), 67 deletions(-)

-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 1/4] x86/vvmx: Unconditionally initialise vmxon_region_pa during vcpu construction

[Andrew Cooper]

This is a stopgap solution until the toolstack side of initialisation can be
sorted out, but it does result in the nvmx_vcpu_in_vmx() predicate working
correctly even when nested virt hasn't been enabled for the domain.

Update nvmx_handle_vmx_insn() to include the in-vmx mode check (for all
instructions other than VMXON) to complete the set of #UD checks.

In addition, sanity check that the nested vmexit handler has worked correctly,
and that we are only providing emulation of the VT-x instructions to L1
guests.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
 xen/arch/x86/hvm/vmx/vmx.c  |  2 ++
 xen/arch/x86/hvm/vmx/vvmx.c | 11 ++++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 7a49075..00a7014 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -429,6 +429,8 @@ static int vmx_vcpu_initialise(struct vcpu *v)
 
     INIT_LIST_HEAD(&v->arch.hvm.vmx.pi_blocking.list);
 
+    vcpu_2_nvmx(v).vmxon_region_pa = INVALID_PADDR;
+
     if ( (rc = vmx_create_vmcs(v)) != 0 )
     {
         dprintk(XENLOG_WARNING,
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index aa202e0..09e105e 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1987,7 +1987,8 @@ int nvmx_handle_vmx_insn(struct cpu_user_regs *regs, unsigned int exit_reason)
 
     if ( !(curr->arch.hvm.guest_cr[4] & X86_CR4_VMXE) ||
          !nestedhvm_enabled(curr->domain) ||
-         (vmx_guest_x86_mode(curr) < (hvm_long_mode_active(curr) ? 8 : 2)) )
+         (vmx_guest_x86_mode(curr) < (hvm_long_mode_active(curr) ? 8 : 2)) ||
+         (exit_reason != EXIT_REASON_VMXON && !nvmx_vcpu_in_vmx(curr)) )
     {
         hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
         return X86EMUL_EXCEPTION;
@@ -1999,6 +2000,14 @@ int nvmx_handle_vmx_insn(struct cpu_user_regs *regs, unsigned int exit_reason)
         return X86EMUL_EXCEPTION;
     }
 
+    if ( nestedhvm_vcpu_in_guestmode(curr) )
+    {
+        /* Should have been handled by nvmx_n2_vmexit_handler()... */
+        ASSERT_UNREACHABLE();
+        domain_crash(curr->domain);
+        return X86EMUL_UNHANDLEABLE;
+    }
+
     switch ( exit_reason )
     {
     case EXIT_REASON_VMXOFF:
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 2/4] x86/vvmx: Drop the now-obsolete vmx_inst_check_privilege()

[Andrew Cooper]

Now that nvmx_handle_vmx_insn() performs all VT-x instruction checks, there is
no need for redundant checking in vmx_inst_check_privilege().  Remove it, and
take out the vmxon_check boolean which was plumbed through decode_vmx_inst().

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c | 75 ++++++---------------------------------------
 1 file changed, 10 insertions(+), 65 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 09e105e..9390705 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -377,48 +377,9 @@ static inline u32 __n2_secondary_exec_control(struct vcpu *v)
     return second_ctrl;
 }
 
-static int vmx_inst_check_privilege(struct cpu_user_regs *regs, int vmxop_check)
-{
-    struct vcpu *v = current;
-
-    if ( vmxop_check )
-    {
-        if ( !(v->arch.hvm.guest_cr[0] & X86_CR0_PE) ||
-             !(v->arch.hvm.guest_cr[4] & X86_CR4_VMXE) )
-            goto invalid_op;
-    }
-    else if ( !nvmx_vcpu_in_vmx(v) )
-        goto invalid_op;
-
-    if ( vmx_guest_x86_mode(v) < (hvm_long_mode_active(v) ? 8 : 2) )
-        goto invalid_op;
-    else if ( nestedhvm_vcpu_in_guestmode(v) )
-        goto vmexit;
-
-    if ( vmx_get_cpl() > 0 )
-        goto gp_fault;
-
-    return X86EMUL_OKAY;
-
-vmexit:
-    gdprintk(XENLOG_ERR, "vmx_inst_check_privilege: vmexit\n");
-    vcpu_nestedhvm(v).nv_vmexit_pending = 1;
-    return X86EMUL_EXCEPTION;
-    
-invalid_op:
-    gdprintk(XENLOG_ERR, "vmx_inst_check_privilege: invalid_op\n");
-    hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
-    return X86EMUL_EXCEPTION;
-
-gp_fault:
-    gdprintk(XENLOG_ERR, "vmx_inst_check_privilege: gp_fault\n");
-    hvm_inject_hw_exception(TRAP_gp_fault, 0);
-    return X86EMUL_EXCEPTION;
-}
-
 static int decode_vmx_inst(struct cpu_user_regs *regs,
                            struct vmx_inst_decoded *decode,
-                           unsigned long *poperandS, int vmxon_check)
+                           unsigned long *poperandS)
 {
     struct vcpu *v = current;
     union vmx_inst_info info;
@@ -426,9 +387,6 @@ static int decode_vmx_inst(struct cpu_user_regs *regs,
     unsigned long base, index, seg_base, disp, offset;
     int scale, size;
 
-    if ( vmx_inst_check_privilege(regs, vmxon_check) != X86EMUL_OKAY )
-        return X86EMUL_EXCEPTION;
-
     __vmread(VMX_INSTRUCTION_INFO, &offset);
     info.word = offset;
 
@@ -1480,7 +1438,7 @@ static int nvmx_handle_vmxon(struct cpu_user_regs *regs)
     uint32_t nvmcs_revid;
     int rc;
 
-    rc = decode_vmx_inst(regs, &decode, &gpa, 1);
+    rc = decode_vmx_inst(regs, &decode, &gpa);
     if ( rc != X86EMUL_OKAY )
         return rc;
 
@@ -1526,11 +1484,6 @@ static int nvmx_handle_vmxoff(struct cpu_user_regs *regs)
 {
     struct vcpu *v=current;
     struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
-    int rc;
-
-    rc = vmx_inst_check_privilege(regs, 0);
-    if ( rc != X86EMUL_OKAY )
-        return rc;
 
     nvmx_purge_vvmcs(v);
     nvmx->vmxon_region_pa = INVALID_PADDR;
@@ -1617,10 +1570,6 @@ static int nvmx_handle_vmresume(struct cpu_user_regs *regs)
     struct vcpu *v = current;
     struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
     unsigned long intr_shadow;
-    int rc = vmx_inst_check_privilege(regs, 0);
-
-    if ( rc != X86EMUL_OKAY )
-        return rc;
 
     if ( vcpu_nestedhvm(v).nv_vvmcxaddr == INVALID_PADDR )
     {
@@ -1651,10 +1600,7 @@ static int nvmx_handle_vmlaunch(struct cpu_user_regs *regs)
     struct vcpu *v = current;
     struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
     unsigned long intr_shadow;
-    int rc = vmx_inst_check_privilege(regs, 0);
-
-    if ( rc != X86EMUL_OKAY )
-        return rc;
+    int rc;
 
     if ( vcpu_nestedhvm(v).nv_vvmcxaddr == INVALID_PADDR )
     {
@@ -1696,7 +1642,7 @@ static int nvmx_handle_vmptrld(struct cpu_user_regs *regs)
     unsigned long gpa = 0;
     int rc;
 
-    rc = decode_vmx_inst(regs, &decode, &gpa, 0);
+    rc = decode_vmx_inst(regs, &decode, &gpa);
     if ( rc != X86EMUL_OKAY )
         return rc;
 
@@ -1768,7 +1714,7 @@ static int nvmx_handle_vmptrst(struct cpu_user_regs *regs)
     unsigned long gpa = 0;
     int rc;
 
-    rc = decode_vmx_inst(regs, &decode, &gpa, 0);
+    rc = decode_vmx_inst(regs, &decode, &gpa);
     if ( rc != X86EMUL_OKAY )
         return rc;
 
@@ -1794,7 +1740,7 @@ static int nvmx_handle_vmclear(struct cpu_user_regs *regs)
     void *vvmcs;
     int rc;
 
-    rc = decode_vmx_inst(regs, &decode, &gpa, 0);
+    rc = decode_vmx_inst(regs, &decode, &gpa);
     if ( rc != X86EMUL_OKAY )
         return rc;
 
@@ -1844,7 +1790,7 @@ static int nvmx_handle_vmread(struct cpu_user_regs *regs)
     u64 value = 0;
     int rc;
 
-    rc = decode_vmx_inst(regs, &decode, NULL, 0);
+    rc = decode_vmx_inst(regs, &decode, NULL);
     if ( rc != X86EMUL_OKAY )
         return rc;
 
@@ -1887,8 +1833,7 @@ static int nvmx_handle_vmwrite(struct cpu_user_regs *regs)
     bool_t okay = 1;
     enum vmx_insn_errno err;
 
-    if ( decode_vmx_inst(regs, &decode, &operand, 0)
-             != X86EMUL_OKAY )
+    if ( decode_vmx_inst(regs, &decode, &operand) != X86EMUL_OKAY )
         return X86EMUL_EXCEPTION;
 
     if ( vcpu_nestedhvm(v).nv_vvmcxaddr == INVALID_PADDR )
@@ -1932,7 +1877,7 @@ static int nvmx_handle_invept(struct cpu_user_regs *regs)
     unsigned long eptp;
     int ret;
 
-    if ( (ret = decode_vmx_inst(regs, &decode, &eptp, 0)) != X86EMUL_OKAY )
+    if ( (ret = decode_vmx_inst(regs, &decode, &eptp)) != X86EMUL_OKAY )
         return ret;
 
     switch ( reg_read(regs, decode.reg2) )
@@ -1960,7 +1905,7 @@ static int nvmx_handle_invvpid(struct cpu_user_regs *regs)
     unsigned long vpid;
     int ret;
 
-    if ( (ret = decode_vmx_inst(regs, &decode, &vpid, 0)) != X86EMUL_OKAY )
+    if ( (ret = decode_vmx_inst(regs, &decode, &vpid)) != X86EMUL_OKAY )
         return ret;
 
     switch ( reg_read(regs, decode.reg2) )
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 3/4] x86/vvmx: INVVPID instructions should be handled at by L1

[Andrew Cooper]

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 9390705..d1c8a41 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -2348,6 +2348,7 @@ int nvmx_n2_vmexit_handler(struct cpu_user_regs *regs,
     case EXIT_REASON_VMXOFF:
     case EXIT_REASON_VMXON:
     case EXIT_REASON_INVEPT:
+    case EXIT_REASON_INVVPID:
     case EXIT_REASON_XSETBV:
         /* inject to L1 */
         nvcpu->nv_vmexit_pending = 1;
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 4/4] x86/vvmx: Don't handle unknown nested vmexit reasons at L0

[Andrew Cooper]

This is very dangerous from a security point of view, because a missing entry
will cause L2's action to be interpreted as L1's action.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index d1c8a41..817d85f 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -2609,8 +2609,9 @@ int nvmx_n2_vmexit_handler(struct cpu_user_regs *regs,
             nvcpu->nv_vmexit_pending = 1;
         break;
     default:
-        gprintk(XENLOG_ERR, "Unexpected nested vmexit: reason %u\n",
+        gprintk(XENLOG_ERR, "Unhandled nested vmexit: reason %u\n",
                 exit_reason);
+        domain_crash(v->domain);
     }
 
     return ( nvcpu->nv_vmexit_pending == 1 );
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/4] x86/vvmx: INVVPID instructions should be handled at by L1

[Sergey Dyasli]

On 25/10/2018 16:38, Andrew Cooper wrote:

> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index 9390705..d1c8a41 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -2348,6 +2348,7 @@ int nvmx_n2_vmexit_handler(struct cpu_user_regs *regs,
>      case EXIT_REASON_VMXOFF:
>      case EXIT_REASON_VMXON:
>      case EXIT_REASON_INVEPT:
> +    case EXIT_REASON_INVVPID:
>      case EXIT_REASON_XSETBV:
>          /* inject to L1 */
>          nvcpu->nv_vmexit_pending = 1;

Reviewed-by: Sergey Dyasli <sergey.dyasli@citrix.com>

I think this patch should be the first one in the series.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/4] x86/vvmx: Don't handle unknown nested vmexit reasons at L0

[Sergey Dyasli]

On 25/10/2018 16:39, Andrew Cooper wrote:
> This is very dangerous from a security point of view, because a missing entry
> will cause L2's action to be interpreted as L1's action.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Sergey Dyasli <sergey.dyasli@citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> ---
>  xen/arch/x86/hvm/vmx/vvmx.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index d1c8a41..817d85f 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -2609,8 +2609,9 @@ int nvmx_n2_vmexit_handler(struct cpu_user_regs *regs,
>              nvcpu->nv_vmexit_pending = 1;
>          break;
>      default:
> -        gprintk(XENLOG_ERR, "Unexpected nested vmexit: reason %u\n",
> +        gprintk(XENLOG_ERR, "Unhandled nested vmexit: reason %u\n",
>                  exit_reason);
> +        domain_crash(v->domain);
>      }
>  
>      return ( nvcpu->nv_vmexit_pending == 1 );

Can you consider adding handling for the following?

	EXIT_REASON_INVD
	EXIT_REASON_RDTSCP
	EXIT_REASON_VMFUNC

But in any case:

Reviewed-by: Sergey Dyasli <sergey.dyasli@citrix.com>

--
Thanks,
Sergey

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/4] x86/vvmx: Unconditionally initialise vmxon_region_pa during vcpu construction

[Sergey Dyasli]

On 25/10/2018 16:36, Andrew Cooper wrote:
> This is a stopgap solution until the toolstack side of initialisation can be
> sorted out, but it does result in the nvmx_vcpu_in_vmx() predicate working
> correctly even when nested virt hasn't been enabled for the domain.
> 
> Update nvmx_handle_vmx_insn() to include the in-vmx mode check (for all
> instructions other than VMXON) to complete the set of #UD checks.
> 
> In addition, sanity check that the nested vmexit handler has worked correctly,
> and that we are only providing emulation of the VT-x instructions to L1
> guests.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Sergey Dyasli <sergey.dyasli@citrix.com>

--
Thanks,
Sergey

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/4] x86/vvmx: Drop the now-obsolete vmx_inst_check_privilege()

[Sergey Dyasli]

On 25/10/2018 16:38, Andrew Cooper wrote:
> Now that nvmx_handle_vmx_insn() performs all VT-x instruction checks, there is
> no need for redundant checking in vmx_inst_check_privilege().  Remove it, and
> take out the vmxon_check boolean which was plumbed through decode_vmx_inst().
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Sergey Dyasli <sergey.dyasli@citrix.com>

--
Thanks,
Sergey

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/4] x86/vvmx: Don't handle unknown nested vmexit reasons at L0

[Andrew Cooper]

On 26/10/2018 10:05, Sergey Dyasli wrote:
>
> On 25/10/2018 16:39, Andrew Cooper wrote:
>> This is very dangerous from a security point of view, because a missing entry
>> will cause L2's action to be interpreted as L1's action.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Sergey Dyasli <sergey.dyasli@citrix.com>
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Jun Nakajima <jun.nakajima@intel.com>
>> CC: Kevin Tian <kevin.tian@intel.com>
>> ---
>>  xen/arch/x86/hvm/vmx/vvmx.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
>> index d1c8a41..817d85f 100644
>> --- a/xen/arch/x86/hvm/vmx/vvmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
>> @@ -2609,8 +2609,9 @@ int nvmx_n2_vmexit_handler(struct cpu_user_regs *regs,
>>              nvcpu->nv_vmexit_pending = 1;
>>          break;
>>      default:
>> -        gprintk(XENLOG_ERR, "Unexpected nested vmexit: reason %u\n",
>> +        gprintk(XENLOG_ERR, "Unhandled nested vmexit: reason %u\n",
>>                  exit_reason);
>> +        domain_crash(v->domain);
>>      }
>>  
>>      return ( nvcpu->nv_vmexit_pending == 1 );
> Can you consider adding handling for the following?
>
> 	EXIT_REASON_INVD
> 	EXIT_REASON_RDTSCP
> 	EXIT_REASON_VMFUNC

Looks like these should be merged in with the INVVPID change, if your
happy with that?

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/4] x86/vvmx: Don't handle unknown nested vmexit reasons at L0

[Sergey Dyasli]

On 26/10/2018 10:10, Andrew Cooper wrote:
> On 26/10/2018 10:05, Sergey Dyasli wrote:
>>
>> On 25/10/2018 16:39, Andrew Cooper wrote:
>>> This is very dangerous from a security point of view, because a missing entry
>>> will cause L2's action to be interpreted as L1's action.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: Sergey Dyasli <sergey.dyasli@citrix.com>
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Wei Liu <wei.liu2@citrix.com>
>>> CC: Jun Nakajima <jun.nakajima@intel.com>
>>> CC: Kevin Tian <kevin.tian@intel.com>
>>> ---
>>>  xen/arch/x86/hvm/vmx/vvmx.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
>>> index d1c8a41..817d85f 100644
>>> --- a/xen/arch/x86/hvm/vmx/vvmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
>>> @@ -2609,8 +2609,9 @@ int nvmx_n2_vmexit_handler(struct cpu_user_regs *regs,
>>>              nvcpu->nv_vmexit_pending = 1;
>>>          break;
>>>      default:
>>> -        gprintk(XENLOG_ERR, "Unexpected nested vmexit: reason %u\n",
>>> +        gprintk(XENLOG_ERR, "Unhandled nested vmexit: reason %u\n",
>>>                  exit_reason);
>>> +        domain_crash(v->domain);
>>>      }
>>>  
>>>      return ( nvcpu->nv_vmexit_pending == 1 );
>> Can you consider adding handling for the following?
>>
>> 	EXIT_REASON_INVD
>> 	EXIT_REASON_RDTSCP
>> 	EXIT_REASON_VMFUNC
> 
> Looks like these should be merged in with the INVVPID change, if your
> happy with that?

Yes, that would be a cleaner way, thanks.

--
Sergey

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/4] x86/vvmx: Don't handle unknown nested vmexit reasons at L0

[Andrew Cooper]

On 26/10/18 10:13, Sergey Dyasli wrote:
> On 26/10/2018 10:10, Andrew Cooper wrote:
>> On 26/10/2018 10:05, Sergey Dyasli wrote:
>>> On 25/10/2018 16:39, Andrew Cooper wrote:
>>>> This is very dangerous from a security point of view, because a missing entry
>>>> will cause L2's action to be interpreted as L1's action.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> ---
>>>> CC: Sergey Dyasli <sergey.dyasli@citrix.com>
>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>> CC: Wei Liu <wei.liu2@citrix.com>
>>>> CC: Jun Nakajima <jun.nakajima@intel.com>
>>>> CC: Kevin Tian <kevin.tian@intel.com>
>>>> ---
>>>>  xen/arch/x86/hvm/vmx/vvmx.c | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
>>>> index d1c8a41..817d85f 100644
>>>> --- a/xen/arch/x86/hvm/vmx/vvmx.c
>>>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
>>>> @@ -2609,8 +2609,9 @@ int nvmx_n2_vmexit_handler(struct cpu_user_regs *regs,
>>>>              nvcpu->nv_vmexit_pending = 1;
>>>>          break;
>>>>      default:
>>>> -        gprintk(XENLOG_ERR, "Unexpected nested vmexit: reason %u\n",
>>>> +        gprintk(XENLOG_ERR, "Unhandled nested vmexit: reason %u\n",
>>>>                  exit_reason);
>>>> +        domain_crash(v->domain);
>>>>      }
>>>>  
>>>>      return ( nvcpu->nv_vmexit_pending == 1 );
>>> Can you consider adding handling for the following?
>>>
>>> 	EXIT_REASON_INVD
>>> 	EXIT_REASON_RDTSCP
>>> 	EXIT_REASON_VMFUNC
>> Looks like these should be merged in with the INVVPID change, if your
>> happy with that?
> Yes, that would be a cleaner way, thanks.

Ok - that patch is now as follows:

x86/vvmx: Let L1 handle all the unconditional vmexit instructions

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
...
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index aa202e0..7051eb3 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -2383,6 +2383,8 @@ int nvmx_n2_vmexit_handler(struct cpu_user_regs *regs,
     case EXIT_REASON_TRIPLE_FAULT:
     case EXIT_REASON_TASK_SWITCH:
     case EXIT_REASON_CPUID:
+    case EXIT_REASON_GETSEC:
+    case EXIT_REASON_INVD:
     case EXIT_REASON_VMCALL:
     case EXIT_REASON_VMCLEAR:
     case EXIT_REASON_VMLAUNCH:
@@ -2395,6 +2397,7 @@ int nvmx_n2_vmexit_handler(struct cpu_user_regs *regs,
     case EXIT_REASON_VMXON:
     case EXIT_REASON_INVEPT:
     case EXIT_REASON_XSETBV:
+    case EXIT_REASON_INVVPID:
         /* inject to L1 */
         nvcpu->nv_vmexit_pending = 1;
         break;

RDTSCP and VMFUNC mustn't be blindly forwarded to L1.  We need to check
whether the intercepts are active and either forward to L1 or handle at L0.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/4] x86/vvmx: Don't handle unknown nested vmexit reasons at L0

[Sergey Dyasli]

On 26/10/2018 12:09, Andrew Cooper wrote:
> Ok - that patch is now as follows:
> 
> x86/vvmx: Let L1 handle all the unconditional vmexit instructions
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ...
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index aa202e0..7051eb3 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -2383,6 +2383,8 @@ int nvmx_n2_vmexit_handler(struct cpu_user_regs *regs,
>      case EXIT_REASON_TRIPLE_FAULT:
>      case EXIT_REASON_TASK_SWITCH:
>      case EXIT_REASON_CPUID:
> +    case EXIT_REASON_GETSEC:
> +    case EXIT_REASON_INVD:
>      case EXIT_REASON_VMCALL:
>      case EXIT_REASON_VMCLEAR:
>      case EXIT_REASON_VMLAUNCH:
> @@ -2395,6 +2397,7 @@ int nvmx_n2_vmexit_handler(struct cpu_user_regs *regs,
>      case EXIT_REASON_VMXON:
>      case EXIT_REASON_INVEPT:
>      case EXIT_REASON_XSETBV:
> +    case EXIT_REASON_INVVPID:
>          /* inject to L1 */
>          nvcpu->nv_vmexit_pending = 1;
>          break;
> 
> RDTSCP and VMFUNC mustn't be blindly forwarded to L1.  We need to check
> whether the intercepts are active and either forward to L1 or handle at L0.

I think the below will do for RDTSCP.
VMFUNC is trickier but Xen currently doesn't provide this capability to
L1 anyway.

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index aa202e0d12..1d1a0921af 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -2653,6 +2653,13 @@ int nvmx_n2_vmexit_handler(struct cpu_user_regs
*regs,
         if ( ctrl & CPU_BASED_TPR_SHADOW )
             nvcpu->nv_vmexit_pending = 1;
         break;
+
+    case EXIT_REASON_RDTSCP:
+        ctrl = __n2_exec_control(v);
+        if ( ctrl & CPU_BASED_RDTSC_EXITING )
+            nvcpu->nv_vmexit_pending = 1;
+        break;
+
     default:
         gprintk(XENLOG_ERR, "Unexpected nested vmexit: reason %u\n",
                 exit_reason);

---
Thanks,
Sergey

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/4] x86/vvmx: Unconditionally initialise vmxon_region_pa during vcpu construction

[Jan Beulich]

>>> On 25.10.18 at 17:36, <andrew.cooper3@citrix.com> wrote:
> This is a stopgap solution until the toolstack side of initialisation can be
> sorted out, but it does result in the nvmx_vcpu_in_vmx() predicate working
> correctly even when nested virt hasn't been enabled for the domain.
> 
> Update nvmx_handle_vmx_insn() to include the in-vmx mode check (for all
> instructions other than VMXON) to complete the set of #UD checks.
> 
> In addition, sanity check that the nested vmexit handler has worked correctly,
> and that we are only providing emulation of the VT-x instructions to L1
> guests.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/4] x86/vvmx: Drop the now-obsolete vmx_inst_check_privilege()

[Jan Beulich]

>>> On 25.10.18 at 17:38, <andrew.cooper3@citrix.com> wrote:
> Now that nvmx_handle_vmx_insn() performs all VT-x instruction checks, there is
> no need for redundant checking in vmx_inst_check_privilege().  Remove it, and
> take out the vmxon_check boolean which was plumbed through 
> decode_vmx_inst().
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/4] x86/vvmx: Unconditionally initialise vmxon_region_pa during vcpu construction

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Thursday, October 25, 2018 11:37 PM
> 
> This is a stopgap solution until the toolstack side of initialisation can be
> sorted out, but it does result in the nvmx_vcpu_in_vmx() predicate working
> correctly even when nested virt hasn't been enabled for the domain.
> 
> Update nvmx_handle_vmx_insn() to include the in-vmx mode check (for all
> instructions other than VMXON) to complete the set of #UD checks.
> 
> In addition, sanity check that the nested vmexit handler has worked
> correctly,
> and that we are only providing emulation of the VT-x instructions to L1
> guests.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/4] x86/vvmx: Drop the now-obsolete vmx_inst_check_privilege()

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Thursday, October 25, 2018 11:39 PM
> 
> Now that nvmx_handle_vmx_insn() performs all VT-x instruction checks,
> there is
> no need for redundant checking in vmx_inst_check_privilege().  Remove it,
> and
> take out the vmxon_check boolean which was plumbed through
> decode_vmx_inst().
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/4] x86/vvmx: INVVPID instructions should be handled at by L1

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Thursday, October 25, 2018 11:39 PM
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/4] x86/vvmx: Don't handle unknown nested vmexit reasons at L0

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Thursday, October 25, 2018 11:39 PM
> 
> This is very dangerous from a security point of view, because a missing
> entry
> will cause L2's action to be interpreted as L1's action.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel