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

[Tom Lendacky <thomas.lendacky@amd.com>]

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)
>  {