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

[Wanpeng Li]

2017-01-05 18:24 GMT+08:00 Paolo Bonzini <pbonzini@xxxxxxxxxx>:
>
>
> 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!

NP. :)

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

Yeah, I can continue to improve the patch if we reach an agreement with Alex. :)

Regards,
Wanpeng Li