Re: [PATCH v15 03/41] KVM: SEV: Validate XCR0 provided by guest in GHCB

Tom Lendacky
2025-09-15T18:41:06Z

On 9/12/25 18:22, Sean Christopherson wrote:
> Use __kvm_set_xcr() to propagate XCR0 changes from the GHCB to KVM's
> software model in order to validate the new XCR0 against KVM's view of
> the supported XCR0. Allowing garbage is thankfully mostly benign, as
> kvm_load_{guest,host}_xsave_state() bail early for vCPUs with protected
> state, xstate_required_size() will simply provide garbage back to the
> guest, and attempting to save/restore the bad value via KVM_{G,S}ET_XCRS
> will only harm the guest (setting XCR0 will fail).
>
> However, allowing the guest to put junk into a field that KVM assumes is
> valid is a CVE waiting to happen. And as a bonus, using the proper API
> eliminates the ugly open coding of setting arch.cpuid_dynamic_bits_dirty.
>
> Simply ignore bad values, as either the guest managed to get an
> unsupported value into hardware, or the guest is misbehaving and providing
> pure garbage. In either case, KVM can't fix the broken guest.
>
> Note, using __kvm_set_xcr() also avoids recomputing dynamic CPUID bits
> if XCR0 isn't actually changing (relatively to KVM's previous snapshot).
>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Fixes: 291bd20d5d88 ("KVM: SVM: Add initial support for a VMGEXIT VMEXIT")
> Signed-off-by: Sean Christopherson <seanjc@google.com>

A question below, but otherwise:

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>

(successfully booted and ran some quick tests against the first 3
patches without any issues on both an SEV-ES and SEV-SNP guest).

> ---
> arch/x86/include/asm/kvm_host.h | 1 +
> arch/x86/kvm/svm/sev.c | 6 ++----
> arch/x86/kvm/x86.c | 3 ++-
> 3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index cb86f3cca3e9..2762554cbb7b 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -2209,6 +2209,7 @@ int kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val);
> unsigned long kvm_get_dr(struct kvm_vcpu *vcpu, int dr);
> 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_get_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 37abbda28685..0cd77a87dd84 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3303,10 +3303,8 @@ 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 = kvm_ghcb_get_xcr0(ghcb);
> - vcpu->arch.cpuid_dynamic_bits_dirty = true;
> - }
> + if (kvm_ghcb_xcr0_is_valid(svm))
> + __kvm_set_xcr(vcpu, 0, kvm_ghcb_get_xcr0(ghcb));

Would a vcpu_unimpl() be approprite here if __kvm_set_xcr() returns
something other than 0? It might help with debugging if the guest is
doing something it shouldn't.

Thanks,
Tom

>
> /* Copy the GHCB exit information into the VMCB fields */
> exit_code = kvm_ghcb_get_sw_exit_code(ghcb);
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 6d85fbafc679..ba4915456615 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1235,7 +1235,7 @@ static inline u64 kvm_guest_supported_xfd(struct kvm_vcpu *vcpu)
> }
> #endif
>
> -static int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
> +int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
> {
> u64 xcr0 = xcr;
> u64 old_xcr0 = vcpu->arch.xcr0;
> @@ -1279,6 +1279,7 @@ static int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
> vcpu->arch.cpuid_dynamic_bits_dirty = true;
> return 0;
> }
> +EXPORT_SYMBOL_GPL(__kvm_set_xcr);
>
> int kvm_emulate_xsetbv(struct kvm_vcpu *vcpu)
> {