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

[PATCH v2] 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 | 42 +++++++++++++++++++++++++++++-------------
 1 file changed, 29 insertions(+), 13 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 45e784462ec6..8d8b4a974ab0 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2936,6 +2936,21 @@ static bool vma_is_valid(struct vm_area_struct *vma, bool write_fault)
 	return true;
 }
 
+static inline int do_fixup_user_fault(unsigned long address, unsigned int fault_flags,
+				      struct follow_pfnmap_args *args)
+{
+	bool unlocked = false;
+	int r;
+
+	r = fixup_user_fault(current->mm, address, fault_flags, &unlocked);
+	if (unlocked)
+		return -EAGAIN;
+	if (r)
+		return r;
+
+	return follow_pfnmap_start(args);
+}
+
 static int hva_to_pfn_remapped(struct vm_area_struct *vma,
 			       struct kvm_follow_pfn *kfp, kvm_pfn_t *p_pfn)
 {
@@ -2957,27 +2972,28 @@ static int hva_to_pfn_remapped(struct vm_area_struct *vma,
 		 * 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;
-		r = fixup_user_fault(current->mm, kfp->hva,
-				     (write_fault ? FAULT_FLAG_WRITE : 0),
-				     &unlocked);
-		if (unlocked)
-			return -EAGAIN;
-		if (r)
-			return r;
-
-		r = follow_pfnmap_start(&args);
+		r = do_fixup_user_fault(kfp->hva,
+					(write_fault ? FAULT_FLAG_WRITE : 0),
+					&args);
 		if (r)
 			return r;
 	}
 
 	if (write_fault && !args.writable) {
-		*p_pfn = KVM_PFN_ERR_RO_FAULT;
-		goto out;
+		/*
+		 * 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.
+		 */
+		follow_pfnmap_end(&args);
+		r = do_fixup_user_fault(kfp->hva, FAULT_FLAG_WRITE, &args);
+		if (r) {
+			*p_pfn = KVM_PFN_ERR_RO_FAULT;
+			return 0;
+		}
 	}
 
 	*p_pfn = kvm_resolve_pfn(kfp, NULL, &args, args.writable);
-out:
 	follow_pfnmap_end(&args);
 	return r;
 }
-- 
2.55.0

Re: [PATCH v2] 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:
- [High] Error codes from the second `do_fixup_user_fault()` are swallowed, breaking the mmap_lock retry mechanism and masking errors.
--

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

This patch addresses an issue where guest writes to unpopulated PTEs backed
by DRM GEM BOs trigger an EFAULT due to the fault handler installing a
read-only PTE. It introduces a second fixup_user_fault() call to trigger a
write-upgrade for these mappings.

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index b4baa69a55402..8fd2b8644cfad 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c

[ ... ]

> @@ -2957,27 +2972,28 @@ static int hva_to_pfn_remapped(struct vm_area_struct *vma,

[ ... ]

>  	if (write_fault && !args.writable) {
> -		*p_pfn = KVM_PFN_ERR_RO_FAULT;
> -		goto out;
> +		/*
> +		 * 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.
> +		 */
> +		follow_pfnmap_end(&args);
> +		r = do_fixup_user_fault(kfp->hva, FAULT_FLAG_WRITE, &args);
> +		if (r) {
> +			*p_pfn = KVM_PFN_ERR_RO_FAULT;
> +			return 0;
> +		}

[Severity: High]
Does this code swallow important error codes from do_fixup_user_fault()?

If do_fixup_user_fault() returns an error such as -EAGAIN (due to mmap_read_lock
being dropped and re-acquired under contention) or -EHWPOISON, overriding it
by returning 0 with KVM_PFN_ERR_RO_FAULT seems to break the mmap_lock retry
mechanism and mask hardware errors.

KVM might then incorrectly fall back to instruction emulation instead of
retrying the fault, or attempt to emulate accesses on poisoned memory.

Should the actual error 'r' be propagated back to the caller instead?

>  	}
>  
>  	*p_pfn = kvm_resolve_pfn(kfp, NULL, &args, args.writable);

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