linux-kernel - Re: [PATCH v4 2/2] KVM: SEV: Configure "ALLOWED_SEV

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: <85d324ff-a1d3-4d7c-ae2c-68588b12deb3@amd.com>
Date: Thu, 6 Mar 2025 06:28:18 +0100
From: "Gupta, Pankaj" <pankaj.gupta@....com>
To: Kim Phillips <kim.phillips@....com>, kvm@...r.kernel.org,
 linux-coco@...ts.linux.dev, linux-kernel@...r.kernel.org
Cc: Tom Lendacky <thomas.lendacky@....com>,
 Michael Roth <michael.roth@....com>, Ashish Kalra <ashish.kalra@....com>,
 "Nikunj A . Dadhania" <nikunj@....com>, Borislav Petkov <bp@...en8.de>,
 Dave Hansen <dave.hansen@...ux.intel.com>,
 Sean Christopherson <seanjc@...gle.com>, Paolo Bonzini
 <pbonzini@...hat.com>, Ingo Molnar <mingo@...hat.com>,
 "H. Peter Anvin" <hpa@...or.com>, Thomas Gleixner <tglx@...utronix.de>,
 Kishon Vijay Abraham I <kvijayab@....com>
Subject: Re: [PATCH v4 2/2] KVM: SEV: Configure "ALLOWED_SEV_FEATURES" VMCB
 Field

On 3/6/2025 1:38 AM, Kim Phillips wrote:
> AMD EPYC 5th generation processors have introduced a feature that allows
> the hypervisor to control the SEV_FEATURES that are set for, or by, a
> guest [1].  ALLOWED_SEV_FEATURES can be used by the hypervisor to enforce
> that SEV-ES and SEV-SNP guests cannot enable features that the
> hypervisor does not want to be enabled.
> 
> Always enable ALLOWED_SEV_FEATURES.  A VMRUN will fail if any
> non-reserved bits are 1 in SEV_FEATURES but are 0 in
> ALLOWED_SEV_FEATURES.
> 
> Some SEV_FEATURES - currently PmcVirtualization and SecureAvic
> (see Appendix B, Table B-4) - require an opt-in via ALLOWED_SEV_FEATURES,
> i.e. are off-by-default, whereas all other features are effectively
> on-by-default, but still honor ALLOWED_SEV_FEATURES.
> 
> [1] Section 15.36.20 "Allowed SEV Features", AMD64 Architecture
>      Programmer's Manual, Pub. 24593 Rev. 3.42 - March 2024:
>      https://bugzilla.kernel.org/attachment.cgi?id=306250
> 
> Co-developed-by: Kishon Vijay Abraham I <kvijayab@....com>
> Signed-off-by: Kishon Vijay Abraham I <kvijayab@....com>
> Signed-off-by: Kim Phillips <kim.phillips@....com>
> ---
>   arch/x86/include/asm/svm.h |  7 ++++++-
>   arch/x86/kvm/svm/sev.c     | 13 +++++++++++++
>   2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
> index 9b7fa99ae951..b382fd251e5b 100644
> --- a/arch/x86/include/asm/svm.h
> +++ b/arch/x86/include/asm/svm.h
> @@ -159,7 +159,10 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
>   	u64 avic_physical_id;	/* Offset 0xf8 */
>   	u8 reserved_7[8];
>   	u64 vmsa_pa;		/* Used for an SEV-ES guest */
> -	u8 reserved_8[720];
> +	u8 reserved_8[40];
> +	u64 allowed_sev_features;	/* Offset 0x138 */
> +	u64 guest_sev_features;		/* Offset 0x140 */

Just thinking, if dumping error in logs would be
useful for Admin in case of failure Or maybe we
want to leave this to userspace?

In any case, this patch looks good to me.

Reviewed-by: Pankaj Gupta <pankaj.gupta@....com>


> +	u8 reserved_9[664];
>   	/*
>   	 * Offset 0x3e0, 32 bytes reserved
>   	 * for use by hypervisor/software.
> @@ -291,6 +294,8 @@ static_assert((X2AVIC_MAX_PHYSICAL_ID & AVIC_PHYSICAL_MAX_INDEX_MASK) == X2AVIC_
>   #define SVM_SEV_FEAT_ALTERNATE_INJECTION		BIT(4)
>   #define SVM_SEV_FEAT_DEBUG_SWAP				BIT(5)
>   
> +#define VMCB_ALLOWED_SEV_FEATURES_VALID			BIT_ULL(63)
> +
>   struct vmcb_seg {
>   	u16 selector;
>   	u16 attrib;
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 0bc708ee2788..7f6cb950edcf 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -793,6 +793,14 @@ static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   	return ret;
>   }
>   
> +static u64 allowed_sev_features(struct kvm_sev_info *sev)
> +{
> +	if (cpu_feature_enabled(X86_FEATURE_ALLOWED_SEV_FEATURES))
> +		return sev->vmsa_features | VMCB_ALLOWED_SEV_FEATURES_VALID;
> +
> +	return 0;
> +}
> +
>   static int sev_es_sync_vmsa(struct vcpu_svm *svm)
>   {
>   	struct kvm_vcpu *vcpu = &svm->vcpu;
> @@ -891,6 +899,7 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
>   static int __sev_launch_update_vmsa(struct kvm *kvm, struct kvm_vcpu *vcpu,
>   				    int *error)
>   {
> +	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>   	struct sev_data_launch_update_vmsa vmsa;
>   	struct vcpu_svm *svm = to_svm(vcpu);
>   	int ret;
> @@ -900,6 +909,8 @@ static int __sev_launch_update_vmsa(struct kvm *kvm, struct kvm_vcpu *vcpu,
>   		return -EINVAL;
>   	}
>   
> +	svm->vmcb->control.allowed_sev_features = allowed_sev_features(sev);
> +
>   	/* Perform some pre-encryption checks against the VMSA */
>   	ret = sev_es_sync_vmsa(svm);
>   	if (ret)
> @@ -2426,6 +2437,8 @@ static int snp_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   		struct vcpu_svm *svm = to_svm(vcpu);
>   		u64 pfn = __pa(svm->sev_es.vmsa) >> PAGE_SHIFT;
>   
> +		svm->vmcb->control.allowed_sev_features = allowed_sev_features(sev);
> +
>   		ret = sev_es_sync_vmsa(svm);
>   		if (ret)
>   			return ret;