linux-kernel - Re: [PATCH 07/15] KVM: x86: Add support for RDPID without RDTSCP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3699a28f75fbb541ab14e90d5856c4b3a583497e.camel@redhat.com>
Date:   Mon, 10 May 2021 11:20:45 +0300
From:   Maxim Levitsky <mlevitsk@...hat.com>
To:     Sean Christopherson <seanjc@...gle.com>,
        Paolo Bonzini <pbonzini@...hat.com>
Cc:     Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, Xiaoyao Li <xiaoyao.li@...el.com>,
        Reiji Watanabe <reijiw@...gle.com>
Subject: Re: [PATCH 07/15] KVM: x86: Add support for RDPID without RDTSCP

On Tue, 2021-05-04 at 10:17 -0700, Sean Christopherson wrote:
> Allow userspace to enable RDPID for a guest without also enabling RDTSCP.
> Aside from checking for RDPID support in the obvious flows, VMX also needs
> to set ENABLE_RDTSCP=1 when RDPID is exposed.
> 
> For the record, there is no known scenario where enabling RDPID without
> RDTSCP is desirable.  But, both AMD and Intel architectures allow for the
> condition, i.e. this is purely to make KVM more architecturally accurate.
> 
> Fixes: 41cd02c6f7f6 ("kvm: x86: Expose RDPID in KVM_GET_SUPPORTED_CPUID")
> Cc: stable@...r.kernel.org
> Reported-by: Reiji Watanabe <reijiw@...gle.com>
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>  arch/x86/kvm/svm/svm.c |  6 ++++--
>  arch/x86/kvm/vmx/vmx.c | 27 +++++++++++++++++++++++----
>  arch/x86/kvm/x86.c     |  3 ++-
>  3 files changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index b3153d40cc4d..231b9650d864 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -2669,7 +2669,8 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		if (tsc_aux_uret_slot < 0)
>  			return 1;
>  		if (!msr_info->host_initiated &&
> -		    !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP))
> +		    !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP) &&
> +		    !guest_cpuid_has(vcpu, X86_FEATURE_RDPID))
>  			return 1;
>  		msr_info->data = svm->tsc_aux;
>  		break;
> @@ -2891,7 +2892,8 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>  			return 1;
>  
>  		if (!msr->host_initiated &&
> -		    !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP))
> +		    !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP) &&
> +		    !guest_cpuid_has(vcpu, X86_FEATURE_RDPID))
>  			return 1;
>  
>  		/*
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 990ee339a05f..42e4bbaa299a 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -1788,7 +1788,8 @@ static void setup_msrs(struct vcpu_vmx *vmx)
>  	if (update_transition_efer(vmx))
>  		vmx_setup_uret_msr(vmx, MSR_EFER);
>  
> -	if (guest_cpuid_has(&vmx->vcpu, X86_FEATURE_RDTSCP))
> +	if (guest_cpuid_has(&vmx->vcpu, X86_FEATURE_RDTSCP)  ||
> +	    guest_cpuid_has(&vmx->vcpu, X86_FEATURE_RDPID))
>  		vmx_setup_uret_msr(vmx, MSR_TSC_AUX);
>  
>  	vmx_setup_uret_msr(vmx, MSR_IA32_TSX_CTRL);
> @@ -1994,7 +1995,8 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		break;
>  	case MSR_TSC_AUX:
>  		if (!msr_info->host_initiated &&
> -		    !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP))
> +		    !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP) &&
> +		    !guest_cpuid_has(vcpu, X86_FEATURE_RDPID))
>  			return 1;
>  		goto find_uret_msr;
>  	case MSR_IA32_DEBUGCTLMSR:
> @@ -2314,7 +2316,8 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		break;
>  	case MSR_TSC_AUX:
>  		if (!msr_info->host_initiated &&
> -		    !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP))
> +		    !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP) &&
> +		    !guest_cpuid_has(vcpu, X86_FEATURE_RDPID))
>  			return 1;
>  		/* Check reserved bit, higher 32 bits should be zero */
>  		if ((data >> 32) != 0)
> @@ -4368,7 +4371,23 @@ static void vmx_compute_secondary_exec_control(struct vcpu_vmx *vmx)
>  						  xsaves_enabled, false);
>  	}
>  
> -	vmx_adjust_sec_exec_feature(vmx, &exec_control, rdtscp, RDTSCP);
> +	/*
> +	 * RDPID is also gated by ENABLE_RDTSCP, turn on the control if either
> +	 * feature is exposed to the guest.  This creates a virtualization hole
> +	 * if both are supported in hardware but only one is exposed to the
> +	 * guest, but letting the guest execute RDTSCP or RDPID when either one
> +	 * is advertised is preferable to emulating the advertised instruction
> +	 * in KVM on #UD, and obviously better than incorrectly injecting #UD.
> +	 */
> +	if (cpu_has_vmx_rdtscp()) {
> +		bool rdpid_or_rdtscp_enabled =
> +			guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP) ||
> +			guest_cpuid_has(vcpu, X86_FEATURE_RDPID);
> +
> +		vmx_adjust_secondary_exec_control(vmx, &exec_control,
> +						  SECONDARY_EXEC_ENABLE_RDTSCP,
> +						  rdpid_or_rdtscp_enabled, false);
> +	}
>  	vmx_adjust_sec_exec_feature(vmx, &exec_control, invpcid, INVPCID);
>  
>  	vmx_adjust_sec_exec_exiting(vmx, &exec_control, rdrand, RDRAND);
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e304447be42d..b4516d303413 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5978,7 +5978,8 @@ static void kvm_init_msr_list(void)
>  				continue;
>  			break;
>  		case MSR_TSC_AUX:
> -			if (!kvm_cpu_cap_has(X86_FEATURE_RDTSCP))
> +			if (!kvm_cpu_cap_has(X86_FEATURE_RDTSCP) &&
> +			    !kvm_cpu_cap_has(X86_FEATURE_RDPID))
>  				continue;
>  			break;
>  		case MSR_IA32_UMWAIT_CONTROL:

Reviewed-by : Maxim Levitsky <mlevitsk@...hat.com>

Best regards,
	Maxim Levitsky