kvm: nVMX: Check memory operand to INVVPID

Message ID	20170627175917.88528-1-jmattson@google.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <kvm-owner@kernel.org> From: Jim Mattson <jmattson@google.com> To: kvm@vger.kernel.org Cc: Jim Mattson <jmattson@google.com> Subject: [PATCH] kvm: nVMX: Check memory operand to INVVPID Date: Tue, 27 Jun 2017 10:59:17 -0700 Message-Id: <20170627175917.88528-1-jmattson@google.com> Sender: kvm-owner@vger.kernel.org Precedence: bulk

Message ID

20170627175917.88528-1-jmattson@google.com (mailing list archive)

State

New, archived

Headers

From: Jim Mattson <jmattson@google.com>
To: kvm@vger.kernel.org
Cc: Jim Mattson <jmattson@google.com>
Subject: [PATCH] kvm: nVMX: Check memory operand to INVVPID
Date: Tue, 27 Jun 2017 10:59:17 -0700
Message-Id: <20170627175917.88528-1-jmattson@google.com>
Sender: kvm-owner@vger.kernel.org
Precedence: bulk

Commit Message

Jim Mattson June 27, 2017, 5:59 p.m. UTC

The memory operand fetched for INVVPID is 128 bits. Bits 63:16 are
reserved and must be zero.  Otherwise, the instruction fails with
VMfail(Invalid operand to INVEPT/INVVPID).  If the INVVPID_TYPE is 0
(individual address invalidation), then bits 127:64 must be in
canonical form, or the instruction fails with VMfail(Invalid operand
to INVEPT/INVVPID).

Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/vmx.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

Comments

David Hildenbrand June 28, 2017, 8:48 a.m. UTC | #1

On 27.06.2017 19:59, Jim Mattson wrote:
> The memory operand fetched for INVVPID is 128 bits. Bits 63:16 are
> reserved and must be zero.  Otherwise, the instruction fails with
> VMfail(Invalid operand to INVEPT/INVVPID).  If the INVVPID_TYPE is 0
> (individual address invalidation), then bits 127:64 must be in
> canonical form, or the instruction fails with VMfail(Invalid operand
> to INVEPT/INVVPID).
> 
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---
>  arch/x86/kvm/vmx.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 42db3eb2d13b..9c34a98cc051 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -7651,7 +7651,11 @@ static int handle_invvpid(struct kvm_vcpu *vcpu)
>  	unsigned long type, types;
>  	gva_t gva;
>  	struct x86_exception e;
> -	int vpid;
> +	struct {
> +		u64 vpid : 16;
> +		u64 rsvd : 48;
> +		u64 gla;
> +	} operand;
>  
>  	if (!(vmx->nested.nested_vmx_secondary_ctls_high &
>  	      SECONDARY_EXEC_ENABLE_VPID) ||
> @@ -7681,17 +7685,28 @@ static int handle_invvpid(struct kvm_vcpu *vcpu)
>  	if (get_vmx_mem_address(vcpu, vmcs_readl(EXIT_QUALIFICATION),
>  			vmx_instruction_info, false, &gva))
>  		return 1;
> -	if (kvm_read_guest_virt(&vcpu->arch.emulate_ctxt, gva, &vpid,
> -				sizeof(u32), &e)) {
> +	if (kvm_read_guest_virt(&vcpu->arch.emulate_ctxt, gva, &operand,
> +				sizeof(operand), &e)) {
>  		kvm_inject_page_fault(vcpu, &e);
>  		return 1;
>  	}
> +	if (operand.rsvd) {
> +		nested_vmx_failValid(vcpu,
> +			VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID);
> +		return kvm_skip_emulated_instruction(vcpu);
> +	}
>  
>  	switch (type) {
>  	case VMX_VPID_EXTENT_INDIVIDUAL_ADDR:
> +		if (is_noncanonical_address(operand.gla)) {
> +			nested_vmx_failValid(vcpu,
> +				VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID);
> +			return kvm_skip_emulated_instruction(vcpu);
> +		}
> +		/* fall through */
>  	case VMX_VPID_EXTENT_SINGLE_CONTEXT:
>  	case VMX_VPID_EXTENT_SINGLE_NON_GLOBAL:
> -		if (!vpid) {
> +		if (!operand.vpid) {
>  			nested_vmx_failValid(vcpu,
>  				VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID);
>  			return kvm_skip_emulated_instruction(vcpu);
> 

Looks good to me!

Reviewed-by: David Hildenbrand <david@redhat.com>

Paolo Bonzini June 28, 2017, 12:12 p.m. UTC | #2

On 27/06/2017 19:59, Jim Mattson wrote:
> The memory operand fetched for INVVPID is 128 bits. Bits 63:16 are
> reserved and must be zero.  Otherwise, the instruction fails with
> VMfail(Invalid operand to INVEPT/INVVPID).  If the INVVPID_TYPE is 0
> (individual address invalidation), then bits 127:64 must be in
> canonical form, or the instruction fails with VMfail(Invalid operand
> to INVEPT/INVVPID).
> 
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---
>  arch/x86/kvm/vmx.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 42db3eb2d13b..9c34a98cc051 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -7651,7 +7651,11 @@ static int handle_invvpid(struct kvm_vcpu *vcpu)
>  	unsigned long type, types;
>  	gva_t gva;
>  	struct x86_exception e;
> -	int vpid;
> +	struct {
> +		u64 vpid : 16;
> +		u64 rsvd : 48;

I prefer using an u64 here (and checking the reserved bits via "&
~0xFFFF").  I find bit-endianness confusing because bits are in the
opposite order of the declarations (unlike byte-endianness, bits within
one word are usually drawn high to low).

Paolo

> +		u64 gla;
> +	} operand;
>  
>  	if (!(vmx->nested.nested_vmx_secondary_ctls_high &
>  	      SECONDARY_EXEC_ENABLE_VPID) ||
> @@ -7681,17 +7685,28 @@ static int handle_invvpid(struct kvm_vcpu *vcpu)
>  	if (get_vmx_mem_address(vcpu, vmcs_readl(EXIT_QUALIFICATION),
>  			vmx_instruction_info, false, &gva))
>  		return 1;
> -	if (kvm_read_guest_virt(&vcpu->arch.emulate_ctxt, gva, &vpid,
> -				sizeof(u32), &e)) {
> +	if (kvm_read_guest_virt(&vcpu->arch.emulate_ctxt, gva, &operand,
> +				sizeof(operand), &e)) {
>  		kvm_inject_page_fault(vcpu, &e);
>  		return 1;
>  	}
> +	if (operand.rsvd) {
> +		nested_vmx_failValid(vcpu,
> +			VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID);
> +		return kvm_skip_emulated_instruction(vcpu);
> +	}
>  
>  	switch (type) {
>  	case VMX_VPID_EXTENT_INDIVIDUAL_ADDR:
> +		if (is_noncanonical_address(operand.gla)) {
> +			nested_vmx_failValid(vcpu,
> +				VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID);
> +			return kvm_skip_emulated_instruction(vcpu);
> +		}
> +		/* fall through */
>  	case VMX_VPID_EXTENT_SINGLE_CONTEXT:
>  	case VMX_VPID_EXTENT_SINGLE_NON_GLOBAL:
> -		if (!vpid) {
> +		if (!operand.vpid) {
>  			nested_vmx_failValid(vcpu,
>  				VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID);
>  			return kvm_skip_emulated_instruction(vcpu);
>

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 42db3eb2d13b..9c34a98cc051 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -7651,7 +7651,11 @@  static int handle_invvpid(struct kvm_vcpu *vcpu)
 	unsigned long type, types;
 	gva_t gva;
 	struct x86_exception e;
-	int vpid;
+	struct {
+		u64 vpid : 16;
+		u64 rsvd : 48;
+		u64 gla;
+	} operand;
 
 	if (!(vmx->nested.nested_vmx_secondary_ctls_high &
 	      SECONDARY_EXEC_ENABLE_VPID) ||
@@ -7681,17 +7685,28 @@  static int handle_invvpid(struct kvm_vcpu *vcpu)
 	if (get_vmx_mem_address(vcpu, vmcs_readl(EXIT_QUALIFICATION),
 			vmx_instruction_info, false, &gva))
 		return 1;
-	if (kvm_read_guest_virt(&vcpu->arch.emulate_ctxt, gva, &vpid,
-				sizeof(u32), &e)) {
+	if (kvm_read_guest_virt(&vcpu->arch.emulate_ctxt, gva, &operand,
+				sizeof(operand), &e)) {
 		kvm_inject_page_fault(vcpu, &e);
 		return 1;
 	}
+	if (operand.rsvd) {
+		nested_vmx_failValid(vcpu,
+			VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID);
+		return kvm_skip_emulated_instruction(vcpu);
+	}
 
 	switch (type) {
 	case VMX_VPID_EXTENT_INDIVIDUAL_ADDR:
+		if (is_noncanonical_address(operand.gla)) {
+			nested_vmx_failValid(vcpu,
+				VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID);
+			return kvm_skip_emulated_instruction(vcpu);
+		}
+		/* fall through */
 	case VMX_VPID_EXTENT_SINGLE_CONTEXT:
 	case VMX_VPID_EXTENT_SINGLE_NON_GLOBAL:
-		if (!vpid) {
+		if (!operand.vpid) {
 			nested_vmx_failValid(vcpu,
 				VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID);
 			return kvm_skip_emulated_instruction(vcpu);

kvm: nVMX: Check memory operand to INVVPID

Commit Message

Comments

Patch