> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
