[Xen-devel,v2,3/4] vvmx: check the operand of L1 vmxon

Message ID 20161218140249.4eb5udpyftqjd3s7@hz-desktop
State New
Headers show

Commit Message

Haozhong Zhang Dec. 18, 2016, 2:02 p.m.
On 12/14/16 18:11 +0800, Haozhong Zhang wrote:
>Check whether the operand of L1 vmxon is a valid VMXON region address

>and whether the VMXON region at that address contains a valid revision

>ID.

>

>Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

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

>Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

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

>---

> xen/arch/x86/hvm/vmx/vvmx.c | 16 ++++++++++++++++

> 1 file changed, 16 insertions(+)

>

>diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c

>index e765b60..5523146 100644

>--- a/xen/arch/x86/hvm/vmx/vvmx.c

>+++ b/xen/arch/x86/hvm/vmx/vvmx.c

>@@ -1383,6 +1383,7 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs)

>     struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);

>     struct vmx_inst_decoded decode;

>     unsigned long gpa = 0;

>+    uint32_t nvmcs_revid;

>     int rc;

>

>     rc = decode_vmx_inst(regs, &decode, &gpa, 1);

>@@ -1397,6 +1398,21 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs)

>         return X86EMUL_OKAY;

>     }

>

>+    if ( (gpa & ~PAGE_MASK) || (gpa >> v->domain->arch.paging.gfn_bits) )

                                                                 ^^^^^^^^

I mistaken it as the number of valid bits of physical address and
therefore missed adding PAGE_SHIFT here. The correct patch should be
the one attached. I notice the wrong patch has been in the staging
branch, so should I send a patch(set) to fix my mistake on the staging
branch?

Thanks,
Haozhong

Comments

Andrew Cooper Dec. 18, 2016, 2:06 p.m. | #1
On 18/12/16 14:02, Haozhong Zhang wrote:
> On 12/14/16 18:11 +0800, Haozhong Zhang wrote:
>> Check whether the operand of L1 vmxon is a valid VMXON region address
>> and whether the VMXON region at that address contains a valid revision
>> ID.
>>
>> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Acked-by: Kevin Tian <kevin.tian@intel.com>
>> ---
>> xen/arch/x86/hvm/vmx/vvmx.c | 16 ++++++++++++++++
>> 1 file changed, 16 insertions(+)
>>
>> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
>> index e765b60..5523146 100644
>> --- a/xen/arch/x86/hvm/vmx/vvmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
>> @@ -1383,6 +1383,7 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs)
>>     struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
>>     struct vmx_inst_decoded decode;
>>     unsigned long gpa = 0;
>> +    uint32_t nvmcs_revid;
>>     int rc;
>>
>>     rc = decode_vmx_inst(regs, &decode, &gpa, 1);
>> @@ -1397,6 +1398,21 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs)
>>         return X86EMUL_OKAY;
>>     }
>>
>> +    if ( (gpa & ~PAGE_MASK) || (gpa >>
>> v->domain->arch.paging.gfn_bits) )
>                                                                 ^^^^^^^^
>
> I mistaken it as the number of valid bits of physical address and
> therefore missed adding PAGE_SHIFT here. The correct patch should be
> the one attached. I notice the wrong patch has been in the staging
> branch, so should I send a patch(set) to fix my mistake on the staging
> branch?

Yes please.  All new code gets committed into staging.

master is fast-forwarded into staging when OSSTest is happy that no
regressions have occurred.

~Andrew

Patch hide | download patch | download mbox

From 809cf1ee317527d2eb8c2d8bac3be46b4d446b63 Mon Sep 17 00:00:00 2001
From: Haozhong Zhang <haozhong.zhang@intel.com>
Date: Tue, 13 Dec 2016 19:49:48 +0800
Subject: [RESEND PATCH v2 3/5] vvmx: check the operand of L1 vmxon

Check whether the operand of L1 vmxon is a valid VMXON region address
and whether the VMXON region at that address contains a valid revision
ID.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index e765b60..a1f8e16 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1383,6 +1383,7 @@  int nvmx_handle_vmxon(struct cpu_user_regs *regs)
     struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
     struct vmx_inst_decoded decode;
     unsigned long gpa = 0;
+    uint32_t nvmcs_revid;
     int rc;
 
     rc = decode_vmx_inst(regs, &decode, &gpa, 1);
@@ -1397,6 +1398,22 @@  int nvmx_handle_vmxon(struct cpu_user_regs *regs)
         return X86EMUL_OKAY;
     }
 
+    if ( (gpa & ~PAGE_MASK) ||
+         (gpa >> (v->domain->arch.paging.gfn_bits + PAGE_SHIFT)) )
+    {
+        vmreturn(regs, VMFAIL_INVALID);
+        return X86EMUL_OKAY;
+    }
+
+    rc = hvm_copy_from_guest_phys(&nvmcs_revid, gpa, sizeof(nvmcs_revid));
+    if ( rc != HVMCOPY_okay ||
+         (nvmcs_revid & ~VMX_BASIC_REVISION_MASK) ||
+         ((nvmcs_revid ^ vmx_basic_msr) & VMX_BASIC_REVISION_MASK) )
+    {
+        vmreturn(regs, VMFAIL_INVALID);
+        return X86EMUL_OKAY;
+    }
+
     nvmx->vmxon_region_pa = gpa;
 
     /*
-- 
2.10.1