KASAN: use-after-free Read in kvm_put_kvm

[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

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+f87f60...@syzkaller.appspotmail.com

L1TF CPU bug present and SMT on, data leak possible. See CVE-2018-3646 and
https://www.kernel.org/doc/html/latest/admin-guide/l1tf.html for details.
==================================================================
BUG: KASAN: use-after-free in kvm_iodevice_destructor
include/kvm/iodev.h:72 [inline]
BUG: KASAN: use-after-free in kvm_io_bus_destroy
arch/x86/kvm/../../../virt/kvm/kvm_main.c:3401 [inline]
BUG: KASAN: use-after-free in kvm_destroy_vm
arch/x86/kvm/../../../virt/kvm/kvm_main.c:740 [inline]
BUG: KASAN: use-after-free in kvm_put_kvm+0xd7c/0xff0
arch/x86/kvm/../../../virt/kvm/kvm_main.c:770
Read of size 8 at addr ffff8801cd479910 by task syz-executor822/5583

CPU: 0 PID: 5583 Comm: syz-executor822 Not tainted
4.19.0-rc8-next-20181019+ #98
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x244/0x39d lib/dump_stack.c:113
print_address_description.cold.7+0x9/0x1ff mm/kasan/report.c:256
kasan_report_error mm/kasan/report.c:354 [inline]
kasan_report.cold.8+0x242/0x309 mm/kasan/report.c:412
__asan_report_load8_noabort+0x14/0x20 mm/kasan/report.c:433
kvm_iodevice_destructor include/kvm/iodev.h:72 [inline]
kvm_io_bus_destroy arch/x86/kvm/../../../virt/kvm/kvm_main.c:3401 [inline]
kvm_destroy_vm arch/x86/kvm/../../../virt/kvm/kvm_main.c:740 [inline]
kvm_put_kvm+0xd7c/0xff0 arch/x86/kvm/../../../virt/kvm/kvm_main.c:770
kvm_vm_release+0x42/0x50 arch/x86/kvm/../../../virt/kvm/kvm_main.c:781
__fput+0x3bc/0xa70 fs/file_table.c:279
____fput+0x15/0x20 fs/file_table.c:312
task_work_run+0x1e8/0x2a0 kernel/task_work.c:113
exit_task_work include/linux/task_work.h:22 [inline]
do_exit+0x1ad1/0x26d0 kernel/exit.c:867
do_group_exit+0x177/0x440 kernel/exit.c:970
__do_sys_exit_group kernel/exit.c:981 [inline]
__se_sys_exit_group kernel/exit.c:979 [inline]
__x64_sys_exit_group+0x3e/0x50 kernel/exit.c:979
do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x43ecf8
Code: Bad RIP value.
RSP: 002b:00007ffdc564f908 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 000000000043ecf8
RDX: 0000000000000000 RSI: 000000000000003c RDI: 0000000000000000
RBP: 00000000004be5a8 R08: 00000000000000e7 R09: ffffffffffffffd0
R10: 00000000004002c8 R11: 0000000000000246 R12: 0000000000000001
R13: 00000000006d0180 R14: 0000000000000000 R15: 0000000000000000

Allocated by task 5583:
save_stack+0x43/0xd0 mm/kasan/kasan.c:448
set_track mm/kasan/kasan.c:460 [inline]
kasan_kmalloc+0xc7/0xe0 mm/kasan/kasan.c:553
kmem_cache_alloc_trace+0x152/0x750 mm/slab.c:3620
kmalloc include/linux/slab.h:546 [inline]
kzalloc include/linux/slab.h:741 [inline]
kvm_vm_ioctl_register_coalesced_mmio+0xe8/0x4f0
arch/x86/kvm/../../../virt/kvm/coalesced_mmio.c:147
kvm_vm_ioctl+0x594/0x1d60 arch/x86/kvm/../../../virt/kvm/kvm_main.c:3014
vfs_ioctl fs/ioctl.c:46 [inline]
file_ioctl fs/ioctl.c:501 [inline]
do_vfs_ioctl+0x1de/0x1720 fs/ioctl.c:685
ksys_ioctl+0xa9/0xd0 fs/ioctl.c:702
__do_sys_ioctl fs/ioctl.c:709 [inline]
__se_sys_ioctl fs/ioctl.c:707 [inline]
__x64_sys_ioctl+0x73/0xb0 fs/ioctl.c:707
do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe

Freed by task 5583:
save_stack+0x43/0xd0 mm/kasan/kasan.c:448
set_track mm/kasan/kasan.c:460 [inline]
__kasan_slab_free+0x102/0x150 mm/kasan/kasan.c:521
kasan_slab_free+0xe/0x10 mm/kasan/kasan.c:528
__cache_free mm/slab.c:3498 [inline]
kfree+0xcf/0x230 mm/slab.c:3817
coalesced_mmio_destructor+0x1ad/0x2a0
arch/x86/kvm/../../../virt/kvm/coalesced_mmio.c:99
kvm_iodevice_destructor include/kvm/iodev.h:73 [inline]
kvm_vm_ioctl_unregister_coalesced_mmio+0x263/0x330
arch/x86/kvm/../../../virt/kvm/coalesced_mmio.c:184
kvm_vm_ioctl+0x6bc/0x1d60 arch/x86/kvm/../../../virt/kvm/kvm_main.c:3023
vfs_ioctl fs/ioctl.c:46 [inline]
file_ioctl fs/ioctl.c:501 [inline]
do_vfs_ioctl+0x1de/0x1720 fs/ioctl.c:685
ksys_ioctl+0xa9/0xd0 fs/ioctl.c:702
__do_sys_ioctl fs/ioctl.c:709 [inline]
__se_sys_ioctl fs/ioctl.c:707 [inline]
__x64_sys_ioctl+0x73/0xb0 fs/ioctl.c:707
do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe

The buggy address belongs to the object at ffff8801cd479900
which belongs to the cache kmalloc-64 of size 64
The buggy address is located 16 bytes inside of
64-byte region [ffff8801cd479900, ffff8801cd479940)
The buggy address belongs to the page:
page:ffffea0007351e40 count:1 mapcount:0 mapping:ffff8801da800340 index:0x0
flags: 0x2fffc0000000200(slab)
raw: 02fffc0000000200 ffffea000736b488 ffffea000736d808 ffff8801da800340
raw: 0000000000000000 ffff8801cd479000 0000000100000020 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
ffff8801cd479800: 00 00 00 00 00 00 fc fc fc fc fc fc fc fc fc fc
ffff8801cd479880: 00 00 00 00 00 00 fc fc fc fc fc fc fc fc fc fc
> ffff8801cd479900: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
^
ffff8801cd479980: 00 00 00 00 00 00 00 00 fc fc fc fc fc fc fc fc
ffff8801cd479a00: 00 00 00 00 00 00 00 00 fc fc fc fc fc fc fc fc
==================================================================


---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with
syzbot.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches

[Paolo Bonzini]

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+f87f60...@syzkaller.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)

[Eric Biggers]

Hi Peng and Paolo,

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...@zte.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);

[peng...@zte.com.cn]

In qemu, we define different interfaces for regiter/unregister coalesced pio/mmio. For pio, call
kvm_coalesce_pio_add/del and for mmio, call kvm_coalesce_mmio_region/
kvm_uncoalesce_mmio_region. So it doesn't take into account how your test
cases are used.
Maybe it is necessary to your fix for reliability. Your fix looks good to me.
Thanks.

[Paolo Bonzini]

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

Paolo

[Eric Biggers]

From: Eric Biggers <ebig...@google.com>

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 it frees the kvm_coalesced_mmio_dev anyway, causing a
use-after-free.

Fix it by only unregistering and freeing the zone if the correct value
of 'pio' is provided.

Reported-by: syzbot+f87f60...@syzkaller.appspotmail.com
Fixes: 0804c849f1df ("kvm/x86 : add coalesced pio support")
Signed-off-by: Eric Biggers <ebig...@google.com>
---
virt/kvm/coalesced_mmio.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

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);

--
2.20.0.405.gbc1bbc6f85-goog

[Paolo Bonzini]

On 16/12/18 19:55, Eric Biggers wrote:

Thanks for the patch, I queued it.

Paolo