[PATCH 0/3] KVM: Fixes and a cleanup for coalesced MMIO

[PATCH 0/3] KVM: Fixes and a cleanup for coalesced MMIO

[Sean Christopherson]

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

[PATCH 1/3] KVM: Destroy I/O bus devices on unregister failure _after_ sync'ing SRCU

[Sean Christopherson]

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

[PATCH 2/3] KVM: Stop looking for coalesced MMIO zones if the bus is destroyed

[Sean Christopherson]

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

[PATCH 3/3] KVM: Add proper lockdep assertion in I/O bus unregister

[Sean Christopherson]

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

Re: [PATCH 1/3] KVM: Destroy I/O bus devices on unregister failure _after_ sync'ing SRCU

[Jim Mattson]

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>

Re: [PATCH 3/3] KVM: Add proper lockdep assertion in I/O bus unregister

[Jim Mattson]

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>

Re: [PATCH 3/3] KVM: Add proper lockdep assertion in I/O bus unregister

[Sean Christopherson]

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();
        }

Re: [PATCH 2/3] KVM: Stop looking for coalesced MMIO zones if the bus is destroyed

[Jim Mattson]

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>

Re: [PATCH 0/3] KVM: Fixes and a cleanup for coalesced MMIO

[Paolo Bonzini]

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