linux-kernel - Re: [PATCH v15 02/41] KVM: SEV: Read save fields from GHCB exactly once

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: <43b841ed-a5c3-4f65-9c7e-0c09f15cce3f@amd.com>
Date: Mon, 15 Sep 2025 12:32:26 -0500
From: Tom Lendacky <thomas.lendacky@....com>
To: Sean Christopherson <seanjc@...gle.com>,
 Paolo Bonzini <pbonzini@...hat.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
 Mathias Krause <minipli@...ecurity.net>, John Allen <john.allen@....com>,
 Rick Edgecombe <rick.p.edgecombe@...el.com>, Chao Gao <chao.gao@...el.com>,
 Maxim Levitsky <mlevitsk@...hat.com>, Xiaoyao Li <xiaoyao.li@...el.com>,
 Zhang Yi Z <yi.z.zhang@...ux.intel.com>
Subject: Re: [PATCH v15 02/41] KVM: SEV: Read save fields from GHCB exactly
 once

On 9/12/25 18:22, Sean Christopherson wrote:
> Wrap all reads of GHCB save fields with READ_ONCE() via a KVM-specific
> GHCB get() utility to help guard against TOCTOU bugs.  Using READ_ONCE()
> doesn't completely prevent such bugs, e.g. doesn't prevent KVM from
> redoing get() after checking the initial value, but at least addresses
> all potential TOCTOU issues in the current KVM code base.
> 
> Opportunistically reduce the indentation of the macro-defined helpers and
> clean up the alignment.
> 
> Fixes: 4e15a0ddc3ff ("KVM: SEV: snapshot the GHCB before accessing it")
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>

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

Just wondering if we should make the kvm_ghcb_get_*() routines take just
a struct vcpu_svm routine so that they don't get confused with the
ghcb_get_*() routines? The current uses are just using svm->sev_es.ghcb
to set the ghcb variable that gets used anyway. That way the KVM
versions look specifically like KVM versions.

> ---
>  arch/x86/kvm/svm/sev.c |  8 ++++----
>  arch/x86/kvm/svm/svm.h | 26 ++++++++++++++++----------
>  2 files changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index fe8d148b76c0..37abbda28685 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3304,16 +3304,16 @@ 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);
> +		vcpu->arch.xcr0 = kvm_ghcb_get_xcr0(ghcb);
>  		vcpu->arch.cpuid_dynamic_bits_dirty = true;
>  	}
>  
>  	/* Copy the GHCB exit information into the VMCB fields */
> -	exit_code = ghcb_get_sw_exit_code(ghcb);
> +	exit_code = kvm_ghcb_get_sw_exit_code(ghcb);
>  	control->exit_code = lower_32_bits(exit_code);
>  	control->exit_code_hi = upper_32_bits(exit_code);
> -	control->exit_info_1 = ghcb_get_sw_exit_info_1(ghcb);
> -	control->exit_info_2 = ghcb_get_sw_exit_info_2(ghcb);
> +	control->exit_info_1 = kvm_ghcb_get_sw_exit_info_1(ghcb);
> +	control->exit_info_2 = kvm_ghcb_get_sw_exit_info_2(ghcb);
>  	svm->sev_es.sw_scratch = kvm_ghcb_get_sw_scratch_if_valid(svm, ghcb);
>  
>  	/* Clear the valid entries fields */
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 5d39c0b17988..c2316adde3cc 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -913,16 +913,22 @@ void __svm_sev_es_vcpu_run(struct vcpu_svm *svm, bool spec_ctrl_intercepted,
>  void __svm_vcpu_run(struct vcpu_svm *svm, bool spec_ctrl_intercepted);
>  
>  #define DEFINE_KVM_GHCB_ACCESSORS(field)						\
> -	static __always_inline bool kvm_ghcb_##field##_is_valid(const struct vcpu_svm *svm) \
> -	{									\
> -		return test_bit(GHCB_BITMAP_IDX(field),				\
> -				(unsigned long *)&svm->sev_es.valid_bitmap);	\
> -	}									\
> -										\
> -	static __always_inline u64 kvm_ghcb_get_##field##_if_valid(struct vcpu_svm *svm, struct ghcb *ghcb) \
> -	{									\
> -		return kvm_ghcb_##field##_is_valid(svm) ? ghcb->save.field : 0;	\
> -	}									\
> +static __always_inline u64 kvm_ghcb_get_##field(struct ghcb *ghcb)			\
> +{											\
> +	return READ_ONCE(ghcb->save.field);						\
> +}											\
> +											\
> +static __always_inline bool kvm_ghcb_##field##_is_valid(const struct vcpu_svm *svm)	\
> +{											\
> +	return test_bit(GHCB_BITMAP_IDX(field),						\
> +			(unsigned long *)&svm->sev_es.valid_bitmap);			\
> +}											\
> +											\
> +static __always_inline u64 kvm_ghcb_get_##field##_if_valid(struct vcpu_svm *svm,	\
> +							   struct ghcb *ghcb)		\
> +{											\
> +	return kvm_ghcb_##field##_is_valid(svm) ? kvm_ghcb_get_##field(ghcb) : 0;	\
> +}
>  
>  DEFINE_KVM_GHCB_ACCESSORS(cpl)
>  DEFINE_KVM_GHCB_ACCESSORS(rax)