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

Ackerley Tng <[email protected]> writes:

> [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...?
>

I looked closer, "15.36.12 Running SNP-Active Virtual Machines" in APM
Vol2 says "After a successful VMRUN, the VMCB page, as well as any AVIC
Backing Page and VMSA Page are marked as in-use by hardware, and any
attempt to modify the RMP entries for these pages via instructions like
RMPUPDATE will result in a FAIL_INUSE response. The in-use marking is
automatically cleared by hardware after a #VMEXIT event".

So it's not true that rmp_make_shared() will always fail on a VMSA page,
it's only true if the guest vCPU is running.

RMPADJUST sets up the VMSA page, and telling the host using
SVM_VMGEXIT_AP_CREATE is how the VMSA is set up to be usable. Then, the
other vCPU needs to be started and running, and then:

If the host truncates the VMSA page or host closes the guest_memfd file
containing some other vCPU's VMSA page, sev_gmem_make_shared() on the
VMSA page should fail, but the page is released anyway and we have a
problem.

An unsolicited shared->private conversion is fine since the VMSA page is
already private.

sev_gmem_make_shared() would fail on an unsolicited private->shared
conversion. The RMP entry would still indicate private, and guest_memfd
would track it as shared...? Bad.

I've been trying to write a mini reproducer like a selftest but I've
still not gotten it to work yet. If someone knows there's no issue here
please let me know!

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