diff mbox series

[v2,03/10] KVM: SVM: Refuse to attempt VRMUN if an SEV-ES+ guest has an invalid VMSA

Message ID 20250227012541.3234589-4-seanjc@google.com (mailing list archive)
State New
Headers show
Series KVM: SVM: Attempt to cleanup SEV_FEATURES | expand

Commit Message

Sean Christopherson Feb. 27, 2025, 1:25 a.m. UTC 
Explicitly reject KVM_RUN with KVM_EXIT_FAIL_ENTRY if userspace "coerces"
KVM into running an SEV-ES+ guest with an invalid VMSA, e.g. by modifying
a vCPU's mp_state to be RUNNABLE after an SNP vCPU has undergone a Destroy
event.  On Destroy or failed Create, KVM marks the vCPU HALTED so that
*KVM* doesn't run the vCPU, but nothing prevents a misbehaving VMM from
manually making the vCPU RUNNABLE via KVM_SET_MP_STATE.

Attempting VMRUN with an invalid VMSA should be harmless, but knowingly
executing VMRUN with bad control state is at best dodgy.

Fixes: e366f92ea99e ("KVM: SEV: Support SEV-SNP AP Creation NAE event")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/sev.c | 16 +++++++++++++---
 arch/x86/kvm/svm/svm.c | 11 +++++++++--
 arch/x86/kvm/svm/svm.h |  2 +-
 3 files changed, 23 insertions(+), 6 deletions(-)

Comments

Tom Lendacky Feb. 27, 2025, 4:03 p.m. UTC | #1 
On 2/26/25 19:25, Sean Christopherson wrote:
> Explicitly reject KVM_RUN with KVM_EXIT_FAIL_ENTRY if userspace "coerces"
> KVM into running an SEV-ES+ guest with an invalid VMSA, e.g. by modifying
> a vCPU's mp_state to be RUNNABLE after an SNP vCPU has undergone a Destroy
> event.  On Destroy or failed Create, KVM marks the vCPU HALTED so that
> *KVM* doesn't run the vCPU, but nothing prevents a misbehaving VMM from
> manually making the vCPU RUNNABLE via KVM_SET_MP_STATE.
> 
> Attempting VMRUN with an invalid VMSA should be harmless, but knowingly
> executing VMRUN with bad control state is at best dodgy.
> 
> Fixes: e366f92ea99e ("KVM: SEV: Support SEV-SNP AP Creation NAE event")
> Signed-off-by: Sean Christopherson <seanjc@google.com>

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

> ---
>  arch/x86/kvm/svm/sev.c | 16 +++++++++++++---
>  arch/x86/kvm/svm/svm.c | 11 +++++++++--
>  arch/x86/kvm/svm/svm.h |  2 +-
>  3 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 719cd48330f1..218738a360ba 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3452,10 +3452,19 @@ void sev_es_unmap_ghcb(struct vcpu_svm *svm)
>  	svm->sev_es.ghcb = NULL;
>  }
>  
> -void pre_sev_run(struct vcpu_svm *svm, int cpu)
> +int pre_sev_run(struct vcpu_svm *svm, int cpu)
>  {
>  	struct svm_cpu_data *sd = per_cpu_ptr(&svm_data, cpu);
> -	unsigned int asid = sev_get_asid(svm->vcpu.kvm);
> +	struct kvm *kvm = svm->vcpu.kvm;
> +	unsigned int asid = sev_get_asid(kvm);
> +
> +	/*
> +	 * Reject KVM_RUN if userspace attempts to run the vCPU with an invalid
> +	 * VMSA, e.g. if userspace forces the vCPU to be RUNNABLE after an SNP
> +	 * AP Destroy event.
> +	 */
> +	if (sev_es_guest(kvm) && !VALID_PAGE(svm->vmcb->control.vmsa_pa))
> +		return -EINVAL;
>  
>  	/* Assign the asid allocated with this SEV guest */
>  	svm->asid = asid;
> @@ -3468,11 +3477,12 @@ void pre_sev_run(struct vcpu_svm *svm, int cpu)
>  	 */
>  	if (sd->sev_vmcbs[asid] == svm->vmcb &&
>  	    svm->vcpu.arch.last_vmentry_cpu == cpu)
> -		return;
> +		return 0;
>  
>  	sd->sev_vmcbs[asid] = svm->vmcb;
>  	svm->vmcb->control.tlb_ctl = TLB_CONTROL_FLUSH_ASID;
>  	vmcb_mark_dirty(svm->vmcb, VMCB_ASID);
> +	return 0;
>  }
>  
>  #define GHCB_SCRATCH_AREA_LIMIT		(16ULL * PAGE_SIZE)
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index b8aa0f36850f..f72bcf2e590e 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3587,7 +3587,7 @@ static int svm_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
>  	return svm_invoke_exit_handler(vcpu, exit_code);
>  }
>  
> -static void pre_svm_run(struct kvm_vcpu *vcpu)
> +static int pre_svm_run(struct kvm_vcpu *vcpu)
>  {
>  	struct svm_cpu_data *sd = per_cpu_ptr(&svm_data, vcpu->cpu);
>  	struct vcpu_svm *svm = to_svm(vcpu);
> @@ -3609,6 +3609,8 @@ static void pre_svm_run(struct kvm_vcpu *vcpu)
>  	/* FIXME: handle wraparound of asid_generation */
>  	if (svm->current_vmcb->asid_generation != sd->asid_generation)
>  		new_asid(svm, sd);
> +
> +	return 0;
>  }
>  
>  static void svm_inject_nmi(struct kvm_vcpu *vcpu)
> @@ -4231,7 +4233,12 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu,
>  	if (force_immediate_exit)
>  		smp_send_reschedule(vcpu->cpu);
>  
> -	pre_svm_run(vcpu);
> +	if (pre_svm_run(vcpu)) {
> +		vcpu->run->exit_reason = KVM_EXIT_FAIL_ENTRY;
> +		vcpu->run->fail_entry.hardware_entry_failure_reason = SVM_EXIT_ERR;
> +		vcpu->run->fail_entry.cpu = vcpu->cpu;
> +		return EXIT_FASTPATH_EXIT_USERSPACE;
> +	}
>  
>  	sync_lapic_to_cr8(vcpu);
>  
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 5b159f017055..e51852977b70 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -713,7 +713,7 @@ void avic_refresh_virtual_apic_mode(struct kvm_vcpu *vcpu);
>  
>  /* sev.c */
>  
> -void pre_sev_run(struct vcpu_svm *svm, int cpu);
> +int pre_sev_run(struct vcpu_svm *svm, int cpu);
>  void sev_init_vmcb(struct vcpu_svm *svm);
>  void sev_vcpu_after_set_cpuid(struct vcpu_svm *svm);
>  int sev_es_string_io(struct vcpu_svm *svm, int size, unsigned int port, int in);
Gupta, Pankaj Feb. 27, 2025, 4:56 p.m. UTC | #2 
On 2/27/2025 2:25 AM, Sean Christopherson wrote:
> Explicitly reject KVM_RUN with KVM_EXIT_FAIL_ENTRY if userspace "coerces"
> KVM into running an SEV-ES+ guest with an invalid VMSA, e.g. by modifying
> a vCPU's mp_state to be RUNNABLE after an SNP vCPU has undergone a Destroy
> event.  On Destroy or failed Create, KVM marks the vCPU HALTED so that
> *KVM* doesn't run the vCPU, but nothing prevents a misbehaving VMM from
> manually making the vCPU RUNNABLE via KVM_SET_MP_STATE.
> 
> Attempting VMRUN with an invalid VMSA should be harmless, but knowingly
> executing VMRUN with bad control state is at best dodgy.
> 
> Fixes: e366f92ea99e ("KVM: SEV: Support SEV-SNP AP Creation NAE event")
> Signed-off-by: Sean Christopherson <seanjc@google.com>

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

> ---
>   arch/x86/kvm/svm/sev.c | 16 +++++++++++++---
>   arch/x86/kvm/svm/svm.c | 11 +++++++++--
>   arch/x86/kvm/svm/svm.h |  2 +-
>   3 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 719cd48330f1..218738a360ba 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3452,10 +3452,19 @@ void sev_es_unmap_ghcb(struct vcpu_svm *svm)
>   	svm->sev_es.ghcb = NULL;
>   }
>   
> -void pre_sev_run(struct vcpu_svm *svm, int cpu)
> +int pre_sev_run(struct vcpu_svm *svm, int cpu)
>   {
>   	struct svm_cpu_data *sd = per_cpu_ptr(&svm_data, cpu);
> -	unsigned int asid = sev_get_asid(svm->vcpu.kvm);
> +	struct kvm *kvm = svm->vcpu.kvm;
> +	unsigned int asid = sev_get_asid(kvm);
> +
> +	/*
> +	 * Reject KVM_RUN if userspace attempts to run the vCPU with an invalid
> +	 * VMSA, e.g. if userspace forces the vCPU to be RUNNABLE after an SNP
> +	 * AP Destroy event.
> +	 */
> +	if (sev_es_guest(kvm) && !VALID_PAGE(svm->vmcb->control.vmsa_pa))
> +		return -EINVAL;
>   
>   	/* Assign the asid allocated with this SEV guest */
>   	svm->asid = asid;
> @@ -3468,11 +3477,12 @@ void pre_sev_run(struct vcpu_svm *svm, int cpu)
>   	 */
>   	if (sd->sev_vmcbs[asid] == svm->vmcb &&
>   	    svm->vcpu.arch.last_vmentry_cpu == cpu)
> -		return;
> +		return 0;
>   
>   	sd->sev_vmcbs[asid] = svm->vmcb;
>   	svm->vmcb->control.tlb_ctl = TLB_CONTROL_FLUSH_ASID;
>   	vmcb_mark_dirty(svm->vmcb, VMCB_ASID);
> +	return 0;
>   }
>   
>   #define GHCB_SCRATCH_AREA_LIMIT		(16ULL * PAGE_SIZE)
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index b8aa0f36850f..f72bcf2e590e 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3587,7 +3587,7 @@ static int svm_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
>   	return svm_invoke_exit_handler(vcpu, exit_code);
>   }
>   
> -static void pre_svm_run(struct kvm_vcpu *vcpu)
> +static int pre_svm_run(struct kvm_vcpu *vcpu)
>   {
>   	struct svm_cpu_data *sd = per_cpu_ptr(&svm_data, vcpu->cpu);
>   	struct vcpu_svm *svm = to_svm(vcpu);
> @@ -3609,6 +3609,8 @@ static void pre_svm_run(struct kvm_vcpu *vcpu)
>   	/* FIXME: handle wraparound of asid_generation */
>   	if (svm->current_vmcb->asid_generation != sd->asid_generation)
>   		new_asid(svm, sd);
> +
> +	return 0;
>   }
>   
>   static void svm_inject_nmi(struct kvm_vcpu *vcpu)
> @@ -4231,7 +4233,12 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu,
>   	if (force_immediate_exit)
>   		smp_send_reschedule(vcpu->cpu);
>   
> -	pre_svm_run(vcpu);
> +	if (pre_svm_run(vcpu)) {
> +		vcpu->run->exit_reason = KVM_EXIT_FAIL_ENTRY;
> +		vcpu->run->fail_entry.hardware_entry_failure_reason = SVM_EXIT_ERR;
> +		vcpu->run->fail_entry.cpu = vcpu->cpu;
> +		return EXIT_FASTPATH_EXIT_USERSPACE;
> +	}
>   
>   	sync_lapic_to_cr8(vcpu);
>   
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 5b159f017055..e51852977b70 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -713,7 +713,7 @@ void avic_refresh_virtual_apic_mode(struct kvm_vcpu *vcpu);
>   
>   /* sev.c */
>   
> -void pre_sev_run(struct vcpu_svm *svm, int cpu);
> +int pre_sev_run(struct vcpu_svm *svm, int cpu);
>   void sev_init_vmcb(struct vcpu_svm *svm);
>   void sev_vcpu_after_set_cpuid(struct vcpu_svm *svm);
>   int sev_es_string_io(struct vcpu_svm *svm, int size, unsigned int port, int in);
diff mbox series

Patch

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 719cd48330f1..218738a360ba 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3452,10 +3452,19 @@  void sev_es_unmap_ghcb(struct vcpu_svm *svm)
 	svm->sev_es.ghcb = NULL;
 }
 
-void pre_sev_run(struct vcpu_svm *svm, int cpu)
+int pre_sev_run(struct vcpu_svm *svm, int cpu)
 {
 	struct svm_cpu_data *sd = per_cpu_ptr(&svm_data, cpu);
-	unsigned int asid = sev_get_asid(svm->vcpu.kvm);
+	struct kvm *kvm = svm->vcpu.kvm;
+	unsigned int asid = sev_get_asid(kvm);
+
+	/*
+	 * Reject KVM_RUN if userspace attempts to run the vCPU with an invalid
+	 * VMSA, e.g. if userspace forces the vCPU to be RUNNABLE after an SNP
+	 * AP Destroy event.
+	 */
+	if (sev_es_guest(kvm) && !VALID_PAGE(svm->vmcb->control.vmsa_pa))
+		return -EINVAL;
 
 	/* Assign the asid allocated with this SEV guest */
 	svm->asid = asid;
@@ -3468,11 +3477,12 @@  void pre_sev_run(struct vcpu_svm *svm, int cpu)
 	 */
 	if (sd->sev_vmcbs[asid] == svm->vmcb &&
 	    svm->vcpu.arch.last_vmentry_cpu == cpu)
-		return;
+		return 0;
 
 	sd->sev_vmcbs[asid] = svm->vmcb;
 	svm->vmcb->control.tlb_ctl = TLB_CONTROL_FLUSH_ASID;
 	vmcb_mark_dirty(svm->vmcb, VMCB_ASID);
+	return 0;
 }
 
 #define GHCB_SCRATCH_AREA_LIMIT		(16ULL * PAGE_SIZE)
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index b8aa0f36850f..f72bcf2e590e 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3587,7 +3587,7 @@  static int svm_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
 	return svm_invoke_exit_handler(vcpu, exit_code);
 }
 
-static void pre_svm_run(struct kvm_vcpu *vcpu)
+static int pre_svm_run(struct kvm_vcpu *vcpu)
 {
 	struct svm_cpu_data *sd = per_cpu_ptr(&svm_data, vcpu->cpu);
 	struct vcpu_svm *svm = to_svm(vcpu);
@@ -3609,6 +3609,8 @@  static void pre_svm_run(struct kvm_vcpu *vcpu)
 	/* FIXME: handle wraparound of asid_generation */
 	if (svm->current_vmcb->asid_generation != sd->asid_generation)
 		new_asid(svm, sd);
+
+	return 0;
 }
 
 static void svm_inject_nmi(struct kvm_vcpu *vcpu)
@@ -4231,7 +4233,12 @@  static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu,
 	if (force_immediate_exit)
 		smp_send_reschedule(vcpu->cpu);
 
-	pre_svm_run(vcpu);
+	if (pre_svm_run(vcpu)) {
+		vcpu->run->exit_reason = KVM_EXIT_FAIL_ENTRY;
+		vcpu->run->fail_entry.hardware_entry_failure_reason = SVM_EXIT_ERR;
+		vcpu->run->fail_entry.cpu = vcpu->cpu;
+		return EXIT_FASTPATH_EXIT_USERSPACE;
+	}
 
 	sync_lapic_to_cr8(vcpu);
 
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 5b159f017055..e51852977b70 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -713,7 +713,7 @@  void avic_refresh_virtual_apic_mode(struct kvm_vcpu *vcpu);
 
 /* sev.c */
 
-void pre_sev_run(struct vcpu_svm *svm, int cpu);
+int pre_sev_run(struct vcpu_svm *svm, int cpu);
 void sev_init_vmcb(struct vcpu_svm *svm);
 void sev_vcpu_after_set_cpuid(struct vcpu_svm *svm);
 int sev_es_string_io(struct vcpu_svm *svm, int size, unsigned int port, int in);