Re: [PATCH v9 08/17] KVM: guest_memfd: Check that userspace_addr and fd+offset refer to same range

[James Houghton <jthoughton@google.com>]

On Tue, May 13, 2025 at 9:34 AM Fuad Tabba <tabba@google.com> wrote:
>
> From: Ackerley Tng <ackerleytng@google.com>
>
> On binding of a guest_memfd with a memslot, check that the slot's
> userspace_addr and the requested fd and offset refer to the same memory
> range.
>
> This check is best-effort: nothing prevents userspace from later mapping
> other memory to the same provided in slot->userspace_addr and breaking
> guest operation.
>
> Suggested-by: David Hildenbrand <david@redhat.com>
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Suggested-by: Yan Zhao <yan.y.zhao@intel.com>
> Signed-off-by: Ackerley Tng <ackerleytng@google.com>
> Signed-off-by: Fuad Tabba <tabba@google.com>
> ---
>  virt/kvm/guest_memfd.c | 37 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 34 insertions(+), 3 deletions(-)
>
> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index 8e6d1866b55e..2f499021df66 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -556,6 +556,32 @@ int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args)
>         return __kvm_gmem_create(kvm, size, flags);
>  }
>
> +static bool kvm_gmem_is_same_range(struct kvm *kvm,
> +                                  struct kvm_memory_slot *slot,
> +                                  struct file *file, loff_t offset)
> +{
> +       struct mm_struct *mm = kvm->mm;
> +       loff_t userspace_addr_offset;
> +       struct vm_area_struct *vma;
> +       bool ret = false;
> +
> +       mmap_read_lock(mm);
> +
> +       vma = vma_lookup(mm, slot->userspace_addr);
> +       if (!vma)
> +               goto out;
> +
> +       if (vma->vm_file != file)
> +               goto out;
> +
> +       userspace_addr_offset = slot->userspace_addr - vma->vm_start;
> +       ret = userspace_addr_offset + (vma->vm_pgoff << PAGE_SHIFT) == offset;
> +out:
> +       mmap_read_unlock(mm);
> +
> +       return ret;
> +}
> +
>  int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
>                   unsigned int fd, loff_t offset)
>  {
> @@ -585,9 +611,14 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
>             offset + size > i_size_read(inode))
>                 goto err;
>
> -       if (kvm_gmem_supports_shared(inode) &&
> -           !kvm_arch_vm_supports_gmem_shared_mem(kvm))
> -               goto err;
> +       if (kvm_gmem_supports_shared(inode)) {
> +               if (!kvm_arch_vm_supports_gmem_shared_mem(kvm))
> +                       goto err;
> +
> +               if (slot->userspace_addr &&
> +                   !kvm_gmem_is_same_range(kvm, slot, file, offset))
> +                       goto err;

This is very nit-picky, but I would rather this not be -EINVAL, maybe
-EIO instead? Or maybe a pr_warn_once() and let the call proceed?

The userspace_addr we got isn't invalid per se, we're just trying to
give a hint to the user that their VMAs (or the userspace address they
gave us) are messed up. I don't really like lumping this in with truly
invalid arguments.

> +       }
>
>         filemap_invalidate_lock(inode->i_mapping);
>
> --
> 2.49.0.1045.g170613ef41-goog
>