| Message ID | 20170105090135.3899d667@t450s.home (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2017-01-06 0:01 GMT+08:00 Alex Williamson <alex.williamson@redhat.com>: > On Thu, 5 Jan 2017 11:24:08 +0100 > Paolo Bonzini <pbonzini@redhat.com> wrote: > >> On 05/01/2017 10:05, Wanpeng Li wrote: >> > From: Wanpeng Li <wanpeng.li@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? Thanks for the great proposal Alex, it works. :) I just sent out v2. Regards, Wanpeng Li > > 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 majordomo@vger.kernel.org >> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) {