Re: [RFC PATCH 02/14] KVM: guest_memfd: Support read-only frames from PFN mode providers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] KVM: guest_memfd: triggerable WARN_ON_ONCE(!writable) in kvm_gmem_get_pfn()
- [High] KVM: arm64: kvm_translate_vncr() rejects read-only frames on guest reads
- [High] KVM: guest_memfd: premature unlock in kvm_gmem_pfn_mkwrite() allows RO bypass
- [High] KVM: guest_memfd: Add block range before kvm_mmu_unmap_gfn_range()
--

commit 421ea1b76fdc664724f686d68f71491b37e2d2b4
Author: Fred Griffoul <fgriffo@amazon.co.uk>

KVM: guest_memfd: Support read-only frames from PFN mode providers

This commit adds an attributes output parameter to the PFN-mode provider
get_pfn() callback and defines GUEST_MEMFD_PFN_READONLY so providers can
lend read-only frames to guest_memfd. KVM maps such frames read-only in
guest and host page tables, exits to userspace on guest writes, raises
SIGBUS on host writes, and disallows using them to back private or
populated pages.

> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index f1c414f1c3faf..ec3bcdc230cf9 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c
> @@ -1615,12 +1615,19 @@ static int kvm_translate_vncr(struct kvm_vcpu *vcpu, bool *is_gmem)
>  			return -EFAULT;
>  		}
>  	} else {
> -		ret = kvm_gmem_get_pfn(vcpu->kvm, memslot, gfn, &pfn, NULL);
> +		ret = kvm_gmem_get_pfn(vcpu->kvm, memslot, gfn, &pfn, NULL,
> +				       &writable);
>  		if (ret) {
>  			kvm_prepare_memory_fault_exit(vcpu, vt->wr.pa, PAGE_SIZE,
>  					      write_fault, false, false);
>  			return ret;
>  		}
> +		/* The CPU writes the VNCR page: a read-only frame cannot back it. */
> +		if (!writable) {
> +			kvm_prepare_memory_fault_exit(vcpu, vt->wr.pa, PAGE_SIZE,
> +					      true, false, false);
> +			return -EFAULT;
> +		}
>  
>  		writable = !(memslot->flags & KVM_MEM_READONLY);
>  	}

[Severity: High]
Does kvm_translate_vncr() need to reject read-only frames even when the
guest is only reading from the VNCR page?

Under FEAT_NV2, the CPU reads from the VNCR_EL2 page when the guest
executes an MRS of a redirected EL1 system register (write_fault is false)
and writes to it when the guest executes an MSR (write_fault is true).
The rest of the VNCR handling in kvm_handle_vncr_abort() and
kvm_map_l1_vncr() already supports read-only backing frames (such as a
KVM_MEM_READONLY memslot) by recording vt->hpa_writable = writable and
mapping the fixmap as PAGE_KERNEL_RO when vt->hpa_writable is false:

arch/arm64/kvm/nested.c:kvm_handle_vncr_abort() {
	...
	case 0:
		perm = kvm_is_write_fault(vcpu) ? vt->wr.pw && vt->hpa_writable : vt->wr.pr;
		if (!perm)
			handle_vncr_perm(vcpu);
		break;
}

arch/arm64/kvm/nested.c:kvm_map_l1_vncr() {
	...
	if (vt->hpa_writable && vt->wr.pw && vt->wr.pr)
		prot = PAGE_KERNEL;
	else if (vt->wr.pr)
		prot = PAGE_KERNEL_RO;
	else
		prot = PAGE_NONE;
}

By checking !writable unconditionally in kvm_translate_vncr() and
exiting to userspace with write_fault hardcoded to true, a guest MRS read
of a VNCR-redirected register backed by a GUEST_MEMFD_PFN_READONLY frame
fails with KVM_EXIT_MEMORY_FAULT instead of mapping the VNCR page
read-only. In addition, if the guest's own EL2 Stage-1 translation lacks
permission (!vt->wr.pr or !vt->wr.pw), exiting here bypasses
handle_vncr_perm() and exits to host userspace instead of injecting a
Stage-1 permission fault into the guest.

Should kvm_translate_vncr() instead fold writable into
writable = writable && !(memslot->flags & KVM_MEM_READONLY), or only exit
when write_fault && !writable after verifying guest Stage-1 permissions?

[ ... ]

> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index be26bd16d4207..1554a17906da8 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c

[ ... ]

> @@ -721,13 +728,71 @@ static bool kvm_gmem_supports_mmap(struct inode *inode)

[ ... ]

> +/*
> + * A write to a read-only PTE, e.g. after mprotect(): allow it unless the frame
> + * is read only.  The core upgrades the PTE under its lock only if it did not
> + * change, and a provider that makes the frame read only revokes it first.
> + */
> +static vm_fault_t kvm_gmem_pfn_mkwrite(struct vm_fault *vmf)
> +{
> +	struct inode *inode = file_inode(vmf->vma->vm_file);
> +	vm_fault_t ret = 0;
> +	kvm_pfn_t pfn;
> +	u32 attrs;
> +	int r;
> +
> +	filemap_invalidate_lock_shared(inode->i_mapping);
> +	r = gmem_provider_get_pfn(GMEM_I(inode), vmf->pgoff, &pfn, NULL, &attrs);
> +	if (r)
> +		ret = vmf_error(r);
> +	else if (attrs & GUEST_MEMFD_PFN_READONLY)
> +		ret = VM_FAULT_SIGBUS;
> +	filemap_invalidate_unlock_shared(inode->i_mapping);
> +	return ret;
> +}

[Severity: High]
Can releasing filemap_invalidate_lock_shared() before returning from
kvm_gmem_pfn_mkwrite() allow a read-only frame to be upgraded to a
writable PTE after kvm_gmem_revoke()?

In mm/memory.c, wp_pfn_shared() drops the page table lock before calling
vma->vm_ops->pfn_mkwrite(vmf), and then calls finish_mkwrite_fault(),
which re-locks the PTE and upgrades it to writable if
pte_same(ptep_get(vmf->pte), vmf->orig_pte) holds:

mm/memory.c:wp_pfn_shared() {
	if (vma->vm_ops && vma->vm_ops->pfn_mkwrite) {
		...
		pte_unmap_unlock(vmf->pte, vmf->ptl);
		...
		ret = vma->vm_ops->pfn_mkwrite(vmf);
		if (ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE))
			return ret;
		return finish_mkwrite_fault(vmf, NULL);
	}
}

If a VMA with VM_WRITE | VM_SHARED has a present read-only special PTE
mapping PFN P (for example after mprotect(PROT_READ) followed by
mprotect(PROT_READ | PROT_WRITE), or faulted in while P was read-only and
later made writable), consider what happens when thread A writes to that
address:

1. Thread A enters wp_pfn_shared() -> kvm_gmem_pfn_mkwrite(), acquires
   filemap_invalidate_lock_shared(), sees attrs == 0 for PFN P, and drops
   filemap_invalidate_lock_shared().
2. Before thread A reaches finish_mkwrite_fault(), the provider marks PFN P
   as GUEST_MEMFD_PFN_READONLY and calls kvm_gmem_revoke(), which takes
   filemap_invalidate_lock() exclusively, zaps the PTE via
   unmap_mapping_range(), and drops filemap_invalidate_lock().
3. Thread B triggers a read fault on the same address.
   kvm_gmem_fault_user_mapping() -> kvm_gmem_pfn_fault() sees
   GUEST_MEMFD_PFN_READONLY and calls vmf_insert_pfn_prot() with
   kvm_gmem_pfn_prot(vma, GUEST_MEMFD_PFN_READONLY), installing a
   read-only special PTE for PFN P that is bit-for-bit identical to
   thread A's vmf->orig_pte.
4. Thread A enters finish_mkwrite_fault(), sees
   pte_same(ptep_get(vmf->pte), vmf->orig_pte) is true, and calls
   wp_page_reuse(), making the PTE writable even though PFN P is now
   GUEST_MEMFD_PFN_READONLY and revocation already completed.

Also, kvm_gmem_pfn_mkwrite() does not check kvm_gmem_is_shared_mem() the
way kvm_gmem_fault_user_mapping() does.

[ ... ]

> @@ -1588,14 +1660,22 @@ static int kvm_gmem_pfn_map_order(struct inode *inode, pgoff_t index,
>  	return order;
>  }
>  
> +/*
> + * @writable, if not NULL, is set to whether the guest may write the page.  It
> + * may be NULL only for private pages, which are always writable.
> + */
>  int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
> -		     gfn_t gfn, kvm_pfn_t *pfn, int *max_order)
> +		     gfn_t gfn, kvm_pfn_t *pfn, int *max_order,
> +		     bool *writable)
>  {
>  	pgoff_t index = kvm_gmem_get_index(slot, gfn);
>  	struct folio *folio;
>  	int r = 0, __order;
> +	u32 attrs = 0;
>  
>  	max_order = max_order ?: &__order;
> +	if (writable)
> +		*writable = true;
>  
>  	CLASS(gmem_get_file, file)(slot);
>  	if (!file)
> @@ -1603,15 +1683,28 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
>  
>  	filemap_invalidate_lock_shared(file_inode(file)->i_mapping);
>  
> -	folio = __kvm_gmem_get_pfn(file, slot, index, pfn, max_order);
> +	folio = __kvm_gmem_get_pfn(file, slot, index, pfn, max_order, &attrs);
>  	if (IS_ERR(folio)) {
>  		r = PTR_ERR(folio);
>  		goto out;
>  	}
>  
> -	if (kvm_gmem_is_pfn_mode(GMEM_I(file_inode(file))))
> +	if (kvm_gmem_is_pfn_mode(GMEM_I(file_inode(file)))) {
> +		/*
> +		 * A read-only frame is not the guest's alone, so it never
> +		 * backs a private page: a private fault on it is a hole.
> +		 */
> +		if (attrs & GUEST_MEMFD_PFN_READONLY) {
> +			if (kvm_gmem_is_private_mem(file_inode(file), index) ||
> +			    WARN_ON_ONCE(!writable)) {
> +				r = -EFAULT;
> +				goto out;
> +			}
> +			*writable = false;
> +		}

[Severity: High]
Can a guest or host userspace trigger this WARN_ON_ONCE(!writable)?

In arch/x86/kvm/svm/sev.c, both __sev_snp_reload_vmsa() and
sev_handle_rmp_fault() pass writable = NULL to kvm_gmem_get_pfn():

arch/x86/kvm/svm/sev.c:__sev_snp_reload_vmsa() {
	...
	if (kvm_gmem_get_pfn(vcpu->kvm, slot, gfn, &pfn, NULL, NULL))
		return;
}

arch/x86/kvm/svm/sev.c:sev_handle_rmp_fault() {
	...
	if (!kvm_is_private_gfn(kvm, gfn)) {
		...
		return;
	}
	...
	ret = kvm_gmem_get_pfn(kvm, slot, gfn, &pfn, &order, NULL);
}

In __sev_snp_reload_vmsa(), there is no check that the GFN is marked
private before calling kvm_gmem_get_pfn(). An SEV-SNP guest can supply an
arbitrary GPA via SVM_VMGEXIT_AP_CREATE or SVM_VMGEXIT_AP_CREATE_ON_INIT
in sev_snp_ap_creation(), or host userspace can convert an existing VMSA
GPA from private to shared via KVM_SET_MEMORY_ATTRIBUTES2 (which triggers
sev_gmem_invalidate_range() and requests KVM_REQ_VMSA_PAGE_RELOAD). Similarly,
in sev_handle_rmp_fault(), kvm_is_private_gfn() is checked before
filemap_invalidate_lock_shared() is acquired in kvm_gmem_get_pfn(), so a
concurrent private-to-shared attribute conversion can complete in between.

If the GFN is shared (so kvm_gmem_is_private_mem() returns false) and is
backed by a GUEST_MEMFD_PFN_READONLY frame, kvm_gmem_get_pfn() will
evaluate WARN_ON_ONCE(!writable) and warn in the host kernel.

>  		*max_order = kvm_gmem_pfn_map_order(file_inode(file), index, gfn,
>  						    *pfn, *max_order);
> +	}

[Severity: High]
When a provider changes GUEST_MEMFD_PFN_READONLY on a sub-range of a huge
PFN-mode block and calls kvm_gmem_revoke(), is the invalidation order in
__kvm_gmem_invalidate_start() sufficient to prevent a concurrent fault on
another GFN in the same block from installing a stale writable 2M/1G page?

In __kvm_gmem_invalidate_start(), kvm_mmu_unmap_gfn_range() is called
before kvm_mmu_invalidate_range_add() widens the invalidation range to the
PUD_ORDER-aligned block:

virt/kvm/guest_memfd.c:__kvm_gmem_invalidate_start() {
	...
		flush |= kvm_mmu_unmap_gfn_range(kvm, &gfn_range);

		kvm_mmu_invalidate_range_add(kvm,
					     round_down(gfn_range.start, blk),
					     round_up(gfn_range.end, blk));
}

Inside kvm_mmu_unmap_gfn_range(), only the unaligned [gfn_range.start,
gfn_range.end) sub-range is passed to kvm_mmu_invalidate_range_add() before
kvm_unmap_gfn_range() runs. Because gfn_range.may_block is true,
kvm_unmap_gfn_range() can yield kvm->mmu_lock while walking rmaps or roots
before __kvm_gmem_invalidate_start() adds the rounded-out block range.

If a vCPU faults on GFN B in the same 2M/1G block, finishes
kvm_gmem_get_pfn() (observing writable = true and *max_order >= PMD_ORDER)
just before kvm_gmem_revoke() takes filemap_invalidate_lock(), and then
acquires kvm->mmu_lock while kvm_unmap_gfn_range() has yielded it on GFN A,
mmu_invalidate_retry_gfn() will not see GFN B in
[mmu_invalidate_range_start, mmu_invalidate_range_end) and
mmu_invalidate_seq has not yet been incremented. Couldn't that vCPU install
a writable 2M/1G mapping covering GFN A after kvm_unmap_gfn_range()
already zapped it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027154649.91571-1-griffoul@gmail.com?part=2