From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1161308AbdAEKYU (ORCPT ); Thu, 5 Jan 2017 05:24:20 -0500 Received: from mail-wj0-f195.google.com ([209.85.210.195]:36718 "EHLO mail-wj0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1161273AbdAEKYQ (ORCPT ); Thu, 5 Jan 2017 05:24:16 -0500 Subject: Re: [PATCH] KVM: eventfd: fix NULL deref irqbypass consumer To: Wanpeng Li , linux-kernel@vger.kernel.org, kvm@vger.kernel.org References: <1483607111-2780-1-git-send-email-wanpeng.li@hotmail.com> Cc: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , Dmitry Vyukov , Wanpeng Li , Alex Williamson From: Paolo Bonzini Message-ID: <618d113d-e71a-076c-1ef8-4143110a5f39@redhat.com> Date: Thu, 5 Jan 2017 11:24:08 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <1483607111-2780-1-git-send-email-wanpeng.li@hotmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 05/01/2017 10:05, Wanpeng Li wrote: > From: Wanpeng Li > > 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? Thanks, Paolo > > 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 > >