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

[Paolo Bonzini]

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@xxxxxxxxxx>
> Fixes: 6affcbedcac7 ("KVM: x86: Add kvm_skip_emulated_instruction and use it.")
> Signed-off-by: Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx>
> ---
>  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