Re: [RFC PATCH v3 2/5] KVM: guest_memfd: Add ioctl KVM_LINK_GUEST_MEMFD

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] KVM: guest_memfd: multiple same-VM links to one inode in kvm_gmem_link()
- [High] KVM: x86: kvm_arch_gmem_link_allowed() allows unsafe TDX and SEV guest_memfd linking
--

commit f69df48ff894e872fc6ccbcb9ce7d8cfde5a15f3
Author: Ackerley Tng <ackerleytng@google.com>

KVM: guest_memfd: Add ioctl KVM_LINK_GUEST_MEMFD

This commit introduces the KVM_LINK_GUEST_MEMFD ioctl to link an existing
guest_memfd's underlying inode to a new file descriptor bound to a target VM,
allowing compatible VMs to share the same underlying guest_memfd memory.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c081e4b9b25de..4427d32290c0f 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10648,6 +10648,16 @@ bool kvm_arch_supports_gmem_init_shared(struct kvm *kvm)
>  	return gmem_in_place_conversion || !kvm_arch_has_private_mem(kvm);
>  }
>  
> +bool kvm_arch_gmem_link_allowed(struct kvm *dst, struct kvm *src)
> +{
> +	/*
> +	 * Different VM types have different requirements for, and different
> +	 * (vendor) state associated with, guest_memfd memory, e.g. memory that
> +	 * is private to a TDX guest must never be mapped into a non-TDX VM.
> +	 */
> +	return dst->arch.vm_type == src->arch.vm_type;
> +}

[Severity: High]
Is checking dst->arch.vm_type == src->arch.vm_type in
kvm_arch_gmem_link_allowed() sufficient for confidential VM types on x86?

On Intel TDX (KVM_X86_TDX_VM), each physical page in a guest_memfd is
exclusively owned in the TDX Module PAMT by a single TD's TDR and encrypted
with that TD's private HKID. If userspace links a guest_memfd between two TDX
VMs and both VMs populate or fault in the same private page via
tdx_mem_page_add() or tdx_mem_page_aug(), the second TD's SEAMCALL will fail
with TDX_PAGE_NOT_FREE and trigger a host WARN_ON_ONCE via TDX_BUG_ON_2():

arch/x86/kvm/vmx/tdx.c:tdx_mem_page_aug() {
	err = tdh_mem_page_aug(&kvm_tdx->td, gpa, level, pfn, &entry, &level_state);
	if (unlikely(tdx_operand_busy(err)))
		return -EBUSY;

	if (TDX_BUG_ON_2(err, TDH_MEM_PAGE_AUG, entry, level_state, kvm))
		return -EIO;
}

On AMD SEV-SNP (KVM_X86_SNP_VM), each physical page's hardware RMP entry
records a single (ASID, GPA) owner. If two SNP VMs link the same guest_memfd:

1. When dst faults in a private page already assigned in the RMP to src,
   sev_gmem_make_private() checks snp_lookup_rmpentry() without verifying the
   owning ASID or GPA, and returns 0, installing an NPT mapping in dst for a
   PFN owned by src's ASID:

arch/x86/kvm/svm/sev.c:sev_gmem_make_private() {
	rc = snp_lookup_rmpentry(pfn, &assigned, &level);
	...
	if (assigned) {
		pr_debug("%s: already assigned: gfn %llx pfn %llx nr_pages %llx level %d\n",
			 __func__, gfn, pfn, nr_pages, level);
		return 0;
	}
}

2. If src closes its guest_memfd and is destroyed while dst keeps the linked
   guest_memfd open, the shared inode's folios are not freed, so
   kvm_gmem_free_folio() -> sev_gmem_make_shared() does not run.
   sev_vm_destroy() then decommissions src's SNP context and frees src's ASID
   via sev_asid_free() while physical pages in the guest_memfd remain
   Assigned=1 to that freed/recycled ASID in the RMP.

3. If dst converts pages to shared or punches a hole while src has pages
   marked Immutable=1 in the RMP via KVM_SEV_SNP_LAUNCH_UPDATE,
   sev_gmem_make_shared() -> rmp_make_shared() fails and triggers WARN_ONCE().

In addition, for legacy SEV and SEV-ES VMs initialized via KVM_SEV_INIT or
KVM_SEV_ES_INIT, sev_guest_init() leaves kvm->arch.vm_type set to
KVM_X86_DEFAULT_VM, so kvm_arch_gmem_link_allowed() treats legacy SEV/SEV-ES
VMs and non-SEV default VMs as having the same vm_type.

Should kvm_arch_gmem_link_allowed() disallow linking when either VM is a
confidential VM (or only allow KVM_X86_DEFAULT_VM without SEV enabled)?

> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index 3ca6f64d48040..54b11747a8c48 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
[ ... ]
> @@ -970,7 +979,10 @@ static struct file *kvm_gmem_alloc_file(struct kvm *kvm, struct inode *inode,
>  	kvm_get_kvm(kvm);
>  	f->kvm = kvm;
>  	xa_init(&f->bindings);
> +
> +	filemap_invalidate_lock(inode->i_mapping);
>  	list_add(&f->entry, &GMEM_I(inode)->gmem_file_list);
> +	filemap_invalidate_unlock(inode->i_mapping);
>  
>  	return file;
>  }
> @@ -1026,6 +1038,77 @@ int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args)
>  	return __kvm_gmem_create(kvm, size, flags);
>  }
>  
> +int kvm_gmem_link(struct kvm *kvm, struct kvm_link_guest_memfd *args)
> +{
[ ... ]
> +	ret = -EINVAL;
> +	if (src_file->f_op != &kvm_gmem_fops)
> +		goto out;
> +
> +	/* Cannot link a gmem file with the same VM again. */
> +	gmem = src_file->private_data;
> +	if (gmem->kvm == kvm)
> +		goto out;
> +
> +	if (!kvm_arch_gmem_link_allowed(kvm, gmem->kvm))
> +		goto out;
> +
> +	fd = get_unused_fd_flags(0);
> +	if (fd < 0) {
> +		ret = fd;
> +		goto out;
> +	}
> +
> +	inode = file_inode(src_file);
> +	ihold(inode);
> +
> +	dst_file = kvm_gmem_alloc_file(kvm, inode, name);
> +	if (IS_ERR(dst_file)) {
> +		iput(inode);
> +		ret = PTR_ERR(dst_file);
> +		goto out_fd;
> +	}

[Severity: High]
Can kvm_gmem_link() still link the same guest_memfd inode to the same VM
multiple times?

In kvm_gmem_link(), the check only verifies whether src_file's immediate
gmem_file->kvm matches the target kvm, and does so without holding
filemap_invalidate_lock(inode->i_mapping).

If userspace creates fd1 on VM1 and links fd1 to VM2 (producing fd2), it can
then:
1. Call KVM_LINK_GUEST_MEMFD(fd1) on VM2 a second time,
2. Call KVM_LINK_GUEST_MEMFD(fd2) back onto VM1, or
3. Call KVM_LINK_GUEST_MEMFD(fd1) concurrently from two threads on VM2.

In all three cases, gmem->kvm == kvm is false, so kvm_gmem_alloc_file()
inserts a second struct gmem_file for the same struct kvm onto
GMEM_I(inode)->gmem_file_list.

Having multiple struct gmem_file instances for the same VM on the same inode
breaks two assumptions elsewhere in guest_memfd.c:

First, kvm_gmem_commit_memory_region() only checks f->bindings on the single
gmem_file being bound:

virt/kvm/guest_memfd.c:kvm_gmem_commit_memory_region() {
	if (!xa_empty(&f->bindings) &&
	    xa_find(&f->bindings, &start, end - 1, XA_PRESENT)) {
		filemap_invalidate_unlock(inode->i_mapping);
		return -EEXIST;
	}
}

With two gmem_file fds for the same VM and inode, a single VM can bind two
memslots at different GPAs to the exact same pgoff range of the same
guest_memfd inode.

Second, kvm_gmem_invalidate_start() iterates over gmem_file_list and calls
__kvm_gmem_invalidate_start() per gmem_file, which acquires and releases
KVM_MMU_LOCK(kvm) per gmem_file while mmu_invalidate_in_progress is active.

Should GMEM_I(inode)->gmem_file_list be walked while holding
filemap_invalidate_lock(inode->i_mapping) to reject any link where an entry
with f->kvm == kvm already exists on the inode?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026064424.3003536-1-gongtao@google.com?part=2