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: <Z70UsI0kgcZu844d@google.com>
Date: Mon, 24 Feb 2025 16:54:08 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Tom Lendacky <thomas.lendacky@....com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, 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 Mon, Feb 24, 2025, Tom Lendacky wrote:
> On 2/24/25 16:55, Sean Christopherson wrote:
> > On Mon, Feb 24, 2025, Tom Lendacky wrote:
> >> On 2/18/25 19:26, Sean Christopherson wrote:
> >>> -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
> > 
> > I haven't tested, but based on what the APM says, I'm pretty sure this would crash
> > the host due to a #GP on VMRUN, i.e. due to the resulting kvm_spurious_fault().
> > 
> >   IF (rAX contains an unsupported physical address)
> >     EXCEPTION [#GP]
> 
> Well that's for the VMCB, the VMSA is pointed to by the VMCB and results
> in a VMEXIT code of -1 if you don't supply a proper page-aligned,
> physical address.

Ah, good to know (and somewhat of a relief :-) ).

> >>>  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;
> 
> In testing this out, I think userspace continues on because I eventually
> get:
> 
> KVM_GET_PIT2 failed: Input/output error
> /tmp/cmdline.98112: line 1: 98163 Aborted (core dumped) ...
> 
> Haven't looked too close, but maybe an exit_reason needs to be set to
> get qemu to quit sooner?

Oh, the irony.  In trying to do the right thing (exit to userspace), I managed to
do the wrong thing.

If KVM tried to re-enter the guest, vcpu_enter_guest() would have encountered
the KVM_REQ_DEAD and exited with -EIO.

		if (kvm_check_request(KVM_REQ_VM_DEAD, vcpu)) {
			r = -EIO;
			goto out;
		}

By returning EXIT_FASTPATH_EXIT_USERSPACE, KVM exited to userspace more directly
and returned '0' instead of -EIO.

Getting KVM to return -EIO is easy, but doing so feels wrong, especially if we
take the quick-and-dirty route like so:

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 454fd6b8f3db..9c8b400e04f2 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -11102,7 +11102,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
                kvm_lapic_sync_from_vapic(vcpu);
 
        if (unlikely(exit_fastpath == EXIT_FASTPATH_EXIT_USERSPACE))
-               return 0;
+               return kvm_test_request(KVM_REQ_VM_DEAD, vcpu) ? -EIO : 0;
 
        r = kvm_x86_call(handle_exit)(vcpu, exit_fastpath);
        return r;

Given that, IIUC, KVM would eventually return KVM_EXIT_FAIL_ENTRY, I like your
idea of returning meaningful information.  And unless I'm missing something, that
would obviate any need to terminate the VM, which would address your earlier point
of whether terminating the VM is truly better than returning than returning a
familiar error code.

So this? (completely untested)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 7345cac6f93a..71b340cbe561 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3463,10 +3463,8 @@ int pre_sev_run(struct vcpu_svm *svm, int cpu)
         * 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 (sev_es_guest(kvm) && !VALID_PAGE(svm->vmcb->control.vmsa_pa))
+               return -EINVAL;
 
        /* Assign the asid allocated with this SEV guest */
        svm->asid = asid;
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 46e0b65a9fec..f72bcf2e590e 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4233,8 +4233,12 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu,
        if (force_immediate_exit)
                smp_send_reschedule(vcpu->cpu);
 
-       if (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);