Re: KASAN: use-after-free Read in kvm_put_kvm (caused by: "kvm/x86: add coalesced pio support")

[Eric Biggers]

Hi Peng and Paolo,

On Sun, Oct 21, 2018 at 12:01:55PM +0200, Paolo Bonzini wrote:
> On 20/10/2018 18:57, syzbot wrote:
> > Hello,
> > 
> > syzbot found the following crash on:
> > 
> > HEAD commit:    8c60c36d0b8c Add linux-next specific files for 20181019
> > git tree:       linux-next
> > console output: https://syzkaller.appspot.com/x/log.txt?x=12d808b5400000
> > kernel config:  https://syzkaller.appspot.com/x/.config?x=8b6d7c4c81535e89
> > dashboard link:
> > https://syzkaller.appspot.com/bug?extid=f87f60bb6f13f39b54e3
> > compiler:       gcc (GCC) 8.0.1 20180413 (experimental)
> > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=17cf2f5e400000
> > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=1036dcce400000
> > 
> > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > Reported-by: syzbot+f87f60bb6f13f39b54e3@xxxxxxxxxxxxxxxxxxxxxxxxx
> 
> Tentative (untested) patch:
> 
> diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c
> index 3710342cf6ad..dc76cc8d24fd 100644
> --- a/virt/kvm/coalesced_mmio.c
> +++ b/virt/kvm/coalesced_mmio.c
> @@ -90,6 +90,7 @@ static int coalesced_mmio_write(struct kvm_vcpu *vcpu,
>  	return 0;
>  }
> 
> +/* called with kvm->slots_lock held */
>  static void coalesced_mmio_destructor(struct kvm_io_device *this)
>  {
>  	struct kvm_coalesced_mmio_dev *dev = to_mmio(this);
> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> index b20b751286fc..001e1ef18c8c 100644
> --- a/virt/kvm/eventfd.c
> +++ b/virt/kvm/eventfd.c
> @@ -741,8 +741,8 @@ ioeventfd_write(struct kvm_vcpu *vcpu, struct
> kvm_io_device *this, gpa_t addr,
>  }
> 
>  /*
> - * This function is called as KVM is completely shutting down.  We do not
> - * need to worry about locking just nuke anything we have as quickly as
> possible
> + * This function is called as KVM is completely shutting down, so there
> + * are no RCU readers anymore. Called with kvm->slots_lock held.
>   */
>  static void
>  ioeventfd_destructor(struct kvm_io_device *this)
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index aff1242b7af7..e6f2ae6fedcd 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -783,6 +783,7 @@ static void kvm_destroy_vm(struct kvm *kvm)
>  	list_del(&kvm->vm_list);
>  	spin_unlock(&kvm_lock);
>  	kvm_free_irq_routing(kvm);
> +	mutex_lock(&kvm->slots_lock);
>  	for (i = 0; i < KVM_NR_BUSES; i++) {
>  		struct kvm_io_bus *bus = kvm_get_bus(kvm, i);
> 
> @@ -790,6 +791,7 @@ static void kvm_destroy_vm(struct kvm *kvm)
>  			kvm_io_bus_destroy(bus);
>  		kvm->buses[i] = NULL;
>  	}
> +	mutex_unlock(&kvm->slots_lock);
>  	kvm_coalesced_mmio_free(kvm);
>  #ifdef KVM_DIRTY_LOG_PAGE_OFFSET
>  	if (kvm->dirty_ring_size)
> 

This is still happening.  Paolo, I don't see how your patch matches this bug, as
it has a single threaded reproducer.  I simplified it to:

	#include <fcntl.h>
	#include <linux/kvm.h>
	#include <sys/ioctl.h>

	int main(void)
	{
		int kvm, vm;
		struct kvm_coalesced_mmio_zone zone = { 0 };

		kvm = open("/dev/kvm", O_RDWR);

		vm = ioctl(kvm, KVM_CREATE_VM, 0);

		ioctl(vm, KVM_REGISTER_COALESCED_MMIO, &zone);

		zone.pio = 1;
		ioctl(vm, KVM_UNREGISTER_COALESCED_MMIO, &zone);
	}

The bug is in this commit:

	commit 0804c849f1df0992d39a37c4fc259f7f8b16f385
	Author: Peng Hao <peng.hao2@xxxxxxxxxx>
	Date:   Sun Oct 14 07:09:55 2018 +0800

	    kvm/x86 : add coalesced pio support

The problem is that if you register a kvm_coalesced_mmio_zone with '.pio = 0'
but then unregister it with '.pio = 1', KVM_UNREGISTER_COALESCED_MMIO will try
to unregister it from KVM_PIO_BUS rather than KVM_MMIO_BUS, which is a no-op,
but then it frees the kvm_coalesced_mmio_dev anyway.

Here's one possible fix but I don't know what was intended here.  Are you
supposed to pass in the same value of '.pio' when unregistering or is it
supposed to use the existing value?  Can one of you please point me to the
documentation for these ioctls?

diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c
index 3710342cf6ad0..6855cce3e5287 100644
--- a/virt/kvm/coalesced_mmio.c
+++ b/virt/kvm/coalesced_mmio.c
@@ -175,10 +175,14 @@ int kvm_vm_ioctl_unregister_coalesced_mmio(struct kvm *kvm,
 {
 	struct kvm_coalesced_mmio_dev *dev, *tmp;
 
+	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)
-		if (coalesced_mmio_in_range(dev, zone->addr, zone->size)) {
+		if (zone->pio == dev->zone.pio &&
+		    coalesced_mmio_in_range(dev, zone->addr, zone->size)) {
 			kvm_io_bus_unregister_dev(kvm,
 				zone->pio ? KVM_PIO_BUS : KVM_MMIO_BUS, &dev->dev);
 			kvm_iodevice_destructor(&dev->dev);