Re: [PATCH v2 4/4] KVM: guest_memfd: Stop returning struct page from PFN lookup

[email protected] writes:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [Critical] Concurrent execution of KVM_SEV_SNP_LAUNCH_UPDATE and a vCPU page fault triggers a race condition leading to a host kernel panic.
> --
>
> --- Patch [4]: [PATCH v2 4/4] KVM: guest_memfd: Stop returning struct page from PFN lookup ---
> commit 766989bc1910148a83bffa481ae22b1d1eb71f01
> Author: Sean Christopherson <[email protected]>
>
> KVM: guest_memfd: Stop returning struct page from PFN lookup
>
> This commit stops returning a refcounted struct page from guest_memfd PFN
> lookups, dropping the folio reference immediately instead. This prevents
> concurrent guest page faults from holding elevated refcounts that would
> interfere with in-place memory conversions between shared and private states.
>
> Link: https://lore.kernel.org/all/[email protected]/
>
>> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
>> index b596486d184ca..589762140c3ef 100644
>> --- a/virt/kvm/guest_memfd.c
>> +++ b/virt/kvm/guest_memfd.c
>
> [ ... ]
>
>> @@ -780,11 +779,7 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
>>  #endif
>>
>>  	folio_unlock(folio);
>> -
>> -	if (!r)
>> -		*page = folio_file_page(folio, index);
>> -	else
>> -		folio_put(folio);
>> +	folio_put(folio);
>>
>>  	return r;
>>  }
>
> [Severity: Critical]
> This is a pre-existing issue, but is it possible for a concurrent page
> fault to trigger a host kernel panic via a hardware RMP fault here?
>

I thought guest vCPU faults and populate are mutually exclusive, but I
couldn't actually find anything that blocks populate from running after
a guest is finalized.

So suppose the vCPU does fault in a page, the zeroing and make_private
happens under folio lock. After the page is obtained on the populate
side, there's no more zeroing, and sev_gmem_post_populate() would fail
since the page is already assigned.

Suppose populate goes first, the folio_lock keeps the vCPU fault
out. The folio is zeroed and made private. This time,
sev_gmem_post_populate() would fail because the guest has been
finalized. Still no zeroing after making private.

If populate finishes successfully, then the folio is marked
uptodate. This is done outside of the folio_lock(), but at this time the
vCPU can't run so by the time any vCPU fault can happen, the folio would
be uptodate, and there would be no more zeroing.

Seems like all is good.

After conversions, filemap_invalidate_lock() would make getting a pfn
and populate exclusive, so that's even better.

Let me know if I missed anything.

> Looking at kvm_gmem_get_pfn() in virt/kvm/guest_memfd.c, if the folio is
> not up to date, it zeroes the page and marks it up to date:
>
>     if (!folio_test_uptodate(folio)) {
>         clear_highpage(folio_page(folio, 0));
>         folio_mark_uptodate(folio);
>     }
>
> However, in the KVM_SEV_SNP_LAUNCH_UPDATE path, __kvm_gmem_populate()
> fetches a folio, unlocks it, and calls post_populate() (which maps to
> sev_gmem_post_populate() and calls rmp_make_private()) before finally
> marking the folio up to date.
>
> If a malicious userspace process races a vCPU page fault against
> KVM_SEV_SNP_LAUNCH_UPDATE, could the vCPU thread running kvm_gmem_get_pfn()
> acquire the lock on the not-yet-uptodate folio and call clear_highpage()
> after rmp_make_private() has already executed?
>
> If clear_highpage() writes to the page after it has been made RMP-private,
> this would trigger a fatal hardware RMP fault on the host CPU. If it writes
> just before, it appears it could silently corrupt the encrypted firmware
> payload.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4