linux-kernel - Re: [PATCH v3 4/5] 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] [day] [month] [year] [list]

Message-ID: <aKvAZeY+Q8r02u0Q@intel.com>
Date: Mon, 25 Aug 2025 09:46:13 +0800
From: Chao Gao <chao.gao@...el.com>
To: John Allen <john.allen@....com>
CC: <kvm@...r.kernel.org>, <linux-kernel@...r.kernel.org>, <x86@...nel.org>,
	<seanjc@...gle.com>, <pbonzini@...hat.com>, <dave.hansen@...el.com>,
	<rick.p.edgecombe@...el.com>, <mlevitsk@...hat.com>,
	<weijiang.yang@...el.com>, <bp@...en8.de>, <dave.hansen@...ux.intel.com>,
	<hpa@...or.com>, <mingo@...hat.com>, <tglx@...utronix.de>,
	<thomas.lendacky@....com>
Subject: Re: [PATCH v3 4/5] KVM: SVM: Add MSR_IA32_XSS to the GHCB for
 hypervisor kernel

On Wed, Aug 06, 2025 at 08:45:09PM +0000, 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.
>
>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")
>v3:
>  - Move guest kernel GHCB_ACCESSORS definition to new series.
>---
> arch/x86/kvm/svm/sev.c | 9 +++++++--
> arch/x86/kvm/svm/svm.h | 1 +
> 2 files changed, 8 insertions(+), 2 deletions(-)
>
>diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>index 3f20f6eb1ef6..2905a62e7bf2 100644
>--- a/arch/x86/kvm/svm/sev.c
>+++ b/arch/x86/kvm/svm/sev.c
>@@ -3239,8 +3239,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);
>+
> 		vcpu->arch.cpuid_dynamic_bits_dirty = true;

It seems a bit odd to me. How about:

	if (kvm_ghcb_xcr0_is_valid(svm)) {
		vcpu->arch.xcr0 = ghcb_get_xcr0(ghcb);
		vcpu->arch.cpuid_dynamic_bits_dirty = true;
	}

	if (kvm_ghcb_xss_is_valid(svm)) {
		vcpu->arch.xss = ghcb_get_xss(ghcb);
		vcpu->arch.cpuid_dynamic_bits_dirty = true;
	}

This looks better because it has less indentation and reduces the number
of "if" statements by one.

> 	}
> 
>diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
>index dabd69d6fd15..b189647d8389 100644
>--- a/arch/x86/kvm/svm/svm.h
>+++ b/arch/x86/kvm/svm/svm.h
>@@ -925,5 +925,6 @@ DEFINE_KVM_GHCB_ACCESSORS(sw_exit_info_1)
> DEFINE_KVM_GHCB_ACCESSORS(sw_exit_info_2)
> DEFINE_KVM_GHCB_ACCESSORS(sw_scratch)
> DEFINE_KVM_GHCB_ACCESSORS(xcr0)
>+DEFINE_KVM_GHCB_ACCESSORS(xss)
> 
> #endif
>-- 
>2.34.1
>