Re: FAILED: patch "[PATCH] KVM: kvm_io_bus_unregister_dev() should never fail" failed to apply to 4.10-stable tree

[David Hildenbrand <david@redhat.com>]

On 30.03.2017 10:00, gregkh@linuxfoundation.org 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@vger.kernel.org>.
> 

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?

Thanks!

> thanks,
> 
> greg k-h
> 
> ------------------ original commit in Linus's tree ------------------
> 
> From 90db10434b163e46da413d34db8d0e77404cc645 Mon Sep 17 00:00:00 2001
> From: David Hildenbrand <david@redhat.com>
> 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@vger.kernel.org # 3.4+
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> 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)
>