Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752633AbdGPONh (ORCPT ); Sun, 16 Jul 2017 10:13:37 -0400 Received: from shadbolt.e.decadent.org.uk ([88.96.1.126]:44258 "EHLO shadbolt.e.decadent.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751310AbdGPONe (ORCPT ); Sun, 16 Jul 2017 10:13:34 -0400 Content-Type: text/plain; charset="UTF-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit MIME-Version: 1.0 From: Ben Hutchings To: linux-kernel@vger.kernel.org, stable@vger.kernel.org CC: akpm@linux-foundation.org, "Dmitry Vyukov" , "Paolo Bonzini" , "David Hildenbrand" , "Cornelia Huck" Date: Sun, 16 Jul 2017 14:56:46 +0100 Message-ID: X-Mailer: LinuxStableQueue (scripts by bwh) Subject: [PATCH 3.16 085/178] KVM: kvm_io_bus_unregister_dev() should never fail In-Reply-To: X-SA-Exim-Connect-IP: 2a02:8011:400e:2:6f00:88c8:c921:d332 X-SA-Exim-Mail-From: ben@decadent.org.uk X-SA-Exim-Scanned: No (on shadbolt.decadent.org.uk); SAEximRunCond expanded to false Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4805 Lines: 163 3.16.46-rc1 review patch. If anyone has any objections, please let me know. ------------------ From: David Hildenbrand commit 90db10434b163e46da413d34db8d0e77404cc645 upstream. No caller currently checks the return value of kvm_io_bus_unregister_dev(). This is evil, as all callers silently go on freeing their device. A stale reference will remain in the io_bus, getting at least used again, when the iobus gets teared down on kvm_destroy_vm() - leading to use after free errors. There is nothing the callers could do, except retrying over and over again. So let's simply remove the bus altogether, print an error and make sure no one can access this broken bus again (returning -ENOMEM on any attempt to access it). Fixes: e93f8a0f821e ("KVM: convert io_bus to SRCU") Reported-by: Dmitry Vyukov Reviewed-by: Cornelia Huck Signed-off-by: David Hildenbrand Signed-off-by: Paolo Bonzini [bwh: Backported to 3.16: - Drop changes to kvm_io_bus_get_dev() - Adjust context] Signed-off-by: Ben Hutchings --- --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -177,8 +177,8 @@ int kvm_io_bus_read(struct kvm *kvm, enu void *val); int kvm_io_bus_register_dev(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr, int len, struct kvm_io_device *dev); -int kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, - struct kvm_io_device *dev); +void kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, + struct kvm_io_device *dev); #ifdef CONFIG_KVM_ASYNC_PF struct kvm_async_pf { --- a/virt/kvm/eventfd.c +++ b/virt/kvm/eventfd.c @@ -794,7 +794,8 @@ kvm_deassign_ioeventfd_idx(struct kvm *k continue; kvm_io_bus_unregister_dev(kvm, bus_idx, &p->dev); - kvm->buses[bus_idx]->ioeventfd_count--; + if (kvm->buses[bus_idx]) + kvm->buses[bus_idx]->ioeventfd_count--; ioeventfd_release(p); ret = 0; break; --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -607,7 +607,8 @@ static void kvm_destroy_vm(struct kvm *k spin_unlock(&kvm_lock); kvm_free_irq_routing(kvm); for (i = 0; i < KVM_NR_BUSES; i++) { - kvm_io_bus_destroy(kvm->buses[i]); + if (kvm->buses[i]) + kvm_io_bus_destroy(kvm->buses[i]); kvm->buses[i] = NULL; } kvm_coalesced_mmio_free(kvm); @@ -2908,6 +2909,8 @@ int kvm_io_bus_write(struct kvm *kvm, en }; bus = srcu_dereference(kvm->buses[bus_idx], &kvm->srcu); + if (!bus) + return -ENOMEM; r = __kvm_io_bus_write(bus, &range, val); return r < 0 ? r : 0; } @@ -2925,6 +2928,8 @@ int kvm_io_bus_write_cookie(struct kvm * }; bus = srcu_dereference(kvm->buses[bus_idx], &kvm->srcu); + if (!bus) + return -ENOMEM; /* First try the device referenced by cookie. */ if ((cookie >= 0) && (cookie < bus->dev_count) && @@ -2975,6 +2980,8 @@ int kvm_io_bus_read(struct kvm *kvm, enu }; bus = srcu_dereference(kvm->buses[bus_idx], &kvm->srcu); + if (!bus) + return -ENOMEM; r = __kvm_io_bus_read(bus, &range, val); return r < 0 ? r : 0; } @@ -2987,6 +2994,9 @@ int kvm_io_bus_register_dev(struct kvm * struct kvm_io_bus *new_bus, *bus; bus = kvm->buses[bus_idx]; + if (!bus) + return -ENOMEM; + /* exclude ioeventfd which is limited by maximum fd */ if (bus->dev_count - bus->ioeventfd_count > NR_IOBUS_DEVS - 1) return -ENOSPC; @@ -3006,45 +3016,41 @@ int kvm_io_bus_register_dev(struct kvm * } /* Caller must hold slots_lock. */ -int kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, - struct kvm_io_device *dev) +void kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, + struct kvm_io_device *dev) { - int i, r; + int i; struct kvm_io_bus *new_bus, *bus; bus = kvm->buses[bus_idx]; - - /* - * It's possible the bus being released before hand. If so, - * we're done here. - */ if (!bus) - return 0; + return; - r = -ENOENT; for (i = 0; i < bus->dev_count; i++) if (bus->range[i].dev == dev) { - r = 0; break; } - if (r) - return r; + if (i == bus->dev_count) + return; new_bus = kzalloc(sizeof(*bus) + ((bus->dev_count - 1) * sizeof(struct kvm_io_range)), GFP_KERNEL); - if (!new_bus) - return -ENOMEM; + if (!new_bus) { + pr_err("kvm: failed to shrink bus, removing it completely\n"); + goto broken; + } memcpy(new_bus, bus, sizeof(*bus) + i * sizeof(struct kvm_io_range)); new_bus->dev_count--; memcpy(new_bus->range + i, bus->range + i + 1, (new_bus->dev_count - i) * sizeof(struct kvm_io_range)); +broken: rcu_assign_pointer(kvm->buses[bus_idx], new_bus); synchronize_srcu_expedited(&kvm->srcu); kfree(bus); - return r; + return; } static struct notifier_block kvm_cpu_notifier = {