From: David Woodhouse <[email protected]> kvm_create_plane() publishes the new plane in kvm->planes[] before kvm_vm_ioctl_create_plane() can still fail at anon_inode_getfile(), and the error path freed the plane without unpublishing it. Anything that later reads kvm->planes[] reaches the freed plane, and VM teardown destroys it a second time.
Unpublishing it on the error path is not enough either. Some readers of kvm->planes[] take no lock: the SEV VMPL-switch target lookup takes the level from the guest, so it can pick up a plane that is published and then freed with no grace period in between. Publish the plane in kvm_vm_ioctl_create_plane() only after anon_inode_getfile() succeeds, so a failing create frees a plane that no reader has ever seen. kvm_create_vm() publishes plane 0 itself, before the VM is reachable. The store stays smp_store_release(), and the lock-free readers (MSI delivery and the SEV VMPL target lookup) now load the pointer with READ_ONCE() to pair with it. Found by a local Sashiko review. Fixes the error path in "kvm: Implement KVM_CREATE_PLANE ioctl". Fixes: https://lore.kernel.org/all/[email protected]/ Signed-off-by: David Woodhouse <[email protected]> --- arch/x86/kvm/irq.c | 5 +++-- arch/x86/kvm/svm/sev.c | 2 +- virt/kvm/kvm_main.c | 16 ++++++++++++---- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c index f3044aec7188..26912161997e 100644 --- a/arch/x86/kvm/irq.c +++ b/arch/x86/kvm/irq.c @@ -235,7 +235,7 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e, return -1; kvm_msi_to_lapic_irq(kvm, e, &irq); - plane = kvm->planes[e->msi.plane_level]; + plane = READ_ONCE(kvm->planes[e->msi.plane_level]); return kvm_irq_delivery_to_apic(plane, NULL, &irq); } @@ -260,7 +260,8 @@ int kvm_arch_set_irq_inatomic(struct kvm_kernel_irq_routing_entry *e, kvm_msi_to_lapic_irq(kvm, e, &irq); - if (kvm_irq_delivery_to_apic_fast(kvm->planes[e->msi.plane_level], NULL, &irq, &r)) + if (kvm_irq_delivery_to_apic_fast(READ_ONCE(kvm->planes[e->msi.plane_level]), + NULL, &irq, &r)) return r; break; diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index fd7605e47216..32a4ba4ffdbb 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -4249,7 +4249,7 @@ static int sev_snp_ap_creation(struct kvm_vcpu *vcpu) vmpl = array_index_nospec(vmpl, SVM_SEV_VMPL_MAX); /* Obtain the target plane and vCPU */ - target_plane = vcpu->kvm->planes[vmpl]; + target_plane = READ_ONCE(vcpu->kvm->planes[vmpl]); if (target_plane) { target_vcpu = plane_get_vcpu(target_plane, apic_id); } else { diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 95de904f0c61..d256bbf301f4 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -1252,8 +1252,7 @@ static struct kvm_plane *kvm_create_plane(struct kvm *kvm, unsigned plane_level) if (kvm_arch_plane_init(kvm, plane, plane_level)) goto out_free_plane; - kvm->planes[plane_level] = plane; - + /* The caller publishes the plane in kvm->planes[]. */ return plane; out_free_plane: @@ -1312,7 +1311,8 @@ static struct kvm *kvm_create_vm(unsigned long type, const char *fdname) BUILD_BUG_ON(KVM_MEM_SLOTS_NUM > SHRT_MAX); /* Initialize planes array and allocate plane 0 */ - if (kvm_create_plane(kvm, 0) == NULL) { + kvm->planes[0] = kvm_create_plane(kvm, 0); + if (!kvm->planes[0]) { r = -ENOMEM; goto out_no_planes; } @@ -5627,13 +5627,21 @@ static int kvm_vm_ioctl_create_plane(struct kvm *kvm, unsigned id) goto put_kvm; } + /* + * Publish only once nothing can fail, so that the error path frees + * a plane no reader has seen. Pairs with the READ_ONCE() of the + * lock-free readers of kvm->planes[] (MSI delivery, SEV VMPL + * targeting): the plane's fields must be visible before the + * pointer is. + */ + smp_store_release(&kvm->planes[id], plane); kvm->has_planes = true; fd_install(fd, file); return fd; put_kvm: - kvm_put_kvm(kvm); kvm_destroy_one_plane(plane); + kvm_put_kvm(kvm); put_fd: put_unused_fd(fd); return r; -- 2.50.1
