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

[Paolo Bonzini <pbonzini@redhat.com>]

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