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

[Paolo Bonzini]

On 16/12/18 19:55, Eric Biggers wrote:
> 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);
> 

Yes, this is the fix.  The same range can be registered both with .pio =
0 and .pio = 1.

Paolo