On 03/04/2017 10:23, David Hildenbrand wrote:
> On 30.03.2017 10:00, gregkh@xxxxxxxxxxxxxxxxxxx wrote:
>>
>> The patch below does not apply to the 4.10-stable tree.
>> If someone wants it applied there, or to any other stable or longterm
>> tree, then please email the backport, including the original git commit
>> id to <stable@xxxxxxxxxxxxxxx>.
>>
>
> Hi Greg,
>
> this commit can be cherry-picked cleanly after cherry picking
> df630b8c1e85 ("KVM: x86: clear bus pointer when destroyed"). Can you
> also apply that commit or should I send a backport of the stable patch,
> including the necessary check from the prerequisite patch?
This extra commit looks good for stable, thanks.
Paolo
>> thanks,
>>
>> greg k-h
>>
>> ------------------ original commit in Linus's tree ------------------
>>
>> From 90db10434b163e46da413d34db8d0e77404cc645 Mon Sep 17 00:00:00 2001
>> From: David Hildenbrand <david@xxxxxxxxxx>
>> Date: Thu, 23 Mar 2017 18:24:19 +0100
>> Subject: [PATCH] KVM: kvm_io_bus_unregister_dev() should never fail
>>
>> 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@xxxxxxxxxxxxxxx # 3.4+
>> Reported-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
>> Reviewed-by: Cornelia Huck <cornelia.huck@xxxxxxxxxx>
>> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
>> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
>>
>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> index 2c14ad9809da..d0250744507a 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/eventfd.c b/virt/kvm/eventfd.c
>> index a29786dd9522..4d28a9ddbee0 100644
>> --- a/virt/kvm/eventfd.c
>> +++ b/virt/kvm/eventfd.c
>> @@ -870,7 +870,8 @@ kvm_deassign_ioeventfd_idx(struct kvm *kvm, enum kvm_bus bus_idx,
>> 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;
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index 7445566fadc1..ef1aa7f1ed7a 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);
>> @@ -3476,6 +3477,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 +3496,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 +3548,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 +3562,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 +3584,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 +3631,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)
>>