From patchwork Thu Jan 5 16:01:35 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alex Williamson X-Patchwork-Id: 9499245 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 9B619606DE for ; Thu, 5 Jan 2017 16:01:57 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 8C9E6283BA for ; Thu, 5 Jan 2017 16:01:57 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 7FB552841A; Thu, 5 Jan 2017 16:01:57 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 2632D283BA for ; Thu, 5 Jan 2017 16:01:57 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S936452AbdAEQBh (ORCPT ); Thu, 5 Jan 2017 11:01:37 -0500 Received: from mx1.redhat.com ([209.132.183.28]:34038 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751851AbdAEQBg (ORCPT ); Thu, 5 Jan 2017 11:01:36 -0500 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5B8EC81252; Thu, 5 Jan 2017 16:01:37 +0000 (UTC) Received: from t450s.home (ovpn04.gateway.prod.ext.phx2.redhat.com [10.5.9.4]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v05G1apo028898; Thu, 5 Jan 2017 11:01:36 -0500 Date: Thu, 5 Jan 2017 09:01:35 -0700 From: Alex Williamson To: Paolo Bonzini Cc: Wanpeng Li , linux-kernel@vger.kernel.org, kvm@vger.kernel.org, Radim =?UTF-8?B?S3LEjW3DocWZ?= , Dmitry Vyukov , Wanpeng Li Subject: Re: [PATCH] KVM: eventfd: fix NULL deref irqbypass consumer Message-ID: <20170105090135.3899d667@t450s.home> In-Reply-To: <618d113d-e71a-076c-1ef8-4143110a5f39@redhat.com> References: <1483607111-2780-1-git-send-email-wanpeng.li@hotmail.com> <618d113d-e71a-076c-1ef8-4143110a5f39@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.68 on 10.5.11.27 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Thu, 05 Jan 2017 16:01:37 +0000 (UTC) Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Thu, 5 Jan 2017 11:24:08 +0100 Paolo Bonzini wrote: > 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? 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/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) {