From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S937902AbdLSGb6 (ORCPT ); Tue, 19 Dec 2017 01:31:58 -0500 Received: from mga07.intel.com ([134.134.136.100]:12149 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933272AbdLSGb5 (ORCPT ); Tue, 19 Dec 2017 01:31:57 -0500 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,425,1508828400"; d="scan'208";a="17207662" Subject: Re: [PATCH] KVM/Eventfd: Avoid crash when assign and deassign same eventfd in parallel. To: David Hildenbrand References: <1513554007-12302-1-git-send-email-tianyu.lan@intel.com> Cc: pbonzini@redhat.com, rkrcmar@redhat.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, dvyukov@google.com, kernellwp@gmail.com From: Lan Tianyu Message-ID: Date: Tue, 19 Dec 2017 14:21:29 +0800 User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi David: Thanks for your review. On 2017年12月18日 16:30, David Hildenbrand wrote: > 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()) The new lock should be always held in assign path otherwise we need to lookup irqfds list frequently, right? At first, I tried to use a mutex lock between assign and deassign path but assign path already involves some locks and add new lock maybe introduce dead lock. So I used flag check to replace with new lock. > >> 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? Yes, I used git version 1.8.3.1 :) This also confused me. I will try newer version. > > 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 >> > > -- Best regards Tianyu Lan