linux-kernel - Re: [PATCH 2/2] KVM: SVM: Add support for MPK feature on AMD

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:   Wed, 6 May 2020 15:26:43 -0700
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Babu Moger <babu.moger@....com>
Cc:     corbet@....net, tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
        hpa@...or.com, pbonzini@...hat.com, x86@...nel.org,
        vkuznets@...hat.com, wanpengli@...cent.com, jmattson@...gle.com,
        joro@...tes.org, dave.hansen@...ux.intel.com, luto@...nel.org,
        peterz@...radead.org, mchehab+samsung@...nel.org,
        changbin.du@...el.com, namit@...are.com, bigeasy@...utronix.de,
        yang.shi@...ux.alibaba.com, asteinhauser@...gle.com,
        anshuman.khandual@....com, jan.kiszka@...mens.com,
        akpm@...ux-foundation.org, steven.price@....com,
        rppt@...ux.vnet.ibm.com, peterx@...hat.com,
        dan.j.williams@...el.com, arjunroy@...gle.com, logang@...tatee.com,
        thellstrom@...are.com, aarcange@...hat.com, justin.he@....com,
        robin.murphy@....com, ira.weiny@...el.com, keescook@...omium.org,
        jgross@...e.com, andrew.cooper3@...rix.com,
        pawan.kumar.gupta@...ux.intel.com, fenghua.yu@...el.com,
        vineela.tummalapalli@...el.com, yamada.masahiro@...ionext.com,
        sam@...nborg.org, acme@...hat.com, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Subject: Re: [PATCH 2/2] KVM: SVM: Add support for MPK feature on AMD

On Wed, May 06, 2020 at 05:02:21PM -0500, Babu Moger wrote:
>  static __init int svm_hardware_setup(void)
> @@ -1300,6 +1304,8 @@ static void svm_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  		indirect_branch_prediction_barrier();
>  	}
>  	avic_vcpu_load(vcpu, cpu);
> +
> +	svm->host_pkru = read_pkru();

Move vcpu_vmx's host_prku to kvm_vcpu_arch instead of duplicating it to
SVM.  And I'm 99% certain "vcpu->arch.host_pkru = read_pkru()" can be moved
to kvm_arch_vcpu_load().  The only direct calls to vmx_vcpu_load() are to
get the right VMCS loaded.  Actually, those calls shouldn't be using
vmx_vcpu_load(), especially since that'll trigger IBPB.  I'll send a patch
for that.

>  }
>  
>  static void svm_vcpu_put(struct kvm_vcpu *vcpu)
> @@ -3318,6 +3324,12 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu)
>  	clgi();
>  	kvm_load_guest_xsave_state(vcpu);
>  
> +	/* Load the guest pkru state */
> +	if (static_cpu_has(X86_FEATURE_PKU) &&
> +	    kvm_read_cr4_bits(vcpu, X86_CR4_PKE) &&
> +	    vcpu->arch.pkru != svm->host_pkru)
> +		__write_pkru(vcpu->arch.pkru);

This and the restoration should be moved to common x86 helpers, at a glance
they look identical.

In short, pretty much all of this belongs in common x86.

> +
>  	if (lapic_in_kernel(vcpu) &&
>  		vcpu->arch.apic->lapic_timer.timer_advance_ns)
>  		kvm_wait_lapic_expire(vcpu);
> @@ -3371,6 +3383,14 @@ static void svm_vcpu_run(struct kvm_vcpu *vcpu)
>  	if (unlikely(svm->vmcb->control.exit_code == SVM_EXIT_NMI))
>  		kvm_before_interrupt(&svm->vcpu);
>  
> +	/* Save the guest pkru state and restore the host pkru state back */
> +	if (static_cpu_has(X86_FEATURE_PKU) &&
> +	    kvm_read_cr4_bits(vcpu, X86_CR4_PKE)) {
> +		vcpu->arch.pkru = rdpkru();
> +		if (vcpu->arch.pkru != svm->host_pkru)
> +			__write_pkru(svm->host_pkru);
> +	}
> +
>  	kvm_load_host_xsave_state(vcpu);
>  	stgi();
>  
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index df3474f4fb02..5d20a28c1b0e 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -158,6 +158,8 @@ struct vcpu_svm {
>  	u64 *avic_physical_id_cache;
>  	bool avic_is_running;
>  
> +	u32 host_pkru;
> +
>  	/*
>  	 * Per-vcpu list of struct amd_svm_iommu_ir:
>  	 * This is used mainly to store interrupt remapping information used
>