[PATCH v2] KVM: SEV: Enforce minimum GHCB version requirement for SEV-SNP guests

[PATCH v2] KVM: SEV: Enforce minimum GHCB version requirement for SEV-SNP guests

[Nikunj A Dadhania]

Require a minimum GHCB version of 2 when starting SEV-SNP guests through
KVM_SEV_INIT2. When a VMM attempts to start an SEV-SNP guest with an
incompatible GHCB version (less than 2), reject the request early rather
than allowing the guest kernel to start with an incorrect protocol version
and fail later with GHCB_SNP_UNSUPPORTED guest termination.

Hypervisor logs the guest termination with GHCB_SNP_UNSUPPORTED error code:

kvm_amd: SEV-ES guest requested termination: 0x0:0x2

SNP guest fails with the below error message:

KVM: unknown exit reason 24
EAX=00000000 EBX=00000000 ECX=00000000 EDX=00a00f11
ESI=00000000 EDI=00000000 EBP=00000000 ESP=00000000
EIP=0000fff0 EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
ES =0000 00000000 0000ffff 00009300
CS =f000 ffff0000 0000ffff 00009b00
SS =0000 00000000 0000ffff 00009300
DS =0000 00000000 0000ffff 00009300
FS =0000 00000000 0000ffff 00009300
GS =0000 00000000 0000ffff 00009300
LDT=0000 00000000 0000ffff 00008200
TR =0000 00000000 0000ffff 00008b00
GDT=     00000000 0000ffff
IDT=     00000000 0000ffff
CR0=60000010 CR2=00000000 CR3=00000000 CR4=00000000
DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000
DR6=00000000ffff0ff0 DR7=0000000000000400
EFER=0000000000000000

Fixes: 4af663c2f64a ("KVM: SEV: Allow per-guest configuration of GHCB protocol version")
Cc: Thomas Lendacky <thomas.lendacky@amd.com>
Cc: Sean Christopherson <seanjc@google.com>
Cc: Michael Roth <michael.roth@amd.com>
Cc: stable@vger.kernel.org
Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>

---

Changes since v1:
* Add failure logs in the commit and drop @stable tag (Sean)
---
 arch/x86/kvm/svm/sev.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 95668e84ab86..fdc1309c68cb 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -406,6 +406,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
 	struct kvm_sev_info *sev = to_kvm_sev_info(kvm);
 	struct sev_platform_init_args init_args = {0};
 	bool es_active = vm_type != KVM_X86_SEV_VM;
+	bool snp_active = vm_type == KVM_X86_SNP_VM;
 	u64 valid_vmsa_features = es_active ? sev_supported_vmsa_features : 0;
 	int ret;
 
@@ -424,6 +425,9 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
 	if (unlikely(sev->active))
 		return -EINVAL;
 
+	if (snp_active && data->ghcb_version && data->ghcb_version < 2)
+		return -EINVAL;
+
 	sev->active = true;
 	sev->es_active = es_active;
 	sev->vmsa_features = data->vmsa_features;
@@ -437,7 +441,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
 	if (sev->es_active && !sev->ghcb_version)
 		sev->ghcb_version = GHCB_VERSION_DEFAULT;
 
-	if (vm_type == KVM_X86_SNP_VM)
+	if (snp_active)
 		sev->vmsa_features |= SVM_SEV_FEAT_SNP_ACTIVE;
 
 	ret = sev_asid_new(sev);
@@ -455,7 +459,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
 	}
 
 	/* This needs to happen after SEV/SNP firmware initialization. */
-	if (vm_type == KVM_X86_SNP_VM) {
+	if (snp_active) {
 		ret = snp_guest_req_init(kvm);
 		if (ret)
 			goto e_free;

base-commit: 772d50d9b87bec08b56ecee0a880d6b2ee5c7da5
-- 
2.43.0

Re: [PATCH v2] KVM: SEV: Enforce minimum GHCB version requirement for SEV-SNP guests

[Tom Lendacky]

On 7/16/25 00:56, Nikunj A Dadhania wrote:
> Require a minimum GHCB version of 2 when starting SEV-SNP guests through
> KVM_SEV_INIT2. When a VMM attempts to start an SEV-SNP guest with an
> incompatible GHCB version (less than 2), reject the request early rather
> than allowing the guest kernel to start with an incorrect protocol version
> and fail later with GHCB_SNP_UNSUPPORTED guest termination.
> 
> Hypervisor logs the guest termination with GHCB_SNP_UNSUPPORTED error code:
> 
> kvm_amd: SEV-ES guest requested termination: 0x0:0x2
> 
> SNP guest fails with the below error message:
> 
> KVM: unknown exit reason 24
> EAX=00000000 EBX=00000000 ECX=00000000 EDX=00a00f11
> ESI=00000000 EDI=00000000 EBP=00000000 ESP=00000000
> EIP=0000fff0 EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
> ES =0000 00000000 0000ffff 00009300
> CS =f000 ffff0000 0000ffff 00009b00
> SS =0000 00000000 0000ffff 00009300
> DS =0000 00000000 0000ffff 00009300
> FS =0000 00000000 0000ffff 00009300
> GS =0000 00000000 0000ffff 00009300
> LDT=0000 00000000 0000ffff 00008200
> TR =0000 00000000 0000ffff 00008b00
> GDT=     00000000 0000ffff
> IDT=     00000000 0000ffff
> CR0=60000010 CR2=00000000 CR3=00000000 CR4=00000000
> DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000
> DR6=00000000ffff0ff0 DR7=0000000000000400
> EFER=0000000000000000
> 
> Fixes: 4af663c2f64a ("KVM: SEV: Allow per-guest configuration of GHCB protocol version")
> Cc: Thomas Lendacky <thomas.lendacky@amd.com>
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: Michael Roth <michael.roth@amd.com>
> Cc: stable@vger.kernel.org

You kept the stable email.

Minor comment below about placement, but otherwise...

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

> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
> 
> ---
> 
> Changes since v1:
> * Add failure logs in the commit and drop @stable tag (Sean)
> ---
>  arch/x86/kvm/svm/sev.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 95668e84ab86..fdc1309c68cb 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -406,6 +406,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
>  	struct kvm_sev_info *sev = to_kvm_sev_info(kvm);
>  	struct sev_platform_init_args init_args = {0};
>  	bool es_active = vm_type != KVM_X86_SEV_VM;
> +	bool snp_active = vm_type == KVM_X86_SNP_VM;
>  	u64 valid_vmsa_features = es_active ? sev_supported_vmsa_features : 0;
>  	int ret;
>  
> @@ -424,6 +425,9 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
>  	if (unlikely(sev->active))
>  		return -EINVAL;
>  
> +	if (snp_active && data->ghcb_version && data->ghcb_version < 2)
> +		return -EINVAL;
> +

Would it make sense to move this up a little bit so that it follows the
other ghcb_version check? This way the checks are grouped.

Thanks,
Tom

>  	sev->active = true;
>  	sev->es_active = es_active;
>  	sev->vmsa_features = data->vmsa_features;
> @@ -437,7 +441,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
>  	if (sev->es_active && !sev->ghcb_version)
>  		sev->ghcb_version = GHCB_VERSION_DEFAULT;
>  
> -	if (vm_type == KVM_X86_SNP_VM)
> +	if (snp_active)
>  		sev->vmsa_features |= SVM_SEV_FEAT_SNP_ACTIVE;
>  
>  	ret = sev_asid_new(sev);
> @@ -455,7 +459,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
>  	}
>  
>  	/* This needs to happen after SEV/SNP firmware initialization. */
> -	if (vm_type == KVM_X86_SNP_VM) {
> +	if (snp_active) {
>  		ret = snp_guest_req_init(kvm);
>  		if (ret)
>  			goto e_free;
> 
> base-commit: 772d50d9b87bec08b56ecee0a880d6b2ee5c7da5

Re: [PATCH v2] KVM: SEV: Enforce minimum GHCB version requirement for SEV-SNP guests

[Sean Christopherson]

On Wed, Jul 16, 2025, Tom Lendacky wrote:
> On 7/16/25 00:56, Nikunj A Dadhania wrote:
> > ---
> >  arch/x86/kvm/svm/sev.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > index 95668e84ab86..fdc1309c68cb 100644
> > --- a/arch/x86/kvm/svm/sev.c
> > +++ b/arch/x86/kvm/svm/sev.c
> > @@ -406,6 +406,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
> >  	struct kvm_sev_info *sev = to_kvm_sev_info(kvm);
> >  	struct sev_platform_init_args init_args = {0};
> >  	bool es_active = vm_type != KVM_X86_SEV_VM;
> > +	bool snp_active = vm_type == KVM_X86_SNP_VM;
> >  	u64 valid_vmsa_features = es_active ? sev_supported_vmsa_features : 0;
> >  	int ret;
> >  
> > @@ -424,6 +425,9 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
> >  	if (unlikely(sev->active))
> >  		return -EINVAL;
> >  
> > +	if (snp_active && data->ghcb_version && data->ghcb_version < 2)
> > +		return -EINVAL;
> > +
> 
> Would it make sense to move this up a little bit so that it follows the
> other ghcb_version check? This way the checks are grouped.

Yes, because there's a lot going on here, and this:

  data->ghcb_version && data->ghcb_version < 2

is an unnecesarily bizarre way of writing

  data->ghcb_version == 1

And *that* is super confusing because it begs the question of why version 0 is
ok, but version 1 is not.  And then further down I see this: 

	/*
	 * Currently KVM supports the full range of mandatory features defined
	 * by version 2 of the GHCB protocol, so default to that for SEV-ES
	 * guests created via KVM_SEV_INIT2.
	 */
	if (sev->es_active && !sev->ghcb_version)
		sev->ghcb_version = GHCB_VERSION_DEFAULT;

Rather than have a funky sequence with odd logic, set data->ghcb_version before
the SNP check.  We should also tweak the comment, because "Currently" implies
that KVM might *drop* support for mandatory features, and that definitely isn't
going to happen.  And because the reader shouldn't have to go look at sev_guest_init()
to understand what's special about KVM_SEV_INIT2.

Lastly, I think we should open code '2' and drop GHCB_VERSION_DEFAULT, because:

 - it's a conditional default
 - is not enumerated to userspace
 - changing GHCB_VERSION_DEFAULT will impact ABI and could break existing setups
 - will result in a stale if GHCB_VERSION_DEFAULT is modified
 - this new check makes me want to assert GHCB_VERSION_DEFAULT > 2

As a result, if we combine all of the above, then we effectively end up with:

	if (es_active && !data->ghcb_version)
		data->ghcb_version = GHCB_VERSION_DEFAULT;

	BUILD_BUG_ON(GHCB_VERSION_DEFAULT != 2);

which is quite silly.

So this?  Completely untested, and should probably be split over 2-3 patches.

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 2fbdebf79fbb..f068cd466ae3 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -37,7 +37,6 @@
 #include "trace.h"
 
 #define GHCB_VERSION_MAX       2ULL
-#define GHCB_VERSION_DEFAULT   2ULL
 #define GHCB_VERSION_MIN       1ULL
 
 #define GHCB_HV_FT_SUPPORTED   (GHCB_HV_FT_SNP | GHCB_HV_FT_SNP_AP_CREATION)
@@ -405,6 +404,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
 {
        struct kvm_sev_info *sev = to_kvm_sev_info(kvm);
        struct sev_platform_init_args init_args = {0};
+       bool snp_active = vm_type == KVM_X86_SNP_VM;
        bool es_active = vm_type != KVM_X86_SEV_VM;
        u64 valid_vmsa_features = es_active ? sev_supported_vmsa_features : 0;
        int ret;
@@ -418,7 +418,18 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
        if (data->vmsa_features & ~valid_vmsa_features)
                return -EINVAL;
 
-       if (data->ghcb_version > GHCB_VERSION_MAX || (!es_active && data->ghcb_version))
+       if (!es_active && data->ghcb_version)
+               return -EINVAL;
+
+       /*
+        * KVM supports the full range of mandatory features defined by version
+        * 2 of the GHCB protocol, so default to that for SEV-ES guests created
+        * via KVM_SEV_INIT2 (KVM_SEV_INIT forces version 1).
+        */
+       if (es_active && !data->ghcb_version)
+               data->ghcb_version = 2;
+
+       if (snp_active && data->ghcb_version < 2)
                return -EINVAL;
 
        if (unlikely(sev->active))
@@ -429,15 +440,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
        sev->vmsa_features = data->vmsa_features;
        sev->ghcb_version = data->ghcb_version;
 
-       /*
-        * Currently KVM supports the full range of mandatory features defined
-        * by version 2 of the GHCB protocol, so default to that for SEV-ES
-        * guests created via KVM_SEV_INIT2.
-        */
-       if (sev->es_active && !sev->ghcb_version)
-               sev->ghcb_version = GHCB_VERSION_DEFAULT;
-
-       if (vm_type == KVM_X86_SNP_VM)
+       if (snp_active)
                sev->vmsa_features |= SVM_SEV_FEAT_SNP_ACTIVE;
 
        ret = sev_asid_new(sev);
@@ -455,7 +458,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
        }
 
        /* This needs to happen after SEV/SNP firmware initialization. */
-       if (vm_type == KVM_X86_SNP_VM) {
+       if (snp_active) {
                ret = snp_guest_req_init(kvm);
                if (ret)
                        goto e_free;

Re: [PATCH v2] KVM: SEV: Enforce minimum GHCB version requirement for SEV-SNP guests

[Nikunj A Dadhania]

Sean Christopherson <seanjc@google.com> writes:

> On Wed, Jul 16, 2025, Tom Lendacky wrote:
>> On 7/16/25 00:56, Nikunj A Dadhania wrote:
>> > ---
>> >  arch/x86/kvm/svm/sev.c | 8 ++++++--
>> >  1 file changed, 6 insertions(+), 2 deletions(-)
>> > 
>> > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> > index 95668e84ab86..fdc1309c68cb 100644
>> > --- a/arch/x86/kvm/svm/sev.c
>> > +++ b/arch/x86/kvm/svm/sev.c
>> > @@ -406,6 +406,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
>> >  	struct kvm_sev_info *sev = to_kvm_sev_info(kvm);
>> >  	struct sev_platform_init_args init_args = {0};
>> >  	bool es_active = vm_type != KVM_X86_SEV_VM;
>> > +	bool snp_active = vm_type == KVM_X86_SNP_VM;
>> >  	u64 valid_vmsa_features = es_active ? sev_supported_vmsa_features : 0;
>> >  	int ret;
>> >  
>> > @@ -424,6 +425,9 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
>> >  	if (unlikely(sev->active))
>> >  		return -EINVAL;
>> >  
>> > +	if (snp_active && data->ghcb_version && data->ghcb_version < 2)
>> > +		return -EINVAL;
>> > +
>> 
>> Would it make sense to move this up a little bit so that it follows the
>> other ghcb_version check? This way the checks are grouped.
>
> Yes, because there's a lot going on here, and this:
>
>   data->ghcb_version && data->ghcb_version < 2
>
> is an unnecesarily bizarre way of writing
>
>   data->ghcb_version == 1
>
> And *that* is super confusing because it begs the question of why version 0 is
> ok, but version 1 is not.

Yes, and had done the previous version because that.

> And then further down I see this:
>
> 	/*
> 	 * Currently KVM supports the full range of mandatory features defined
> 	 * by version 2 of the GHCB protocol, so default to that for SEV-ES
> 	 * guests created via KVM_SEV_INIT2.
> 	 */
> 	if (sev->es_active && !sev->ghcb_version)
> 		sev->ghcb_version = GHCB_VERSION_DEFAULT;
>
> Rather than have a funky sequence with odd logic, set data->ghcb_version before
> the SNP check.  We should also tweak the comment, because "Currently" implies
> that KVM might *drop* support for mandatory features, and that definitely isn't
> going to happen.  And because the reader shouldn't have to go look at sev_guest_init()
> to understand what's special about KVM_SEV_INIT2.
>
> Lastly, I think we should open code '2' and drop GHCB_VERSION_DEFAULT, because:
>
>  - it's a conditional default
>  - is not enumerated to userspace
>  - changing GHCB_VERSION_DEFAULT will impact ABI and could break existing setups
>  - will result in a stale if GHCB_VERSION_DEFAULT is modified
>  - this new check makes me want to assert GHCB_VERSION_DEFAULT > 2
>
> As a result, if we combine all of the above, then we effectively end up with:
>
> 	if (es_active && !data->ghcb_version)
> 		data->ghcb_version = GHCB_VERSION_DEFAULT;
>
> 	BUILD_BUG_ON(GHCB_VERSION_DEFAULT != 2);
>
> which is quite silly.
>
> So this?  Completely untested, and should probably be split over 2-3 patches.

Sure, will test and send updated patches.

> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 2fbdebf79fbb..f068cd466ae3 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -37,7 +37,6 @@
>  #include "trace.h"
>  
>  #define GHCB_VERSION_MAX       2ULL
> -#define GHCB_VERSION_DEFAULT   2ULL
>  #define GHCB_VERSION_MIN       1ULL
>  
>  #define GHCB_HV_FT_SUPPORTED   (GHCB_HV_FT_SNP | GHCB_HV_FT_SNP_AP_CREATION)
> @@ -405,6 +404,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
>  {
>         struct kvm_sev_info *sev = to_kvm_sev_info(kvm);
>         struct sev_platform_init_args init_args = {0};
> +       bool snp_active = vm_type == KVM_X86_SNP_VM;
>         bool es_active = vm_type != KVM_X86_SEV_VM;
>         u64 valid_vmsa_features = es_active ? sev_supported_vmsa_features : 0;
>         int ret;
> @@ -418,7 +418,18 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
>         if (data->vmsa_features & ~valid_vmsa_features)
>                 return -EINVAL;
>  
> -       if (data->ghcb_version > GHCB_VERSION_MAX || (!es_active &&
> data->ghcb_version))

Any specific reason to get rid of the first check for GHCB_VERSION_MAX ?

Newer QEMU with support for ghcb_version = 3 and older KVM hypervisor
that still does not have say version 3 supported ?

> +       if (!es_active && data->ghcb_version)
> +               return -EINVAL;
> +
> +       /*
> +        * KVM supports the full range of mandatory features defined by version
> +        * 2 of the GHCB protocol, so default to that for SEV-ES guests created
> +        * via KVM_SEV_INIT2 (KVM_SEV_INIT forces version 1).
> +        */
> +       if (es_active && !data->ghcb_version)
> +               data->ghcb_version = 2;
> +
> +       if (snp_active && data->ghcb_version < 2)
>                 return -EINVAL;

Makes sense and is clear.

Thanks
Nikunj

Re: [PATCH v2] KVM: SEV: Enforce minimum GHCB version requirement for SEV-SNP guests

[Sean Christopherson]

On Thu, Jul 24, 2025, Nikunj A Dadhania wrote:
> Sean Christopherson <seanjc@google.com> writes:
> > @@ -418,7 +418,18 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
> >         if (data->vmsa_features & ~valid_vmsa_features)
> >                 return -EINVAL;
> >  
> > -       if (data->ghcb_version > GHCB_VERSION_MAX || (!es_active &&
> > data->ghcb_version))
> 
> Any specific reason to get rid of the first check for GHCB_VERSION_MAX ?

Purely a goof on my part, I didn't intend to drop it.