On Thu, 5 Jan 2017 11:24:08 +0100
Paolo Bonzini <pbonz...@redhat.com> wrote:
> On 05/01/2017 10:05, Wanpeng Li wrote:
> > From: Wanpeng Li <wanpeng...@hotmail.com>
> >
> > Reported syzkaller:
> >
> > BUG: unable to handle kernel NULL pointer dereference at
> > 0000000000000008
> > IP: irq_bypass_unregister_consumer+0x9d/0xb70 [irqbypass]
> > PGD 0
> >
> > Oops: 0002 [#1] SMP
> > CPU: 1 PID: 125 Comm: kworker/1:1 Not tainted 4.9.0+ #1
> > Workqueue: kvm-irqfd-cleanup irqfd_shutdown [kvm]
> > task: ffff9bbe0dfbb900 task.stack: ffffb61802014000
> > RIP: 0010:irq_bypass_unregister_consumer+0x9d/0xb70 [irqbypass]
> > Call Trace:
> > irqfd_shutdown+0x66/0xa0 [kvm]
> > process_one_work+0x16b/0x480
> > worker_thread+0x4b/0x500
> > kthread+0x101/0x140
> > ? process_one_work+0x480/0x480
> > ? kthread_create_on_node+0x60/0x60
> > ret_from_fork+0x25/0x30
> > RIP: irq_bypass_unregister_consumer+0x9d/0xb70 [irqbypass] RSP:
> > ffffb61802017e20
> > CR2: 0000000000000008
> >
> > The syzkaller folks reported a NULL pointer dereference that due to
> > unregister
> > an consumer which fails registration before. The syzkaller creates two VMs
> > w/
> > an equal eventfd occassionally. So the second VM fails to register an
> > irqbypass
> > consumer. It will make irqfd as inactive and queue an workqueue work to
> > shutdown
> > irqfd and unregister the irqbypass consumer when eventfd is closed.
> > However, the
> > second consumer has been initialized though it fails registration. So the
> > token
> > (same as the first VM's) is taken to unregister the consumer in the
> > workqueue,
> > the consumer of the first VM is found and unregistered, then NULL deref
> > incurred
> > in the path of deleting consumer from the consumers list.
>
> Thanks Wanpend!
>
> However, I'm wondering if we should improve the API instead. Maybe the
> token should be passed to irq_bypass_register_{producer,consumer} and
> only assigned if the functions succeed. Alex, what do you think?
Yes, I agree, we should improve the API rather than placing the burden
on the caller to cleanup on failure. I'm actually wondering why
irq_bypass_unregister_consumer() looks for the consumer entry based on
token matching rather than the consumer pointer itself. For instance,
doesn't something like this (untested) solve it?
diff --git a/virt/lib/irqbypass.c b/virt/lib/irqbypass.c
index 52abac4..6d2fcd6 100644
--- a/virt/lib/irqbypass.c
+++ b/virt/lib/irqbypass.c
@@ -195,7 +195,7 @@ int irq_bypass_register_consumer(struct irq_bypass_consumer
*consumer)
mutex_lock(&lock);
list_for_each_entry(tmp, &consumers, node) {
- if (tmp->token == consumer->token) {
+ if (tmp->token == consumer->token || tmp == consumer) {
mutex_unlock(&lock);
module_put(THIS_MODULE);
return -EBUSY;
@@ -245,7 +245,7 @@ void irq_bypass_unregister_consumer(struct
irq_bypass_consumer *consumer)
mutex_lock(&lock);
list_for_each_entry(tmp, &consumers, node) {
- if (tmp->token != consumer->token)
+ if (tmp != consumer)
continue;
list_for_each_entry(producer, &producers, node) {
> > diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> > index a29786d..eeaf056 100644
> > --- a/virt/kvm/eventfd.c
> > +++ b/virt/kvm/eventfd.c
> > @@ -415,9 +415,15 @@ kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd
> > *args)
> > irqfd->consumer.stop = kvm_arch_irq_bypass_stop;
> > irqfd->consumer.start = kvm_arch_irq_bypass_start;
> > ret = irq_bypass_register_consumer(&irqfd->consumer);
> > - if (ret)
> > + if (ret) {
> > pr_info("irq bypass consumer (token %p) registration
> > fails: %d\n",
> > irqfd->consumer.token, ret);
> > + irqfd->consumer.token = NULL;
> > + irqfd->consumer.add_producer = NULL;
> > + irqfd->consumer.del_producer = NULL;
> > + irqfd->consumer.stop = NULL;
> > + irqfd->consumer.start = NULL;
> > + }
> > }
> > #endif
> >
> >
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
