Re: [PATCH v1] KVM: kvm_io_bus_unregister_dev() should never fail

[Cornelia Huck]

On Thu, 23 Mar 2017 15:34:41 +0100
David Hildenbrand <david@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx # 3.4+
> Reported-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>

>  /* 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;
>  }