Re: [PATCH v2 3/4] KVM: SEV: Drop page refcount early in VMSA reload

[email protected] writes:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [Critical] A preexisting vulnerability in `sev_gmem_make_shared` allows a malicious SEV-SNP guest to cause a host kernel panic by transitioning a `guest_memfd` page to VMSA state. When the page is subsequently freed, KVM fails to reclaim it and returns the still-firmware-owned page to the host page allocator.
> --
>
> --- Patch [3]: [PATCH v2 3/4] KVM: SEV: Drop page refcount early in VMSA reload ---
> commit 597ff1c52e82911967f2122ff1d77aa79a4a2d9b
> Author: Ackerley Tng <[email protected]>
>
> KVM: SEV: Drop page refcount early in VMSA reload
>
> This commit modifies the guest VMSA reload path for SEV-SNP vCPUs to drop
> the page reference immediately after retrieving the PFN from guest_memfd.
> This avoids holding the reference across MMU lock acquisition and prepares
> for future changes. The page's presence remains governed by KVM's MMU
> invalidation protocol.
>
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index b34b11d7f8fad..d3d620bc04dce 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -4062,6 +4062,7 @@ static void __sev_snp_reload_vmsa(struct kvm_vcpu *vcpu, gpa_t gpa)
>>  	 */
>>  	if (kvm_gmem_get_pfn(vcpu->kvm, slot, gfn, &pfn, &page, NULL))
>>  		return;
>> +	kvm_release_page_clean(page);
>>
>>  	read_lock(&kvm->mmu_lock);
>>  	/*
>> @@ -4076,8 +4077,6 @@ static void __sev_snp_reload_vmsa(struct kvm_vcpu *vcpu, gpa_t gpa)
>>  	else
>>  		svm->vmcb->control.vmsa_pa = pfn_to_hpa(pfn);
>>  	read_unlock(&kvm->mmu_lock);
>> -
>> -	kvm_release_page_clean(page);
>>  }
>
> [Severity: Critical]
> This is a pre-existing issue, but can this allow a malicious guest to
> cause a host kernel panic?
>
> Looking at sev_gmem_make_shared() in arch/x86/kvm/svm/sev.c:
>
> arch/x86/kvm/svm/sev.c:sev_gmem_make_shared() {
>     ...
> 		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;
>     ...
> }
>
> If an untrusted SEV-SNP guest modifies a guest_memfd page state to VMSA
> using RMPADJUST, and the page is subsequently freed, rmp_make_shared()
> will architecturally fail on the VMSA page.
>

I need some help with this one.

For the initial VMSA page, IIUC snp_safe_alloc_page() gets a
non-guest_memfd page. This won't ever go through sev_gmem_make_shared(),
so we're good. snp_leak_pages() doesn't actually leak pages, it just
tracks leaked pages, but __free_page() is skipped so the page is really
leaked.

> Because the error is bypassed without calling snp_leak_pages(), the
> firmware-owned page is returned to the host page allocator.
>

For guest-provided VMSA pages, those can be guest_memfd pages. The
refcount taken is the one guest_memfd holds. Without considering
conversions, just considering truncation, sev_gmem_make_shared() is
called from kvm_gmem_free_folio(). Sashiko is saying that
rmp_make_shared() will fail, and the folio is freed.

Sounds like this would be a problem if the folio is used elsewhere...?

> Could this lead to an RMP violation and a host kernel panic when the host
> later attempts to access the freed page?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3