Re: [PATCH v2 1/3] KVM: x86: Reject nested CAP enablement if nested virtualization is disabled

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Sean Christopherson <seanjc@google.com> writes:

> Add a flag to explicitly track if nested virtualization is enabled, and use
> it enumerate that various nested CAPs are unsupported, and to reject
> enablement of said CAPs.  When the nested ops hooks were moved to their
> own structure, KVM's NULL-by-default behavior was deliberately dropped,
> with the changelog asserting that all was well.  That wasn't quite true;
> there is no danger to KVM, but now KVM is over-reporting support for
> KVM_CAP_NESTED_STATE and KVM_CAP_HYPERV_ENLIGHTENED_VMCS.
>
> Fixes: 33b22172452f ("KVM: x86: move nested-related kvm_x86_ops to a separate struct")
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  2 ++
>  arch/x86/kvm/hyperv.c           |  3 ++-
>  arch/x86/kvm/svm/svm.c          |  1 +
>  arch/x86/kvm/vmx/vmx.c          |  1 +
>  arch/x86/kvm/x86.c              | 12 +++++++-----
>  5 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index b517257a6315..36de689a579c 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1906,6 +1906,8 @@ struct kvm_x86_ops {
>  };
>  
>  struct kvm_x86_nested_ops {
> +	bool enabled;
> +
>  	void (*leave_nested)(struct kvm_vcpu *vcpu);
>  	bool (*is_exception_vmexit)(struct kvm_vcpu *vcpu, u8 vector,
>  				    u32 error_code);
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index 1ee0d23f8949..7184622992d0 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -2787,7 +2787,8 @@ int kvm_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
>  	};
>  	int i, nent = ARRAY_SIZE(cpuid_entries);
>  
> -	if (kvm_x86_ops.nested_ops->get_evmcs_version)
> +	if (kvm_x86_ops.nested_ops->enabled &&
> +	    kvm_x86_ops.nested_ops->get_evmcs_version)
>  		evmcs_ver = kvm_x86_ops.nested_ops->get_evmcs_version(vcpu);
>  
>  	if (cpuid->nent < nent)
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index ef69a51ab27f..163440fca59f 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -5646,6 +5646,7 @@ static __init int svm_hardware_setup(void)
>  		if (r)
>  			return r;
>  	}
> +	svm_nested_ops.enabled = nested;
>  
>  	/*
>  	 * KVM's MMU doesn't support using 2-level paging for itself, and thus
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 3681d565f177..ef9e1bc39edb 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -8786,6 +8786,7 @@ __init int vmx_hardware_setup(void)
>  		if (r)
>  			return r;
>  	}
> +	vmx_nested_ops.enabled = nested;
>  
>  	kvm_set_posted_intr_wakeup_handler(pi_wakeup_handler);
>  
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 0626e835e9eb..cd9a5ff47f6f 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2346,7 +2346,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  			r &= ~KVM_X2APIC_ENABLE_SUPPRESS_EOI_BROADCAST;
>  		break;
>  	case KVM_CAP_NESTED_STATE:
> -		r = kvm_x86_ops.nested_ops->get_state ?
> +		r = kvm_x86_ops.nested_ops->enabled ?
>  			kvm_x86_ops.nested_ops->get_state(NULL, NULL, 0) : 0;
>  		break;
>  #ifdef CONFIG_KVM_HYPERV
> @@ -2354,7 +2354,8 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  		r = kvm_x86_ops.enable_l2_tlb_flush != NULL;
>  		break;
>  	case KVM_CAP_HYPERV_ENLIGHTENED_VMCS:
> -		r = kvm_x86_ops.nested_ops->enable_evmcs != NULL;
> +		r = kvm_x86_ops.nested_ops->enabled &&
> +		    kvm_x86_ops.nested_ops->enable_evmcs != NULL;
>  		break;
>  #endif
>  	case KVM_CAP_SMALLER_MAXPHYADDR:
> @@ -3366,7 +3367,8 @@ static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
>  			uint16_t vmcs_version;
>  			void __user *user_ptr;
>  
> -			if (!kvm_x86_ops.nested_ops->enable_evmcs)
> +			if (!kvm_x86_ops.nested_ops->enabled ||
> +			    !kvm_x86_ops.nested_ops->enable_evmcs)
>  				return -ENOTTY;
>  			r = kvm_x86_ops.nested_ops->enable_evmcs(vcpu, &vmcs_version);
>  			if (!r) {
> @@ -3732,7 +3734,7 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>  		u32 user_data_size;
>  
>  		r = -EINVAL;
> -		if (!kvm_x86_ops.nested_ops->get_state)
> +		if (!kvm_x86_ops.nested_ops->enabled)
>  			break;
>  
>  		BUILD_BUG_ON(sizeof(user_data_size) != sizeof(user_kvm_nested_state->size));
> @@ -3762,7 +3764,7 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>  		int idx;
>  
>  		r = -EINVAL;
> -		if (!kvm_x86_ops.nested_ops->set_state)
> +		if (!kvm_x86_ops.nested_ops->enabled)
>  			break;
>  
>  		r = -EFAULT;

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly