[PATCH] KVM: have hva_to_pfn_remapped write-upgrade PTEs

[PATCH] KVM: have hva_to_pfn_remapped write-upgrade PTEs

[Sergio Lopez]

After 28e39181 ("drm/gem-shmem: Track folio accessed/dirty status in
mmap") was merged, a guest write to an unpopulated PTE from a mapping
backed by a DRM GEM BO triggers a VM exit with EFAULT, with
hva_to_pfn_remapped setting p_pfn to KVM_PFN_ERR_RO_FAULT.

This happens because that commit implements pfn_mkwrite for
drm_gem_shmem_vm_ops. With that function present, vma_wants_writenotify
returns true in vma_set_page_prot, clearing VM_SHARED and leading to the
entry to be installed as read-only. This is done on purpose so the
fault handler gets nofitied when the entry is going to be written.

In KVM, hva_to_pfn_remapped calls to fixup_user_fault to trigger the
fault handler but, as seen above, this one might install a read-only PTE
even with FAULT_FLAG_WRITE present in fault_flags. The check a the end
of hvf_to_pfn_remapped notices that the entry is not writable despite
this being a write fault and sets p_pfn to KVM_PFN_ERR_RO_FAULT.

To address this issue, have hva_to_pfn_remapped issue a second
fixup_user_fault call when needed for write-upgrading the PTE.

Signed-off-by: Sergio Lopez <slp@redhat.com>
---
 virt/kvm/kvm_main.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 45e784462ec6..a398f4677854 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2958,6 +2958,8 @@ static int hva_to_pfn_remapped(struct vm_area_struct *vma,
 		 * not call the fault handler, so do it here.
 		 */
 		bool unlocked = false;
+		bool write_upgrade = false;
+do_write_upgrade:
 		r = fixup_user_fault(current->mm, kfp->hva,
 				     (write_fault ? FAULT_FLAG_WRITE : 0),
 				     &unlocked);
@@ -2969,6 +2971,17 @@ static int hva_to_pfn_remapped(struct vm_area_struct *vma,
 		r = follow_pfnmap_start(&args);
 		if (r)
 			return r;
+
+		if (write_fault && !args.writable && !write_upgrade) {
+			/*
+			 * VM_PFNMAP fault handlers may install read-only PTEs
+			 * via vmf_insert_pfn(), deferring the write upgrade to
+			 * a second fault. Trigger that upgrade now.
+			 */
+			write_upgrade = true;
+			follow_pfnmap_end(&args);
+			goto do_write_upgrade;
+		}
 	}
 
 	if (write_fault && !args.writable) {
-- 
2.55.0

Re: [PATCH] KVM: have hva_to_pfn_remapped write-upgrade PTEs

[sashiko-bot]

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

Pre-existing issues:
- [High] Incomplete fix for VM_PFNMAP write-upgrades, bypassing already-populated read-only PTEs
--

--- Patch [1]: [PATCH] KVM: have hva_to_pfn_remapped write-upgrade PTEs ---
commit 8ea7b308a68b0aabea9b703efac8ec9af51170aa
Author: Sergio Lopez <slp@redhat.com>
Subject: KVM: have hva_to_pfn_remapped write-upgrade PTEs

This patch introduces a second fixup_user_fault call to handle
write-upgrades for VM_PFNMAP mappings that install read-only PTEs on
write faults (such as DRM GEM BOs).

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index b4baa69a55402..b7fb9278f198b 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2958,6 +2958,8 @@ static int hva_to_pfn_remapped(struct vm_area_struct *vma,
[ ... ]
>  	r = follow_pfnmap_start(&args);
>  	if (r) {
>  		/*
>  		 * get_user_pages fails for VM_IO and VM_PFNMAP vmas and does
>  		 * not call the fault handler, so do it here.
>  		 */
>  		bool unlocked = false;
> +		bool write_upgrade = false;
> +do_write_upgrade:
>  		r = fixup_user_fault(current->mm, kfp->hva,
>  				     (write_fault ? FAULT_FLAG_WRITE : 0),
>  				     &unlocked);
> @@ -2969,6 +2971,17 @@ static int hva_to_pfn_remapped(struct vm_area_struct *vma,
>  		r = follow_pfnmap_start(&args);
>  		if (r)
>  			return r;
> +
> +		if (write_fault && !args.writable && !write_upgrade) {
> +			/*
> +			 * VM_PFNMAP fault handlers may install read-only PTEs
> +			 * via vmf_insert_pfn(), deferring the write upgrade to
> +			 * a second fault. Trigger that upgrade now.
> +			 */
> +			write_upgrade = true;
> +			follow_pfnmap_end(&args);
> +			goto do_write_upgrade;
> +		}
>  	}
>  
>  	if (write_fault && !args.writable) {
        ^^^^

[Severity: High]
This is a pre-existing issue, but does this placement miss the case where
the PTE is already populated but read-only?

If a guest first reads from the DRM GEM BO, triggering a read fault, the
DRM GEM fault handler installs a read-only PTE. If the guest later attempts
to write to the same page, hva_to_pfn_remapped() calls follow_pfnmap_start(),
which finds the read-only PTE and returns 0 (success).

Since r == 0, the entire if (r) block containing the new write upgrade logic
is bypassed. The code then reaches this check and incorrectly returns
KVM_PFN_ERR_RO_FAULT instead of performing the write upgrade.

Should the upgrade logic be placed outside of the if (r) block so it can also
handle already-populated read-only PTEs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729014223.17937-1-slp@redhat.com?part=1

Re: [PATCH] KVM: have hva_to_pfn_remapped write-upgrade PTEs

[Sergio Lopez Pascual]

sashiko-bot@kernel.org writes:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Incomplete fix for VM_PFNMAP write-upgrades, bypassing already-populated read-only PTEs
> --
>
> --- Patch [1]: [PATCH] KVM: have hva_to_pfn_remapped write-upgrade PTEs ---
> commit 8ea7b308a68b0aabea9b703efac8ec9af51170aa
> Author: Sergio Lopez <slp@redhat.com>
> Subject: KVM: have hva_to_pfn_remapped write-upgrade PTEs
>
> This patch introduces a second fixup_user_fault call to handle
> write-upgrades for VM_PFNMAP mappings that install read-only PTEs on
> write faults (such as DRM GEM BOs).
>
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index b4baa69a55402..b7fb9278f198b 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -2958,6 +2958,8 @@ static int hva_to_pfn_remapped(struct vm_area_struct *vma,
> [ ... ]
>>  	r = follow_pfnmap_start(&args);
>>  	if (r) {
>>  		/*
>>  		 * get_user_pages fails for VM_IO and VM_PFNMAP vmas and does
>>  		 * not call the fault handler, so do it here.
>>  		 */
>>  		bool unlocked = false;
>> +		bool write_upgrade = false;
>> +do_write_upgrade:
>>  		r = fixup_user_fault(current->mm, kfp->hva,
>>  				     (write_fault ? FAULT_FLAG_WRITE : 0),
>>  				     &unlocked);
>> @@ -2969,6 +2971,17 @@ static int hva_to_pfn_remapped(struct vm_area_struct *vma,
>>  		r = follow_pfnmap_start(&args);
>>  		if (r)
>>  			return r;
>> +
>> +		if (write_fault && !args.writable && !write_upgrade) {
>> +			/*
>> +			 * VM_PFNMAP fault handlers may install read-only PTEs
>> +			 * via vmf_insert_pfn(), deferring the write upgrade to
>> +			 * a second fault. Trigger that upgrade now.
>> +			 */
>> +			write_upgrade = true;
>> +			follow_pfnmap_end(&args);
>> +			goto do_write_upgrade;
>> +		}
>>  	}
>>
>>  	if (write_fault && !args.writable) {
>         ^^^^
>
> [Severity: High]
> This is a pre-existing issue, but does this placement miss the case where
> the PTE is already populated but read-only?
>
> If a guest first reads from the DRM GEM BO, triggering a read fault, the
> DRM GEM fault handler installs a read-only PTE. If the guest later attempts
> to write to the same page, hva_to_pfn_remapped() calls follow_pfnmap_start(),
> which finds the read-only PTE and returns 0 (success).
>
> Since r == 0, the entire if (r) block containing the new write upgrade logic
> is bypassed. The code then reaches this check and incorrectly returns
> KVM_PFN_ERR_RO_FAULT instead of performing the write upgrade.
>
> Should the upgrade logic be placed outside of the if (r) block so it can also
> handle already-populated read-only PTEs?

Yes, I've confirmed this scenario is possible. I'll send a v2.

Thanks,
Sergio.