Re: [PATCH] KVM: eventfd: fix NULL deref irqbypass consumer

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

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