[PATCH 6.12.y 0/2] KVM: SEV: Backports for GHCB leak fix

[PATCH 6.12.y 0/2] KVM: SEV: Backports for GHCB leak fix

[Sean Christopherson]

Backports for what are effectively patches 2/4 and 4/4 from this chunk of
commits (1/4 and 3/4 are already in 6.12.y).

  8618004d3e89 KVM: Don't WARN if memory is dirtied without a vCPU when the VM is dying
  08385c5e1814 KVM: SEV: Move sev_free_vcpu() down below sev_es_unmap_ghcb()
  f041dc80de4a KVM: SEV: Decouple the need to sync the GHCB SA from the need to free the SA
  db38bcb33110 KVM: SEV: Unmap and unpin the GHCB as needed on vCPU free

Cc: Jack Wang <jinpu.wang@ionos.com>

Sean Christopherson (2):
  KVM: SEV: Move sev_free_vcpu() down below sev_es_unmap_ghcb()
  KVM: SEV: Unmap and unpin the GHCB as needed on vCPU free

 arch/x86/kvm/svm/sev.c | 84 ++++++++++++++++++++++--------------------
 1 file changed, 45 insertions(+), 39 deletions(-)


base-commit: 0b8f247169e487eff2d4c2dd531bc43f7efda2cb
-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH 6.12.y 1/2] KVM: SEV: Move sev_free_vcpu() down below sev_es_unmap_ghcb()

[Sean Christopherson]

[ Upstream commit 08385c5e1814edee829ffe475d559ed730354335 ]

Relocate sev_free_vcpu() down in sev.c so that it's definition comes after
sev_es_unmap_ghcb().  This will allow sharing unmap functionality between
the two functions without needing a forward declaration (or weird placement
of the common code).

No functional change intended.

Cc: stable@vger.kernel.org
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Reviewed-by: Michael Roth <michael.roth@amd.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
Message-ID: <20260501202250.2115252-16-seanjc@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <20260529183549.1104619-16-pbonzini@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
[sean: Preserve use of sev_es_guest() as is_sev_es_guest() doesn't exist
       in 6.12, resolve superficial conflict due to pre_sev_run()
       prototype mismatch.]
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/sev.c | 62 +++++++++++++++++++++---------------------
 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 115c59c86f44..0f79e052ac42 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3168,37 +3168,6 @@ void sev_guest_memory_reclaimed(struct kvm *kvm)
 	wbinvd_on_all_cpus();
 }
 
-void sev_free_vcpu(struct kvm_vcpu *vcpu)
-{
-	struct vcpu_svm *svm;
-
-	if (!sev_es_guest(vcpu->kvm))
-		return;
-
-	svm = to_svm(vcpu);
-
-	/*
-	 * If it's an SNP guest, then the VMSA was marked in the RMP table as
-	 * a guest-owned page. Transition the page to hypervisor state before
-	 * releasing it back to the system.
-	 */
-	if (sev_snp_guest(vcpu->kvm)) {
-		u64 pfn = __pa(svm->sev_es.vmsa) >> PAGE_SHIFT;
-
-		if (kvm_rmp_make_shared(vcpu->kvm, pfn, PG_LEVEL_4K))
-			goto skip_vmsa_free;
-	}
-
-	if (vcpu->arch.guest_state_protected)
-		sev_flush_encrypted_page(vcpu, svm->sev_es.vmsa);
-
-	__free_page(virt_to_page(svm->sev_es.vmsa));
-
-skip_vmsa_free:
-	if (svm->sev_es.ghcb_sa_free)
-		kvfree(svm->sev_es.ghcb_sa);
-}
-
 static void dump_ghcb(struct vcpu_svm *svm)
 {
 	struct ghcb *ghcb = svm->sev_es.ghcb;
@@ -3475,6 +3444,37 @@ void sev_es_unmap_ghcb(struct vcpu_svm *svm)
 	svm->sev_es.ghcb = NULL;
 }
 
+void sev_free_vcpu(struct kvm_vcpu *vcpu)
+{
+	struct vcpu_svm *svm;
+
+	if (!sev_es_guest(vcpu->kvm))
+		return;
+
+	svm = to_svm(vcpu);
+
+	/*
+	 * If it's an SNP guest, then the VMSA was marked in the RMP table as
+	 * a guest-owned page. Transition the page to hypervisor state before
+	 * releasing it back to the system.
+	 */
+	if (sev_snp_guest(vcpu->kvm)) {
+		u64 pfn = __pa(svm->sev_es.vmsa) >> PAGE_SHIFT;
+
+		if (kvm_rmp_make_shared(vcpu->kvm, pfn, PG_LEVEL_4K))
+			goto skip_vmsa_free;
+	}
+
+	if (vcpu->arch.guest_state_protected)
+		sev_flush_encrypted_page(vcpu, svm->sev_es.vmsa);
+
+	__free_page(virt_to_page(svm->sev_es.vmsa));
+
+skip_vmsa_free:
+	if (svm->sev_es.ghcb_sa_free)
+		kvfree(svm->sev_es.ghcb_sa);
+}
+
 void pre_sev_run(struct vcpu_svm *svm, int cpu)
 {
 	struct svm_cpu_data *sd = per_cpu_ptr(&svm_data, cpu);
-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH 6.12.y 2/2] KVM: SEV: Unmap and unpin the GHCB as needed on vCPU free

[Sean Christopherson]

[ Upstream commit db38bcb3311053954f62b865cd2d86e164b04351 ]

Unmap and unpin the GHCB as needed when freeing a vCPU.  If the VM is
destroyed after mapping+pinning the GHCB on #VMGEXIT, without re-running
the vCPU, KVM will effectively leak the GHCB and any mappings created for
the GHCB.

Fixes: 291bd20d5d88 ("KVM: SVM: Add initial support for a VMGEXIT VMEXIT")
Cc: stable@vger.kernel.org
Tested-by: Michael Roth <michael.roth@amd.com>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Reviewed-by: Michael Roth <michael.roth@amd.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
Message-ID: <20260501202250.2115252-18-seanjc@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <20260529183549.1104619-18-pbonzini@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
[sean: Preserve @dirty=true param to kvm_vcpu_unmap()]
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/sev.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 0f79e052ac42..7914cdea4cdd 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3412,6 +3412,20 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
 	return 1;
 }
 
+static void __sev_es_unmap_ghcb(struct vcpu_svm *svm)
+{
+	if (svm->sev_es.ghcb_sa_free) {
+		kvfree(svm->sev_es.ghcb_sa);
+		svm->sev_es.ghcb_sa = NULL;
+		svm->sev_es.ghcb_sa_free = false;
+	}
+
+	if (svm->sev_es.ghcb) {
+		kvm_vcpu_unmap(&svm->vcpu, &svm->sev_es.ghcb_map, true);
+		svm->sev_es.ghcb = NULL;
+	}
+}
+
 void sev_es_unmap_ghcb(struct vcpu_svm *svm)
 {
 	/* Clear any indication that the vCPU is in a type of AP Reset Hold */
@@ -3430,18 +3444,11 @@ void sev_es_unmap_ghcb(struct vcpu_svm *svm)
 		svm->sev_es.ghcb_sa_sync = false;
 	}
 
-	if (svm->sev_es.ghcb_sa_free) {
-		kvfree(svm->sev_es.ghcb_sa);
-		svm->sev_es.ghcb_sa = NULL;
-		svm->sev_es.ghcb_sa_free = false;
-	}
-
 	trace_kvm_vmgexit_exit(svm->vcpu.vcpu_id, svm->sev_es.ghcb);
 
 	sev_es_sync_to_ghcb(svm);
 
-	kvm_vcpu_unmap(&svm->vcpu, &svm->sev_es.ghcb_map, true);
-	svm->sev_es.ghcb = NULL;
+	__sev_es_unmap_ghcb(svm);
 }
 
 void sev_free_vcpu(struct kvm_vcpu *vcpu)
@@ -3471,8 +3478,7 @@ void sev_free_vcpu(struct kvm_vcpu *vcpu)
 	__free_page(virt_to_page(svm->sev_es.vmsa));
 
 skip_vmsa_free:
-	if (svm->sev_es.ghcb_sa_free)
-		kvfree(svm->sev_es.ghcb_sa);
+	__sev_es_unmap_ghcb(svm);
 }
 
 void pre_sev_run(struct vcpu_svm *svm, int cpu)
-- 
2.55.0.rc0.799.gd6f94ed593-goog

Re: [PATCH 6.12.y 2/2] KVM: SEV: Unmap and unpin the GHCB as needed on vCPU free

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] SEV-ES GHCB unmap functions execute sleep-capable and page-faulting operations while preemption is disabled, leading to kernel panics and silent guest data corruption.
--

commit 33a2dedac2ed0fec45371ed9062d6e4dbc63f2b1
Author: Sean Christopherson <seanjc@google.com>

KVM: SEV: Unmap and unpin the GHCB as needed on vCPU free
    
Unmap and unpin the GHCB as needed when freeing a vCPU.  If the VM is
destroyed after mapping+pinning the GHCB on #VMGEXIT, without re-running
the vCPU, KVM will effectively leak the GHCB and any mappings created for
the GHCB.

> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 0f79e052ac421..7914cdea4cdd1 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3412,6 +3412,20 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
>  	return 1;
>  }
>  
> +static void __sev_es_unmap_ghcb(struct vcpu_svm *svm)
> +{
> +	if (svm->sev_es.ghcb_sa_free) {
> +		kvfree(svm->sev_es.ghcb_sa);

[Severity: High]
This isn't a bug introduced by this patch, but can this lead to sleeping in
atomic context or a kernel panic?

If a guest triggers a #VMGEXIT that induces a userspace exit while using a
scratch buffer outside the GHCB, it appears we can reach this execution path:

arch/x86/kvm/x86.c:vcpu_enter_guest() {
    ...
    preempt_disable();
    kvm_x86_call(prepare_switch_to_guest)(vcpu);
    ...
}

Where the x86 callback resolves to svm_prepare_switch_to_guest() which calls:
    sev_es_unmap_ghcb()
        __sev_es_unmap_ghcb()

Since __sev_es_unmap_ghcb() calls kvfree() and kvm_vcpu_unmap() (which can
call memunmap), and sev_es_unmap_ghcb() calls kvm_write_guest(), aren't these
sleep-capable operations being executed while preemption is disabled?

> +		svm->sev_es.ghcb_sa = NULL;
> +		svm->sev_es.ghcb_sa_free = false;
> +	}
> +
> +	if (svm->sev_es.ghcb) {
> +		kvm_vcpu_unmap(&svm->vcpu, &svm->sev_es.ghcb_map, true);
> +		svm->sev_es.ghcb = NULL;
> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630172204.279784-1-seanjc@google.com?part=2

Re: [PATCH 6.12.y 0/2] KVM: SEV: Backports for GHCB leak fix

[Sasha Levin]

> Backports for what are effectively patches 2/4 and 4/4 from this chunk of
> commits (1/4 and 3/4 are already in 6.12.y).
>
>   8618004d3e89 KVM: Don't WARN if memory is dirtied without a vCPU when the VM is dying
>   08385c5e1814 KVM: SEV: Move sev_free_vcpu() down below sev_es_unmap_ghcb()
>   f041dc80de4a KVM: SEV: Decouple the need to sync the GHCB SA from the need to free the SA
>   db38bcb33110 KVM: SEV: Unmap and unpin the GHCB as needed on vCPU free

Both queued for 6.12.y, thanks.

-- 
Thanks,
Sasha