diff mbox

[v2] KVM: nVMX: fix nested_vmx_check_vmptr failure paths under debugging

Message ID 20170519134851.10616-1-rkrcmar@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Radim Krčmář May 19, 2017, 1:48 p.m. UTC 
kvm_skip_emulated_instruction() will return 0 if userspace is
single-stepping the guest.

kvm_skip_emulated_instruction() uses return status convention of exit
handler: 0 means "exit to userspace" and 1 means "continue vm entries".
The problem is that nested_vmx_check_vmptr() return status means
something else: 0 is ok, 1 is error.

This means we would continue executing after a failure.  Static checker
noticed it because vmptr was not initialized.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Fixes: 6affcbedcac7 ("KVM: x86: Add kvm_skip_emulated_instruction and use it.")
Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 Second try -- moves a lot of code around to make it less ugly and keep
 the same behavior as v1, hopefully.

 arch/x86/kvm/vmx.c | 140 ++++++++++++++++++++++-------------------------------
 1 file changed, 57 insertions(+), 83 deletions(-)

Comments

Paolo Bonzini May 30, 2017, 2:34 p.m. UTC | #1 
On 19/05/2017 15:48, Radim Krčmář wrote:
> kvm_skip_emulated_instruction() will return 0 if userspace is
> single-stepping the guest.
> 
> kvm_skip_emulated_instruction() uses return status convention of exit
> handler: 0 means "exit to userspace" and 1 means "continue vm entries".
> The problem is that nested_vmx_check_vmptr() return status means
> something else: 0 is ok, 1 is error.
> 
> This means we would continue executing after a failure.  Static checker
> noticed it because vmptr was not initialized.
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Fixes: 6affcbedcac7 ("KVM: x86: Add kvm_skip_emulated_instruction and use it.")
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  Second try -- moves a lot of code around to make it less ugly and keep
>  the same behavior as v1, hopefully.
> 
>  arch/x86/kvm/vmx.c | 140 ++++++++++++++++++++++-------------------------------
>  1 file changed, 57 insertions(+), 83 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 9ff1b04502c9..dd274db9bf77 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -6914,97 +6914,21 @@ static int get_vmx_mem_address(struct kvm_vcpu *vcpu,
>  	return 0;
>  }
>  
> -/*
> - * This function performs the various checks including
> - * - if it's 4KB aligned
> - * - No bits beyond the physical address width are set
> - * - Returns 0 on success or else 1
> - * (Intel SDM Section 30.3)
> - */
> -static int nested_vmx_check_vmptr(struct kvm_vcpu *vcpu, int exit_reason,
> -				  gpa_t *vmpointer)
> +static int nested_vmx_get_vmptr(struct kvm_vcpu *vcpu, gpa_t *vmpointer)
>  {
>  	gva_t gva;
> -	gpa_t vmptr;
>  	struct x86_exception e;
> -	struct page *page;
> -	struct vcpu_vmx *vmx = to_vmx(vcpu);
> -	int maxphyaddr = cpuid_maxphyaddr(vcpu);
>  
>  	if (get_vmx_mem_address(vcpu, vmcs_readl(EXIT_QUALIFICATION),
>  			vmcs_read32(VMX_INSTRUCTION_INFO), false, &gva))
>  		return 1;
>  
> -	if (kvm_read_guest_virt(&vcpu->arch.emulate_ctxt, gva, &vmptr,
> -				sizeof(vmptr), &e)) {
> +	if (kvm_read_guest_virt(&vcpu->arch.emulate_ctxt, gva, vmpointer,
> +				sizeof(*vmpointer), &e)) {
>  		kvm_inject_page_fault(vcpu, &e);
>  		return 1;
>  	}
>  
> -	switch (exit_reason) {
> -	case EXIT_REASON_VMON:
> -		/*
> -		 * SDM 3: 24.11.5
> -		 * The first 4 bytes of VMXON region contain the supported
> -		 * VMCS revision identifier
> -		 *
> -		 * Note - IA32_VMX_BASIC[48] will never be 1
> -		 * for the nested case;
> -		 * which replaces physical address width with 32
> -		 *
> -		 */
> -		if (!PAGE_ALIGNED(vmptr) || (vmptr >> maxphyaddr)) {
> -			nested_vmx_failInvalid(vcpu);
> -			return kvm_skip_emulated_instruction(vcpu);
> -		}
> -
> -		page = nested_get_page(vcpu, vmptr);
> -		if (page == NULL) {
> -			nested_vmx_failInvalid(vcpu);
> -			return kvm_skip_emulated_instruction(vcpu);
> -		}
> -		if (*(u32 *)kmap(page) != VMCS12_REVISION) {
> -			kunmap(page);
> -			nested_release_page_clean(page);
> -			nested_vmx_failInvalid(vcpu);
> -			return kvm_skip_emulated_instruction(vcpu);
> -		}
> -		kunmap(page);
> -		nested_release_page_clean(page);
> -		vmx->nested.vmxon_ptr = vmptr;
> -		break;
> -	case EXIT_REASON_VMCLEAR:
> -		if (!PAGE_ALIGNED(vmptr) || (vmptr >> maxphyaddr)) {
> -			nested_vmx_failValid(vcpu,
> -					     VMXERR_VMCLEAR_INVALID_ADDRESS);
> -			return kvm_skip_emulated_instruction(vcpu);
> -		}
> -
> -		if (vmptr == vmx->nested.vmxon_ptr) {
> -			nested_vmx_failValid(vcpu,
> -					     VMXERR_VMCLEAR_VMXON_POINTER);
> -			return kvm_skip_emulated_instruction(vcpu);
> -		}
> -		break;
> -	case EXIT_REASON_VMPTRLD:
> -		if (!PAGE_ALIGNED(vmptr) || (vmptr >> maxphyaddr)) {
> -			nested_vmx_failValid(vcpu,
> -					     VMXERR_VMPTRLD_INVALID_ADDRESS);
> -			return kvm_skip_emulated_instruction(vcpu);
> -		}
> -
> -		if (vmptr == vmx->nested.vmxon_ptr) {
> -			nested_vmx_failValid(vcpu,
> -					     VMXERR_VMPTRLD_VMXON_POINTER);
> -			return kvm_skip_emulated_instruction(vcpu);
> -		}
> -		break;
> -	default:
> -		return 1; /* shouldn't happen */
> -	}
> -
> -	if (vmpointer)
> -		*vmpointer = vmptr;
>  	return 0;
>  }
>  
> @@ -7066,6 +6990,8 @@ static int enter_vmx_operation(struct kvm_vcpu *vcpu)
>  static int handle_vmon(struct kvm_vcpu *vcpu)
>  {
>  	int ret;
> +	gpa_t vmptr;
> +	struct page *page;
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
>  	const u64 VMXON_NEEDED_FEATURES = FEATURE_CONTROL_LOCKED
>  		| FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX;
> @@ -7095,9 +7021,37 @@ static int handle_vmon(struct kvm_vcpu *vcpu)
>  		return 1;
>  	}
>  
> -	if (nested_vmx_check_vmptr(vcpu, EXIT_REASON_VMON, NULL))
> +	if (nested_vmx_get_vmptr(vcpu, &vmptr))
>  		return 1;
> - 
> +
> +	/*
> +	 * SDM 3: 24.11.5
> +	 * The first 4 bytes of VMXON region contain the supported
> +	 * VMCS revision identifier
> +	 *
> +	 * Note - IA32_VMX_BASIC[48] will never be 1 for the nested case;
> +	 * which replaces physical address width with 32
> +	 */
> +	if (!PAGE_ALIGNED(vmptr) || (vmptr >> cpuid_maxphyaddr(vcpu))) {
> +		nested_vmx_failInvalid(vcpu);
> +		return kvm_skip_emulated_instruction(vcpu);
> +	}
> +
> +	page = nested_get_page(vcpu, vmptr);
> +	if (page == NULL) {
> +		nested_vmx_failInvalid(vcpu);
> +		return kvm_skip_emulated_instruction(vcpu);
> +	}
> +	if (*(u32 *)kmap(page) != VMCS12_REVISION) {
> +		kunmap(page);
> +		nested_release_page_clean(page);
> +		nested_vmx_failInvalid(vcpu);
> +		return kvm_skip_emulated_instruction(vcpu);
> +	}
> +	kunmap(page);
> +	nested_release_page_clean(page);
> +
> +	vmx->nested.vmxon_ptr = vmptr;
>  	ret = enter_vmx_operation(vcpu);
>  	if (ret)
>  		return ret;
> @@ -7213,9 +7167,19 @@ static int handle_vmclear(struct kvm_vcpu *vcpu)
>  	if (!nested_vmx_check_permission(vcpu))
>  		return 1;
>  
> -	if (nested_vmx_check_vmptr(vcpu, EXIT_REASON_VMCLEAR, &vmptr))
> +	if (nested_vmx_get_vmptr(vcpu, &vmptr))
>  		return 1;
>  
> +	if (!PAGE_ALIGNED(vmptr) || (vmptr >> cpuid_maxphyaddr(vcpu))) {
> +		nested_vmx_failValid(vcpu, VMXERR_VMCLEAR_INVALID_ADDRESS);
> +		return kvm_skip_emulated_instruction(vcpu);
> +	}
> +
> +	if (vmptr == vmx->nested.vmxon_ptr) {
> +		nested_vmx_failValid(vcpu, VMXERR_VMCLEAR_VMXON_POINTER);
> +		return kvm_skip_emulated_instruction(vcpu);
> +	}
> +
>  	if (vmptr == vmx->nested.current_vmptr)
>  		nested_release_vmcs12(vmx);
>  
> @@ -7545,9 +7509,19 @@ static int handle_vmptrld(struct kvm_vcpu *vcpu)
>  	if (!nested_vmx_check_permission(vcpu))
>  		return 1;
>  
> -	if (nested_vmx_check_vmptr(vcpu, EXIT_REASON_VMPTRLD, &vmptr))
> +	if (nested_vmx_get_vmptr(vcpu, &vmptr))
>  		return 1;
>  
> +	if (!PAGE_ALIGNED(vmptr) || (vmptr >> cpuid_maxphyaddr(vcpu))) {
> +		nested_vmx_failValid(vcpu, VMXERR_VMPTRLD_INVALID_ADDRESS);
> +		return kvm_skip_emulated_instruction(vcpu);
> +	}
> +
> +	if (vmptr == vmx->nested.vmxon_ptr) {
> +		nested_vmx_failValid(vcpu, VMXERR_VMPTRLD_VMXON_POINTER);
> +		return kvm_skip_emulated_instruction(vcpu);
> +	}
> +
>  	if (vmx->nested.current_vmptr != vmptr) {
>  		struct vmcs12 *new_vmcs12;
>  		struct page *page;
> 

Nice, diffstat speaks for itself.  Queuing it.

Thanks,

Paolo
diff mbox

Patch

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 9ff1b04502c9..dd274db9bf77 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -6914,97 +6914,21 @@  static int get_vmx_mem_address(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
-/*
- * This function performs the various checks including
- * - if it's 4KB aligned
- * - No bits beyond the physical address width are set
- * - Returns 0 on success or else 1
- * (Intel SDM Section 30.3)
- */
-static int nested_vmx_check_vmptr(struct kvm_vcpu *vcpu, int exit_reason,
-				  gpa_t *vmpointer)
+static int nested_vmx_get_vmptr(struct kvm_vcpu *vcpu, gpa_t *vmpointer)
 {
 	gva_t gva;
-	gpa_t vmptr;
 	struct x86_exception e;
-	struct page *page;
-	struct vcpu_vmx *vmx = to_vmx(vcpu);
-	int maxphyaddr = cpuid_maxphyaddr(vcpu);
 
 	if (get_vmx_mem_address(vcpu, vmcs_readl(EXIT_QUALIFICATION),
 			vmcs_read32(VMX_INSTRUCTION_INFO), false, &gva))
 		return 1;
 
-	if (kvm_read_guest_virt(&vcpu->arch.emulate_ctxt, gva, &vmptr,
-				sizeof(vmptr), &e)) {
+	if (kvm_read_guest_virt(&vcpu->arch.emulate_ctxt, gva, vmpointer,
+				sizeof(*vmpointer), &e)) {
 		kvm_inject_page_fault(vcpu, &e);
 		return 1;
 	}
 
-	switch (exit_reason) {
-	case EXIT_REASON_VMON:
-		/*
-		 * SDM 3: 24.11.5
-		 * The first 4 bytes of VMXON region contain the supported
-		 * VMCS revision identifier
-		 *
-		 * Note - IA32_VMX_BASIC[48] will never be 1
-		 * for the nested case;
-		 * which replaces physical address width with 32
-		 *
-		 */
-		if (!PAGE_ALIGNED(vmptr) || (vmptr >> maxphyaddr)) {
-			nested_vmx_failInvalid(vcpu);
-			return kvm_skip_emulated_instruction(vcpu);
-		}
-
-		page = nested_get_page(vcpu, vmptr);
-		if (page == NULL) {
-			nested_vmx_failInvalid(vcpu);
-			return kvm_skip_emulated_instruction(vcpu);
-		}
-		if (*(u32 *)kmap(page) != VMCS12_REVISION) {
-			kunmap(page);
-			nested_release_page_clean(page);
-			nested_vmx_failInvalid(vcpu);
-			return kvm_skip_emulated_instruction(vcpu);
-		}
-		kunmap(page);
-		nested_release_page_clean(page);
-		vmx->nested.vmxon_ptr = vmptr;
-		break;
-	case EXIT_REASON_VMCLEAR:
-		if (!PAGE_ALIGNED(vmptr) || (vmptr >> maxphyaddr)) {
-			nested_vmx_failValid(vcpu,
-					     VMXERR_VMCLEAR_INVALID_ADDRESS);
-			return kvm_skip_emulated_instruction(vcpu);
-		}
-
-		if (vmptr == vmx->nested.vmxon_ptr) {
-			nested_vmx_failValid(vcpu,
-					     VMXERR_VMCLEAR_VMXON_POINTER);
-			return kvm_skip_emulated_instruction(vcpu);
-		}
-		break;
-	case EXIT_REASON_VMPTRLD:
-		if (!PAGE_ALIGNED(vmptr) || (vmptr >> maxphyaddr)) {
-			nested_vmx_failValid(vcpu,
-					     VMXERR_VMPTRLD_INVALID_ADDRESS);
-			return kvm_skip_emulated_instruction(vcpu);
-		}
-
-		if (vmptr == vmx->nested.vmxon_ptr) {
-			nested_vmx_failValid(vcpu,
-					     VMXERR_VMPTRLD_VMXON_POINTER);
-			return kvm_skip_emulated_instruction(vcpu);
-		}
-		break;
-	default:
-		return 1; /* shouldn't happen */
-	}
-
-	if (vmpointer)
-		*vmpointer = vmptr;
 	return 0;
 }
 
@@ -7066,6 +6990,8 @@  static int enter_vmx_operation(struct kvm_vcpu *vcpu)
 static int handle_vmon(struct kvm_vcpu *vcpu)
 {
 	int ret;
+	gpa_t vmptr;
+	struct page *page;
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	const u64 VMXON_NEEDED_FEATURES = FEATURE_CONTROL_LOCKED
 		| FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX;
@@ -7095,9 +7021,37 @@  static int handle_vmon(struct kvm_vcpu *vcpu)
 		return 1;
 	}
 
-	if (nested_vmx_check_vmptr(vcpu, EXIT_REASON_VMON, NULL))
+	if (nested_vmx_get_vmptr(vcpu, &vmptr))
 		return 1;
- 
+
+	/*
+	 * SDM 3: 24.11.5
+	 * The first 4 bytes of VMXON region contain the supported
+	 * VMCS revision identifier
+	 *
+	 * Note - IA32_VMX_BASIC[48] will never be 1 for the nested case;
+	 * which replaces physical address width with 32
+	 */
+	if (!PAGE_ALIGNED(vmptr) || (vmptr >> cpuid_maxphyaddr(vcpu))) {
+		nested_vmx_failInvalid(vcpu);
+		return kvm_skip_emulated_instruction(vcpu);
+	}
+
+	page = nested_get_page(vcpu, vmptr);
+	if (page == NULL) {
+		nested_vmx_failInvalid(vcpu);
+		return kvm_skip_emulated_instruction(vcpu);
+	}
+	if (*(u32 *)kmap(page) != VMCS12_REVISION) {
+		kunmap(page);
+		nested_release_page_clean(page);
+		nested_vmx_failInvalid(vcpu);
+		return kvm_skip_emulated_instruction(vcpu);
+	}
+	kunmap(page);
+	nested_release_page_clean(page);
+
+	vmx->nested.vmxon_ptr = vmptr;
 	ret = enter_vmx_operation(vcpu);
 	if (ret)
 		return ret;
@@ -7213,9 +7167,19 @@  static int handle_vmclear(struct kvm_vcpu *vcpu)
 	if (!nested_vmx_check_permission(vcpu))
 		return 1;
 
-	if (nested_vmx_check_vmptr(vcpu, EXIT_REASON_VMCLEAR, &vmptr))
+	if (nested_vmx_get_vmptr(vcpu, &vmptr))
 		return 1;
 
+	if (!PAGE_ALIGNED(vmptr) || (vmptr >> cpuid_maxphyaddr(vcpu))) {
+		nested_vmx_failValid(vcpu, VMXERR_VMCLEAR_INVALID_ADDRESS);
+		return kvm_skip_emulated_instruction(vcpu);
+	}
+
+	if (vmptr == vmx->nested.vmxon_ptr) {
+		nested_vmx_failValid(vcpu, VMXERR_VMCLEAR_VMXON_POINTER);
+		return kvm_skip_emulated_instruction(vcpu);
+	}
+
 	if (vmptr == vmx->nested.current_vmptr)
 		nested_release_vmcs12(vmx);
 
@@ -7545,9 +7509,19 @@  static int handle_vmptrld(struct kvm_vcpu *vcpu)
 	if (!nested_vmx_check_permission(vcpu))
 		return 1;
 
-	if (nested_vmx_check_vmptr(vcpu, EXIT_REASON_VMPTRLD, &vmptr))
+	if (nested_vmx_get_vmptr(vcpu, &vmptr))
 		return 1;
 
+	if (!PAGE_ALIGNED(vmptr) || (vmptr >> cpuid_maxphyaddr(vcpu))) {
+		nested_vmx_failValid(vcpu, VMXERR_VMPTRLD_INVALID_ADDRESS);
+		return kvm_skip_emulated_instruction(vcpu);
+	}
+
+	if (vmptr == vmx->nested.vmxon_ptr) {
+		nested_vmx_failValid(vcpu, VMXERR_VMPTRLD_VMXON_POINTER);
+		return kvm_skip_emulated_instruction(vcpu);
+	}
+
 	if (vmx->nested.current_vmptr != vmptr) {
 		struct vmcs12 *new_vmcs12;
 		struct page *page;