Re: [PATCH RFC v3 28/88] KVM: guest_memfd: Support reporting PFN attributes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Medium] KVM: arm64: gmem_abort() exits on L2 write when L1 S2 is
  read-only
- [Low] KVM: arm64: nv: unresolved TODO comment in kvm_translate_vncr()
- [Low] KVM: SVM: shadowed read-only PFN check in sev_handle_rmp_fault()
- [Low] KVM: arm64: nv: unaligned GPA passed with PAGE_SIZE in
  kvm_translate_vncr()
--

commit 0c51d556213ce4b6cb7adb6d1befd0cf237a2240
Author: Fred Griffoul <fgriffo@amazon.co.uk>

KVM: guest_memfd: Support reporting PFN attributes

This commit adds a pfn_attrs output parameter to kvm_gmem_get_pfn() and
__kvm_gmem_get_pfn(), defines GUEST_MEMFD_PFN_ATTRS_READONLY, and
handles read-only guest_memfd frames across x86 and arm64 fault paths.

> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 705f6d84770e9..8f0217185973c 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
[ ... ]
> @@ -1773,18 +1775,21 @@ static int gmem_abort(const struct kvm_s2_fault_desc *s2fd,
>  	/* Pairs with the smp_wmb() in kvm_mmu_invalidate_end(). */
>  	smp_rmb();
>  
> -	ret = kvm_gmem_get_pfn(kvm, s2fd->memslot, canonical_gfn, &s2vi.pfn, NULL);
> -	if (ret) {
> +	ret = kvm_gmem_get_pfn(kvm, s2fd->memslot, canonical_gfn, &s2vi.pfn, NULL,
> +			       &pfn_attrs);
> +	writable = !(s2fd->memslot->flags & KVM_MEM_READONLY) &&
> +		   !(pfn_attrs & GUEST_MEMFD_PFN_ATTRS_READONLY);
> +	if (ret || (write_fault && !writable)) {
>  		/* If result is non-NULL this is a synthetic fault. */
>  		if (!result)
>  			kvm_prepare_memory_fault_exit(s2fd->vcpu, gfn_to_gpa(canonical_gfn),
>  						      s2vi.vma_pagesize, write_fault, exec_fault,
>  						      false);
>  		kfree(mapping);
> -		return ret;
> +		return ret ?: -EFAULT;
>  	}

[Severity: Medium]
When L2 writes to an unmapped shadow Stage-2 IPA that L1 mapped
read-only in its own Stage-2 page tables, kvm_s2_handle_perm_fault()
returns 0 on the initial translation fault so that gmem_abort() installs
a read-only shadow Stage-2 PTE and the retried write traps as a
permission fault to be forwarded to L1:

arch/arm64/kvm/nested.c:kvm_s2_handle_perm_fault() {
	if (!kvm_vcpu_trap_is_permission_fault(vcpu))
		return 0;
    ...
}

Does checking (write_fault && !writable) without checking whether
s2fd->nested permits writes cause gmem_abort() to exit to L0 userspace
with KVM_EXIT_MEMORY_FAULT instead of installing the read-only shadow
Stage-2 mapping when the backing PFN has GUEST_MEMFD_PFN_ATTRS_READONLY
set?  Checking (!s2fd->nested || kvm_s2_trans_writable(s2fd->nested))
here would let the read-only shadow mapping be installed so the Stage-2
permission fault is delivered to L1.

[ ... ]

> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index 4d83281a7a28b..4519e92d89d66 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c
> @@ -1615,14 +1615,24 @@ 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);
> +		u64 pfn_attrs;
> +
> +		ret = kvm_gmem_get_pfn(vcpu->kvm, memslot, gfn, &pfn, NULL,
> +				       &pfn_attrs);
>  		if (ret) {
>  			kvm_prepare_memory_fault_exit(vcpu, vt->wr.pa, PAGE_SIZE,
>  					      write_fault, false, false);
>  			return ret;
>  		}
>  
> -		writable = !(memslot->flags & KVM_MEM_READONLY);
> +		writable = !(memslot->flags & KVM_MEM_READONLY) &&
> +			   !(pfn_attrs & GUEST_MEMFD_PFN_ATTRS_READONLY);
> +		/* TODO: Explain why vt->wr.pw needs to be checked. */

[Severity: Low]
This isn't a bug, but this placeholder TODO comment was left in place
instead of explaining that !vt->wr.pw must return 0 so
kvm_handle_vncr_abort() injects a stage-1 permission fault into the
guest via handle_vncr_perm() rather than exiting to host userspace.

> +		if (write_fault && vt->wr.pw && !writable) {
> +			kvm_prepare_memory_fault_exit(vcpu, vt->wr.pa, PAGE_SIZE,
> +						      true, false, false);
> +			return -EFAULT;
> +		}
>  	}

[Severity: Low]
VNCR_EL2 and vt->wr.pa are 4KB-aligned regardless of the host page size.
On a 16KB or 64KB page size host, does passing vt->wr.pa with PAGE_SIZE
to kvm_prepare_memory_fault_exit() report an unaligned GPA range
[vt->wr.pa, vt->wr.pa + PAGE_SIZE) that straddles into gfn + 1 instead
of gfn_to_gpa(gfn)?

[ ... ]

> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 6427db390b1cd..5e37ea4bc7937 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
[ ... ]
> @@ -5092,13 +5097,19 @@ void sev_handle_rmp_fault(struct kvm_vcpu *vcpu, gpa_t gpa, u64 error_code)
>  	mmu_seq = kvm->mmu_invalidate_seq;
>  	smp_rmb();
>  
> -	ret = kvm_gmem_get_pfn(kvm, slot, gfn, &pfn, &order);
> +	ret = kvm_gmem_get_pfn(kvm, slot, gfn, &pfn, &order, &pfn_attrs);
>  	if (ret) {
>  		pr_warn_ratelimited("SEV: Unexpected RMP fault, no backing page for private GPA 0x%llx\n",
>  				    gpa);
>  		return;
>  	}
>  
> +	if (pfn_attrs & GUEST_MEMFD_PFN_ATTRS_READONLY) {
> +		pr_warn_ratelimited("SEV: Unexpected RMP fault, read-only backing page for GPA 0x%llx\n",
> +				    gpa);
> +		return;
> +	}

[Severity: Low]
Can this GUEST_MEMFD_PFN_ATTRS_READONLY check be reached for a private
GPA?  In kvm_gmem_get_pfn(), a private page with
GUEST_MEMFD_PFN_ATTRS_READONLY set already fails with -EFAULT, so the
if (ret) branch above catches it first and logs "no backing page for
private GPA" instead of "read-only backing page".

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-gmem-alternative-providers-v3-0-8484dde66a7e@google.com?part=28