* [PATCH 0/3] KVM: Fixes and a cleanup for coalesced MMIO
@ 2021-04-12 22:20 Sean Christopherson
2021-04-12 22:20 ` [PATCH 1/3] KVM: Destroy I/O bus devices on unregister failure _after_ sync'ing SRCU Sean Christopherson
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Sean Christopherson @ 2021-04-12 22:20 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kvm, linux-kernel, Sean Christopherson, Hao Sun
Fix two bugs that are exposed if unregistered a device on an I/O bus fails
due to OOM. Tack on opportunistic cleanup in the related code.
Sean Christopherson (3):
KVM: Destroy I/O bus devices on unregister failure _after_ sync'ing
SRCU
KVM: Stop looking for coalesced MMIO zones if the bus is destroyed
KVM: Add proper lockdep assertion in I/O bus unregister
include/linux/kvm_host.h | 4 ++--
virt/kvm/coalesced_mmio.c | 19 +++++++++++++++++--
virt/kvm/kvm_main.c | 26 ++++++++++++++++----------
3 files changed, 35 insertions(+), 14 deletions(-)
--
2.31.1.295.g9ea45b61b8-goog
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/3] KVM: Destroy I/O bus devices on unregister failure _after_ sync'ing SRCU 2021-04-12 22:20 [PATCH 0/3] KVM: Fixes and a cleanup for coalesced MMIO Sean Christopherson @ 2021-04-12 22:20 ` Sean Christopherson 2021-04-15 21:50 ` Jim Mattson 2021-04-12 22:20 ` [PATCH 2/3] KVM: Stop looking for coalesced MMIO zones if the bus is destroyed Sean Christopherson ` (2 subsequent siblings) 3 siblings, 1 reply; 9+ messages in thread From: Sean Christopherson @ 2021-04-12 22:20 UTC (permalink / raw) To: Paolo Bonzini; +Cc: kvm, linux-kernel, Sean Christopherson, Hao Sun If allocating a new instance of an I/O bus fails when unregistering a device, wait to destroy the device until after all readers are guaranteed to see the new null bus. Destroying devices before the bus is nullified could lead to use-after-free since readers expect the devices on their reference of the bus to remain valid. Fixes: f65886606c2d ("KVM: fix memory leak in kvm_io_bus_unregister_dev()") Cc: stable@vger.kernel.org Signed-off-by: Sean Christopherson <seanjc@google.com> --- virt/kvm/kvm_main.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 383df23514b9..d6e2b570e430 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -4511,7 +4511,13 @@ void kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, new_bus->dev_count--; memcpy(new_bus->range + i, bus->range + i + 1, flex_array_size(new_bus, range, new_bus->dev_count - i)); - } else { + } + + rcu_assign_pointer(kvm->buses[bus_idx], new_bus); + synchronize_srcu_expedited(&kvm->srcu); + + /* Destroy the old bus _after_ installing the (null) bus. */ + if (!new_bus) { pr_err("kvm: failed to shrink bus, removing it completely\n"); for (j = 0; j < bus->dev_count; j++) { if (j == i) @@ -4520,8 +4526,6 @@ void kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, } } - rcu_assign_pointer(kvm->buses[bus_idx], new_bus); - synchronize_srcu_expedited(&kvm->srcu); kfree(bus); return; } -- 2.31.1.295.g9ea45b61b8-goog ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] KVM: Destroy I/O bus devices on unregister failure _after_ sync'ing SRCU 2021-04-12 22:20 ` [PATCH 1/3] KVM: Destroy I/O bus devices on unregister failure _after_ sync'ing SRCU Sean Christopherson @ 2021-04-15 21:50 ` Jim Mattson 0 siblings, 0 replies; 9+ messages in thread From: Jim Mattson @ 2021-04-15 21:50 UTC (permalink / raw) To: Sean Christopherson; +Cc: Paolo Bonzini, kvm list, LKML, Hao Sun On Mon, Apr 12, 2021 at 3:21 PM Sean Christopherson <seanjc@google.com> wrote: > > If allocating a new instance of an I/O bus fails when unregistering a > device, wait to destroy the device until after all readers are guaranteed > to see the new null bus. Destroying devices before the bus is nullified > could lead to use-after-free since readers expect the devices on their > reference of the bus to remain valid. > > Fixes: f65886606c2d ("KVM: fix memory leak in kvm_io_bus_unregister_dev()") > Cc: stable@vger.kernel.org > Signed-off-by: Sean Christopherson <seanjc@google.com> Reviewed-by: Jim Mattson <jmattson@google.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/3] KVM: Stop looking for coalesced MMIO zones if the bus is destroyed 2021-04-12 22:20 [PATCH 0/3] KVM: Fixes and a cleanup for coalesced MMIO Sean Christopherson 2021-04-12 22:20 ` [PATCH 1/3] KVM: Destroy I/O bus devices on unregister failure _after_ sync'ing SRCU Sean Christopherson @ 2021-04-12 22:20 ` Sean Christopherson 2021-04-15 22:22 ` Jim Mattson 2021-04-12 22:20 ` [PATCH 3/3] KVM: Add proper lockdep assertion in I/O bus unregister Sean Christopherson 2021-04-17 13:11 ` [PATCH 0/3] KVM: Fixes and a cleanup for coalesced MMIO Paolo Bonzini 3 siblings, 1 reply; 9+ messages in thread From: Sean Christopherson @ 2021-04-12 22:20 UTC (permalink / raw) To: Paolo Bonzini; +Cc: kvm, linux-kernel, Sean Christopherson, Hao Sun Abort the walk of coalesced MMIO zones if kvm_io_bus_unregister_dev() fails to allocate memory for the new instance of the bus. If it can't instantiate a new bus, unregister_dev() destroys all devices _except_ the target device. But, it doesn't tell the caller that it obliterated the bus and invoked the destructor for all devices that were on the bus. In the coalesced MMIO case, this can result in a deleted list entry dereference due to attempting to continue iterating on coalesced_zones after future entries (in the walk) have been deleted. Opportunistically add curly braces to the for-loop, which encompasses many lines but sneaks by without braces due to the guts being a single if statement. Fixes: f65886606c2d ("KVM: fix memory leak in kvm_io_bus_unregister_dev()") Cc: stable@vger.kernel.org Reported-by: Hao Sun <sunhao.th@gmail.com> Signed-off-by: Sean Christopherson <seanjc@google.com> --- include/linux/kvm_host.h | 4 ++-- virt/kvm/coalesced_mmio.c | 19 +++++++++++++++++-- virt/kvm/kvm_main.c | 10 +++++----- 3 files changed, 24 insertions(+), 9 deletions(-) diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 1b65e7204344..99dccea4293c 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -192,8 +192,8 @@ int kvm_io_bus_read(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx, gpa_t addr, int len, 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); -void kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, - struct kvm_io_device *dev); +int kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, + struct kvm_io_device *dev); struct kvm_io_device *kvm_io_bus_get_dev(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr); diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c index 62bd908ecd58..f08f5e82460b 100644 --- a/virt/kvm/coalesced_mmio.c +++ b/virt/kvm/coalesced_mmio.c @@ -174,21 +174,36 @@ int kvm_vm_ioctl_unregister_coalesced_mmio(struct kvm *kvm, struct kvm_coalesced_mmio_zone *zone) { struct kvm_coalesced_mmio_dev *dev, *tmp; + int r; if (zone->pio != 1 && zone->pio != 0) return -EINVAL; mutex_lock(&kvm->slots_lock); - list_for_each_entry_safe(dev, tmp, &kvm->coalesced_zones, list) + list_for_each_entry_safe(dev, tmp, &kvm->coalesced_zones, list) { if (zone->pio == dev->zone.pio && coalesced_mmio_in_range(dev, zone->addr, zone->size)) { - kvm_io_bus_unregister_dev(kvm, + r = kvm_io_bus_unregister_dev(kvm, zone->pio ? KVM_PIO_BUS : KVM_MMIO_BUS, &dev->dev); kvm_iodevice_destructor(&dev->dev); + + /* + * On failure, unregister destroys all devices on the + * bus _except_ the target device, i.e. coalesced_zones + * has been modified. No need to restart the walk as + * there aren't any zones left. + */ + if (r) + break; } + } mutex_unlock(&kvm->slots_lock); + /* + * Ignore the result of kvm_io_bus_unregister_dev(), from userspace's + * perspective, the coalesced MMIO is most definitely unregistered. + */ return 0; } diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index d6e2b570e430..ab1fa6f92c82 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -4486,15 +4486,15 @@ int kvm_io_bus_register_dev(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr, } /* Caller must hold slots_lock. */ -void kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, - struct kvm_io_device *dev) +int kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, + struct kvm_io_device *dev) { int i, j; struct kvm_io_bus *new_bus, *bus; bus = kvm_get_bus(kvm, bus_idx); if (!bus) - return; + return 0; for (i = 0; i < bus->dev_count; i++) if (bus->range[i].dev == dev) { @@ -4502,7 +4502,7 @@ void kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, } if (i == bus->dev_count) - return; + return 0; new_bus = kmalloc(struct_size(bus, range, bus->dev_count - 1), GFP_KERNEL_ACCOUNT); @@ -4527,7 +4527,7 @@ void kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, } kfree(bus); - return; + return new_bus ? 0 : -ENOMEM; } struct kvm_io_device *kvm_io_bus_get_dev(struct kvm *kvm, enum kvm_bus bus_idx, -- 2.31.1.295.g9ea45b61b8-goog ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] KVM: Stop looking for coalesced MMIO zones if the bus is destroyed 2021-04-12 22:20 ` [PATCH 2/3] KVM: Stop looking for coalesced MMIO zones if the bus is destroyed Sean Christopherson @ 2021-04-15 22:22 ` Jim Mattson 0 siblings, 0 replies; 9+ messages in thread From: Jim Mattson @ 2021-04-15 22:22 UTC (permalink / raw) To: Sean Christopherson; +Cc: Paolo Bonzini, kvm list, LKML, Hao Sun On Mon, Apr 12, 2021 at 3:21 PM Sean Christopherson <seanjc@google.com> wrote: > > Abort the walk of coalesced MMIO zones if kvm_io_bus_unregister_dev() > fails to allocate memory for the new instance of the bus. If it can't > instantiate a new bus, unregister_dev() destroys all devices _except_ the > target device. But, it doesn't tell the caller that it obliterated the > bus and invoked the destructor for all devices that were on the bus. In > the coalesced MMIO case, this can result in a deleted list entry > dereference due to attempting to continue iterating on coalesced_zones > after future entries (in the walk) have been deleted. > > Opportunistically add curly braces to the for-loop, which encompasses > many lines but sneaks by without braces due to the guts being a single > if statement. > > Fixes: f65886606c2d ("KVM: fix memory leak in kvm_io_bus_unregister_dev()") > Cc: stable@vger.kernel.org > Reported-by: Hao Sun <sunhao.th@gmail.com> > Signed-off-by: Sean Christopherson <seanjc@google.com> Reviewed-by: Jim Mattson <jmattson@google.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] KVM: Add proper lockdep assertion in I/O bus unregister 2021-04-12 22:20 [PATCH 0/3] KVM: Fixes and a cleanup for coalesced MMIO Sean Christopherson 2021-04-12 22:20 ` [PATCH 1/3] KVM: Destroy I/O bus devices on unregister failure _after_ sync'ing SRCU Sean Christopherson 2021-04-12 22:20 ` [PATCH 2/3] KVM: Stop looking for coalesced MMIO zones if the bus is destroyed Sean Christopherson @ 2021-04-12 22:20 ` Sean Christopherson 2021-04-15 21:58 ` Jim Mattson 2021-04-17 13:11 ` [PATCH 0/3] KVM: Fixes and a cleanup for coalesced MMIO Paolo Bonzini 3 siblings, 1 reply; 9+ messages in thread From: Sean Christopherson @ 2021-04-12 22:20 UTC (permalink / raw) To: Paolo Bonzini; +Cc: kvm, linux-kernel, Sean Christopherson, Hao Sun Convert a comment above kvm_io_bus_unregister_dev() into an actual lockdep assertion, and opportunistically add curly braces to a multi-line for-loop. Signed-off-by: Sean Christopherson <seanjc@google.com> --- virt/kvm/kvm_main.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index ab1fa6f92c82..ccc2ef1dbdda 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -4485,21 +4485,23 @@ int kvm_io_bus_register_dev(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr, return 0; } -/* Caller must hold slots_lock. */ int kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, struct kvm_io_device *dev) { int i, j; struct kvm_io_bus *new_bus, *bus; + lockdep_assert_held(&kvm->slots_lock); + bus = kvm_get_bus(kvm, bus_idx); if (!bus) return 0; - for (i = 0; i < bus->dev_count; i++) + for (i = 0; i < bus->dev_count; i++) { if (bus->range[i].dev == dev) { break; } + } if (i == bus->dev_count) return 0; -- 2.31.1.295.g9ea45b61b8-goog ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] KVM: Add proper lockdep assertion in I/O bus unregister 2021-04-12 22:20 ` [PATCH 3/3] KVM: Add proper lockdep assertion in I/O bus unregister Sean Christopherson @ 2021-04-15 21:58 ` Jim Mattson 2021-04-15 22:15 ` Sean Christopherson 0 siblings, 1 reply; 9+ messages in thread From: Jim Mattson @ 2021-04-15 21:58 UTC (permalink / raw) To: Sean Christopherson; +Cc: Paolo Bonzini, kvm list, LKML, Hao Sun On Mon, Apr 12, 2021 at 3:23 PM Sean Christopherson <seanjc@google.com> wrote: > > Convert a comment above kvm_io_bus_unregister_dev() into an actual > lockdep assertion, and opportunistically add curly braces to a multi-line > for-loop. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > virt/kvm/kvm_main.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index ab1fa6f92c82..ccc2ef1dbdda 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -4485,21 +4485,23 @@ int kvm_io_bus_register_dev(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr, > return 0; > } > > -/* Caller must hold slots_lock. */ > int kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, > struct kvm_io_device *dev) > { > int i, j; > struct kvm_io_bus *new_bus, *bus; > > + lockdep_assert_held(&kvm->slots_lock); > + > bus = kvm_get_bus(kvm, bus_idx); > if (!bus) > return 0; > > - for (i = 0; i < bus->dev_count; i++) > + for (i = 0; i < bus->dev_count; i++) { > if (bus->range[i].dev == dev) { > break; > } > + } Per coding-style.rst, neither the for loop nor the if-block should have braces. "Do not unnecessarily use braces where a single statement will do." Stylistic nits aside, Reviewed-by: Jim Mattson <jmattson@google.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] KVM: Add proper lockdep assertion in I/O bus unregister 2021-04-15 21:58 ` Jim Mattson @ 2021-04-15 22:15 ` Sean Christopherson 0 siblings, 0 replies; 9+ messages in thread From: Sean Christopherson @ 2021-04-15 22:15 UTC (permalink / raw) To: Jim Mattson; +Cc: Paolo Bonzini, kvm list, LKML, Hao Sun On Thu, Apr 15, 2021, Jim Mattson wrote: > On Mon, Apr 12, 2021 at 3:23 PM Sean Christopherson <seanjc@google.com> wrote: > > > > Convert a comment above kvm_io_bus_unregister_dev() into an actual > > lockdep assertion, and opportunistically add curly braces to a multi-line > > for-loop. > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > --- > > virt/kvm/kvm_main.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index ab1fa6f92c82..ccc2ef1dbdda 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -4485,21 +4485,23 @@ int kvm_io_bus_register_dev(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr, > > return 0; > > } > > > > -/* Caller must hold slots_lock. */ > > int kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, > > struct kvm_io_device *dev) > > { > > int i, j; > > struct kvm_io_bus *new_bus, *bus; > > > > + lockdep_assert_held(&kvm->slots_lock); > > + > > bus = kvm_get_bus(kvm, bus_idx); > > if (!bus) > > return 0; > > > > - for (i = 0; i < bus->dev_count; i++) > > + for (i = 0; i < bus->dev_count; i++) { > > if (bus->range[i].dev == dev) { > > break; > > } > > + } > Per coding-style.rst, neither the for loop nor the if-block should have braces. > > "Do not unnecessarily use braces where a single statement will do." Doh, the if-statement should indeed not use braces. I think I meant to clean that up, and then saw something shiny... But the for-loop... keep reading :-D Also, use braces when a loop contains more than a single simple statement: .. code-block:: c while (condition) { if (test) do_something(); } ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/3] KVM: Fixes and a cleanup for coalesced MMIO 2021-04-12 22:20 [PATCH 0/3] KVM: Fixes and a cleanup for coalesced MMIO Sean Christopherson ` (2 preceding siblings ...) 2021-04-12 22:20 ` [PATCH 3/3] KVM: Add proper lockdep assertion in I/O bus unregister Sean Christopherson @ 2021-04-17 13:11 ` Paolo Bonzini 3 siblings, 0 replies; 9+ messages in thread From: Paolo Bonzini @ 2021-04-17 13:11 UTC (permalink / raw) To: Sean Christopherson; +Cc: kvm, linux-kernel, Hao Sun On 13/04/21 00:20, Sean Christopherson wrote: > Fix two bugs that are exposed if unregistered a device on an I/O bus fails > due to OOM. Tack on opportunistic cleanup in the related code. > > Sean Christopherson (3): > KVM: Destroy I/O bus devices on unregister failure _after_ sync'ing > SRCU > KVM: Stop looking for coalesced MMIO zones if the bus is destroyed > KVM: Add proper lockdep assertion in I/O bus unregister > > include/linux/kvm_host.h | 4 ++-- > virt/kvm/coalesced_mmio.c | 19 +++++++++++++++++-- > virt/kvm/kvm_main.c | 26 ++++++++++++++++---------- > 3 files changed, 35 insertions(+), 14 deletions(-) > Queued, thanks. Paolo ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-04-17 13:11 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-12 22:20 [PATCH 0/3] KVM: Fixes and a cleanup for coalesced MMIO Sean Christopherson 2021-04-12 22:20 ` [PATCH 1/3] KVM: Destroy I/O bus devices on unregister failure _after_ sync'ing SRCU Sean Christopherson 2021-04-15 21:50 ` Jim Mattson 2021-04-12 22:20 ` [PATCH 2/3] KVM: Stop looking for coalesced MMIO zones if the bus is destroyed Sean Christopherson 2021-04-15 22:22 ` Jim Mattson 2021-04-12 22:20 ` [PATCH 3/3] KVM: Add proper lockdep assertion in I/O bus unregister Sean Christopherson 2021-04-15 21:58 ` Jim Mattson 2021-04-15 22:15 ` Sean Christopherson 2021-04-17 13:11 ` [PATCH 0/3] KVM: Fixes and a cleanup for coalesced MMIO Paolo Bonzini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).