linux-kernel - Re: [PATCH 3/4] KVM/VMX: Add identical ept table pointer check

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]

Date:   Mon, 02 Jul 2018 17:09:57 +0200
From:   Vitaly Kuznetsov <vkuznets@...hat.com>
To:     Tianyu Lan <Tianyu.Lan@...rosoft.com>
Cc:     "pbonzini\@redhat.com" <pbonzini@...hat.com>,
        "rkrcmar\@redhat.com" <rkrcmar@...hat.com>,
        "tglx\@linutronix.de" <tglx@...utronix.de>,
        "mingo\@redhat.com" <mingo@...hat.com>,
        "hpa\@zytor.com" <hpa@...or.com>,
        "x86\@kernel.org" <x86@...nel.org>,
        "kvm\@vger.kernel.org" <kvm@...r.kernel.org>,
        "linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>,
        "Michael Kelley \(EOSG\)" <Michael.H.Kelley@...rosoft.com>,
        KY Srinivasan <kys@...rosoft.com>
Subject: Re: [PATCH 3/4] KVM/VMX: Add identical ept table pointer check

Tianyu Lan <Tianyu.Lan@...rosoft.com> writes:

> This patch is to check ept table pointer of each cpus when set ept
> tables and store identical ept table pointer if all ept table pointers
> of single VM are same. This is for support of para-virt ept flush
> hypercall.
>
> Signed-off-by: Lan Tianyu <Tianyu.Lan@...rosoft.com>
> ---
>  arch/x86/kvm/vmx.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 1689f433f3a0..0b1e4e9fef2b 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -194,6 +194,9 @@ struct kvm_vmx {
>  	unsigned int tss_addr;
>  	bool ept_identity_pagetable_done;
>  	gpa_t ept_identity_map_addr;
> +
> +	u64 identical_ept_pointer;
> +	spinlock_t ept_pointer_lock;
>  };
>
>  #define NR_AUTOLOAD_MSRS 8
> @@ -853,6 +856,7 @@ struct vcpu_vmx {
>  	 */
>  	u64 msr_ia32_feature_control;
>  	u64 msr_ia32_feature_control_valid_bits;
> +	u64 ept_pointer;
>  };
>
>  enum segment_cache_field {
> @@ -4958,6 +4962,29 @@ static u64 construct_eptp(struct kvm_vcpu *vcpu, unsigned long root_hpa)
>  	return eptp;
>  }
>
> +static void check_ept_pointer(struct kvm_vcpu *vcpu, u64 eptp)
> +{
> +	struct kvm *kvm = vcpu->kvm;
> +	u64 tmp_eptp = INVALID_PAGE;
> +	int i;
> +
> +	spin_lock(&to_kvm_vmx(kvm)->ept_pointer_lock);
> +	to_vmx(vcpu)->ept_pointer = eptp;
> +
> +	kvm_for_each_vcpu(i, vcpu, kvm) {
> +		if (!VALID_PAGE(tmp_eptp)) {
> +			tmp_eptp = to_vmx(vcpu)->ept_pointer;
> +		} else if (tmp_eptp != to_vmx(vcpu)->ept_pointer) {
> +			to_kvm_vmx(kvm)->identical_ept_pointer = INVALID_PAGE;
> +			spin_unlock(&to_kvm_vmx(kvm)->ept_pointer_lock);
> +			return;
> +		}
> +	}
> +
> +	to_kvm_vmx(kvm)->identical_ept_pointer = tmp_eptp;
> +	spin_unlock(&to_kvm_vmx(kvm)->ept_pointer_lock);

It seems we can get away with identical_ept_pointer being just 'bool':
go through the vCPU list and compare ept_pointer with ept_pointer for
the current vcpu. It would also make sense to rename it to something
like 'ept_pointers_match'.

I'm also not sure we need a dedicated ept_pointer_lock, can't we just
use the already existent mmu_lock from struct kvm?

> +}
> +
>  static void vmx_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
>  {
>  	unsigned long guest_cr3;
> @@ -4967,6 +4994,8 @@ static void vmx_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
>  	if (enable_ept) {
>  		eptp = construct_eptp(vcpu, cr3);
>  		vmcs_write64(EPT_POINTER, eptp);
> +		check_ept_pointer(vcpu, eptp);

Do we always get here when we need? E.g, do we need to enforce
CPU_BASED_CR3_STORE_EXITING?

> +
>  		if (enable_unrestricted_guest || is_paging(vcpu) ||
>  		    is_guest_mode(vcpu))
>  			guest_cr3 = kvm_read_cr3(vcpu);
> @@ -10383,6 +10412,8 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
>
>  static int vmx_vm_init(struct kvm *kvm)
>  {
> +	spin_lock_init(&to_kvm_vmx(kvm)->ept_pointer_lock);
> +
>  	if (!ple_gap)
>  		kvm->arch.pause_in_guest = true;
>  	return 0;

-- 
  Vitaly