linux-kernel - Re: [PATCH v4 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <aMHsjTjog6SqPRpD@google.com>
Date: Wed, 10 Sep 2025 14:24:29 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: John Allen <john.allen@....com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org, x86@...nel.org, 
	pbonzini@...hat.com, dave.hansen@...el.com, rick.p.edgecombe@...el.com, 
	mlevitsk@...hat.com, weijiang.yang@...el.com, chao.gao@...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 v4 4/5] KVM: SVM: Add MSR_IA32_XSS to the GHCB for
 hypervisor kernel

On Mon, Sep 08, 2025, John Allen wrote:
> When a guest issues a cpuid instruction for Fn0000000D_x0B_{x00,x01}, KVM will
> be intercepting the CPUID instruction and will need to access the guest
> MSR_IA32_XSS value. For SEV-ES, the XSS value 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.

Except that broke _this_ series.

arch/x86/kvm/svm/sev.c: In function ‘sev_es_sync_from_ghcb’:
arch/x86/kvm/svm/sev.c:3293:39: error: implicit declaration of function ‘ghcb_get_xss’;
                                       did you mean ‘ghcb_get_rsi’? [-Wimplicit-function-declaration]
 3293 |                 vcpu->arch.ia32_xss = ghcb_get_xss(ghcb);
      |                                       ^~~~~~~~~~~~
      |                                       ghcb_get_rsi
  AR      drivers/base/built-in.a
  AR      drivers/built-in.a

> v4:
>   - Change logic structure to be more intuitive.
> ---
>  arch/x86/kvm/svm/sev.c | 5 +++++
>  arch/x86/kvm/svm/svm.h | 1 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index f4381878a9e5..33c42dd853b3 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3310,6 +3310,11 @@ static void sev_es_sync_from_ghcb(struct vcpu_svm *svm)
>  		vcpu->arch.cpuid_dynamic_bits_dirty = true;
>  	}
>  
> +	if (kvm_ghcb_xss_is_valid(svm)) {
> +		vcpu->arch.ia32_xss = ghcb_get_xss(ghcb);

Honestly, I think the ghcb_get_xxx() helpers do more harm than good.  For set()
and if_valid(), I'm totally on board with a wrapper.  For get(), unless we WARN
on trying to read an invalid field, I just don't see the point.  Ugh, and we
_can't_ WARN, at least not in KVM, because of the whole TOCTOU mess.

Case in point, this and the xcr0 check can elide setting cpuid_dynamic_bits_dirty
if XCR0/XSS isn't actually changing, but then this

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

looks wonky unless the reader knows that ghcb_get_xcr0() is just reading a struct
field, which obviously isn't terribly difficult to figure out, but the macros
make it more than a bit annoying.

Argh, even worse, that check is technically subject to a TOCTOU bug as well.  It
just doesn't matter in practice because the guest can only hose it self, e.g. by
swizzling XCR0/XSS.  But it's still flawed.

And for both XCR0/XSS, KVM lets the guest throw garbage into vcpu->arch.xcr0 and
now vcpu->arch.xss.  Maybe that's not problematic in practice, but I'd rather not
find out the hard way.

Lastly, open coding the write to cpuid_dynamic_bits_dirty and vcpu->arch.xcr0 is
just gross.

So to avoid a rather pointless dependency for CET, which I'm trying my darndest
to land in 6.18, I'm going to put together a separate fixup patch and replace
this patchh to end up with code that does:

	if (kvm_ghcb_xcr0_is_valid(svm)
		__kvm_set_xcr(vcpu, 0, kvm_ghcb_get_xcr0(ghcb));

	if (kvm_ghcb_xss_is_valid(svm))
		__kvm_emulate_msr_write(vcpu, MSR_IA32_XSS, kvm_ghcb_get_xss(ghcb));