linux-kernel - Re: [PATCH v2 6/9] KVM: SVM: Add MSR_IA32_XSS to the GHCB for hypervisor kernel

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: <ZjLTr0n0nwBrZW36@google.com>
Date: Wed, 1 May 2024 16:43:43 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: John Allen <john.allen@....com>
Cc: kvm@...r.kernel.org, weijiang.yang@...el.com, rick.p.edgecombe@...el.com, 
	thomas.lendacky@....com, bp@...en8.de, pbonzini@...hat.com, 
	mlevitsk@...hat.com, linux-kernel@...r.kernel.org, x86@...nel.org
Subject: Re: [PATCH v2 6/9] KVM: SVM: Add MSR_IA32_XSS to the GHCB for
 hypervisor kernel

On Mon, Feb 26, 2024, John Allen wrote:
> When a guest issues a cpuid instruction for Fn0000000D_x0B
> (CetUserOffset), KVM will intercept and need to access the guest
> MSR_IA32_XSS value. For SEV-ES, this is encrypted and needs to be
> included in the GHCB to be visible to the hypervisor.

Heh, too many pronouns and implicit subjects.  I read this, several times, as:

  When a guest issues a cpuid instruction for Fn0000000D_x0B
  (CetUserOffset), KVM will intercept MSR_IA32_XSS and need to access the
  guest MSR_IA32_XSS value.

I think you mean this?

  When a vCPU executes CPUID.0xD.0xB (CetUserOffset), KVM will intercept
  and emulate CPUID.  To emulate CPUID, KVM needs access to the vCPU's
  MSR_IA32_XSS value.  For SEV-ES guests, XSS is encrypted, and so the guest
  must include its XSS value in the GHCB as part of the CPUID request.

Hmm, I suspect that last sentence is wrong though.  Question on that below.

> Signed-off-by: John Allen <john.allen@....com>
> ---
> v2:
>   - Omit passing through XSS as this has already been properly
>     implemented in a26b7cd22546 ("KVM: SEV: Do not intercept
>     accesses to MSR_IA32_XSS for SEV-ES guests") 
> ---
>  arch/x86/include/asm/svm.h | 1 +
>  arch/x86/kvm/svm/sev.c     | 9 +++++++--
>  arch/x86/kvm/svm/svm.h     | 1 +
>  3 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
> index 728c98175b9c..44cd41e2fb68 100644
> --- a/arch/x86/include/asm/svm.h
> +++ b/arch/x86/include/asm/svm.h
> @@ -673,5 +673,6 @@ DEFINE_GHCB_ACCESSORS(sw_exit_info_1)
>  DEFINE_GHCB_ACCESSORS(sw_exit_info_2)
>  DEFINE_GHCB_ACCESSORS(sw_scratch)
>  DEFINE_GHCB_ACCESSORS(xcr0)
> +DEFINE_GHCB_ACCESSORS(xss)
>  
>  #endif
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index f06f9e51ad9d..c3060d2068eb 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2458,8 +2458,13 @@ static void sev_es_sync_from_ghcb(struct vcpu_svm *svm)
>  
>  	svm->vmcb->save.cpl = kvm_ghcb_get_cpl_if_valid(svm, ghcb);
>  
> -	if (kvm_ghcb_xcr0_is_valid(svm)) {
> -		vcpu->arch.xcr0 = ghcb_get_xcr0(ghcb);
> +	if (kvm_ghcb_xcr0_is_valid(svm) || kvm_ghcb_xss_is_valid(svm)) {
> +		if (kvm_ghcb_xcr0_is_valid(svm))
> +			vcpu->arch.xcr0 = ghcb_get_xcr0(ghcb);
> +
> +		if (kvm_ghcb_xss_is_valid(svm))
> +			vcpu->arch.ia32_xss = ghcb_get_xss(ghcb);
> +
>  		kvm_update_cpuid_runtime(vcpu);

Pre-existing code, but isn't updating CPUID runtime on every VMGEXIT super wasteful?
Or is the guest behavior to mark XCR0 and XSS as valid only when changing XCR0/XSS?
If so, the last sentence of the changelog should be something like:

  MSR_IA32_XSS value.  For SEV-ES guests, XSS is encrypted, and so the guest
  must notify the host of XSS changes by performing a ??? VMGEXIT and
  providing its XSS value in the GHCB.