[PATCH 0/7] KVM: syzkaller fixes

[PATCH 0/7] KVM: syzkaller fixes

[Paolo Bonzini]

These fix most of the bugs reported by Dmitry Vyukov a while back.
I couldn't reproduce one of the bugs (patch 7) but the fix is easy.
Probably, more VM ioctls should take kvm->lock, but I have not looked
at it yet.

I have only marked for stable the two patches that fix an oops.  However,
all of patches 1-6 could go in 4.7-rc, I think.

Thanks,

Paolo

Paolo Bonzini (7):
  kvm: x86: avoid warning on repeated KVM_SET_TSS_ADDR
  KVM: x86: avoid vmalloc(0) in the KVM_SET_CPUID
  KVM: fail KVM_SET_VCPU_EVENTS with invalid exception number
  KVM: irqfd: fix NULL pointer dereference in kvm_irq_map_gsi
  KVM: x86: avoid vmalloc(0) in the KVM_SET_CPUID
  KVM: x86: fix OOPS after invalid KVM_SET_DEBUGREGS
  KVM: x86: protect KVM_CREATE_PIT/KVM_CREATE_PIT2 with kvm->lock

 arch/x86/kvm/cpuid.c |   22 ++++++++++++----------
 arch/x86/kvm/i8254.c |    4 +++-
 arch/x86/kvm/x86.c   |   15 ++++++++++++---
 virt/kvm/irqchip.c   |    2 +-
 virt/kvm/kvm_main.c  |   22 ++++++++++++----------
 5 files changed, 40 insertions(+), 25 deletions(-)

-- 
1.8.3.1

[PATCH 1/7] kvm: x86: avoid warning on repeated KVM_SET_TSS_ADDR

[Paolo Bonzini]

Found by syzkaller:

    WARNING: CPU: 3 PID: 15175 at arch/x86/kvm/x86.c:7705 __x86_set_memory_region+0x1dc/0x1f0 [kvm]()
    CPU: 3 PID: 15175 Comm: a.out Tainted: G        W       4.4.6-300.fc23.x86_64 #1
    Hardware name: LENOVO 2325F51/2325F51, BIOS G2ET32WW (1.12 ) 05/30/2012
     0000000000000286 00000000950899a7 ffff88011ab3fbf0 ffffffff813b542e
     0000000000000000 ffffffffa0966496 ffff88011ab3fc28 ffffffff810a40f2
     00000000000001fd 0000000000003000 ffff88014fc50000 0000000000000000
    Call Trace:
     [<ffffffff813b542e>] dump_stack+0x63/0x85
     [<ffffffff810a40f2>] warn_slowpath_common+0x82/0xc0
     [<ffffffff810a423a>] warn_slowpath_null+0x1a/0x20
     [<ffffffffa09251cc>] __x86_set_memory_region+0x1dc/0x1f0 [kvm]
     [<ffffffffa092521b>] x86_set_memory_region+0x3b/0x60 [kvm]
     [<ffffffffa09bb61c>] vmx_set_tss_addr+0x3c/0x150 [kvm_intel]
     [<ffffffffa092f4d4>] kvm_arch_vm_ioctl+0x654/0xbc0 [kvm]
     [<ffffffffa091d31a>] kvm_vm_ioctl+0x9a/0x6f0 [kvm]
     [<ffffffff81241248>] do_vfs_ioctl+0x298/0x480
     [<ffffffff812414a9>] SyS_ioctl+0x79/0x90
     [<ffffffff817a04ee>] entry_SYSCALL_64_fastpath+0x12/0x71

Testcase:

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

    long r[8];

    int main()
    {
        memset(r, -1, sizeof(r));
	r[2] = open("/dev/kvm", O_RDONLY|O_TRUNC);
        r[3] = ioctl(r[2], KVM_CREATE_VM, 0x0ul);
        r[5] = ioctl(r[3], KVM_SET_TSS_ADDR, 0x20000000ul);
        r[7] = ioctl(r[3], KVM_SET_TSS_ADDR, 0x20000000ul);
        return 0;
    }

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/x86.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 199a87c20a98..6c9793c64522 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7828,7 +7828,7 @@ int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size)
 
 	slot = id_to_memslot(slots, id);
 	if (size) {
-		if (WARN_ON(slot->npages))
+		if (slot->npages)
 			return -EEXIST;
 
 		/*
-- 
1.8.3.1

[PATCH 2/7] KVM: x86: avoid vmalloc(0) in the KVM_SET_CPUID

[Paolo Bonzini]

This causes an ugly dmesg splat.  Beautified syzkaller testcase:

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

    long r[8];

    int main()
    {
        struct kvm_cpuid2 c = { 0 };
        r[2] = open("/dev/kvm", O_RDWR);
        r[3] = ioctl(r[2], KVM_CREATE_VM, 0);
        r[4] = ioctl(r[3], KVM_CREATE_VCPU, 0x8);
        r[7] = ioctl(r[4], KVM_SET_CPUID, &c);
        return 0;
    }

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/cpuid.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 769af907f824..7597b42a8a88 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -181,19 +181,22 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
 			     struct kvm_cpuid_entry __user *entries)
 {
 	int r, i;
-	struct kvm_cpuid_entry *cpuid_entries;
+	struct kvm_cpuid_entry *cpuid_entries = NULL;
 
 	r = -E2BIG;
 	if (cpuid->nent > KVM_MAX_CPUID_ENTRIES)
 		goto out;
 	r = -ENOMEM;
-	cpuid_entries = vmalloc(sizeof(struct kvm_cpuid_entry) * cpuid->nent);
-	if (!cpuid_entries)
-		goto out;
-	r = -EFAULT;
-	if (copy_from_user(cpuid_entries, entries,
-			   cpuid->nent * sizeof(struct kvm_cpuid_entry)))
-		goto out_free;
+	if (cpuid->nent) {
+		cpuid_entries = vmalloc(sizeof(struct kvm_cpuid_entry) *
+					cpuid->nent);
+		if (!cpuid_entries)
+			goto out;
+		r = -EFAULT;
+		if (copy_from_user(cpuid_entries, entries,
+				   cpuid->nent * sizeof(struct kvm_cpuid_entry)))
+			goto out;
+	}
 	for (i = 0; i < cpuid->nent; i++) {
 		vcpu->arch.cpuid_entries[i].function = cpuid_entries[i].function;
 		vcpu->arch.cpuid_entries[i].eax = cpuid_entries[i].eax;
@@ -212,9 +215,8 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
 	kvm_x86_ops->cpuid_update(vcpu);
 	r = kvm_update_cpuid(vcpu);
 
-out_free:
-	vfree(cpuid_entries);
 out:
+	vfree(cpuid_entries);
 	return r;
 }
 
-- 
1.8.3.1

[PATCH 3/7] KVM: fail KVM_SET_VCPU_EVENTS with invalid exception number

[Paolo Bonzini]

This cannot be returned by KVM_GET_VCPU_EVENTS, so it is okay to return
EINVAL.  It causes a WARN from exception_type:

    WARNING: CPU: 3 PID: 16732 at arch/x86/kvm/x86.c:345 exception_type+0x49/0x50 [kvm]()
    CPU: 3 PID: 16732 Comm: a.out Tainted: G        W       4.4.6-300.fc23.x86_64 #1
    Hardware name: LENOVO 2325F51/2325F51, BIOS G2ET32WW (1.12 ) 05/30/2012
     0000000000000286 000000006308a48b ffff8800bec7fcf8 ffffffff813b542e
     0000000000000000 ffffffffa0966496 ffff8800bec7fd30 ffffffff810a40f2
     ffff8800552a8000 0000000000000000 00000000002c267c 0000000000000001
    Call Trace:
     [<ffffffff813b542e>] dump_stack+0x63/0x85
     [<ffffffff810a40f2>] warn_slowpath_common+0x82/0xc0
     [<ffffffff810a423a>] warn_slowpath_null+0x1a/0x20
     [<ffffffffa0924809>] exception_type+0x49/0x50 [kvm]
     [<ffffffffa0934622>] kvm_arch_vcpu_ioctl_run+0x10a2/0x14e0 [kvm]
     [<ffffffffa091c04d>] kvm_vcpu_ioctl+0x33d/0x620 [kvm]
     [<ffffffff81241248>] do_vfs_ioctl+0x298/0x480
     [<ffffffff812414a9>] SyS_ioctl+0x79/0x90
     [<ffffffff817a04ee>] entry_SYSCALL_64_fastpath+0x12/0x71
    ---[ end trace b1a0391266848f50 ]---

Testcase (beautified/reduced from syzkaller output):

    #include <unistd.h>
    #include <sys/syscall.h>
    #include <string.h>
    #include <stdint.h>
    #include <fcntl.h>
    #include <sys/ioctl.h>
    #include <linux/kvm.h>

    long r[31];

    int main()
    {
        memset(r, -1, sizeof(r));
        r[2] = open("/dev/kvm", O_RDONLY);
        r[3] = ioctl(r[2], KVM_CREATE_VM, 0);
        r[7] = ioctl(r[3], KVM_CREATE_VCPU, 0);

        struct kvm_vcpu_events ve = {
                .exception.injected = 1,
                .exception.nr = 0xd4
        };
        r[27] = ioctl(r[7], KVM_SET_VCPU_EVENTS, &ve);
        r[30] = ioctl(r[7], KVM_RUN, 0);
        return 0;
    }

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/x86.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 6c9793c64522..71284533f0b7 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2973,6 +2973,10 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
 			      | KVM_VCPUEVENT_VALID_SMM))
 		return -EINVAL;
 
+	if (events->exception.injected &&
+	    (events->exception.nr > 31 || events->exception.nr == NMI_VECTOR))
+		return -EINVAL;
+
 	process_nmi(vcpu);
 	vcpu->arch.exception.pending = events->exception.injected;
 	vcpu->arch.exception.nr = events->exception.nr;
-- 
1.8.3.1

[PATCH 4/7] KVM: irqfd: fix NULL pointer dereference in kvm_irq_map_gsi

[Paolo Bonzini]

Found by syzkaller:

    BUG: unable to handle kernel NULL pointer dereference at 0000000000000120
    IP: [<ffffffffa0797202>] kvm_irq_map_gsi+0x12/0x90 [kvm]
    PGD 6f80b067 PUD b6535067 PMD 0
    Oops: 0000 [#1] SMP
    CPU: 3 PID: 4988 Comm: a.out Not tainted 4.4.9-300.fc23.x86_64 #1
    [...]
    Call Trace:
     [<ffffffffa0795f62>] irqfd_update+0x32/0xc0 [kvm]
     [<ffffffffa0796c7c>] kvm_irqfd+0x3dc/0x5b0 [kvm]
     [<ffffffffa07943f4>] kvm_vm_ioctl+0x164/0x6f0 [kvm]
     [<ffffffff81241648>] do_vfs_ioctl+0x298/0x480
     [<ffffffff812418a9>] SyS_ioctl+0x79/0x90
     [<ffffffff817a1062>] tracesys_phase2+0x84/0x89
    Code: b5 71 a7 e0 5b 41 5c 41 5d 5d f3 c3 66 66 66 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 55 48 8b 8f 10 2e 00 00 31 c0 48 89 e5 <39> 91 20 01 00 00 76 6a 48 63 d2 48 8b 94 d1 28 01 00 00 48 85
    RIP  [<ffffffffa0797202>] kvm_irq_map_gsi+0x12/0x90 [kvm]
     RSP <ffff8800926cbca8>
    CR2: 0000000000000120

Testcase:

    #include <unistd.h>
    #include <sys/syscall.h>
    #include <string.h>
    #include <stdint.h>
    #include <linux/kvm.h>
    #include <fcntl.h>
    #include <sys/ioctl.h>

    long r[26];

    int main()
    {
        memset(r, -1, sizeof(r));
        r[2] = open("/dev/kvm", 0);
        r[3] = ioctl(r[2], KVM_CREATE_VM, 0);

        struct kvm_irqfd ifd;
        ifd.fd = syscall(SYS_eventfd2, 5, 0);
        ifd.gsi = 3;
        ifd.flags = 2;
        ifd.resamplefd = ifd.fd;
        r[25] = ioctl(r[3], KVM_IRQFD, &ifd);
        return 0;
    }

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Cc: stable@vger.kernel.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 virt/kvm/irqchip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
index fe84e1a95dd5..8db197bb6c7a 100644
--- a/virt/kvm/irqchip.c
+++ b/virt/kvm/irqchip.c
@@ -40,7 +40,7 @@ int kvm_irq_map_gsi(struct kvm *kvm,
 
 	irq_rt = srcu_dereference_check(kvm->irq_routing, &kvm->irq_srcu,
 					lockdep_is_held(&kvm->irq_lock));
-	if (gsi < irq_rt->nr_rt_entries) {
+	if (irq_rt && gsi < irq_rt->nr_rt_entries) {
 		hlist_for_each_entry(e, &irq_rt->map[gsi], link) {
 			entries[n] = *e;
 			++n;
-- 
1.8.3.1

[PATCH 5/7] KVM: x86: avoid vmalloc(0) in the KVM_SET_CPUID

[Paolo Bonzini]

This causes an ugly dmesg splat.  Beautified syzkaller testcase:

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

    long r[8];

    int main()
    {
        struct kvm_irq_routing ir = { 0 };
        r[2] = open("/dev/kvm", O_RDWR);
        r[3] = ioctl(r[2], KVM_CREATE_VM, 0);
        r[4] = ioctl(r[3], KVM_SET_GSI_ROUTING, &ir);
        return 0;
    }

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 virt/kvm/kvm_main.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 37af23052470..02e98f3131bd 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2935,7 +2935,7 @@ static long kvm_vm_ioctl(struct file *filp,
 	case KVM_SET_GSI_ROUTING: {
 		struct kvm_irq_routing routing;
 		struct kvm_irq_routing __user *urouting;
-		struct kvm_irq_routing_entry *entries;
+		struct kvm_irq_routing_entry *entries = NULL;
 
 		r = -EFAULT;
 		if (copy_from_user(&routing, argp, sizeof(routing)))
@@ -2945,15 +2945,17 @@ static long kvm_vm_ioctl(struct file *filp,
 			goto out;
 		if (routing.flags)
 			goto out;
-		r = -ENOMEM;
-		entries = vmalloc(routing.nr * sizeof(*entries));
-		if (!entries)
-			goto out;
-		r = -EFAULT;
-		urouting = argp;
-		if (copy_from_user(entries, urouting->entries,
-				   routing.nr * sizeof(*entries)))
-			goto out_free_irq_routing;
+		if (routing.nr) {
+			r = -ENOMEM;
+			entries = vmalloc(routing.nr * sizeof(*entries));
+			if (!entries)
+				goto out;
+			r = -EFAULT;
+			urouting = argp;
+			if (copy_from_user(entries, urouting->entries,
+					   routing.nr * sizeof(*entries)))
+				goto out_free_irq_routing;
+		}
 		r = kvm_set_irq_routing(kvm, entries, routing.nr,
 					routing.flags);
 out_free_irq_routing:
-- 
1.8.3.1

[PATCH 6/7] KVM: x86: fix OOPS after invalid KVM_SET_DEBUGREGS

[Paolo Bonzini]

MOV to DR6 or DR7 causes a #GP if an attempt is made to write a 1 to
any of bits 63:32.  However, this is not detected at KVM_SET_DEBUGREGS
time, and the next KVM_RUN oopses:

   general protection fault: 0000 [#1] SMP
   CPU: 2 PID: 14987 Comm: a.out Not tainted 4.4.9-300.fc23.x86_64 #1
   Hardware name: LENOVO 2325F51/2325F51, BIOS G2ET32WW (1.12 ) 05/30/2012
   [...]
   Call Trace:
    [<ffffffffa072c93d>] kvm_arch_vcpu_ioctl_run+0x141d/0x14e0 [kvm]
    [<ffffffffa071405d>] kvm_vcpu_ioctl+0x33d/0x620 [kvm]
    [<ffffffff81241648>] do_vfs_ioctl+0x298/0x480
    [<ffffffff812418a9>] SyS_ioctl+0x79/0x90
    [<ffffffff817a0f2e>] entry_SYSCALL_64_fastpath+0x12/0x71
   Code: 55 83 ff 07 48 89 e5 77 27 89 ff ff 24 fd 90 87 80 81 0f 23 fe 5d c3 0f 23 c6 5d c3 0f 23 ce 5d c3 0f 23 d6 5d c3 0f 23 de 5d c3 <0f> 23 f6 5d c3 0f 0b 66 66 66 66 66 2e 0f 1f 84 00 00 00 00 00
   RIP  [<ffffffff810639eb>] native_set_debugreg+0x2b/0x40
    RSP <ffff88005836bd50>

Testcase (beautified/reduced from syzkaller output):

    #include <unistd.h>
    #include <sys/syscall.h>
    #include <string.h>
    #include <stdint.h>
    #include <linux/kvm.h>
    #include <fcntl.h>
    #include <sys/ioctl.h>

    long r[8];

    int main()
    {
        struct kvm_debugregs dr = { 0 };

        r[2] = open("/dev/kvm", O_RDONLY);
        r[3] = ioctl(r[2], KVM_CREATE_VM, 0);
        r[4] = ioctl(r[3], KVM_CREATE_VCPU, 7);

        memcpy(&dr,
               "\x5d\x6a\x6b\xe8\x57\x3b\x4b\x7e\xcf\x0d\xa1\x72"
               "\xa3\x4a\x29\x0c\xfc\x6d\x44\x00\xa7\x52\xc7\xd8"
               "\x00\xdb\x89\x9d\x78\xb5\x54\x6b\x6b\x13\x1c\xe9"
               "\x5e\xd3\x0e\x40\x6f\xb4\x66\xf7\x5b\xe3\x36\xcb",
               48);
        r[7] = ioctl(r[4], KVM_SET_DEBUGREGS, &dr);
        r[6] = ioctl(r[4], KVM_RUN, 0);
    }

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Cc: stable@vger.kernel.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/x86.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 71284533f0b7..3fbf88f85a2d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3041,6 +3041,11 @@ static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
 	if (dbgregs->flags)
 		return -EINVAL;
 
+	if (dbgregs->dr6 & ~0xffffffffull)
+		return -EINVAL;
+	if (dbgregs->dr7 & ~0xffffffffull)
+		return -EINVAL;
+
 	memcpy(vcpu->arch.db, dbgregs->db, sizeof(vcpu->arch.db));
 	kvm_update_dr0123(vcpu);
 	vcpu->arch.dr6 = dbgregs->dr6;
-- 
1.8.3.1

[PATCH 7/7] KVM: x86: protect KVM_CREATE_PIT/KVM_CREATE_PIT2 with kvm->lock

[Paolo Bonzini]

The syzkaller folks reported a NULL pointer dereference that seems
to be cause by a race between KVM_CREATE_IRQCHIP and KVM_CREATE_PIT2.
The former takes kvm->lock (except when registering the devices,
which needs kvm->slots_lock); the latter takes kvm->slots_lock only.
Change KVM_CREATE_PIT2 to follow the same model as KVM_CREATE_IRQCHIP.

Testcase:

    #include <pthread.h>
    #include <linux/kvm.h>
    #include <fcntl.h>
    #include <sys/ioctl.h>
    #include <stdint.h>
    #include <string.h>
    #include <stdlib.h>
    #include <sys/syscall.h>
    #include <unistd.h>

    long r[23];

    void* thr1(void* arg)
    {
        struct kvm_pit_config pitcfg = { .flags = 4 };
        switch ((long)arg) {
        case 0: r[2]  = open("/dev/kvm", O_RDONLY|O_ASYNC);    break;
        case 1: r[3]  = ioctl(r[2], KVM_CREATE_VM, 0);         break;
        case 2: r[4]  = ioctl(r[3], KVM_CREATE_IRQCHIP, 0);    break;
        case 3: r[22] = ioctl(r[3], KVM_CREATE_PIT2, &pitcfg); break;
        }
        return 0;
    }

    int main(int argc, char **argv)
    {
        long i;
        pthread_t th[4];

        memset(r, -1, sizeof(r));
        for (i = 0; i < 4; i++) {
            pthread_create(&th[i], 0, thr, (void*)i);
            if (argc > 1 && rand()%2) usleep(rand()%1000);
        }
        usleep(20000);
        return 0;
    }

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/i8254.c | 4 +++-
 arch/x86/kvm/x86.c   | 4 ++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
index a4bf5b45d65a..5fb6c620180e 100644
--- a/arch/x86/kvm/i8254.c
+++ b/arch/x86/kvm/i8254.c
@@ -645,7 +645,6 @@ static const struct kvm_io_device_ops speaker_dev_ops = {
 	.write    = speaker_ioport_write,
 };
 
-/* Caller must hold slots_lock */
 struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags)
 {
 	struct kvm_pit *pit;
@@ -690,6 +689,7 @@ struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags)
 
 	kvm_pit_set_reinject(pit, true);
 
+	mutex_lock(&kvm->slots_lock);
 	kvm_iodevice_init(&pit->dev, &pit_dev_ops);
 	ret = kvm_io_bus_register_dev(kvm, KVM_PIO_BUS, KVM_PIT_BASE_ADDRESS,
 				      KVM_PIT_MEM_LENGTH, &pit->dev);
@@ -704,12 +704,14 @@ struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags)
 		if (ret < 0)
 			goto fail_register_speaker;
 	}
+	mutex_unlock(&kvm->slots_lock);
 
 	return pit;
 
 fail_register_speaker:
 	kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS, &pit->dev);
 fail_register_pit:
+	mutex_unlock(&kvm->slots_lock);
 	kvm_pit_set_reinject(pit, false);
 	kthread_stop(pit->worker_task);
 fail_kthread:
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 3fbf88f85a2d..99bb0cf93747 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3878,7 +3878,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 				   sizeof(struct kvm_pit_config)))
 			goto out;
 	create_pit:
-		mutex_lock(&kvm->slots_lock);
+		mutex_lock(&kvm->lock);
 		r = -EEXIST;
 		if (kvm->arch.vpit)
 			goto create_pit_unlock;
@@ -3887,7 +3887,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		if (kvm->arch.vpit)
 			r = 0;
 	create_pit_unlock:
-		mutex_unlock(&kvm->slots_lock);
+		mutex_unlock(&kvm->lock);
 		break;
 	case KVM_GET_IRQCHIP: {
 		/* 0: PIC master, 1: PIC slave, 2: IOAPIC */
-- 
1.8.3.1

Re: [PATCH 0/7] KVM: syzkaller fixes

[Radim Krčmář]

2016-06-01 14:09+0200, Paolo Bonzini:
> These fix most of the bugs reported by Dmitry Vyukov a while back.
> I couldn't reproduce one of the bugs (patch 7) but the fix is easy.
> Probably, more VM ioctls should take kvm->lock, but I have not looked
> at it yet.

Applied, thanks.

> I have only marked for stable the two patches that fix an oops.  However,
> all of patches 1-6 could go in 4.7-rc, I think.

Normal userspaces don't exercise modified paths, so I'll prepare [1-6/7]
for -rc1, as thorough testing wouldn't make a difference ...

Re: [PATCH 5/7] KVM: x86: avoid vmalloc(0) in the KVM_SET_CPUID

[Wanpeng Li]

2016-06-01 20:09 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
> This causes an ugly dmesg splat.  Beautified syzkaller testcase:
>
>     #include <unistd.h>
>     #include <sys/syscall.h>
>     #include <sys/ioctl.h>
>     #include <fcntl.h>
>     #include <linux/kvm.h>
>
>     long r[8];
>
>     int main()
>     {
>         struct kvm_irq_routing ir = { 0 };
>         r[2] = open("/dev/kvm", O_RDWR);
>         r[3] = ioctl(r[2], KVM_CREATE_VM, 0);
>         r[4] = ioctl(r[3], KVM_SET_GSI_ROUTING, &ir);
>         return 0;
>     }
>

The patch subject is not correct.

Regards,
Wanpeng Li

Re: [PATCH 4/7] KVM: irqfd: fix NULL pointer dereference in kvm_irq_map_gsi

[Steve Rutherford]

Doesn't this same bug also affect kvm_irq_map_chip_pin?

(Note: I haven't chased down whether anything can even reach here if
irq_rt is NULL.)

int kvm_irq_map_chip_pin(struct kvm *kvm, unsigned irqchip, unsigned pin)
{
        struct kvm_irq_routing_table *irq_rt;

        irq_rt = srcu_dereference(kvm->irq_routing, &kvm->irq_srcu);
+
+      if (!irq_rt)
+              return -1;
+
        return irq_rt->chip[irqchip][pin];
}

Steve

On Wed, Jun 1, 2016 at 5:09 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Found by syzkaller:
>
>     BUG: unable to handle kernel NULL pointer dereference at 0000000000000120
>     IP: [<ffffffffa0797202>] kvm_irq_map_gsi+0x12/0x90 [kvm]
>     PGD 6f80b067 PUD b6535067 PMD 0
>     Oops: 0000 [#1] SMP
>     CPU: 3 PID: 4988 Comm: a.out Not tainted 4.4.9-300.fc23.x86_64 #1
>     [...]
>     Call Trace:
>      [<ffffffffa0795f62>] irqfd_update+0x32/0xc0 [kvm]
>      [<ffffffffa0796c7c>] kvm_irqfd+0x3dc/0x5b0 [kvm]
>      [<ffffffffa07943f4>] kvm_vm_ioctl+0x164/0x6f0 [kvm]
>      [<ffffffff81241648>] do_vfs_ioctl+0x298/0x480
>      [<ffffffff812418a9>] SyS_ioctl+0x79/0x90
>      [<ffffffff817a1062>] tracesys_phase2+0x84/0x89
>     Code: b5 71 a7 e0 5b 41 5c 41 5d 5d f3 c3 66 66 66 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 55 48 8b 8f 10 2e 00 00 31 c0 48 89 e5 <39> 91 20 01 00 00 76 6a 48 63 d2 48 8b 94 d1 28 01 00 00 48 85
>     RIP  [<ffffffffa0797202>] kvm_irq_map_gsi+0x12/0x90 [kvm]
>      RSP <ffff8800926cbca8>
>     CR2: 0000000000000120
>
> Testcase:
>
>     #include <unistd.h>
>     #include <sys/syscall.h>
>     #include <string.h>
>     #include <stdint.h>
>     #include <linux/kvm.h>
>     #include <fcntl.h>
>     #include <sys/ioctl.h>
>
>     long r[26];
>
>     int main()
>     {
>         memset(r, -1, sizeof(r));
>         r[2] = open("/dev/kvm", 0);
>         r[3] = ioctl(r[2], KVM_CREATE_VM, 0);
>
>         struct kvm_irqfd ifd;
>         ifd.fd = syscall(SYS_eventfd2, 5, 0);
>         ifd.gsi = 3;
>         ifd.flags = 2;
>         ifd.resamplefd = ifd.fd;
>         r[25] = ioctl(r[3], KVM_IRQFD, &ifd);
>         return 0;
>     }
>
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  virt/kvm/irqchip.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
> index fe84e1a95dd5..8db197bb6c7a 100644
> --- a/virt/kvm/irqchip.c
> +++ b/virt/kvm/irqchip.c
> @@ -40,7 +40,7 @@ int kvm_irq_map_gsi(struct kvm *kvm,
>
>         irq_rt = srcu_dereference_check(kvm->irq_routing, &kvm->irq_srcu,
>                                         lockdep_is_held(&kvm->irq_lock));
> -       if (gsi < irq_rt->nr_rt_entries) {
> +       if (irq_rt && gsi < irq_rt->nr_rt_entries) {
>                 hlist_for_each_entry(e, &irq_rt->map[gsi], link) {
>                         entries[n] = *e;
>                         ++n;
> --
> 1.8.3.1
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] KVM: irqfd: fix NULL pointer dereference in kvm_irq_map_gsi

[Paolo Bonzini]

> Doesn't this same bug also affect kvm_irq_map_chip_pin?
> 
> (Note: I haven't chased down whether anything can even reach here if
> irq_rt is NULL.)

No, it is safe because kvm_irq_map_chip_pin requires having created an
irqchip, and:

- host code (kvm_set_irq) in virt/kvm/irqchip.c goes through kvm_irq_map_gsi,
and until you have a successful kvm_irq_map_gsi you cannot get to
kvm_notify_acked_irq

- guest code (such as ioapic_write_indirect) can call kvm_irq_map_chip_pin,
but it cannot run until KVM_CREATE_IRQCHIP returns.  KVM_CREATE_IRQCHIP
requires that there are no VCPUs, and kvm->lock protects both KVM_CREATE_IRQCHIP
and (the first part of) KVM_CREATE_VCPU.

Maybe it was possible, but very unlikely, before commit 557abc40d121
("KVM: remove kvm_vcpu_compatible", 2016-06-16).

Paolo

> int kvm_irq_map_chip_pin(struct kvm *kvm, unsigned irqchip, unsigned pin)
> {
>         struct kvm_irq_routing_table *irq_rt;
> 
>         irq_rt = srcu_dereference(kvm->irq_routing, &kvm->irq_srcu);
> +
> +      if (!irq_rt)
> +              return -1;
> +
>         return irq_rt->chip[irqchip][pin];
> }
> 
> Steve
> 
> On Wed, Jun 1, 2016 at 5:09 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > Found by syzkaller:
> >
> >     BUG: unable to handle kernel NULL pointer dereference at
> >     0000000000000120
> >     IP: [<ffffffffa0797202>] kvm_irq_map_gsi+0x12/0x90 [kvm]
> >     PGD 6f80b067 PUD b6535067 PMD 0
> >     Oops: 0000 [#1] SMP
> >     CPU: 3 PID: 4988 Comm: a.out Not tainted 4.4.9-300.fc23.x86_64 #1
> >     [...]
> >     Call Trace:
> >      [<ffffffffa0795f62>] irqfd_update+0x32/0xc0 [kvm]
> >      [<ffffffffa0796c7c>] kvm_irqfd+0x3dc/0x5b0 [kvm]
> >      [<ffffffffa07943f4>] kvm_vm_ioctl+0x164/0x6f0 [kvm]
> >      [<ffffffff81241648>] do_vfs_ioctl+0x298/0x480
> >      [<ffffffff812418a9>] SyS_ioctl+0x79/0x90
> >      [<ffffffff817a1062>] tracesys_phase2+0x84/0x89
> >     Code: b5 71 a7 e0 5b 41 5c 41 5d 5d f3 c3 66 66 66 66 2e 0f 1f 84 00 00
> >     00 00 00 0f 1f 44 00 00 55 48 8b 8f 10 2e 00 00 31 c0 48 89 e5 <39> 91
> >     20 01 00 00 76 6a 48 63 d2 48 8b 94 d1 28 01 00 00 48 85
> >     RIP  [<ffffffffa0797202>] kvm_irq_map_gsi+0x12/0x90 [kvm]
> >      RSP <ffff8800926cbca8>
> >     CR2: 0000000000000120
> >
> > Testcase:
> >
> >     #include <unistd.h>
> >     #include <sys/syscall.h>
> >     #include <string.h>
> >     #include <stdint.h>
> >     #include <linux/kvm.h>
> >     #include <fcntl.h>
> >     #include <sys/ioctl.h>
> >
> >     long r[26];
> >
> >     int main()
> >     {
> >         memset(r, -1, sizeof(r));
> >         r[2] = open("/dev/kvm", 0);
> >         r[3] = ioctl(r[2], KVM_CREATE_VM, 0);
> >
> >         struct kvm_irqfd ifd;
> >         ifd.fd = syscall(SYS_eventfd2, 5, 0);
> >         ifd.gsi = 3;
> >         ifd.flags = 2;
> >         ifd.resamplefd = ifd.fd;
> >         r[25] = ioctl(r[3], KVM_IRQFD, &ifd);
> >         return 0;
> >     }
> >
> > Reported-by: Dmitry Vyukov <dvyukov@google.com>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  virt/kvm/irqchip.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
> > index fe84e1a95dd5..8db197bb6c7a 100644
> > --- a/virt/kvm/irqchip.c
> > +++ b/virt/kvm/irqchip.c
> > @@ -40,7 +40,7 @@ int kvm_irq_map_gsi(struct kvm *kvm,
> >
> >         irq_rt = srcu_dereference_check(kvm->irq_routing, &kvm->irq_srcu,
> >                                         lockdep_is_held(&kvm->irq_lock));
> > -       if (gsi < irq_rt->nr_rt_entries) {
> > +       if (irq_rt && gsi < irq_rt->nr_rt_entries) {
> >                 hlist_for_each_entry(e, &irq_rt->map[gsi], link) {
> >                         entries[n] = *e;
> >                         ++n;
> > --
> > 1.8.3.1
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>