linux-kernel - Re: [PATCH] KVM: guest_memfd: Remove bindings on memslot deletion when gmem is dying

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGtprH9H7cHAzdTpPrP-H8Z7yWgRFmTtXNjORDJsuq6AKPbnHg@mail.gmail.com>
Date: Tue, 4 Nov 2025 08:57:47 -0800
From: Vishal Annapurve <vannapurve@...gle.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	syzbot+2479e53d0db9b32ae2aa@...kaller.appspotmail.com, 
	Hillf Danton <hdanton@...a.com>
Subject: Re: [PATCH] KVM: guest_memfd: Remove bindings on memslot deletion
 when gmem is dying

On Mon, Nov 3, 2025 at 5:12 PM Sean Christopherson <seanjc@...gle.com> wrote:
>
> Deliberately don't acquire filemap invalid lock when the file is dying as
> the lifecycle of f_mapping is outside the purview of KVM.  Dereferencing
> the mapping is *probably* fine, but there's no need to invalidate anything
> as memslot deletion is responsible for zapping SPTEs, and the only code
> that can access the dying file is kvm_gmem_release(), whose core code is
> mutually exclusive with unbinding.
>
> Note, the mutual exclusivity is also what makes it self to access the

           ^ safe

> bindings on a dying gmem instance.  Unbinding either runs with slots_lock
> held, or after the last reference to the owning "struct kvm" is put, and
> kvm_gmem_release() nullifies the slot pointer under slots_lock, and puts
> its reference to the VM after that is done.
>
> Reported-by: syzbot+2479e53d0db9b32ae2aa@...kaller.appspotmail.com
> Closes: https://lore.kernel.org/all/68fa7a22.a70a0220.3bf6c6.008b.GAE@google.com
> Tested-by: syzbot+2479e53d0db9b32ae2aa@...kaller.appspotmail.com
> Fixes: a7800aa80ea4 ("KVM: Add KVM_CREATE_GUEST_MEMFD ioctl() for guest-specific backing memory")
> Cc: stable@...r.kernel.org
> Cc: Hillf Danton <hdanton@...a.com>
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>

Reviewed-By: Vishal Annapurve <vannapurve@...gle.com>

> ---
>  virt/kvm/guest_memfd.c | 46 +++++++++++++++++++++++++++++-------------
>  1 file changed, 32 insertions(+), 14 deletions(-)
>
> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index fbca8c0972da..050731922522 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -623,24 +623,11 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
>         return r;
>  }
>
> -void kvm_gmem_unbind(struct kvm_memory_slot *slot)
> +static void __kvm_gmem_unbind(struct kvm_memory_slot *slot, struct kvm_gmem *gmem)
>  {
>         unsigned long start = slot->gmem.pgoff;
>         unsigned long end = start + slot->npages;
> -       struct kvm_gmem *gmem;
> -       struct file *file;
>
> -       /*
> -        * Nothing to do if the underlying file was already closed (or is being
> -        * closed right now), kvm_gmem_release() invalidates all bindings.
> -        */
> -       file = kvm_gmem_get_file(slot);
> -       if (!file)
> -               return;
> -
> -       gmem = file->private_data;
> -
> -       filemap_invalidate_lock(file->f_mapping);
>         xa_store_range(&gmem->bindings, start, end - 1, NULL, GFP_KERNEL);
>
>         /*
> @@ -648,6 +635,37 @@ void kvm_gmem_unbind(struct kvm_memory_slot *slot)
>          * cannot see this memslot.
>          */
>         WRITE_ONCE(slot->gmem.file, NULL);
> +}
> +
> +void kvm_gmem_unbind(struct kvm_memory_slot *slot)
> +{
> +       struct file *file;
> +
> +       /*
> +        * Nothing to do if the underlying file was _already_ closed, as
> +        * kvm_gmem_release() invalidates and nullifies all bindings.
> +        */
> +       if (!slot->gmem.file)
> +               return;
> +
> +       file = kvm_gmem_get_file(slot);
> +
> +       /*
> +        * However, if the file is _being_ closed, then the bindings need to be
> +        * removed as kvm_gmem_release() might not run until after the memslot
> +        * is freed.  Note, modifying the bindings is safe even though the file
> +        * is dying as kvm_gmem_release() nullifies slot->gmem.file under
> +        * slots_lock, and only puts its reference to KVM after destroying all
> +        * bindings.  I.e. reaching this point means kvm_gmem_release() can't
> +        * concurrently destroy the bindings or free the gmem_file.

Maybe a bit more description here is warranted:

Reaching this point also means that kvm_gmem_release() hasn't *yet*
freed the private_data or the bindings.

> +        */
> +       if (!file) {
> +               __kvm_gmem_unbind(slot, slot->gmem.file->private_data);
> +               return;
> +       }
> +
> +       filemap_invalidate_lock(file->f_mapping);
> +       __kvm_gmem_unbind(slot, file->private_data);
>         filemap_invalidate_unlock(file->f_mapping);
>
>         fput(file);
>
> base-commit: 4361f5aa8bfcecbab3fc8db987482b9e08115a6a
> --
> 2.51.2.1006.ga50a493c49-goog
>
>