Skip to content

Commit ae43105

Browse files
sean-jcsean-jc
committed
KVM: guest_memfd: Remove bindings on memslot deletion when gmem is dying
When unbinding a memslot from a guest_memfd instance, remove the bindings even if the guest_memfd file is dying, i.e. even if its file refcount has gone to zero. If the memslot is freed before the file is fully released, nullifying the memslot side of the binding in kvm_gmem_release() will write to freed memory, as detected by syzbot+KASAN: ================================================================== BUG: KASAN: slab-use-after-free in kvm_gmem_release+0x176/0x440 virt/kvm/guest_memfd.c:353 Write of size 8 at addr ffff88807befa508 by task syz.0.17/6022 CPU: 0 UID: 0 PID: 6022 Comm: syz.0.17 Not tainted syzkaller #0 PREEMPT(full) Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/02/2025 Call Trace: <TASK> dump_stack_lvl+0x189/0x250 lib/dump_stack.c:120 print_address_description mm/kasan/report.c:378 [inline] print_report+0xca/0x240 mm/kasan/report.c:482 kasan_report+0x118/0x150 mm/kasan/report.c:595 kvm_gmem_release+0x176/0x440 virt/kvm/guest_memfd.c:353 __fput+0x44c/0xa70 fs/file_table.c:468 task_work_run+0x1d4/0x260 kernel/task_work.c:227 resume_user_mode_work include/linux/resume_user_mode.h:50 [inline] exit_to_user_mode_loop+0xe9/0x130 kernel/entry/common.c:43 exit_to_user_mode_prepare include/linux/irq-entry-common.h:225 [inline] syscall_exit_to_user_mode_work include/linux/entry-common.h:175 [inline] syscall_exit_to_user_mode include/linux/entry-common.h:210 [inline] do_syscall_64+0x2bd/0xfa0 arch/x86/entry/syscall_64.c:100 entry_SYSCALL_64_after_hwframe+0x77/0x7f RIP: 0033:0x7fbeeff8efc9 </TASK> Allocated by task 6023: kasan_save_stack mm/kasan/common.c:56 [inline] kasan_save_track+0x3e/0x80 mm/kasan/common.c:77 poison_kmalloc_redzone mm/kasan/common.c:397 [inline] __kasan_kmalloc+0x93/0xb0 mm/kasan/common.c:414 kasan_kmalloc include/linux/kasan.h:262 [inline] __kmalloc_cache_noprof+0x3e2/0x700 mm/slub.c:5758 kmalloc_noprof include/linux/slab.h:957 [inline] kzalloc_noprof include/linux/slab.h:1094 [inline] kvm_set_memory_region+0x747/0xb90 virt/kvm/kvm_main.c:2104 kvm_vm_ioctl_set_memory_region+0x6f/0xd0 virt/kvm/kvm_main.c:2154 kvm_vm_ioctl+0x957/0xc60 virt/kvm/kvm_main.c:5201 vfs_ioctl fs/ioctl.c:51 [inline] __do_sys_ioctl fs/ioctl.c:597 [inline] __se_sys_ioctl+0xfc/0x170 fs/ioctl.c:583 do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline] do_syscall_64+0xfa/0xfa0 arch/x86/entry/syscall_64.c:94 entry_SYSCALL_64_after_hwframe+0x77/0x7f Freed by task 6023: kasan_save_stack mm/kasan/common.c:56 [inline] kasan_save_track+0x3e/0x80 mm/kasan/common.c:77 kasan_save_free_info+0x46/0x50 mm/kasan/generic.c:584 poison_slab_object mm/kasan/common.c:252 [inline] __kasan_slab_free+0x5c/0x80 mm/kasan/common.c:284 kasan_slab_free include/linux/kasan.h:234 [inline] slab_free_hook mm/slub.c:2533 [inline] slab_free mm/slub.c:6622 [inline] kfree+0x19a/0x6d0 mm/slub.c:6829 kvm_set_memory_region+0x9c4/0xb90 virt/kvm/kvm_main.c:2130 kvm_vm_ioctl_set_memory_region+0x6f/0xd0 virt/kvm/kvm_main.c:2154 kvm_vm_ioctl+0x957/0xc60 virt/kvm/kvm_main.c:5201 vfs_ioctl fs/ioctl.c:51 [inline] __do_sys_ioctl fs/ioctl.c:597 [inline] __se_sys_ioctl+0xfc/0x170 fs/ioctl.c:583 do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline] do_syscall_64+0xfa/0xfa0 arch/x86/entry/syscall_64.c:94 entry_SYSCALL_64_after_hwframe+0x77/0x7f 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 safe to access the 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@syzkaller.appspotmail.com Closes: https://lore.kernel.org/all/68fa7a22.a70a0220.3bf6c6.008b.GAE@google.com Tested-by: syzbot+2479e53d0db9b32ae2aa@syzkaller.appspotmail.com Fixes: a7800aa ("KVM: Add KVM_CREATE_GUEST_MEMFD ioctl() for guest-specific backing memory") Cc: stable@vger.kernel.org Cc: Hillf Danton <hdanton@sina.com> Reviewed-By: Vishal Annapurve <vannapurve@google.com> Link: https://patch.msgid.link/20251104011205.3853541-1-seanjc@google.com Signed-off-by: Sean Christopherson <seanjc@google.com>
1 parent fd92bd3 commit ae43105

File tree

1 file changed

+32
-13
lines changed

1 file changed

+32
-13
lines changed

‎virt/kvm/guest_memfd.c‎

Lines changed: 32 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -623,31 +623,50 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
623623
return r;
624624
}
625625

626-
void kvm_gmem_unbind(struct kvm_memory_slot *slot)
626+
static void __kvm_gmem_unbind(struct kvm_memory_slot *slot, struct kvm_gmem *gmem)
627627
{
628628
unsigned long start = slot->gmem.pgoff;
629629
unsigned long end = start + slot->npages;
630-
struct kvm_gmem *gmem;
630+
631+
xa_store_range(&gmem->bindings, start, end - 1, NULL, GFP_KERNEL);
632+
633+
/*
634+
* synchronize_srcu(&kvm->srcu) ensured that kvm_gmem_get_pfn()
635+
* cannot see this memslot.
636+
*/
637+
WRITE_ONCE(slot->gmem.file, NULL);
638+
}
639+
640+
void kvm_gmem_unbind(struct kvm_memory_slot *slot)
641+
{
631642
struct file *file;
632643

633644
/*
634-
* Nothing to do if the underlying file was already closed (or is being
635-
* closed right now), kvm_gmem_release() invalidates all bindings.
645+
* Nothing to do if the underlying file was _already_ closed, as
646+
* kvm_gmem_release() invalidates and nullifies all bindings.
636647
*/
637-
file = kvm_gmem_get_file(slot);
638-
if (!file)
648+
if (!slot->gmem.file)
639649
return;
640650

641-
gmem = file->private_data;
642-
643-
filemap_invalidate_lock(file->f_mapping);
644-
xa_store_range(&gmem->bindings, start, end - 1, NULL, GFP_KERNEL);
651+
file = kvm_gmem_get_file(slot);
645652

646653
/*
647-
* synchronize_srcu(&kvm->srcu) ensured that kvm_gmem_get_pfn()
648-
* cannot see this memslot.
654+
* However, if the file is _being_ closed, then the bindings need to be
655+
* removed as kvm_gmem_release() might not run until after the memslot
656+
* is freed. Note, modifying the bindings is safe even though the file
657+
* is dying as kvm_gmem_release() nullifies slot->gmem.file under
658+
* slots_lock, and only puts its reference to KVM after destroying all
659+
* bindings. I.e. reaching this point means kvm_gmem_release() hasn't
660+
* yet destroyed the bindings or freed the gmem_file, and can't do so
661+
* until the caller drops slots_lock.
649662
*/
650-
WRITE_ONCE(slot->gmem.file, NULL);
663+
if (!file) {
664+
__kvm_gmem_unbind(slot, slot->gmem.file->private_data);
665+
return;
666+
}
667+
668+
filemap_invalidate_lock(file->f_mapping);
669+
__kvm_gmem_unbind(slot, file->private_data);
651670
filemap_invalidate_unlock(file->f_mapping);
652671

653672
fput(file);

0 commit comments

Comments
 (0)