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

[Wanpeng Li]

2017-01-06 0:01 GMT+08:00 Alex Williamson <alex.williamson@xxxxxxxxxx>:
> On Thu, 5 Jan 2017 11:24:08 +0100
> Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
>
>> On 05/01/2017 10:05, Wanpeng Li wrote:
>> > From: Wanpeng Li <wanpeng.li@xxxxxxxxxxx>
>> >
>> > 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@xxxxxxxxxxxxxxx
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>