| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6d15db4f-8017-97f2-6d31-3640f67f86ba@redhat.com>
Date: Tue, 18 Dec 2018 22:03:39 +0100
From: Paolo Bonzini <pbonzini@...hat.com>
To: Eric Biggers <ebiggers@...nel.org>, Peng Hao <peng.hao2@....com.cn>
Cc: syzbot <syzbot+f87f60bb6f13f39b54e3@...kaller.appspotmail.com>,
kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
rkrcmar@...hat.com, syzkaller-bugs@...glegroups.com
Subject: Re: KASAN: use-after-free Read in kvm_put_kvm (caused by: "kvm/x86:
add coalesced pio support")
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@...kaller.appspotmail.com
>>
>> 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@....com.cn>
> 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);
>
Thanks for the patch, I queued it.
Paolo
Powered by blists - more mailing lists