* [PATCH v1] KVM: kvm_io_bus_unregister_dev() should never fail
@ 2017-03-23 14:34 David Hildenbrand
2017-03-23 16:06 ` Cornelia Huck
0 siblings, 1 reply; 5+ messages in thread
From: David Hildenbrand @ 2017-03-23 14:34 UTC (permalink / raw)
To: kvm
Cc: Paolo Bonzini, rkrcmar, Dmitry Vyukov, Marcelo Tosatti, stable,
LKML, david
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")
Cc: stable@vger.kernel.org # 3.4+
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
Based on kvm/queue, where we just got 2a108a4e7c1 ("KVM: x86: clear bus
pointer when destroyed"), which added a check we need here.
---
include/linux/kvm_host.h | 4 ++--
virt/kvm/kvm_main.c | 39 +++++++++++++++++++++++----------------
2 files changed, 25 insertions(+), 18 deletions(-)
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 2c14ad9..d025074 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -162,8 +162,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);
-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);
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/kvm_main.c b/virt/kvm/kvm_main.c
index 7445566..e1be4b4 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3476,6 +3476,8 @@ int kvm_io_bus_write(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx, gpa_t addr,
};
bus = srcu_dereference(vcpu->kvm->buses[bus_idx], &vcpu->kvm->srcu);
+ if (!bus)
+ return -ENOMEM;
r = __kvm_io_bus_write(vcpu, bus, &range, val);
return r < 0 ? r : 0;
}
@@ -3493,6 +3495,8 @@ int kvm_io_bus_write_cookie(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx,
};
bus = srcu_dereference(vcpu->kvm->buses[bus_idx], &vcpu->kvm->srcu);
+ if (!bus)
+ return -ENOMEM;
/* First try the device referenced by cookie. */
if ((cookie >= 0) && (cookie < bus->dev_count) &&
@@ -3543,6 +3547,8 @@ int kvm_io_bus_read(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx, gpa_t addr,
};
bus = srcu_dereference(vcpu->kvm->buses[bus_idx], &vcpu->kvm->srcu);
+ if (!bus)
+ return -ENOMEM;
r = __kvm_io_bus_read(vcpu, bus, &range, val);
return r < 0 ? r : 0;
}
@@ -3555,6 +3561,9 @@ int kvm_io_bus_register_dev(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
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;
@@ -3574,45 +3583,41 @@ int kvm_io_bus_register_dev(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
}
/* 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 = kmalloc(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;
}
struct kvm_io_device *kvm_io_bus_get_dev(struct kvm *kvm, enum kvm_bus bus_idx,
@@ -3625,6 +3630,8 @@ struct kvm_io_device *kvm_io_bus_get_dev(struct kvm *kvm, enum kvm_bus bus_idx,
srcu_idx = srcu_read_lock(&kvm->srcu);
bus = srcu_dereference(kvm->buses[bus_idx], &kvm->srcu);
+ if (!bus)
+ goto out_unlock;
dev_idx = kvm_io_bus_get_first_dev(bus, addr, 1);
if (dev_idx < 0)
--
2.9.3
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v1] KVM: kvm_io_bus_unregister_dev() should never fail
2017-03-23 14:34 [PATCH v1] KVM: kvm_io_bus_unregister_dev() should never fail David Hildenbrand
@ 2017-03-23 16:06 ` Cornelia Huck
2017-03-23 16:20 ` David Hildenbrand
0 siblings, 1 reply; 5+ messages in thread
From: Cornelia Huck @ 2017-03-23 16:06 UTC (permalink / raw)
To: David Hildenbrand
Cc: kvm, Paolo Bonzini, rkrcmar, Dmitry Vyukov, Marcelo Tosatti,
stable, LKML
On Thu, 23 Mar 2017 15:34:41 +0100
David Hildenbrand <david@redhat.com> wrote:
> 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")
> Cc: stable@vger.kernel.org # 3.4+
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> /* 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 = kmalloc(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);
As this may set kvm->buses[bus_idx] to NULL, don't you also need to
guard for bus == NULL in kvm_io_bus_destroy()? (I looked at the code on
kvm/queue.)
> synchronize_srcu_expedited(&kvm->srcu);
> kfree(bus);
> - return r;
> + return;
> }
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v1] KVM: kvm_io_bus_unregister_dev() should never fail
2017-03-23 16:06 ` Cornelia Huck
@ 2017-03-23 16:20 ` David Hildenbrand
2017-03-23 16:40 ` Cornelia Huck
0 siblings, 1 reply; 5+ messages in thread
From: David Hildenbrand @ 2017-03-23 16:20 UTC (permalink / raw)
To: Cornelia Huck
Cc: kvm, Paolo Bonzini, rkrcmar, Dmitry Vyukov, Marcelo Tosatti,
stable, LKML
> As this may set kvm->buses[bus_idx] to NULL, don't you also need to
> guard for bus == NULL in kvm_io_bus_destroy()? (I looked at the code on
> kvm/queue.)
very right, so something like this?
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index e1be4b4..ef1aa7f 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -728,7 +728,8 @@ static void kvm_destroy_vm(struct kvm *kvm)
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);
Thanks!
>
>> synchronize_srcu_expedited(&kvm->srcu);
>> kfree(bus);
>> - return r;
>> + return;
>> }
>
--
Thanks,
David
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v1] KVM: kvm_io_bus_unregister_dev() should never fail
2017-03-23 16:20 ` David Hildenbrand
@ 2017-03-23 16:40 ` Cornelia Huck
2017-03-23 17:15 ` David Hildenbrand
0 siblings, 1 reply; 5+ messages in thread
From: Cornelia Huck @ 2017-03-23 16:40 UTC (permalink / raw)
To: David Hildenbrand
Cc: kvm, Paolo Bonzini, rkrcmar, Dmitry Vyukov, Marcelo Tosatti,
stable, LKML
On Thu, 23 Mar 2017 17:20:48 +0100
David Hildenbrand <david@redhat.com> wrote:
>
> > As this may set kvm->buses[bus_idx] to NULL, don't you also need to
> > guard for bus == NULL in kvm_io_bus_destroy()? (I looked at the code on
> > kvm/queue.)
>
> very right, so something like this?
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index e1be4b4..ef1aa7f 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -728,7 +728,8 @@ static void kvm_destroy_vm(struct kvm *kvm)
> 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);
>
> Thanks!
>
> >
> >> synchronize_srcu_expedited(&kvm->srcu);
> >> kfree(bus);
> >> - return r;
> >> + return;
> >> }
> >
>
>
Either that, or an early exit for bus == NULL in kvm_io_bus_destroy().
(I think the second option is more straightforward.)
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v1] KVM: kvm_io_bus_unregister_dev() should never fail
2017-03-23 16:40 ` Cornelia Huck
@ 2017-03-23 17:15 ` David Hildenbrand
0 siblings, 0 replies; 5+ messages in thread
From: David Hildenbrand @ 2017-03-23 17:15 UTC (permalink / raw)
To: Cornelia Huck
Cc: kvm, Paolo Bonzini, rkrcmar, Dmitry Vyukov, Marcelo Tosatti,
stable, LKML
>
> Either that, or an early exit for bus == NULL in kvm_io_bus_destroy().
> (I think the second option is more straightforward.)
I prefer to have the checks where kvm->buses[x] is actually accessed. So
the chance of missing yet another check is easier to verify.
Will send out v2 in a couple of minutes. Thanks!
--
Thanks,
David
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-03-23 17:15 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-23 14:34 [PATCH v1] KVM: kvm_io_bus_unregister_dev() should never fail David Hildenbrand
2017-03-23 16:06 ` Cornelia Huck
2017-03-23 16:20 ` David Hildenbrand
2017-03-23 16:40 ` Cornelia Huck
2017-03-23 17:15 ` David Hildenbrand
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.