Re: [PATCH 3/4] kvm: svm: Support guest-provided VMSA for launching

[sashiko-bot@kernel.org]

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

New issues:
- [High] Resolving the guest-provided VMSA GPA via `gfn_to_memslot()` occurs without holding `kvm->srcu` or `kvm->slots_lock`, violating KVM's memslot locking hierarchy.

Pre-existing issues:
- [Critical] Guest-provided VMSA pages are freed to the host buddy allocator while still in the RMP Firmware state, allowing userspace to trigger a host kernel panic (Denial of Service).
- [High] KVM-allocated VMSA pages for successfully launched SNP VMs are permanently leaked during VM teardown.
--

commit 7ccfa1323e073ebe13797e68f8f429e2d5dfb5ec
Author: Joerg Roedel <joerg.roedel@amd.com>

kvm: svm: Support guest-provided VMSA for launching
    
Introduce a way to provide a guest GPA as the initial BSP VMSA and
avoid allocating KVM-managed VMSAs in this case. Only one
guest-provided VMSA is supported at the moment as IGVM also only
supports to set a single VMSA.

> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 0aa62cb802fe5..0c42af2e62016 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c

[ ... ]

> @@ -2680,6 +2681,46 @@ static int snp_launch_update(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  	return 0;
>  }
>  
> +static int snp_init_guest_vmsa(struct kvm_vcpu *vcpu, gpa_t vmsa_gpa)
> +{
> +	/* Only one initial guest VMSA can exist (per IGVM) - so it belongs to the BSP */
> +	if (vcpu->vcpu_idx != 0)
> +		return 0;
> +
> +	/* VMSA already private and encrypted via LAUNCH_UPDATE */
> +	sev_es_set_guest_vmsa(vcpu, vmsa_gpa);
> +
> +	return 0;
> +}

[Severity: Critical]
This is a pre-existing issue, but can unprivileged userspace panic the host
kernel by punching a hole in the guest_memfd file backing the guest-provided
VMSA pages?

Guest-provided VMSA pages are placed into the VMSA (Firmware) RMP state.
However, KVM drops its page reference immediately after lookup:

arch/x86/kvm/svm/sev.c:sev_es_vmsa_pa() {
    ...
    kvm_release_page_clean(page);
    ...
}

If userspace triggers a page release, the page is freed by the core MM
subsystem. During freeing, sev_gmem_invalidate() attempts to transition the
page via rmp_make_shared(), which architecturally fails on VMSA-state pages:

arch/x86/kvm/svm/sev.c:sev_gmem_invalidate() {
    ...
    rc = rmp_make_shared(pfn, use_2m_update ? PG_LEVEL_2M : PG_LEVEL_4K);
    if (WARN_ONCE(rc, "SEV: Failed to update RMP entry for PFN 0x%llx error %d\n", pfn, rc))
        goto next_pfn;
    ...
}

Because the failure is only logged as a warning, the page is returned to the
buddy allocator in Firmware state. When the host reallocates and accesses
this page, it triggers a fatal RMP #PF and panics the kernel. KVM must
properly reclaim VMSA pages before allowing them to be freed.

> +
> +static int snp_init_kvm_vmsa(struct kvm_vcpu *vcpu,
> +			     struct sev_data_snp_launch_update *data,
> +			     struct kvm_sev_cmd *argp)
> +{
> +	struct vcpu_svm *svm = to_svm(vcpu);
> +	int ret;
> +	void *vmsa;
> +
> +	ret = sev_es_sync_vmsa(svm);
> +	if (ret)
> +		return ret;
> +
> +	vmsa = sev_es_vmsa_ref(vcpu);
> +
> +	ret = sev_es_vcpu_vmsa_make_private(vcpu);
> +	if (ret)
> +		return ret;
> +
> +	/* Issue the SNP command to encrypt the VMSA */
> +	data->address = __sme_pa(vmsa);
> +	ret = __sev_issue_cmd(argp->sev_fd, SEV_CMD_SNP_LAUNCH_UPDATE,
> +			      data, &argp->error);
> +	if (ret)
> +		sev_snp_vcpu_reclaim_vmsa(vcpu);
> +
> +	return ret;
> +}

[Severity: High]
This is a pre-existing issue, but does KVM permanently leak successfully 
launched SNP VM VMSA pages during VM teardown?

During normal teardown of an SNP guest, KVM attempts to transition its
allocated VMSA pages to shared state via kvm_rmp_make_shared():

arch/x86/kvm/svm/sev.c:sev_es_vcpu_free_vmsa() {
    ...
    if (kvm_rmp_make_shared(vcpu->kvm, pfn, PG_LEVEL_4K)) {
        pr_err("Failed to make VMSA page shared - leaking it to avoid re-use\n");
        goto out;
    }
    ...
}

kvm_rmp_make_shared() uses the RMPUPDATE instruction, which architecturally
fails on pages that are in the VMSA state. The function detects the failure,
logs the error, and permanently leaks the page. Because VMs are destroyed
routinely, this will eventually exhaust host memory. KVM must use
snp_page_reclaim() instead of kvm_rmp_make_shared() for pages in the VMSA
state.

>  static int snp_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  {
>  	struct kvm_sev_info *sev = to_kvm_sev_info(kvm);
> @@ -2700,28 +2741,13 @@ static int snp_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  
>  	kvm_for_each_vcpu(i, vcpu, kvm) {
>  		struct vcpu_svm *svm = to_svm(vcpu);
> -		void *vmsa;
>  
> -		ret = sev_es_sync_vmsa(svm);
> +		ret = VALID_PAGE(sev->initial_vmsa_gpa) ?
> +			snp_init_guest_vmsa(vcpu, sev->initial_vmsa_gpa) :
> +			snp_init_kvm_vmsa(vcpu, &data, argp);
>  		if (ret)
>  			goto out;
>  
> -		vmsa = sev_es_vmsa_ref(vcpu);
> -
> -		ret = sev_es_vcpu_vmsa_make_private(vcpu);
> -		if (ret)
> -			goto out;
> -
> -		/* Issue the SNP command to encrypt the VMSA */
> -		data.address = __sme_pa(vmsa);
> -		ret = __sev_issue_cmd(argp->sev_fd, SEV_CMD_SNP_LAUNCH_UPDATE,
> -				      &data, &argp->error);
> -		if (ret) {
> -			sev_snp_vcpu_reclaim_vmsa(vcpu);
> -
> -			goto out;
> -		}
> -
>  		svm->vcpu.arch.guest_state_protected = true;
>  
>  		/* VMSA encrypted - put it into the VMCB */

[Severity: High]
Does this code call sev_es_vmsa_pa() without holding kvm->srcu or 
kvm->slots_lock?

Looking at the execution path immediately following this comment in 
snp_launch_update_vmsa():

        /* VMSA encrypted - put it into the VMCB */
        svm->vmcb->control.vmsa_pa = sev_es_vmsa_pa(vcpu);

arch/x86/kvm/svm/sev.c:sev_es_vmsa_pa() {
    ...
    slot = gfn_to_memslot(vcpu->kvm, gfn);
    ...
}

gfn_to_memslot() dereferences the KVM memslots array. Since this is called 
without SRCU or slots_lock, a concurrent thread executing 
KVM_SET_USER_MEMORY_REGION can swap and free the memslots array while 
gfn_to_memslot() is accessing it, leading to a use-after-free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611123528.572255-1-joro@8bytes.org?part=3