| 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
| ||
|
Message-ID: <aMx4TwOLS62ccHTQ@AUSJOHALLEN.amd.com>
Date: Thu, 18 Sep 2025 16:23:27 -0500
From: John Allen <john.allen@....com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org,
linux-kernel@...r.kernel.org,
Tom Lendacky <thomas.lendacky@....com>,
Mathias Krause <minipli@...ecurity.net>,
Rick Edgecombe <rick.p.edgecombe@...el.com>,
Chao Gao <chao.gao@...el.com>, Maxim Levitsky <mlevitsk@...hat.com>,
Xiaoyao Li <xiaoyao.li@...el.com>
Subject: Re: [PATCH v15 29/41] KVM: SEV: Synchronize MSR_IA32_XSS from the
GHCB when it's valid
On Thu, Sep 18, 2025 at 01:44:13PM -0700, Sean Christopherson wrote:
> On Thu, Sep 18, 2025, Sean Christopherson wrote:
> > On Thu, Sep 18, 2025, John Allen wrote:
> > > On Tue, Sep 16, 2025 at 05:55:33PM -0500, John Allen wrote:
> > > > Interesting, I see "Guest CPUID doesn't have XSAVES" times the number of
> > > > cpus followed by "XSS already set to val = 0, eliding updates" times the
> > > > number of cpus. This is with host tracing only. I can try with guest
> > > > tracing too in the morning.
> > >
> > > Ok, I think I see the problem. The cases above where we were seeing the
> > > added print statements from kvm_set_msr_common were not situations where
> > > we were going through the __kvm_emulate_msr_write via
> > > sev_es_sync_from_ghcb. When we call __kvm_emulate_msr_write from this
> > > context, we never end up getting to kvm_set_msr_common because we hit
> > > the following statement at the top of svm_set_msr:
> > >
> > > if (sev_es_prevent_msr_access(vcpu, msr))
> > > return vcpu->kvm->arch.has_protected_state ? -EINVAL : 0;
> >
> > Gah, I was looking for something like that but couldn't find it, obviously.
> >
> > > So I'm not sure if this would force using the original method of
> > > directly setting arch.ia32_xss or if there's some additional handling
> > > here that we need in this scenario to allow the msr access.
> >
> > Does this fix things? If so, I'll slot in a patch to extract setting XSS to
> > the helper, and then this patch can use that API. I like the symmetry between
> > __kvm_set_xcr() and __kvm_set_xss(), and I especially like not doing a generic
> > end-around on svm_set_msr() by calling kvm_set_msr_common() directly.
>
> Scratch that, KVM supports intra-host (and inter-host?) migration of SEV-ES
> guests and so needs to allow the host to save/restore XSS, otherwise a guest
> that *knows* its XSS hasn't change could get stale/bad CPUID emulation if the
> guest doesn't provide XSS in the GHCB on every exit.
>
> So while seemingly hacky, I'm pretty sure the right solution is actually:
>
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index cabe1950b160..d48bf20c865b 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -2721,8 +2721,8 @@ static int svm_get_feature_msr(u32 msr, u64 *data)
> static bool sev_es_prevent_msr_access(struct kvm_vcpu *vcpu,
> struct msr_data *msr_info)
> {
> - return sev_es_guest(vcpu->kvm) &&
> - vcpu->arch.guest_state_protected &&
> + return sev_es_guest(vcpu->kvm) && vcpu->arch.guest_state_protected &&
> + msr_info->index != MSR_IA32_XSS &&
> !msr_write_intercepted(vcpu, msr_info->index);
> }
Yes, it looks like this fixes the regression. Thanks!
The 32bit selftest still doesn't work properly with sev-es, but that was
a problem with the previous version too. I suspect there's some
incompatibility between sev-es and the test, but I haven't been able to
get a good answer on why that might be.
Thanks,
John
>
> Side topic, checking msr_write_intercepted() is likely wrong. It's a bad
> heuristic for "managed in the VMSA". MSRs that _KVM_ loads into hardware and
> context switches should still be accessible. I haven't looked to see if this is
> a problem in practice.
>
> >
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index 945f7da60107..ace9f321d2c9 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -2213,6 +2213,7 @@ unsigned long kvm_get_cr8(struct kvm_vcpu *vcpu);
> > void kvm_lmsw(struct kvm_vcpu *vcpu, unsigned long msw);
> > int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr);
> > int kvm_emulate_xsetbv(struct kvm_vcpu *vcpu);
> > +int __kvm_set_xss(struct kvm_vcpu *vcpu, u64 xss);
> >
> > int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr);
> > int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr);
> > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > index 94d9acc94c9a..462aebc54135 100644
> > --- a/arch/x86/kvm/svm/sev.c
> > +++ b/arch/x86/kvm/svm/sev.c
> > @@ -3355,7 +3355,7 @@ static void sev_es_sync_from_ghcb(struct vcpu_svm *svm)
> > __kvm_set_xcr(vcpu, 0, kvm_ghcb_get_xcr0(svm));
> >
> > if (kvm_ghcb_xss_is_valid(svm))
> > - __kvm_emulate_msr_write(vcpu, MSR_IA32_XSS, kvm_ghcb_get_xss(svm));
> > + __kvm_set_xss(vcpu, kvm_ghcb_get_xss(svm));
> >
> > /* Copy the GHCB exit information into the VMCB fields */
> > exit_code = kvm_ghcb_get_sw_exit_code(svm);
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 5bbc187ab428..9b81e92a8de5 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -1313,6 +1313,22 @@ int kvm_emulate_xsetbv(struct kvm_vcpu *vcpu)
> > }
> > EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_emulate_xsetbv);
> >
> > +int __kvm_set_xss(struct kvm_vcpu *vcpu, u64 xss)
> > +{
> > + if (!guest_cpuid_has(vcpu, X86_FEATURE_XSAVES))
> > + return KVM_MSR_RET_UNSUPPORTED;
> > +
> > + if (xss & ~vcpu->arch.guest_supported_xss)
> > + return 1;
> > + if (vcpu->arch.ia32_xss == xss)
> > + return 0;
> > +
> > + vcpu->arch.ia32_xss = xss;
> > + vcpu->arch.cpuid_dynamic_bits_dirty = true;
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(__kvm_set_xss);
> > +
> > static bool kvm_is_valid_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
> > {
> > return __kvm_is_valid_cr4(vcpu, cr4) &&
> > @@ -4119,16 +4135,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > }
> > break;
> > case MSR_IA32_XSS:
> > - if (!guest_cpuid_has(vcpu, X86_FEATURE_XSAVES))
> > - return KVM_MSR_RET_UNSUPPORTED;
> > -
> > - if (data & ~vcpu->arch.guest_supported_xss)
> > - return 1;
> > - if (vcpu->arch.ia32_xss == data)
> > - break;
> > - vcpu->arch.ia32_xss = data;
> > - vcpu->arch.cpuid_dynamic_bits_dirty = true;
> > - break;
> > + return __kvm_set_xss(vcpu, data);
> > case MSR_SMI_COUNT:
> > if (!msr_info->host_initiated)
> > return 1;
Powered by blists - more mailing lists