KASAN: use-after-free Read in kvm_put_kvm

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 [email protected]

Quoted 14 lines · syzbot

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

Quoted 2 lines

IMPORTANT: if you fix the bug, please add the following tag to the commit:

Reported-by [email protected]

Tentative (untested) patch:

virt/kvm/coalesced_mmio.c +1 -0

index 3710342cf6ad..dc76cc8d24fd 100644
@@ -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);

virt/kvm/eventfd.c +0 -2

index b20b751286fc..001e1ef18c8c 100644
@@ -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)

virt/kvm/kvm_main.c +1 -0

index aff1242b7af7..e6f2ae6fedcd 100644
@@ -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);
 

Hi Peng and Paolo,

Quoted 63 lines · Paolo Bonzini

Quoted 14 lines · syzbot

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

Quoted 2 lines

IMPORTANT: if you fix the bug, please add the following tag to the commit:

Reported-by [email protected]

Tentative (untested) patch:

virt/kvm/coalesced_mmio.c +1 -0

index 3710342cf6ad..dc76cc8d24fd 100644
@@ -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);

virt/kvm/eventfd.c +0 -2

index b20b751286fc..001e1ef18c8c 100644
@@ -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)

virt/kvm/kvm_main.c +1 -0

index aff1242b7af7..e6f2ae6fedcd 100644
@@ -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);
 

virt/kvm/kvm_main.c static void kvm_destroy_vm(struct kvm *kvm) · line 797

Quoted 7 lines

virt/kvm/kvm_main.c +1 -0

@@ -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 <[email protected]>
	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?

virt/kvm/coalesced_mmio.c +2 -0

index 3710342cf6ad0..6855cce3e5287 100644
@@ -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;

Hi Peng and Paolo,

Quoted 63 lines · Paolo Bonzini

Quoted 14 lines · syzbot

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

Quoted 2 lines

IMPORTANT: if you fix the bug, please add the following tag to the commit:

Reported-by [email protected]

Tentative (untested) patch:

virt/kvm/coalesced_mmio.c +1 -0

index 3710342cf6ad..dc76cc8d24fd 100644
@@ -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);

virt/kvm/eventfd.c +0 -2

index b20b751286fc..001e1ef18c8c 100644
@@ -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)

virt/kvm/kvm_main.c +1 -0

index aff1242b7af7..e6f2ae6fedcd 100644
@@ -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);
 

virt/kvm/kvm_main.c static void kvm_destroy_vm(struct kvm *kvm) · line 797

Quoted 7 lines

virt/kvm/kvm_main.c +1 -0

@@ -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 <[email protected]>
	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?

virt/kvm/coalesced_mmio.c +2 -0

index 3710342cf6ad0..6855cce3e5287 100644
@@ -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;