[PATCH] KVM: SEV-ES: Fix svm_get_msr()/svm_set_msr() for KVM_SEV_ES_INIT guests

[PATCH] KVM: SEV-ES: Fix svm_get_msr()/svm_set_msr() for KVM_SEV_ES_INIT guests

[Michael Roth]

With commit 27bd5fdc24c0 ("KVM: SEV-ES: Prevent MSR access post VMSA
encryption"), older VMMs like QEMU 9.0 and older will fail when booting
SEV-ES guests with something like the following error:

  qemu-system-x86_64: error: failed to get MSR 0x174
  qemu-system-x86_64: ../qemu.git/target/i386/kvm/kvm.c:3950: kvm_get_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.

This is because older VMMs that might still call
svm_get_msr()/svm_set_msr() for SEV-ES guests after guest boot even if
those interfaces were essentially just noops because of the vCPU state
being encrypted and stored separately in the VMSA. Now those VMMs will
get an -EINVAL and generally crash.

Newer VMMs that are aware of KVM_SEV_INIT2 however are already aware of
the stricter limitations of what vCPU state can be sync'd during
guest run-time, so newer QEMU for instance will work both for legacy
KVM_SEV_ES_INIT interface as well as KVM_SEV_INIT2.

So when using KVM_SEV_INIT2 it's okay to assume userspace can deal with
-EINVAL, whereas for legacy KVM_SEV_ES_INIT the kernel might be dealing
with either an older VMM and so it needs to assume that returning
-EINVAL might break the VMM.

Address this by only returning -EINVAL if the guest was started with
KVM_SEV_INIT2. Otherwise, just silently return.

Cc: Ravi Bangoria <ravi.bangoria@amd.com>
Cc: Nikunj A Dadhania <nikunj@amd.com>
Reported-by: Srikanth Aithal <sraithal@amd.com>
Closes: https://lore.kernel.org/lkml/37usuu4yu4ok7be2hqexhmcyopluuiqj3k266z4gajc2rcj4yo@eujb23qc3zcm/
Fixes: 27bd5fdc24c0 ("KVM: SEV-ES: Prevent MSR access post VMSA encryption")
Signed-off-by: Michael Roth <michael.roth@amd.com>
---
 arch/x86/kvm/svm/svm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index b252a2732b6f..c58da281f14f 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2855,7 +2855,7 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 
 	if (sev_es_prevent_msr_access(vcpu, msr_info)) {
 		msr_info->data = 0;
-		return -EINVAL;
+		return vcpu->kvm->arch.has_protected_state ? -EINVAL : 0;
 	}
 
 	switch (msr_info->index) {
@@ -3010,7 +3010,7 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 	u64 data = msr->data;
 
 	if (sev_es_prevent_msr_access(vcpu, msr))
-		return -EINVAL;
+		return vcpu->kvm->arch.has_protected_state ? -EINVAL : 0;
 
 	switch (ecx) {
 	case MSR_AMD64_TSC_RATIO:
-- 
2.43.0

Re: [PATCH] KVM: SEV-ES: Fix svm_get_msr()/svm_set_msr() for KVM_SEV_ES_INIT guests

[Nikunj A. Dadhania]

On 6/5/2024 5:05 AM, Michael Roth wrote:
> With commit 27bd5fdc24c0 ("KVM: SEV-ES: Prevent MSR access post VMSA
> encryption"), older VMMs like QEMU 9.0 and older will fail when booting
> SEV-ES guests with something like the following error:
> 
>   qemu-system-x86_64: error: failed to get MSR 0x174
>   qemu-system-x86_64: ../qemu.git/target/i386/kvm/kvm.c:3950: kvm_get_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.
> 
> This is because older VMMs that might still call
> svm_get_msr()/svm_set_msr() for SEV-ES guests after guest boot even if
> those interfaces were essentially just noops because of the vCPU state
> being encrypted and stored separately in the VMSA. Now those VMMs will
> get an -EINVAL and generally crash.
> 
> Newer VMMs that are aware of KVM_SEV_INIT2 however are already aware of
> the stricter limitations of what vCPU state can be sync'd during
> guest run-time, so newer QEMU for instance will work both for legacy
> KVM_SEV_ES_INIT interface as well as KVM_SEV_INIT2.
> 
> So when using KVM_SEV_INIT2 it's okay to assume userspace can deal with
> -EINVAL, whereas for legacy KVM_SEV_ES_INIT the kernel might be dealing
> with either an older VMM and so it needs to assume that returning
> -EINVAL might break the VMM.
> 
> Address this by only returning -EINVAL if the guest was started with
> KVM_SEV_INIT2. Otherwise, just silently return.
> 
> Cc: Ravi Bangoria <ravi.bangoria@amd.com>
> Cc: Nikunj A Dadhania <nikunj@amd.com>
> Reported-by: Srikanth Aithal <sraithal@amd.com>
> Closes: https://lore.kernel.org/lkml/37usuu4yu4ok7be2hqexhmcyopluuiqj3k266z4gajc2rcj4yo@eujb23qc3zcm/
> Fixes: 27bd5fdc24c0 ("KVM: SEV-ES: Prevent MSR access post VMSA encryption")
> Signed-off-by: Michael Roth <michael.roth@amd.com>

Reviewed-by: Nikunj A Dadhania <nikunj@amd.com>

> ---
>  arch/x86/kvm/svm/svm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index b252a2732b6f..c58da281f14f 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -2855,7 +2855,7 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  
>  	if (sev_es_prevent_msr_access(vcpu, msr_info)) {
>  		msr_info->data = 0;
> -		return -EINVAL;
> +		return vcpu->kvm->arch.has_protected_state ? -EINVAL : 0;
>  	}
>  
>  	switch (msr_info->index) {
> @@ -3010,7 +3010,7 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>  	u64 data = msr->data;
>  
>  	if (sev_es_prevent_msr_access(vcpu, msr))
> -		return -EINVAL;
> +		return vcpu->kvm->arch.has_protected_state ? -EINVAL : 0;
>  
>  	switch (ecx) {
>  	case MSR_AMD64_TSC_RATIO:
> 

Re: [PATCH] KVM: SEV-ES: Fix svm_get_msr()/svm_set_msr() for KVM_SEV_ES_INIT guests

[Aithal, Srikanth]

On 6/5/2024 5:05 AM, Michael Roth wrote:
> With commit 27bd5fdc24c0 ("KVM: SEV-ES: Prevent MSR access post VMSA
> encryption"), older VMMs like QEMU 9.0 and older will fail when booting
> SEV-ES guests with something like the following error:
> 
>    qemu-system-x86_64: error: failed to get MSR 0x174
>    qemu-system-x86_64: ../qemu.git/target/i386/kvm/kvm.c:3950: kvm_get_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.
> 
> This is because older VMMs that might still call
> svm_get_msr()/svm_set_msr() for SEV-ES guests after guest boot even if
> those interfaces were essentially just noops because of the vCPU state
> being encrypted and stored separately in the VMSA. Now those VMMs will
> get an -EINVAL and generally crash.
> 
> Newer VMMs that are aware of KVM_SEV_INIT2 however are already aware of
> the stricter limitations of what vCPU state can be sync'd during
> guest run-time, so newer QEMU for instance will work both for legacy
> KVM_SEV_ES_INIT interface as well as KVM_SEV_INIT2.
> 
> So when using KVM_SEV_INIT2 it's okay to assume userspace can deal with
> -EINVAL, whereas for legacy KVM_SEV_ES_INIT the kernel might be dealing
> with either an older VMM and so it needs to assume that returning
> -EINVAL might break the VMM.
> 
> Address this by only returning -EINVAL if the guest was started with
> KVM_SEV_INIT2. Otherwise, just silently return.
> 
> Cc: Ravi Bangoria <ravi.bangoria@amd.com>
> Cc: Nikunj A Dadhania <nikunj@amd.com>
> Reported-by: Srikanth Aithal <sraithal@amd.com>
> Closes: https://lore.kernel.org/lkml/37usuu4yu4ok7be2hqexhmcyopluuiqj3k266z4gajc2rcj4yo@eujb23qc3zcm/
> Fixes: 27bd5fdc24c0 ("KVM: SEV-ES: Prevent MSR access post VMSA encryption")
> Signed-off-by: Michael Roth <michael.roth@amd.com>
> ---
>   arch/x86/kvm/svm/svm.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index b252a2732b6f..c58da281f14f 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -2855,7 +2855,7 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>   
>   	if (sev_es_prevent_msr_access(vcpu, msr_info)) {
>   		msr_info->data = 0;
> -		return -EINVAL;
> +		return vcpu->kvm->arch.has_protected_state ? -EINVAL : 0;
>   	}
>   
>   	switch (msr_info->index) {
> @@ -3010,7 +3010,7 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>   	u64 data = msr->data;
>   
>   	if (sev_es_prevent_msr_access(vcpu, msr))
> -		return -EINVAL;
> +		return vcpu->kvm->arch.has_protected_state ? -EINVAL : 0;
>   
>   	switch (ecx) {
>   	case MSR_AMD64_TSC_RATIO:

Tested this out with older version of VMM, issue reported is resolved 
with the patch.
Tested-by: Srikanth Aithal <sraithal@amd.com>

Re: [PATCH] KVM: SEV-ES: Fix svm_get_msr()/svm_set_msr() for KVM_SEV_ES_INIT guests

[Ravi Bangoria]

On 6/5/2024 5:05 AM, Michael Roth wrote:
> With commit 27bd5fdc24c0 ("KVM: SEV-ES: Prevent MSR access post VMSA
> encryption"), older VMMs like QEMU 9.0 and older will fail when booting
> SEV-ES guests with something like the following error:
> 
>   qemu-system-x86_64: error: failed to get MSR 0x174
>   qemu-system-x86_64: ../qemu.git/target/i386/kvm/kvm.c:3950: kvm_get_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.
> 
> This is because older VMMs that might still call
> svm_get_msr()/svm_set_msr() for SEV-ES guests after guest boot even if
> those interfaces were essentially just noops because of the vCPU state
> being encrypted and stored separately in the VMSA. Now those VMMs will
> get an -EINVAL and generally crash.
> 
> Newer VMMs that are aware of KVM_SEV_INIT2 however are already aware of
> the stricter limitations of what vCPU state can be sync'd during
> guest run-time, so newer QEMU for instance will work both for legacy
> KVM_SEV_ES_INIT interface as well as KVM_SEV_INIT2.
> 
> So when using KVM_SEV_INIT2 it's okay to assume userspace can deal with
> -EINVAL, whereas for legacy KVM_SEV_ES_INIT the kernel might be dealing
> with either an older VMM and so it needs to assume that returning
> -EINVAL might break the VMM.
> 
> Address this by only returning -EINVAL if the guest was started with
> KVM_SEV_INIT2. Otherwise, just silently return.
> 
> Cc: Ravi Bangoria <ravi.bangoria@amd.com>
> Cc: Nikunj A Dadhania <nikunj@amd.com>
> Reported-by: Srikanth Aithal <sraithal@amd.com>
> Closes: https://lore.kernel.org/lkml/37usuu4yu4ok7be2hqexhmcyopluuiqj3k266z4gajc2rcj4yo@eujb23qc3zcm/
> Fixes: 27bd5fdc24c0 ("KVM: SEV-ES: Prevent MSR access post VMSA encryption")
> Signed-off-by: Michael Roth <michael.roth@amd.com>

Thanks for the fix.

Reviewed-by: Ravi Bangoria <ravi.bangoria@amd.com>