From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758036AbdLRIa1 (ORCPT ); Mon, 18 Dec 2017 03:30:27 -0500 Received: from mx1.redhat.com ([209.132.183.28]:49900 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757736AbdLRIa0 (ORCPT ); Mon, 18 Dec 2017 03:30:26 -0500 Subject: Re: [PATCH] KVM/Eventfd: Avoid crash when assign and deassign same eventfd in parallel. To: Lan Tianyu Cc: pbonzini@redhat.com, rkrcmar@redhat.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, dvyukov@google.com, kernellwp@gmail.com References: <1513554007-12302-1-git-send-email-tianyu.lan@intel.com> From: David Hildenbrand Organization: Red Hat GmbH Message-ID: Date: Mon, 18 Dec 2017 09:30:23 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <1513554007-12302-1-git-send-email-tianyu.lan@intel.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Mon, 18 Dec 2017 08:30:26 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 18.12.2017 00:40, Lan Tianyu wrote: > Syzroot reports crash in kvm_irqfd_assign() is caused by use-after-free. > Because kvm_irqfd_assign() and kvm_irqfd_deassign() can't run in parallel > for same eventfd. When assign path hasn't been finished after irqfd > has been added to kvm->irqfds.items list, another thead may deassign the > eventfd and free struct kvm_kernel_irqfd(). This causes assign path still > uses struct kvm_kernel_irqfd freed by deassign path. To avoid such issue, > add "initialized" flag in the struct kvm_kernel_irqfd and check the flag before > deactivating irqfd. If irqfd is still in initialization, deassign path > return fault.> > Reported-by: Dmitry Vyukov > Cc: Paolo Bonzini > Cc: Radim Krčmář > Cc: Dmitry Vyukov > Cc: Wanpeng Li > Signed-off-by: Tianyu Lan > --- > include/linux/kvm_irqfd.h | 1 + > virt/kvm/eventfd.c | 11 +++++++++-- > 2 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/include/linux/kvm_irqfd.h b/include/linux/kvm_irqfd.h > index 76c2fbc..be6b254 100644 > --- a/include/linux/kvm_irqfd.h > +++ b/include/linux/kvm_irqfd.h > @@ -66,6 +66,7 @@ struct kvm_kernel_irqfd { > struct work_struct shutdown; > struct irq_bypass_consumer consumer; > struct irq_bypass_producer *producer; > + u8 initialized:1; > }; > > #endif /* __LINUX_KVM_IRQFD_H */ > diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c > index a334399..80f06e6 100644 > --- a/virt/kvm/eventfd.c > +++ b/virt/kvm/eventfd.c > @@ -421,6 +421,7 @@ int __attribute__((weak)) kvm_arch_update_irqfd_routing( > } > #endif > > + irqfd->initialized = 1; The ugly thing in kvm_irqfd_assign() is that we access irqfd without holding a lock. I think that should rather be fixed than working around that issue. (e.g. lock() -> lookup again -> verify still in list -> unlock()) > return 0; > > fail: > @@ -525,6 +526,7 @@ void kvm_unregister_irq_ack_notifier(struct kvm *kvm, > { Which tool are you using to generate diffs? git format-patch? Mentioning, because the indicated function here .... (kvm_irqfd_deassign) > struct kvm_kernel_irqfd *irqfd, *tmp; > struct eventfd_ctx *eventfd; > + int ret = 0; > > eventfd = eventfd_ctx_fdget(args->fd); > if (IS_ERR(eventfd)) > @@ -543,7 +545,12 @@ void kvm_unregister_irq_ack_notifier(struct kvm *kvm, > write_seqcount_begin(&irqfd->irq_entry_sc); > irqfd->irq_entry.type = 0; > write_seqcount_end(&irqfd->irq_entry_sc); > - irqfd_deactivate(irqfd); > + > + if (irqfd->initialized) > + irqfd_deactivate(irqfd); > + else > + ret = -EFAULT; > + > } > } > > @@ -557,7 +564,7 @@ void kvm_unregister_irq_ack_notifier(struct kvm *kvm, > */ and here are wrong and misleading (kvm_irqfd_deassig). (and just noticed also in the second hunk) > flush_workqueue(irqfd_cleanup_wq); > > - return 0; > + return ret; > } > > int > -- Thanks, David / dhildenb