linux-kernel - Re: [PATCH 03/10] KVM: SVM: Terminate the VM if a SEV-ES+ guest is run with an invalid VMSA

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: <4e762d94-97d4-2822-4935-2f5ab409ab29@amd.com>
Date: Mon, 24 Feb 2025 15:03:16 -0600
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,
 Naveen N Rao <naveen@...nel.org>, Kim Phillips <kim.phillips@....com>,
 Alexey Kardashevskiy <aik@....com>
Subject: Re: [PATCH 03/10] KVM: SVM: Terminate the VM if a SEV-ES+ guest is
 run with an invalid VMSA

On 2/18/25 19:26, Sean Christopherson wrote:
> Terminate the VM if userspace attempts to run an SEV-SNP (or -ES) vCPU
> that has an invalid VMSA.  With SNP's AP Create/Destroy hypercalls, it's
> possible for an SNP vCPU to end up with an invalid VMSA, e.g. through a
> deliberate Destroy or a failed Create event.  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.
> 
> Fixes: e366f92ea99e ("KVM: SEV: Support SEV-SNP AP Creation NAE event")
> Cc: stable@...r.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>  arch/x86/kvm/svm/sev.c | 18 +++++++++++++++---
>  arch/x86/kvm/svm/svm.c |  7 +++++--
>  arch/x86/kvm/svm/svm.h |  2 +-
>  3 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 6c6d45e13858..e14a37dbc6ea 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3452,10 +3452,21 @@ 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);
> +
> +	/*
> +	 * Terminate the VM 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)) {
> +		kvm_vm_dead(kvm);
> +		return -EIO;
> +	}

If a VMRUN is performed with the vmsa_pa value set to INVALID_PAGE, the
VMRUN will fail and KVM will dump the VMCB and exit back to userspace
with KVM_EXIT_INTERNAL_ERROR.

Is doing this preferrable to that? If so, should a vcpu_unimpl() message
be issued, too, to better identify the reason for marking the VM dead?

>  
>  	/* Assign the asid allocated with this SEV guest */
>  	svm->asid = asid;
> @@ -3468,11 +3479,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..46e0b65a9fec 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,8 @@ 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))
> +		return EXIT_FASTPATH_EXIT_USERSPACE;

Since the return code from pre_svm_run() is never used, should it just
be a bool function, then?

Thanks,
Tom

>  
>  	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);